Feature/480 FhirClient implementation with HttpClient #507

Devereux-Henley · 2017-12-05T23:54:28Z

Created new implementation of FhirClient using System.Net.Http.HttpClient.
New mirrored classes placed in Hl7.Fhir.Rest.Http namespace
Common functionality extracted to a base class.
Old implementation marked as [Obsolete]
FhirClient now IDisposable
Simply swapping classes will compile, throw exceptions on obsoleted functionality.

…sing ToHttpEntry directly.

… test that was adding headers twice.

…uestMessage objects.

…ons.

…d ones

…mplementedException or be hidden by default.

…gic in base classes.

ewoutkramer · 2017-12-06T09:38:04Z

Thanks! That's a major chunk of work. I will need some time to digest it ;-)

ewoutkramer

Really nice piece of work! Pls review my comments, then I'll pull it as a feature branch!

ewoutkramer · 2017-12-12T11:59:27Z

src/Hl7.Fhir.Core/Rest/Http/HttpToEntryExtensions.cs

+            result.Response.Location = response.Headers.Location?.AbsoluteUri ?? response.Content.Headers.ContentLocation?.AbsoluteUri;
+
+            result.Response.LastModified = response.Content.Headers.LastModified;
+            result.Response.Etag = response.Headers.ETag?.Tag;


I don't think this does 100% what getEtag() did in the original code: the result.Response.Etag will now be set to include the quotes around the etag, while getEtag() removed those (result = result.Trim('\"')).

Will restore trim.

ewoutkramer · 2017-12-12T12:01:10Z

src/Hl7.Fhir.Core/Rest/Http/HttpToEntryExtensions.cs

+            {
+                result.Response.SetBody(body);
+
+                if (Rest.HttpToEntryExtensions.IsBinaryResponse(result.Response.Location, contentType.ToString()))


I think this must be contentType.MediaType, since otherwise it might include the charset parameter, which was removed by the original code using getContentType().

Will replace with MediaType

ewoutkramer · 2017-12-12T12:01:39Z

src/Hl7.Fhir.Core/Rest/Http/HttpToEntryExtensions.cs

+
+                if (Rest.HttpToEntryExtensions.IsBinaryResponse(result.Response.Location, contentType.ToString()))
+                {
+                    result.Resource = Rest.HttpToEntryExtensions.MakeBinaryResource(body, contentType.ToString());


This is actually better than the original, since it will include the charset parameter, which the original code did not.

ewoutkramer · 2017-12-12T12:03:20Z

src/Hl7.Fhir.Core/Rest/Http/HttpToEntryExtensions.cs

+                else
+                {
+                    var bodyText = Rest.HttpToEntryExtensions.DecodeBody(body, charEncoding);
+                    var resource = Rest.HttpToEntryExtensions.ParseResource(bodyText, contentType.ToString(), parserSettings, throwOnFormatException);


Also here, contentType.ToString() would include the charset parameter (if the client passed that in), which will not work well with DecodeBody, I think this should be contentType.MediaType

Will change to MediaType.

ewoutkramer · 2017-12-12T12:27:50Z

src/Hl7.Fhir.Core/Rest/Http/EntryToHttpExtensions.cs

+            var request = new HttpRequestMessage(getMethod(interaction.Method), location.Uri);
+
+            if (!useFormatParameter)
+                request.Headers.Add("Accept", Hl7.Fhir.Rest.ContentType.BuildContentType(format, forBundle: false));


What is the reason you are not doing request.Headers.Accept.Add()?

Did not see static MediaTypeWithQualityHeaderValue.Parse. Replaced this with a call to request.Headers.Accept.Add.

ewoutkramer · 2017-12-12T12:29:46Z

src/Hl7.Fhir.Core/Rest/Http/EntryToHttpExtensions.cs

+            if (!useFormatParameter)
+                request.Headers.Add("Accept", Hl7.Fhir.Rest.ContentType.BuildContentType(format, forBundle: false));
+
+            if (interaction.IfMatch != null) request.Headers.TryAddWithoutValidation("If-Match", interaction.IfMatch);


Is the way we use If-Match in FHIR illegal, so we need to avoid validation?

No. This was added while debugging and is no longer needed. Will replace with Add.

ewoutkramer · 2017-12-12T12:31:45Z

src/Hl7.Fhir.Core/Rest/Http/EntryToHttpExtensions.cs

+
+            request.Content = new ByteArrayContent(body);
+
+            // MediaTypeHeaderValue cannot accept a content type that contains charset at the end, so that value must be split out.


Yes, I noticed the same, weird. Maybe I'll take a look at the reference source to see why.

If you look at https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/Headers/MediaTypeHeaderValue.cs#L203, it seems there is support for parsing mediatypes with parameters...

Will replace with MediaTypeHeaderValue.Parse(contentType)

ewoutkramer · 2017-12-12T12:43:55Z

src/Hl7.Fhir.Core.Tests/Rest/Mocks/MockHttpMessageHandler.cs

+
+namespace Hl7.Fhir.Core.Tests.Rest.Mocks
+{
+    public class MockHttpMessageHandler : HttpClientHandler


This MessageHandler could be used by people who want to still use the OnBefore/OnAfter logic, right? The name "mock" did not suggest that to me (I would think it has something to do with testing). Maybe call it "RequestResponseEventMessageHandler" or something like that?

Oh, this is actually there to support testing ;-) But we could provide it more generally, as I mentioned, right?

Yes, this could be provided more generally. I will move this to the new namespace and rename.

src/Hl7.Fhir.Core/Rest/Http/EntryToHttpExtensions.cs

mmsmits · 2020-06-18T12:48:51Z

@Devereux-Henley Thanks so much for all your efforts on this. We have re-used a lot of your code, in a separate PR which now has (finally) been merged into develop-r4 and develop-stu3. If you like, please take a look to see if you like how we implemented it. We would appreciate your feedback very much. Hope you like it! Thanks again!

Henley_Contractor, Devereux added 30 commits November 20, 2017 15:46

Initial work on converting from WebRequest to HttpClient.

4e8b755

Body now getting written to request.

b525bbc

Fixed tests to use new .NET types and fixed some parsing issues.

4e04239

Changed types for rebase onto develop-dstu3.

1e2e0f3

Fixed missing search parameter argument from rebase and fixed tests u…

2be2b92

…sing ToHttpEntry directly.

Ensured that we don't try to convert an empty body to a response bundle.

3cc2a79

Fixed some comments referencing HttpWeb[Request/Response] and fixed a…

90eb380

… test that was adding headers twice.

Added comments and removed dead code.

c24f532

Fixed formatting.

2ae3d35

All classes wrapping HttpClient are now IDisposables

6d88720

Removed generated docstrings.

bf59ad4

Moved setting of user agent to default request headers.

d7ef495

Dispose of fhir clients during tests.

8bf076e

Finished adding using statements to REST tests.

65fd4eb

Added using statements for Requesters HttpResponseMessage and HttpReq…

f09becc

…uestMessage objects.

Merge branch 'develop-stu3' into feature/httpclient

e244e7c

Moved common code to IFhirCompatibleClient

0b9104d

Added stub for new fhir client.

3de9632

Added stub for httpclient specific requester

31d4b96

Moved new implementations to new namespace, restored old implementati…

ed73dd9

…ons.

Refactored common code to base class for FhirClient and Requester.

349bb4a

Copied modified tests to Http specific test directory.

4ff7dab

Changed Http/ to actually be changed tests.

78dfba5

Fixed constructor not having optional parameter.

674f169

Deleted extra test files and resolved to place new tests alongside ol…

9b6cfa0

…d ones

Added mock message handler and started adding HttpClient tests.

14836e8

Modified client operations to work on fhir client base class.

49c1030

Modified more tests for HttpClient addition

cb751a5

Merge branch 'develop-stu3' into feature/httpclient

8836481

Fixed some using statements in client tests.

4f5e304

Henley_Contractor, Devereux added 9 commits December 5, 2017 13:20

Removed unused delegates.

5d0326f

Obsolete properties now included in common interface, will throw NotI…

ef39dbb

…mplementedException or be hidden by default.

Added operations tests for new client.

d1b8f76

Added async read tests for new client.

1da3f4b

Added async search tests for new client.

aaa0688

Added update/refresh/delete tests for new client.

4439d99

Fixed faulty tests, bad logic in MockHandler, refactored exception lo…

e98ad76

…gic in base classes.

Added new client tests for TerminologyTests.

b0fd949

Reverted System.Net.Http version dependency

43bb2a4

sodell mentioned this pull request Dec 7, 2017

Replace [Obsolete] HttpWebRequest with HttpClient #480

Closed

ewoutkramer reviewed Dec 12, 2017

View reviewed changes

Henley_Contractor, Devereux added 2 commits December 12, 2017 16:35

Made changes per first set of review comments on base fhir-net-api repo.

92e7602

Removed unnecessary include.

79ae559

ewoutkramer reviewed Dec 13, 2017

View reviewed changes

src/Hl7.Fhir.Core/Rest/Http/EntryToHttpExtensions.cs Outdated Show resolved Hide resolved

ewoutkramer changed the base branch from develop-stu3 to feature/480-httpclient December 13, 2017 16:51

ewoutkramer merged commit 70ec9b0 into FirelyTeam:feature/480-httpclient Dec 13, 2017

kennethmyhra mentioned this pull request Apr 12, 2018

Support for manipulating request and response body #496

Closed

BradBarnich mentioned this pull request Oct 15, 2018

For discussion: Replatform FhirClient on HttpClient #727

Closed

ewoutkramer added the requires major version update label Sep 4, 2019

ewoutkramer assigned en-gstoian Sep 4, 2019

ewoutkramer mentioned this pull request May 19, 2020

FHIR Client refactoring R3 #1364

Merged

mmsmits unassigned en-gstoian Jun 18, 2020

mmsmits self-requested a review June 18, 2020 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/480 FhirClient implementation with HttpClient #507

Feature/480 FhirClient implementation with HttpClient #507

Devereux-Henley commented Dec 5, 2017

ewoutkramer commented Dec 6, 2017

ewoutkramer left a comment

ewoutkramer Dec 12, 2017

Devereux-Henley Dec 12, 2017

ewoutkramer Dec 12, 2017

Devereux-Henley Dec 12, 2017

ewoutkramer Dec 12, 2017

ewoutkramer Dec 12, 2017

Devereux-Henley Dec 12, 2017

ewoutkramer Dec 12, 2017

Devereux-Henley Dec 12, 2017

ewoutkramer Dec 12, 2017

Devereux-Henley Dec 12, 2017

ewoutkramer Dec 12, 2017

ewoutkramer Dec 12, 2017

Devereux-Henley Dec 12, 2017

ewoutkramer Dec 12, 2017

ewoutkramer Dec 12, 2017

Devereux-Henley Dec 12, 2017

mmsmits commented Jun 18, 2020


		request.Content = new ByteArrayContent(body);

		// MediaTypeHeaderValue cannot accept a content type that contains charset at the end, so that value must be split out.

Feature/480 FhirClient implementation with HttpClient #507

Feature/480 FhirClient implementation with HttpClient #507

Conversation

Devereux-Henley commented Dec 5, 2017

ewoutkramer commented Dec 6, 2017

ewoutkramer 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmsmits commented Jun 18, 2020