Rob/1291 write request and response handling functions for message parser payloads #1324
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1324 +/- ##
=======================================
Coverage 96.94% 96.94%
=======================================
Files 48 48
Lines 3047 3047
=======================================
Hits 2954 2954
Misses 93 93
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Thanks for getting this started and uncovering the messiness associated with handling requests and responses from /parse_message and fhir_to_phdc in the same functions. I think we should break this out into two functions for each endpoint (see comments below). Let me know if you'd like to discuss further.
| @@ -65,3 +65,81 @@ def unpack_fhir_converter_response(response: Response) -> Tuple[int, str | dict] | |||
| else: | |||
| fhir_msg = converter_response.get("FhirResource") | |||
| return (converter_response.status_code, fhir_msg) | |||
|
|
|||
|
|
|||
| def build_message_parser_request( | |||
There was a problem hiding this comment.
I think what we should do here is have a separate functions for the two endpoints on the message parser. We could have a build_message_parse_message_request and a build_convert_fhir_to_phdc_request. In call_apis based on the name of the step we'll be able to chose the correct handler functions.
There was a problem hiding this comment.
Additionally, every one of the request builder functions need to have the same signature. They should be of the form ( input_msg: str, orchestration_request: OrchestrationRequest ) -> dict. This will let us set message to input_msg.
| if endpoint_type == "/parse_message": | ||
| return { | ||
| "message_format": "fhir", | ||
| "parsing_schema_name": orchestration_request.get("parsing_schema_name"), | ||
| "credential_manager": "azure", | ||
| } | ||
| elif endpoint_type == "/fhir_to_phdc": | ||
| return {"phdc_report_type": "case_report"} |
There was a problem hiding this comment.
We need to include the actual data that will be parsed or converted to PHDC in these requests. For both endpoints this is done by including a message key in the dict.
| return {"phdc_report_type": "case_report"} | ||
|
|
||
|
|
||
| def unpack_message_parser_response(response: Response) -> Tuple[int, str | dict]: |
There was a problem hiding this comment.
By having dedicated unpacking functions for each of the two relevant parse message endpoints we should be able to simplify this quite a bit.
…-functions-for-message-parser-payloads
…-functions-for-message-parser-payloads
| def build_message_parser_message_request( | ||
| input_msg: str, | ||
| orchestration_request: OrchestrationRequest, | ||
| ) -> dict: |
There was a problem hiding this comment.
I think the idea is to put the params for the workflow directly into the function signature. The orchestration service doesn't have an overarching params list it maintains for each service, but with our new workflow files, we do pair those with each step of the workflow, so I think we should just assume the call_apis function will hand off the appropriate dict to the function. Here's how I structured mine:
def build_valiation_request(
input_msg: str,
orchestration_request: OrchestrationRequest,
workflow_params: dict | None = None,
) -> dict:
There was a problem hiding this comment.
This prevents us from having to alter the Orchestration Service's base model before we're ready to commit those changes (we really don't want to add or take away any params from it until we can do so all at once, so we don't disrupt the streamline team)
| # Code idea for future state where we need to know report type | ||
| report_type = orchestration_request.get("params").get("phdc_report_type") |
There was a problem hiding this comment.
With the params moved into the signature, we can delete the comment here and just directly look up phdc report type.
| parsed message created by the service. | ||
| """ | ||
| try: | ||
| converter_response = response.json().get("response") |
There was a problem hiding this comment.
The reason we need a .get("response") accessor in the fhir converter handlers is because of the converter itself. If you look at line 138 of fhir-converter/app/service.py, you'll see the result object is actually encapsulated in another dictionary. None of our other services do this (it's because of the way the command line tool returns 🙄 ), so we don't need the extra .get("response") here. Just calling response.json() is sufficient.
There was a problem hiding this comment.
I'm not sure I follow. without the .get("response") I get this:
{
"response": {
"status_code": 200,
"FhirResource": {
"resourceType": "Bundle",
"identifier": {
"value": "a very contrived FHIR bundle"
},
"entry": [
{
"resource": {
"resourceType": "Organization",
"id": "some-org-we-dont-care-about"
}
},
{
"resource": {
"resourceType": "Patient",
"id": "some-uuid",
"identifier": [
{
"value": "123456",
"type": {
"coding": [
{
"code": "MR",
"system": "http://terminology.hl7.org/CodeSystem/v2-0203",
"display": "Medical record number"
}
]
},
"system": "urn...no idea"
}
],
"name": [
{
"family": "doe",
"given": [
"John ",
" Danger "
],
"use": "official"
}
],
"birthDate": "1983-02-01",
"gender": "female",
"address": [
{
"line": [
"123 Fake St",
"Unit #F"
],
"BuildingNumber": "123",
"city": "Faketon",
"state": "NY",
"postalCode": "10001-0001",
"country": "USA",
"use": "home"
}
],
"telecom": [
{
"use": "home",
"system": "phone",
"value": "123-456-7890"
},
{
"value": "[email protected]",
"system": "email"
}
]
},
"request": {
"method": "GET",
"url": "testing for entry with no resource"
}
}
]
}
}
}so we would still need to access "response", unless I'm thinking of it wrongly.
There was a problem hiding this comment.
nvm, I see what you're saying. I've gotten the FHIR and message parser mixed up 🤦🏽
There was a problem hiding this comment.
updating!
There was a problem hiding this comment.
updated
| converter_response = response.json().get("response") | ||
| status_code = converter_response.get("status_code", response.status_code) | ||
| if status_code != 200: | ||
| error_message = converter_response.get("text", "Unknown error") |
There was a problem hiding this comment.
I think the text of the message will be in the body of the response object proper, rather than the JSON serialization of it, so this should be error_message = response.text (which python's Requests module assures us will always exist so no need to use get)
There was a problem hiding this comment.
updated
| f"Message Parser request failed: {error_message}", | ||
| ) | ||
| else: | ||
| parsed_message = converter_response.get("FhirResource") |
There was a problem hiding this comment.
Does the message parser output actually have a FhirResource key?
There was a problem hiding this comment.
erg, no. I got mixed up here that we need to be looking at a sample message_parser (which has "message" and "parsed_values" as keys. Will update this test.
| error_message = ( | ||
| response.json().get("response", {}).get("text", "Unknown error") | ||
| ) |
There was a problem hiding this comment.
Same caveats as above, I don't think we need the extra .get("response") because of the object structure, and .text will be directly on the response object
| params: Optional[dict] = Field( | ||
| description="A dictionary of the endpoint-specific parameters (if necessary).", | ||
| default=None, | ||
| ) |
There was a problem hiding this comment.
We want this to be in the function signature rather than the request, since the params will change with each step of the workflow
There was a problem hiding this comment.
removed
…-functions-for-message-parser-payloads
|
updated error handling for unpacking after consulting with @DanPaseltiner , for context, @bamader. Curious your thoughts on re-review. |
There was a problem hiding this comment.
LGTM! Thanks for getting all those fixes in! I think the error logic being moved outside of the handler functions is part of the pattern we want. We want call_apis to be responsible for handling those as much as possible, as part of our strategy of percolating the error as high up the call chain as it'll go.
robertandremitchell
deleted the
rob/1291-write-request-and-response-handling-functions-for-message-parser-payloads
branch
…rser payloads (#1324) * adding message parser * testing tests * updating tests * undoing weird merge removal * splitting up phdc vs json message_parser * splitting unpack functions * adding error handling to last test * add params to OrchestrationRequest * moving params to handlers * updating message parser parsing * Updated unpack message parser function and changed names * [pre-commit.ci] auto fixes from pre-commit hooks * updating tests, updating function names --------- Co-authored-by: DanPaseltiner <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
PULL REQUEST
Summary
This adds the handler functions for message_parser.
I'm not entirely sure I'm handling this right. Right now, at least, the only we could tell the difference between whether we're using PHDC or one of the other message_parser's is to compare the endpoints. Are there arguments we should instead add to the
OrchestrationRequest?I couldn't think of a way without specifying the message_parser's json, but not sure.
Also also, I think I handled the json vs XML aspect of this correctly, but curious if you think this should be handled elsewhere and/or more elegantly.
Related Issue
Fixes #1291