updated search function to allow user to specify request headers

[ashankland]

The FHIR specification allows for an FHIRVersion tag to be used in the content headers of requests, and which can change the return content: https://www.hl7.org/fhir/http.html#version-parameter

Currently, the search function doesn't allow the user to set headers for their http requests manually, so there is no way to modify it to include a version tag. This branch adds that functionality as an optional variable.

Note: this is a new copy of the pull request which doesn't omit any changes from the diff.

[ashankland]

I moved to this branch because I was having a lot of trouble properly rebasing the last branch to the latest commits from master. I apologize for the confusion, and hope you'll be patient with me as I work this out.

The original pr and comments can be found here: #132

[arscan]

Thanks for the contribution @ashankland. fhirVersion is definitely something we need in the client. But we have to be a little careful with changing/extending the API of the library, because once we add something like this and people start using it, it becomes very difficult to remove or change in the future.

But before I get into the content of the PR, I believe you can basically accomplish the same thing that you are doing here w/out changing the client code, using the additional_headers configuration option. For example:

client = FHIR::Client.new('http://hapi.fhir.org/baseR4')
client.additional_headers = {Accept: 'application/json+fhir; fhirVersion=4.0'}
reply = client.search(FHIR::Patient, search: {parameters: {name: 'P'}})
puts reply.request[:headers]

In the short term you can consider doing this workaround until we figure out the right way of upgrading the library. But it is a workaround because this was intended more for thinks like custom headers, not for overwriting existing headers, though I believe you can technically do that.

Back to the PR and the proposed approach for the library -- I have a couple of concerns :

• This client tends to figure out headers by itself, instead of having users pass them in. That way, you don't need to specify the Content-Type header on writes, for example -- the client automatically adds it when necessary. The approach in this PR overrides whatever the client decided to use, and it seems likely that it would be a source of bugs. What if the client is configured to use XML and planned to send Accept: application/fhir+xml but the user of the client tells it to send Accept: application/fhir+json; fhirVersion 4.0 because they wanted to specify fhirVersion, didn't care much about the content type, and didn't realize they might interact with an XML server in the future?
• This seems to work for searches, but fhirVersion is something that should be implemented on all interactions (read, etc). Can the same approach be used everywhere? Besides being somewhat tedious (having to add an extra param to every externally available method call), I'm not sure that we could even simply add another parameter everywhere given how the other methods are organized. It would be frustrating for a user of the library if the method for adding this isn't consistent everywhere.
• It seems like we need to do this everywhere MIME is used, including the _format param, which is more than just the header.

Also, before accepting a PR, we would need to update the README with instructions so they would know how to add fhirVersion. And ideally we would add in some tests to make sure that it worked as desired.

In general, a good idea is to stay consistent with how the rest of the library is organized. Since the fhirVersion parameter applicable to just about every HTTP call made to the server, and we already have all the information we need stored in the client to send that information along, I think we could just add a global flag that toggles it on and off, and update the rest of the library to automatically add it when necessary.

So we could add something like use_version_parameter (which defaults to false if we want to start conservatively so initial behavior isn't changed) here: https://github.com/fhir-crucible/fhir_client/blob/master/lib/fhir_client/client.rb#L30

Then update the rest of the library so that the Accept Content-Type headers, and _format param get the right thing fhirVersion appended when necessary.

The API from a user perspective would just be to do:

client.use_version_parameter = true

..or until we decide to make it the default, which probably should wait until a new major version of the library.

Agree / disagree? What do you think @radamson?

[ashankland]

@arscan That makes sense. I can use that workaround to test my changes and get unblocked for now.

Would it make more sense to, rather than have a binary version parameter, let the user specify the version at that top end, and have that reflected as the client is setting version flags, and default to no version parameter if the user doesn't set that value? That could create naming confusion with the fhir_version parameter in https://github.com/fhir-crucible/fhir_client/blob/master/lib/fhir_client/client.rb#L24 (which I assume is the local version the client uses vs. the version of the server it's talking to), but that may be unavoidable under the circumstances.

[radamson]

@arscan awesome level of detail in your response. That makes it really helpful even for me as another reviewer to understand! I agree with your suggestions and was thinking along the same lines so I won't restate it unnecessarily.

I do think we would want use_version_parameter to be a boolean so that we don't cause a mismatch, let alone confusion, between the fhir_version and the use_version_parameter which could cause invalid requests. The client can and should automatically use the right thing without the user having to specify it explicitly because it already has the information necessary to do so. Still, if the user really wants to control it themselves they can override it with the additional_headers setting.

If we were to go this route the first place I would look to implement this is in the fhir_headers method which is responsible for creating the headers used by requests. I don't think it would be too hard to add.

We might also look to how we handle how we already handle the Accept header which varies based on the fhir version of the client as another example where the client determines the correct header based on the version without the user having to be explicit about it (this is a little bit different of a scenario though).