BB2-4233: Ensure _id param is actually parsed for search Patient call…

[JamesDemeryNava]

…s, throwing a 404 if it does not match current session. Remove _has:Coverage from swagger docs

JIRA Ticket:
BB2-4233

What Does This PR Do?

Makes sure the _id and identifier parameters for patient search calls are actually passed to BFD. Before passing the _id param to BFD, we make sure it actually matches the patient id for the current session.

What Should Reviewers Watch For?

• Do read and search patient calls still work as expected?
• Are the _id and identifier parameters now being passed to BFD?
• Are the _id values for patient search calls actually being passed to BFD now, rather than just pulled from the crosswalk table (notes on how to validate this are below)?
• Do we still make sure to at least pass _id for a patient search call if _id and identifier are both not included?

If you're reviewing this PR, please check for these things in particular:

Validation

• Open Postman, use an existing token or authorize a new one
• Run read patient calls for the token:
  • For the actual patient id (get a 200)
  • For a different patient id (get a 404 Not found.)
• Run search patient calls for the token:
  • Pass the actual patient_id in the _id parameter (get a 200)
  • Pass a different patient_id in the _id parameter (get a 404)
  • Test the identifier parameter to ensure it is actually passed and we get a successful result:
    • For BBUser00575 (you can use others, but this is the one I know the mbi hash for), pass the following in the identifier parameter: https://bluebutton.cms.gov/resources/identifier/mbi-hash|36d3906801d908792aa9698284679a6da7d1586f01310354d783304558207f67
    • Ensure a 200 is returned, with the patient data for BBUser00575
  • Test the identifier parameter to ensure it is actually passed and we do notget a successful result:
    • For BBUser00575 (you can use others, but this is the one I know the mbi hash for), pass the following in the identifier parameter: https://bluebutton.cms.gov/resources/identifier/mbi-hash|9993906801d908792aa9698284679a6da7d1586f01310354d783304558207f67
    • Ensure a 200 is returned, but there is no patient data (total attribute should be 0)
• Check http://localhost:8000/docs/openapi#/v2/listSearchViewPatients_v2 to confirm that _has:Coverage is no longer shown in that section

What Security Implications Does This PR Have?

Please indicate if this PR does any of the following:

• Adds any new software dependencies
• Modifies any security controls
• Adds new transmission or storage of data
• Any other changes that could possibly affect security?

• Yes, one or more of the above security implications apply. This PR must not be merged without the ISSO or team
  security engineer's approval.

Any Migrations?

• Yes, there are migrations
  • The migrations should be run PRIOR to the code being deployed
  • The migrations should be run AFTER the code is deployed
  • There is a more complicated migration plan (downtime,
    etc)
• No migrations

[jimmyfagan]

Left a few comments, still need to repeat validation/testing steps, but this seems to be on the right track.

[jimmyfagan]

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?

[jimmyfagan]

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).

[JamesDemeryNava]

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.

[jimmyfagan]

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.

[JamesDemeryNava]

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.

[jimmyfagan]

Looks good, just a couple minor and optional things to address. I'll mark approved, and can give a quick re-review if you do decide to make additional changes based on those comments.

[jimmyfagan]

Maybe we add a test case that would've caught the earlier issue, where _id is given first?

[JamesDemeryNava]

definitely, will add it after the release!

[JamesDemeryNava]

Was actually able to add it now.

[JamesDemeryNava]

will trigger a rerun of the pipeline after the github issue is resolved and we can expect a successful pipeline run.