Changes Needed to Generate Sync RequestAuthenticator - #312
Conversation
| var task = Task.Run(() => m_httpClient.GetAsync(requestUri, completionOption, cancellationToken)); | ||
| task.Wait(); | ||
| return task.Result; |
There was a problem hiding this comment.
I'd be fine with just
=> Task.Run( () => m_httpClient.getAsync( requestUri, completionOption, cancellationToken ) ).Result;but no big deal
…t/InMemoryPublicKeyDataProviderTests.cs
…/Brightspace/D2L.Security.OAuth2 into anacoda/changes-for-sync-reqauth
…/Brightspace/D2L.Security.OAuth2 into anacoda/changes-for-sync-reqauth
j3parker
left a comment
j3parker
left a comment
There was a problem hiding this comment.
Very cool.
Could we try [GenerateSync] on one of the tests for fun? If it works to generate a sync test that would be pretty cool, heh. If it doesn't that's fine for now but I'd be curious about why.
Co-authored-by: Jacob Parker <jacob@solidangle.ca>
|
|
||
| namespace D2L.Security.OAuth2.Utilities | ||
| { | ||
| public class D2LHttpClient : ID2LHttpClient |
There was a problem hiding this comment.
Didn't our strategy pivot to sticking with HttpClient and generating the Task.Run wrapper?
There was a problem hiding this comment.
Correct me if I'm wrong @j3parker but I think this is a form of future-proofing so that we can implement true "sync" versions of HttpClient methods in the future. We do have the ability to wrap them in Task.Run but saved that for one-offs and not the HttpClient class
There was a problem hiding this comment.
I think the interface we're going to need to wrap HTTP is going to look a lot different than what we've got here (like methods like T GetAsJson<T>( string url ); etc., high level stuff).
I'm OK if we can drop D2LHttpClient for now and rely on the generator, or if that's a hassle I don't care if we have this (just it'll change drastically in a bit, that's all.)
There was a problem hiding this comment.
It is a bit of a hassle, because given the common names "Get" and "Send", I would have to check the types of whatever is calling them. Anyway, it would have to be a new package so if things are going to change drastically anyway I feel that we shouldn't make additional changes to the generator only to revert them later
There was a problem hiding this comment.
I guess I'm unsure why we added the ReadAsStringAsync wrapping into the generator, when it was directly related to this HttpClient usage - and I thought we were ultimately adding it to avoid having to wrap HttpClient (at the moment, at least).
Whatever works if we're just working incrementally though. I'd rather us not cut a release of D2L.Security.OAuth2 as-is though.
There was a problem hiding this comment.
I see, it's because ReadAsStringAsync is part of HttpContent not HttpClient which complicates things
b81f5bb to
c0a3371
Compare
Any tests for these changes will be in #316 (force pushed to remove them from this branch) |
omsmith
left a comment
omsmith
left a comment
There was a problem hiding this comment.
There's a number of cases of new code not matching our existing style. Could you get that fixed up?
- opening braces shouldn't have their own line (
class Foo : IFoo {) - method declarations and invocations should have spaces within the parens (
void Foo( string bar ) {,Foo( "Bar" );)
j3parker
left a comment
j3parker
left a comment
There was a problem hiding this comment.
I think we should ultimately wrap HttpClient in something that implements the sync workflows with HttpWebRequest. The interface for that wrapper would be pretty "simple", e.g. it wouldn't return HttpResponseMessage (that object is too complex) and tailored to our (limited needs).
If we do that we maybe don't need the Task.Run wrapping support in the generator, which would be ideal.
I don't think we need to hold up this PR on any of that though, and I'm keen to see some e2e stuff in the LMS.
3 participants
US154650: Async to Sync Generator: Generate RequestAuthenticator
Added [GenerateSync] as needed, partial classes, and small changes (made Task IRequestAuthenticator.AuthenticateAsync() asynchronous)