HttpClient injection #131

martincostello · 2018-07-18T21:05:36Z

Adds a test showing a proof-of-concept for injection of HttpClient into Connection so that IHttpClientFactory can be used for scenarios such as ASP.NET Core 2.1+ applications.

Relates to #125.

Depends on:

grokys

Was the intention that this method could be used by the client to configure the HttpClient passed to the ctor? If so, should it be public?

grokys · 2018-07-19T13:44:35Z

Octokit.GraphQL.Core/Connection.cs

@@ -61,12 +75,26 @@ public Connection(ProductHeaderValue productInformation, Uri uri, ICredentialSto
            }
        }

-        private HttpClient CreateHttpClient(ProductHeaderValue header)
+        private static void ConfigureHttpClient(HttpClient httpClient, ProductHeaderValue header)


Was the intention that this method could be used by the client to configure the HttpClient passed to the ctor? If so, should it be public?

No, this was more sort-of so that if the user didn't configure the client fully, a default configuration would be applied, though turns out I forgot to plumb that into the new constructor.

I'm not entirely sure what the constructor options that supporting this should be yet, I just added a minimum to make the test work to show how you could use it.

StanleyGoldman · 2018-07-25T13:28:57Z

So I had the obvious realization the other day that a lot of this has already come from octokit/octokit.net for good reason and should continue to be based off of that.

The constructor of Connection should more closely match what is found here..

https://github.com/octokit/octokit.net/blob/8407369485541b1fce28f1c9c8d5bf5852579c57/Octokit/Http/Connection.cs#L134-L159

StanleyGoldman · 2018-07-25T13:31:21Z

Octokit.GraphQL.Core/Connection.cs

@@ -40,6 +40,20 @@ public Connection(ProductHeaderValue productInformation, Uri uri, ICredentialSto
            HttpClient = CreateHttpClient(productInformation);
        }

+        /// <inheritdoc />
+        public Connection(HttpClient httpClient, ICredentialStore credentialStore)


All constructors should roll into this one, and the signature should be..

Connection(ProductHeaderValue productInformation, Uri uri, ICredentialStore credentialStore, HttpClient httpClient)

StanleyGoldman · 2018-07-25T13:32:59Z

Octokit.GraphQL.Core/Connection.cs

@@ -40,6 +40,20 @@ public Connection(ProductHeaderValue productInformation, Uri uri, ICredentialSto
            HttpClient = CreateHttpClient(productInformation);
        }

+        /// <inheritdoc />


For some reason we have /// <inheritdoc/> on all these constructors.
Remove this line and merge the branch overeager-inheritdoc into this branch to take care of the others.

StanleyGoldman · 2018-07-25T13:34:42Z

Octokit.GraphQL.Core/Connection.cs

+        public Connection(HttpClient httpClient, ICredentialStore credentialStore)
+        {
+            HttpClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient));
+            CredentialStore = credentialStore ?? throw new ArgumentNullException(nameof(credentialStore));


Good catch, we should have been doing this in the previous constructor.
Be sure to add more for all the arguments when you add them.

StanleyGoldman · 2018-07-25T13:35:55Z

Octokit.GraphQL.Core/Connection.cs

+
+            if (httpClient.BaseAddress == null)
+            {
+                httpClient.BaseAddress = GithubApiUri;


Much like in octokit.net we shouldn't handle this here, just set a member variable and we will handle this later.

StanleyGoldman · 2018-07-25T13:48:21Z

The rest of the differences between us and octokit.net is how we execute things.

They build the request, setting the Uri at that moment.
https://github.com/octokit/octokit.net/blob/8407369485541b1fce28f1c9c8d5bf5852579c57/Octokit/Http/Connection.cs#L206-L211
Everything else is applied when they run the request:
https://github.com/octokit/octokit.net/blob/8407369485541b1fce28f1c9c8d5bf5852579c57/Octokit/Http/Connection.cs#L630-L643

StanleyGoldman · 2018-07-25T14:03:50Z

Octokit.GraphQL.Core/Connection.cs

+            }
+        }
+
+        private static HttpClient CreateHttpClient(ProductHeaderValue header)


This method isn't needed, we should just new HttpClient() in the constructors above.

StanleyGoldman · 2018-07-25T14:04:25Z

Octokit.GraphQL.Core/Connection.cs

+            if (httpClient.DefaultRequestHeaders.UserAgent.Count == 0)
+            {
+                var userAgent = new ProductInfoHeaderValue(header.Name, header.Version);
+                httpClient.DefaultRequestHeaders.UserAgent.Add(userAgent);


Setting userAgent and headers should be performed when we are executing the request.

StanleyGoldman · 2018-07-25T14:06:00Z

The final major difference in octokit.net is they have an interface for HttpClient in case you need to replace it and their own HttpClientAdapter which implements their standard version.
But I think we can leave that much for another time.

StanleyGoldman · 2018-07-25T14:06:50Z

@martincostello thanks for getting this conversation started 🎖.
Let me know if you want to continue chasing this dragon.

martincostello · 2018-07-25T14:07:54Z

@StanleyGoldman Sure, I'll take a look at incorporating the feedback at some point later this week or early next 👍

martincostello · 2018-07-28T11:50:42Z

@StanleyGoldman I've done some refactoring based on the feedback and rebased onto master. The changes I've made are:

Refactored Connection as suggested to add the new constructor that accepts HttpClient following the pattern in Octokit as suggested. I've also added an extra HttpClient-based constructor that makes DI easier in the case where the default URI is used (i.e. for GitHub.com).
Added an optional parameter to Run() that accepts a CancellationToken which is passed to the SendAsync() method of HttpClient. I've not plumbed this in to the query runners as then that goes a bit viral, but happy to add this in this PR or a different one if you think it's worth doing.
Add validation of arguments to constructors and methods.
Changed GitHubApiUri to a get property instead of a read-only field, as that's more usual for C#.
Added ConfigureAwait(false) to places where await is used to prevent potential deadlocks in apps with a synchronization context, like an application with a UI.
I've added XML documentation mostly lifted as-is from Octokit and applied it to all the public members of the Connection class, rather than just delete <inherit-doc/>.
I've updated the test projects to target netcoreapp2.1 as 1.1 is relatively old in .NET Core terms now, as well as update the .NET Core Test SDK and xunit. This then created some code analysis warnings from the xunit analysers that have shipped in-box since 2.3.0, so I fixed those too. I think as an unintended side-effect of the runtime upgrade, some of the tests in ExpressionRewriterTests are now failing because the ToString() representations now include the airity of the type (so IEnumerable`1 instead of IEnumerable). I wasn't sure the best way to fix these, so for now they're still broken. Update tests' target framework #140
Fixed a NullReferenceException in tests that use ExpressionCompiler that can occur when the tests are run in parallel with each other and try to add items to the dictionary at the same time. I've resolved this by changing the code to use ConcurrentDictionary instead. Fix NullReferenceException in tests #139

StanleyGoldman · 2018-07-31T15:08:39Z

Hey @martincostello I 💟 all that you are doing, but we gotta be careful of not doing too much in a single pull request. I haven't gotten the chance to look at this pull request again in deatil, but based on what you said in your last comment. It sounds like we can move some of these changes out to a different pull request.

Would it be possible to rebase this branch and move items 7 and 8 to separate pull requests?

martincostello · 2018-07-31T16:15:19Z

@StanleyGoldman No problem. I'll do those now, then rebase this once those are in. Any ideas roughly on how to go about fixing the issues with the type name formatting breaking those expression tests?

martincostello · 2018-07-31T16:34:13Z

@StanleyGoldman Have raised the two PRs for items 7 and 8.

StanleyGoldman · 2018-07-31T17:05:19Z

Thanks @martincostello

Any ideas roughly on how to go about fixing the issues with the type name formatting breaking those expression tests?

LOL. No clue yet.

StanleyGoldman · 2018-08-01T14:41:33Z

Okay, thanks for entertaining me there and separating out those other issues. @grokys is AFK for a few more days and I'd like to keep on moving forward.

I pushed a temporary branch called custom_target. It is a merge of #139 and #140.

Would you mind rebasing this branch onto that and changing this pull request to target that branch?
That way we can finish working out the changes here, and @grokys can give us the final approval when he gets back.

martincostello · 2018-08-01T14:42:40Z

Sure. I'll do that shortly.

martincostello · 2018-08-01T17:38:40Z

Rebased and target branch updated.

martincostello · 2018-08-01T17:44:47Z

Blargh, I messed up something slightly in #140. I'll fix that now, then rebase this again later if you could update custom_target once I have?

StanleyGoldman · 2018-08-01T18:55:55Z

Merged.

martincostello · 2018-08-02T09:01:31Z

Updated with the latest custom_target changes.

StanleyGoldman · 2018-08-03T13:47:54Z

Wanna rebase this onto master one last time?
My bad..

martincostello · 2018-08-03T14:49:07Z

Sure 😄

Add a test showing a proof-of-concept for injection of HttpClient into Connection so that IHttpClientFactory can be used for scenarios such as ASP.NET Core applications. Relates to #125.

Route all constructors into new constructor that accepts HttpClient. Add extra constructor to simplify dependency injection for GitHub.com. Add XML documentation to class and members based on Octokit. Add parameter validation. Change GithubApiUri to a property. Set HttpRequestMessage properties (User Agent etc.) per request. Add ConfigureAwait(false) to awaits. Add optional parameter for using a CancellationToken.

martincostello · 2018-08-03T14:53:40Z

Rebased. Also, pending further code review, I should add a bunch more unit tests for the changes in Connection (like for parameter validation).

StanleyGoldman · 2018-08-03T16:19:09Z

You are never going to hear me argue against more tests... 😼

Add a TODO comment suggesting changing ICredentialStore to support CancellationToken.

Make the run method in Connection virtual to allow more useful derived classes.

Add unit tests to cover the basic behaviour of managing HTTP requests.

martincostello · 2018-08-04T11:26:40Z

Have added some new unit tests for the Connection class.

I've also made Run() virtual so that if someone made a derived class it would have more utility given the protected members it has.

It also occurred to me that passing the CancellationToken through to ICredentialStore would be useful if an implementation needed to do a network call, given the method is async. As that would be another interface change, I figured I'd pose the question first rather than just change it.

StanleyGoldman · 2018-08-08T17:30:48Z

We haven't forgotten you, just tending to our other projects.

StanleyGoldman · 2018-08-09T16:22:02Z

The interface change makes sense, oddly enough though no one seems to have encountered this issue with octokit.net. Connection.Run

await _authenticator.Apply(request).ConfigureAwait(false);
var response = await _httpClient.Send(request, cancellationToken).ConfigureAwait(false);

StanleyGoldman · 2018-08-09T16:32:56Z

I made that change you were referring to and made some minor tweaks

StanleyGoldman · 2018-08-09T16:33:07Z

Octokit.GraphQL.IntegrationTests/Configuration/HttpClientFactoryTests.cs

+
+namespace Octokit.GraphQL.IntegrationTests.Configuration
+{
+    public static class HttpClientFactoryTests


Is there a reason why the test class is static?

xunit supports classes/methods being static for tests if everything is self contained to the fact/theory - happy to make non-static if that’s your convention.

StanleyGoldman · 2018-08-09T16:33:25Z

Octokit.GraphQL.IntegrationTests/Configuration/HttpClientFactoryTests.cs

+    public static class HttpClientFactoryTests
+    {
+        [Fact]
+        public static async Task Can_Configure_With_HttpClient_Factory()


Same with this method?

StanleyGoldman · 2018-08-09T16:37:18Z

Other than that, this is looking pretty good IMO.

Fix unit test broken by refactoring.

martincostello · 2018-08-09T18:16:43Z

I've fixed up a test, but other than any other feedback, I think I'm done on this PR.

I've also done a commit on a separate branch (17c3cf4) to plumb in CancellationToken to the runners and extension methods that use IConnection that builds on this changes in this PR.

grokys

LGTM!

martincostello mentioned this pull request Jul 18, 2018

Support injection of HttpClient instances #125

Closed

grokys reviewed Jul 19, 2018

View reviewed changes

StanleyGoldman requested changes Jul 25, 2018

View reviewed changes

martincostello changed the title ~~[WIP] Proof-of-concept for HttpClient injection~~ [WIP] HttpClient injection Jul 28, 2018

This was referenced Jul 31, 2018

Fix NullReferenceException in tests #139

Merged

Update tests' target framework #140

Merged

StanleyGoldman added the work-in-progress label Aug 1, 2018

martincostello changed the base branch from master to custom_target August 1, 2018 17:37

martincostello added 2 commits August 3, 2018 15:49

Proof-of-concept for HttpClient injection

55c291e

Add a test showing a proof-of-concept for injection of HttpClient into Connection so that IHttpClientFactory can be used for scenarios such as ASP.NET Core applications. Relates to #125.

martincostello changed the base branch from custom_target to master August 3, 2018 14:52

martincostello added 3 commits August 4, 2018 12:15

Add TODO about using CancellationToken for credentials

9154ab4

Add a TODO comment suggesting changing ICredentialStore to support CancellationToken.

Make the Run method in Connection virtual

2b1a5e0

Make the run method in Connection virtual to allow more useful derived classes.

Add unit tests for the Connection class

63dc83e

Add unit tests to cover the basic behaviour of managing HTTP requests.

StanleyGoldman added 3 commits August 9, 2018 12:24

Passing cancellationToken into ICredentialStore

9f263de

Adding validation and doc comments

197b9dc

Using inheritdoc for brevity

a6b997a

StanleyGoldman reviewed Aug 9, 2018

View reviewed changes

StanleyGoldman changed the title ~~[WIP] HttpClient injection~~ HttpClient injection Aug 9, 2018

StanleyGoldman added ready-for-review and removed work-in-progress labels Aug 9, 2018

Fix unit test

59fe63c

Fix unit test broken by refactoring.

grokys approved these changes Aug 10, 2018

View reviewed changes

StanleyGoldman approved these changes Aug 10, 2018

View reviewed changes

StanleyGoldman merged commit c4cb46e into octokit:master Aug 10, 2018

martincostello deleted the HttpClient-Injection branch August 10, 2018 10:37

martincostello mentioned this pull request Aug 10, 2018

Pass-through CancellationTokens to connections #143

Merged

StanleyGoldman added this to the v0.1.1-beta milestone Aug 10, 2018

HttpClient injection #131

HttpClient injection #131

Conversation

martincostello commented Jul 18, 2018 • edited

grokys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanleyGoldman commented Jul 25, 2018 • edited

StanleyGoldman Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanleyGoldman commented Jul 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanleyGoldman commented Jul 25, 2018

StanleyGoldman commented Jul 25, 2018

martincostello commented Jul 25, 2018

martincostello commented Jul 28, 2018 • edited

StanleyGoldman commented Jul 31, 2018

martincostello commented Jul 31, 2018

martincostello commented Jul 31, 2018

StanleyGoldman commented Jul 31, 2018

StanleyGoldman commented Aug 1, 2018

martincostello commented Aug 1, 2018

martincostello commented Aug 1, 2018

martincostello commented Aug 1, 2018

StanleyGoldman commented Aug 1, 2018

martincostello commented Aug 2, 2018

StanleyGoldman commented Aug 3, 2018

martincostello commented Aug 3, 2018

martincostello commented Aug 3, 2018

StanleyGoldman commented Aug 3, 2018

martincostello commented Aug 4, 2018

StanleyGoldman commented Aug 8, 2018

StanleyGoldman commented Aug 9, 2018

StanleyGoldman commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StanleyGoldman commented Aug 9, 2018

martincostello commented Aug 9, 2018

grokys left a comment

Choose a reason for hiding this comment

martincostello commented Jul 18, 2018 •

edited

StanleyGoldman commented Jul 25, 2018 •

edited

StanleyGoldman Jul 25, 2018 •

edited

StanleyGoldman commented Jul 25, 2018 •

edited

martincostello commented Jul 28, 2018 •

edited