search

FirelyTeam / firely-net-sdk

The official Firely .NET SDK for HL7 FHIR
Other
829 stars 345 forks source link

[FhirClient] Use search using POST #479

Closed ewoutkramer closed 6 years ago

ewoutkramer commented 7 years ago

Currently, search is only supported using url parameters. For security reasons, you might want to put those in the body and POST the search (as specified in the spec here http://hl7.org/fhir/search.html#2.21.1.2).

kennethmyhra commented 6 years ago

Referenced PR #526 is ready for review, will need some input on this. Added several SearchUsingPost methods to interface IFhirClient which are identical to the already existing Search methods, could probably be done differently.

dougludlow commented 6 years ago

We have a requirement for this on a current project. I was just looking into how to implement this in a fork, but it looks like it is currently under way. What is the current status of this? I see a couple of PRs. But no updates since early February.

dougludlow commented 6 years ago

Oh, I see they were merged into develop and develop-stu3. I currently have v0.94 of Hl7.Fhir.Specification.STU3 and it doesn't appear to be there. Are there any plans for another release soon?

brianpos commented 6 years ago

I'm coincidentally integrating this branch into our server today, not sure when the plan to release a new NuGet package is though

dougludlow commented 6 years ago

@ewoutkramer any news on when the latest nuget package release will be?

brianpos commented 6 years ago

Ewout is on leave at the moment.

dougludlow commented 6 years ago

Oh sorry, @brianpos, I didn't realize you were also a maintainer. We're trying to decide whether we should fork the repo and build our own release or if we can wait for you guys to publish one. Do you have any insight into plans on creating a new release/nuget package?

ewoutkramer commented 6 years ago

Hi @dougludlow, back from holidays & I am doing release planning today, so I'll let you know shortly!

ewoutkramer commented 6 years ago

@dougludlow we are going to release the new 0.95 next week. Hope that's on time for you.

dougludlow commented 6 years ago

@ewoutkramer yes, that will work for us, thank you!

ewoutkramer commented 6 years ago

@dougludlow will be released today!

dougludlow commented 6 years ago

@ewoutkramer awesome! Thank you!

dougludlow commented 6 years ago

@ewoutkramer - Thanks so much for releasing this. I've got it installed and it appears to be working great. I had one question. I'm testing against https://vonk.fire.ly/ and the SearchAsync<Patient>(params) and SearchUsingPostAsync<Patient>(params) are pulling back entirely different results. Do you think it's a bug in fhir-net-api or just an issue with the test data on vonk?

ewoutkramer commented 6 years ago

Would need to take a look at what goes over the line using e.g. POST man to be sure - Will also signal @marcovisserFurore , since he's both on the API team and the Vonk team.

dougludlow commented 6 years ago

@ewoutkramer - we put in a fix for this: https://github.com/ewoutkramer/fhir-net-api/pull/587

ewoutkramer commented 6 years ago

Ah, thanks, had not yet realized this was dealing with the same issue.

kennethmyhra commented 6 years ago

Will the PR #597 be included in the Cologne Release?

ewoutkramer commented 6 years ago

I was hoping to, but if I look at @brianpos comments on #597, it seems there is unresolved business there :-( We will release the Cologne Release today or tomorrow, so unless @brianpos tells me it's ok to pull, I don't think I can.

kennethmyhra commented 6 years ago

Unless @ryankelley beats me to it I can have a look at it sometime today and apply the System.Net.Http.FormUrlEncodedContent that @brianpos suggests.

Can I use http://vonk.fire.ly/ endpoint to test the change?

kennethmyhra commented 6 years ago

@ewoutkramer Since the updated code now targets both netstandard and dotnetframework how can I run the unit tests against the netstandard specific code?

ewoutkramer commented 6 years ago

You cannot, really. It's been like this for some time, but until now the codepaths were sufficiently similar that I did not have to worry about it. Should I worry?

ewoutkramer commented 6 years ago

BTW: thanks for taking this up. I just realized @brianpos is on summer holidays, so this helps us a lot!

brianpos commented 6 years ago

Yes, I'm in Prague right now...

ewoutkramer commented 6 years ago

Wonderful city! Enjoy & don't look at github ;-)

kennethmyhra commented 6 years ago

You don't have to worry anymore :). I realized that System.Net.Http.FormUrlEncodedContent was available in .NET Framework 4.5 as well as >= netstandard 1.1, had though to add an assembly reference to System.Net.Http in Hl7.Fhir.Core.

I did a simple integration test against http://vonk.fire.ly using the integration test found here https://github.com/ewoutkramer/fhir-net-api/blob/develop-stu3/src/Hl7.Fhir.Core.Tests/Rest/SearchAsyncTests.cs#L54 and comparing it against the similar GET integration test found here https://github.com/ewoutkramer/fhir-net-api/blob/develop-stu3/src/Hl7.Fhir.Core.Tests/Rest/SearchAsyncTests.cs#L20. Would though appreciate if @dougludlow or @ryankelley ran their tests which detected this error in the first place.

Anyways, I guess using the class System.Net.Http.FormUrlEncodedContent is better than the homegrown way done before.

See PR #614

kennethmyhra commented 6 years ago

I have tested this further using paged searches against http://vonk.fire.ly and ran into an issue.

Initially when adding this feature I was using Spark for testing. Spark caches the search parameters server side and uses GUIDs for its next and previous links as reference to the search on the server side, paging would then work out of the box.

Vonk adds the search parameters to the next and previous links for the GET searches and uses _skip=[number] to skip through the pages. For the POST search the search parameters are omitted and only the _skip parameter is added.

Not quite sure how I can solve this client side. Also, if Vonk adds the search parameters the same way it does with GET, would, with the current client implementation, just make subsequent paging requests result in GET instead of POST against the server.

Response GET request:

"link": [{
        "relation": "self",
        "url": "http://vonk.fire.ly/Observation?subject:Patient=example"
    }, {
        "relation": "last",
        "url": "http://vonk.fire.ly/Observation?subject:Patient=example&_skip=20"
    }, {
        "relation": "next",
        "url": "http://vonk.fire.ly/Observation?subject:Patient=example&_skip=10"
    }
],

Response POST request:

"link": [{
        "relation": "self",
        "url": "http://vonk.fire.ly/Observation/_search"
    }, {
        "relation": "last",
        "url": "http://vonk.fire.ly/Observation/_search?_skip=20"
    }, {
        "relation": "next",
        "url": "http://vonk.fire.ly/Observation/_search?_skip=10"
    }
ewoutkramer commented 6 years ago

Yes, this is partly a Vonk problem (and has been identified as such by the Vonk team), and partly I think an issue with the spec. We have documented paging based on urls, but also search based on POSTs, and we say that the links should reflect the parameters that the server have recognized. There might be cases where the POSTS contain parameters that cannot be represented on urls (e.g. because they are too long).

I've created a gForge issue for HL7 here: https://gforge.hl7.org/gf/project/fhir/tracker/?action=TrackerItemEdit&tracker_item_id=17316

For now, there's nothing we can do client side - our behaviour is correct, so we can just proceed and leave it to the Vonk/spec guys to solve this.