-
Notifications
You must be signed in to change notification settings - Fork 28
BB2-4233: Ensure _id param is actually parsed for search Patient call… #1425
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
Changes from 3 commits
0ad5f69
2f3a5e2
93bdb72
4f35128
7579701
3c5951c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,11 @@ def build_url(self, resource_router, resource_type, *args, **kwargs): | |
| class SearchViewPatient(SearchView): | ||
| # Class used for Patient resource search view | ||
| required_scopes = ['patient/Patient.read', 'patient/Patient.rs', 'patient/Patient.s'] | ||
| QUERY_SCHEMA = { | ||
| **SearchView.QUERY_SCHEMA, | ||
| '_id': str, | ||
| 'identifier': str | ||
| } | ||
|
Comment on lines
+86
to
+90
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Were these params previously not even being processed at all because this was missing? Also is it true that SearchView.QUERY_SCHEMA already has all the other params we might expect here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As an additional general question, I'm seeing that when invalid query parameters are provided, those are just ignored. Are you seeing any mechanism in these QUERY_SCHEMAs or elsewhere that would lead to a 400 error in cases where invalid params are provided? It would be fair to not worry about that in this PR, but to maybe open a separate ticket to do more query param validation that would return 400s when appropriate (maybe even just on v3 endpoints).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my testing in SBX and prod, for Patient search specifically, lastUpdated and startIndex were both being passed to BFD, though i'm not sure how that is happening as there is no QUERY_SCHEMA for patient search in those environments currently. _id and identifier are not currently being parsed/passed in deployed environments. Also appears that cursor is not being passed, though i'm not 100% sure. Looking into a way to throw 400s for invalid params, if I can find something quickly, i'll include it here and update the ticket. Otherwise, will write up a separate ticket for that. When startIndex is included for v3, a 400 is thrown, so there's appear to be some handling of that either by BFD or HAPI FHIR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it inherits last updated and start index from SearchView.QUERY_SCHEMA, so that seems to make sense. I think you're right that cursor is not used and is not passed through, it is only included in tests in such a way that it would make no difference whether cursor was provided or not. We should remove that from our swagger docs, but not in this ticket probably (edit: since we're removing has:coverage in this PR, maybe removing cursor makes sense too, but if we decide not to, maybe we just open a separate ticket for that). I don't think start Index is currently supported in BFD, so that's probably all that is.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm leaning towards writing a new ticket for throwing 400 for unsupported params before we even ping BFD, but not 100% sure yet. Need to understand a bit more the level of effort required around that. I imagine we could include the cursor removal from swagger docs in that ticket. |
||
|
|
||
| def __init__(self, version=1): | ||
| super().__init__(version) | ||
|
|
@@ -91,8 +96,6 @@ def __init__(self, version=1): | |
| def build_parameters(self, request, *args, **kwargs): | ||
| return { | ||
| '_format': 'application/json+fhir', | ||
| # BB2-4166-TODO : this needs to use self.version to determine fhir_id | ||
| '_id': request.crosswalk.fhir_id(2) | ||
| } | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we add a test case that would've caught the earlier issue, where _id is given first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely, will add it after the release!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was actually able to add it now.