Refactor HttpClient usage #51

masojus · 2017-04-16T22:22:07Z

Consolidate HttpClient usage and common tasks like adding headers into KeenHttpClient.
Add some flexibility in construction for testing and maybe eventually for client code to customize HTTP settings (like proxy, headers, caching, etc.). We can customize how HttpClients and KeenHttpClients are created/provided and install HTTP handlers in the pipeline.
Cache HttpClient instances and share them.
Move some of the HTTP request dispatching code down into KeenHttpClient.
Test helper class for writing HTTP-related tests and for integration tests in general.
Fix a few bugs (not_contains filter operator, allow null property value in query filter).
Fix a few places where Master Key was being required when really the Read Key suffices.
Demonstrate usage of new test helper classes.
Get tests working against a real server. Verify changing the Keen URL base authority works so that the custom SSL domains feature can be used via this SDK.
Change default server url to "https://..."
Left a few TODOs since there's more work that can be done here, but it might be best to do a larger refactor after deciding which direction to go--we need to consider what's needed for Access Keys, Saved/Cached Queries and Datasets, as well as for supporting http proxies. All of that should get smoothed out before we jump to the 1.0 release.
Bump version.

- Note the actual code change in EventCachePortable.cs.

- It uses WeakReference though, which means with bad usage of KeenClient, like re-creating a KeenClient for every event push or Query will allow HttpClients to be disposed. - Add some customizability to creation of the HttpClient's handler chain, which will allow for setting a Proxy later, and also allow client code to install their own handlers while Keen's handlers also being there to add the right headers and logging and all that. - Add a KeenHttpClient class which is a view over a *hopefully* shared HttpClient that performs Keen-specific additional work when sending http messages. - Hook the Queries class up to this to see how it works.

… order to test this all out.

- Make most tests work against a real server, too. - `AddEvent_ScopedKeyWrite_Success()` still fails, liked due to Issue #34...should check on what happened with resolving that. - All tests succeed with UseMocks = true. - Note that this fixes Issue #34, which is to say that I verified that all tests succeeded after setting the IProjectSettings.KeenUrl to "https://my.owndoma.in/3.0/" which actually hit Keen's "api.keen.io" endpoints. This should suffice to show that, assuming the SSL cert with the proper SAN is installed, and the CNAME is properly configured, then everything w.r.t. the Custom SSL Domains feature will work as expected.

…deally we need to figure out how much helper code is provided for implementing things like IQueries, and we should split out code for constructing KeenHttpClient, unless we want to allow directly using/extending KeenHttpClient. For now we can keep accessibility more restrictive than maybe it will be later, while keeping in mind that we might relax it and therefore should provide flexibility for if/when these classes are public.

- Fix accessibility for what client code would actually need to use. - Fill in public comments. - Add lazy initialization of the HttpClient's handler pipeline so client code doesn't have to always be creating all that stuff even if the HttpClient is already in the cache. - Add a few utilities to the IHttpClientProvider interface for checking if an instance exists and removing/clearing. - Move most of the construction-related utility code out of KeenHttpClient into KeenHttpClientFactory. The factory is public, but the implementation class is still internal. Eventually we could consider making that extensible if somebody asks for it. For now, most cases should be covered by being able to plug in a DelegatingHandler or HttpClientHandler, so fully implementing IKeenHttpClient shouldn't be necessary except sometimes in tests.

…s where Moq isn't as easy to use as well as in e2e/bvt/integration tests where we want Mocks/Fakes that allow most functionality to pass through to actual implementations. - A few Mock wrappers that can plug into the HttpClient handler pipeline and a few abstractions to hopefully make that easier to configure and use in tests. - An IKeenHttpClientProvider test implementation. - No actual tests using this yet, but those will come next.

- Use this as an example of using some of the new HTTP-related test helpers. - Also fix/refactor a bit in the helper mock handlers. - Not sure why the default server URL wasn't using HTTPS before. - Explain in IProjectSettings that the KeenUrl is expected to end with a slash.

- Also port his tests to use the HTTP-related test helpers recently added. - Use this as an example to use UrlToMessageHandler while not strictly necessary here. Typo in comment for NotContains.

…rty_value` for a QueryFilter: - Fixes Issue #23 - Test it in unit tests as well as query integration tests.

- Note that 0.3.15 was released to NuGet but isn't in the source repo--it was a nuget package change only.

baumatron

Lots of goodness here. Thanks!

baumatron · 2017-04-18T19:22:13Z

Keen.Net/ProjectSettingsProviderFile.cs

        /// with a blank line.</para>
        /// </summary>
        public ProjectSettingsProviderFile(string filePath)
        {
+            // TODO : Add Keen Server URL as one of the lines, optionally.


Would it be more appropriate to create issues for these items instead of TODO's in the code?

Yeah if you think they're all valid. I didn't want to create Issues if I was just thinking about it all wrong :)

baumatron · 2017-04-18T19:25:01Z

Keen/Event.cs

+        internal Event(IProjectSettings prjSettings,
+                       IKeenHttpClientProvider keenHttpClientProvider)
+        {
+            if (null == prjSettings)


I wonder if it might be worth it to create some parameter validation utilities.

Yeah this is totally copy/paste between Event/EventCollection/Queries. I just stopped at some point, but we should continue consolidating and pushing for more DRY code.

There are strange things like Queries.cs housing the generic "call-all" api shim, whereas that should be somewhere shared by all of the various top-level APIs. This will require a bigger refactor to correct, for sure.

baumatron · 2017-04-18T19:26:38Z

Keen/Event.cs

            }
+
+            var responseMsg = await _keenHttpClient


This is so much better!

baumatron · 2017-04-18T19:29:35Z

Keen/Event.cs

+                // like that, this will throw. 
+                jsonResponse = JObject.Parse(responseString);
+
+                // TODO : Why do we not call KeenUtil.CheckApiErrorCode(jsonResponse); ??


I'd say just go ahead and try to normalize interaction with the API and do so unless you can determine a reason not to.

Not understanding these small discrepancies was the only reason I didn't push all the error handling stuff down into KeenHttpClient while I was at it. I might take another stab at running through the shape of responses in error conditions to see if we can just consolidate this and push it down a layer. Really the code in Event/EventCollection/Queries should get trimmed down quite a bit, and be mostly concerned with generating payload and parsing responses, and tons of that code is currently copy/paste code too, so hopefully after another two rounds of refactor, I'm thinking we can be at about half the lines of code in those three classes, maybe less.

Issue #55 and Issue #56

baumatron · 2017-04-18T19:42:56Z

Keen/EventCollection.cs

+            // dispatch, response parsing and error checking is copy/paste across Queries, Event
+            // and EventCollection everywhere we use KeenHttpClient. We could shove some of that
+            // into shared factory functionality (for the ctor stuff) and some of it into the
+            // KeenHttpClient (for the dispatch/response portions).


+1. Additionally, not sure I'm a huge fan of the Event, EventCollection, and Query abstractions as they stand today, either. Event, Query, and EventCollection class names should be reserved for an abstraction of an event, query, or event collection in my mind. Maybe a nomenclature change would make me more comfortable with them, like Event -> EventResource, EventCollection -> EventCollectionResource. Maybe they could share a Resource base class that knows how to do CRUD operations. Could be convinced of something else, too.

Yeah I'm thinking we should do something more drastic around these design decisions before v1 while we can still get away with beta-ish version-to-version breaks in compatibility. I'd rather the interfaces be more about the functionality as leveraged by client code rather than the exact shape of the HTTP resources of the API.

baumatron · 2017-04-18T21:41:19Z

Keen/Query/Queries.cs

-        private string _serverUrl;
+        private readonly IKeenHttpClient _keenHttpClient;
+        private readonly string _queryRelativeUrl;
+        private readonly string _key;


As you mentioned elsewhere, I'm not sure if I prefer storing keys as strings or asking IProjectSettings for them each time they're required. I think I might prefer the latter unless for whatever reason we want to reduce coupling between Queries and IProjectSettings, in which case I'd want to pass them as strings to the constructor of Queries. I could be swayed either way. I guess below there's logic that caches whatever the best key is, so I guess that's something else entirely unless master keys are disallowed.

In general defensive copies are nice, but since strings are immutable it's a little bit less of a concern. Once I started thinking about how we'd use the KeenClient to create Access Keys, I sort of started leaning back toward just requesting the key at message dispatch time.

As I look at it now, after investigating Access Keys a bit more, I think those will be an entirely different API surface that leverages KeenHttpClient or an intermediate layer of abstraction. One shouldn't/needn't create a KeenClient instance until the appropriate keys are generated. AccessKeys will only require the MasterKey, and by whatever mechanism that's acquired, a KeenClient can subsequently be created. Seems like those keys and the KeenUrl ought not change after that. If those keys need to change, recreating the instance won't be the end of the world.

baumatron · 2017-04-18T21:45:46Z

Keen/Query/Queries.cs

-                                                                 select string.Format("{0}={1}", p, Uri.EscapeDataString(parms[p])));
+            // TODO : The Python SDK has changed to not automatically falling back, but rather
+            // throwing so that client devs learn to use the most appropriate key. So here we
+            // really could or should just demand the ReadKey.


I'd say go for it.

baumatron · 2017-04-18T21:50:11Z

Keen/Query/Queries.cs

+                                    _queryRelativeUrl,
+                                    string.IsNullOrWhiteSpace(operation) ? "" : "/" + operation,
+                                    string.IsNullOrWhiteSpace(parmVals) ? "" : "?" + parmVals);
+            var responseMsg = await _keenHttpClient.GetAsync(url, _key).ConfigureAwait(false);


As mentioned elsewhere, hopefully a lot of this can eventually go into a layer on top of IKeenHttpClient like KeenApiClient or in an APIResource base class or something.

baumatron · 2017-04-18T22:00:29Z

Keen.NET.Test/HttpTests.cs

+        }
+
+        [Test]
+        public async Task DefaultHeaders_Success()


baumatron · 2017-04-18T22:04:35Z

Keen.NET.Test/QueryTests_Integration.cs

+
+            // NOTE : This example shows use of UrlToMessageHandler, but since we only make one
+            // request to a single endpoint, we could just directly use the FuncHandler here.
+            var urlHandler = new UrlToMessageHandler(


masojus · 2017-04-20T19:27:42Z

Thanks for taking a look @baumatron, I'll get some changes pushed responding to that feedback soon.

Two general notes about these changes:

Issue EncryptString Produces Invalid Scoped Key #34 is not fixed by this PR as mentioned in the comments for f70c59b--that was a typo. That change fixes Issues Ensure CNAME redirect and Custom SSL Domains work #48 instead.
This PR helps us move toward improving Issue Automated Unit Tests Have Poor Coverage #37.

- They didn't end up being used in several places nor did they turn out to be very complicated.

masojus added 12 commits March 26, 2017 16:36

A few doc/comment/whitespace changes.

736809c

- Note the actual code change in EventCachePortable.cs.

Switch the Event/EventCollection code over to using KeenHttpClient in…

a207927

… order to test this all out.

Port a fix provided by @baumatron to include the "not_contains" filter:

3c6168d

- Also port his tests to use the HTTP-related test helpers recently added. - Use this as an example to use UrlToMessageHandler while not strictly necessary here. Typo in comment for NotContains.

Port Pull Request #26 provided by @dietert to allow for a null `prope…

60a31f3

…rty_value` for a QueryFilter: - Fixes Issue #23 - Test it in unit tests as well as query integration tests.

Update VS project files.

352d632

Bump the assembly version to 0.3.16

9a259b9

- Note that 0.3.15 was released to NuGet but isn't in the source repo--it was a nuget package change only.

masojus requested a review from baumatron April 16, 2017 22:22

baumatron approved these changes Apr 18, 2017

View reviewed changes

masojus added 4 commits June 6, 2017 07:45

Start on PR feedback.

933e4e5

Merge branch 'master' into jm_HttpClient_Refactor

ba52aaa

Remove the CreateRequest() helpers.

5241739

- They didn't end up being used in several places nor did they turn out to be very complicated.

Bump version to 0.3.17 since we released a patch in the interim.

2ad96ec

masojus merged commit ba037e0 into master Jun 7, 2017

masojus mentioned this pull request Jun 7, 2017

allow null value as property_value in a QueryFilter #26

Closed

masojus mentioned this pull request Nov 7, 2017

Datasets #128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor HttpClient usage #51

Refactor HttpClient usage #51

masojus commented Apr 16, 2017

baumatron left a comment

baumatron Apr 18, 2017

masojus Apr 19, 2017

baumatron Apr 18, 2017

masojus Apr 19, 2017

masojus Jun 6, 2017 •

edited

Loading

masojus Jun 7, 2017

baumatron Apr 18, 2017

baumatron Apr 18, 2017

masojus Apr 19, 2017

masojus Jun 7, 2017

baumatron Apr 18, 2017

masojus Apr 19, 2017

masojus Jun 7, 2017

baumatron Apr 18, 2017

masojus Apr 19, 2017

masojus Jun 6, 2017 •

edited

Loading

baumatron Apr 18, 2017

baumatron Apr 18, 2017

baumatron Apr 18, 2017

baumatron Apr 18, 2017

masojus commented Apr 20, 2017

Refactor HttpClient usage #51

Refactor HttpClient usage #51

Conversation

masojus commented Apr 16, 2017

baumatron left a comment

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

Choose a reason for hiding this comment

masojus Jun 6, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masojus Jun 6, 2017 • edited Loading

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

Choose a reason for hiding this comment

masojus commented Apr 20, 2017

masojus Jun 6, 2017 •

edited

Loading

masojus Jun 6, 2017 •

edited

Loading