Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer header processing uses ';' instead of ',' and should support multiple Prefer headers #3318

Closed
kmbarton423 opened this issue Feb 9, 2022 · 11 comments
Assignees
Labels
bug Something isn't working documentation Improvements or additions to documentation P2 Priority 2 - Should Have

Comments

@kmbarton423
Copy link
Contributor

kmbarton423 commented Feb 9, 2022

Working with the current pre-release of FHIR on main branch
Per the conformance documentation: https://ibm.github.io/FHIR/Conformance

_The _sort, _count, _summary, _elements, and total parameters may each only be specified once in a search. In lenient mode, only the first occurrence of each of these parameters is used; additional occurrences are ignored.

Received an error when trying to use multiple occurrences of the _sort, _count, _summary, _elements, and _total when setting Prefer handling=lenient via the header. See attached screenshot.

UPDATE: the issue only occurs when either multiple Prefer headers are set or when a single Prefer header contains multiple components. currently the code tries to split the components of the Prefer header by ; but the relevant rfc states it should be a , instead.

To Reproduce
Default or set 'true' fhir server config parameter 'fhirServer/core/allowClientHandlingPref'

Set the following headers for all requests (in this order):

  • Prefer: return=representation
  • Prefer: handling=lenient

Attempt the following:
GET [base]/Measure?_sort=_lastUpdated&_sort=status
GET [base]/Measure?_count=1&_count=1000
GET [base]/Measure?_summary=text&_summary=count
GET [base]/Measure?_elements=status&_elements=name
GET [base]/Measure?_total=none&_total=accurate

Expected behavior
Expected only the first occurrence of the parameters to be used successfully as stated above.
2022-02-09 13_41_10-Insomnia - Localhost – search Measure sort

QA
Test "Prefer" header with "handling=" and "return=" tokens in various permutations.

@lmsurpre
Copy link
Member

lmsurpre commented Feb 14, 2022

we're pretty sure it used to work as documented.
investigate to make sure what we understand whats happening, confirming:

  1. handling=lenient is working as intended
  2. code seems to be logical / make sense

if both those are working as implemented, it should be a doc-only update

@lmsurpre lmsurpre added the P2 Priority 2 - Should Have label Feb 14, 2022
@tbieste
Copy link
Contributor

tbieste commented Feb 14, 2022

I just tried to reproduce this, and it worked correctly for me. @kmbarton423, I wonder if it wasn't actually running in lenient mode, which would happen if the fhirServer/core/allowClientHandlingPref configuration property was set to false. Can you verify that?

@prb112
Copy link
Contributor

prb112 commented Feb 14, 2022

She's specifically trying some of the more restrictive search types. _summary or _count
We verified she had that correct.

@tbieste
Copy link
Contributor

tbieste commented Feb 14, 2022

Ah, it looks like this is working as expected. Since only the first "Prefer" HTTP header is honored by the IBM FHIR Server, the request is performed using the default of handling=strict, which is why the error is returned.

The way to specify multiple values for the "Prefer" HTTP header is defined in the IBM FHIR Conformance Guide: https://ibm.github.io/FHIR/Conformance#http-headers
The return= and handling= values need to be within a single "Prefer" HTTP header.

@kmbarton423
Copy link
Contributor Author

Confirmed using the single header with ";" works as expected. Should ',' separator work as well?

@tbieste
Copy link
Contributor

tbieste commented Feb 14, 2022

Based on https://datatracker.ietf.org/doc/html/rfc7240:

It seems like since handling= and return= are different tokens (i.e. one is not a parameter of the other). They should actually be separated by ,, instead of ;. In addition, multiple Prefer headers should be allowed.

tbieste added a commit that referenced this issue Feb 14, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
tbieste added a commit that referenced this issue Feb 15, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
@tbieste tbieste added the documentation Improvements or additions to documentation label Feb 15, 2022
tbieste added a commit that referenced this issue Feb 15, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
tbieste added a commit that referenced this issue Feb 15, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
tbieste added a commit that referenced this issue Feb 15, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
tbieste added a commit that referenced this issue Feb 15, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
tbieste added a commit that referenced this issue Feb 15, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
tbieste added a commit that referenced this issue Feb 15, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
tbieste added a commit that referenced this issue Feb 15, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
tbieste added a commit that referenced this issue Feb 15, 2022
Issue #3318 - Improve support for Prefer HTTP header
@lmsurpre lmsurpre changed the title passing multiple occurrences of search result parms in lenient mode causes an error Prefer header processing uses ';' instead of ',' and should support multiple Prefer headers Feb 16, 2022
@d0roppe
Copy link
Collaborator

d0roppe commented Feb 16, 2022

I am now seeing that it does not matter what I send on the Prefer header, I usually send return=OperationOutcome. After pulling the latest code it seems now the default return is set to minimal and I don't get the information back on the API calls I am expecting. Like $healthcheck does not return any data. just a 200 and a header.

tbieste added a commit that referenced this issue Feb 16, 2022
Signed-off-by: Troy Biesterfeld <tbieste@us.ibm.com>
@tbieste
Copy link
Contributor

tbieste commented Feb 16, 2022

Interesting, the case of the "Prefer" header name appears to be different (perhaps between liberty versions, or something else different between Dag's and my machines) when retrieved by the REST servlet filter, regardless of what is in the original request.
To resolve this, I updated the code to be case-insensitive regards to HTTP header name.

tbieste added a commit that referenced this issue Feb 16, 2022
Issue #3318 - Case insensitive check of HTTP header name
@tbieste
Copy link
Contributor

tbieste commented Feb 16, 2022

Upon upgrading to the latest Liberty version, I received the same behavior as Dag. So both of us were able to verify that PR #3356 addressed this issue we were seeing today.

@d0roppe
Copy link
Collaborator

d0roppe commented Feb 16, 2022

After the latest code changes the issue I reported, it is now working as expected.

@kmbarton423
Copy link
Contributor Author

Confirmed mixed case Prefer header, multiple Prefer headers , and Prefer header with multiple tokens separated by a comma.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation P2 Priority 2 - Should Have
Projects
None yet
Development

No branches or pull requests

5 participants