Python json_fhir_string_to_proto raises wrong exception type

[gitpushdashf]

Hi everyone,

According to https://github.com/google/fhir/blame/master/py/google/fhir/r4/json_format.py#L82,L83 json_fhir_string_to_proto() should raise InvalidFhirError.

In this example, it raises ValueError instead. I have not tested with the latest google-fhir but am suspecting the results may be the same.

import json

from google.fhir.fhir_errors import InvalidFhirError
from google.fhir.r4.json_format import json_fhir_string_to_proto
from proto.google.fhir.proto.r4.core.resources.bundle_and_contained_resource_pb2 import (  # noqa: E501
    Bundle,
)

INVALID_BUNDLE_DICT = {
    "resourceType": "Bundle",
    "type": "not a transaction",
    "entry": [
        {
            "fullUrl": "urn:uuid:85e52038-4d69-50e9-9e46-e379b8d830af",
            "resource": {
                "resourceType": "Patient",
                "id": "85e52038-4d69-50e9-9e46-e379b8d830af",
                "name": [
                    {"use": "official", "family": "Simpson", "given": ["Homer", "Jay"]}
                ],
            },
        }
    ],
}

try:
    json_fhir_string_to_proto(json.dumps(INVALID_BUNDLE_DICT), Bundle)
except InvalidFhirError:
    print("Excepts as expected.")

print("Success.")

Let me know if anything is unclear. The error is under the type field. Happy to help dig in further if need be.

Thank you for your time.

[Cam2337]

Hey @gitpushdashf 👋

Within codes, we should be raising an instance of fhir_errors.InvalidFhirError, which would be more informative to the user that their provided code string cannot convert to an accompanying FHIRProto EnumValueDescriptor. Making this change properly throws an instance of fhir_errors.InvalidFhirError for your provided example (thank you for a great example btw 🙂). I merged this internally, so expect this fix in the next release.

As an aside, I've also filed a bug internally to review our error handling for google-fhir. I suspect there might be other places where a similar change should take place. I'm hoping to strike a good balance between raising ValueErrors (and other built-in exceptions) where it makes sense, and only relying on propagating fhir_errors.InvalidFhirError, where it's useful to let a caller know that "your JSON is correct, but it's not valid FHIR".

Thank you again!

[gitpushdashf]

You're welcome. Thank you for looking into this!