Cannot convert nested resource from protobuf to JSON about fhir HOT 15 CLOSED

Awesome, great find and great fix. Unfortunately we're not really set up to accept PRs quite yet, so we'll handle on our end, but I really appreciate you digging in and making our job easier :)

from fhir.

FYI the fix to this change has now been pushed to HEAD - we'll cut a release this afternoon that contains this bugfix.

from fhir.

RE: If you give it an Any it doesn't know how to convert to a contained resource and just ends up being empty

This is actually working as intended. The usage of Any here is not to allow an arbitrary resource, there's still only ONE correct proto type to use here, ContainedResource. The only reason we type it as Any rather than ContainedResource is to avoid the dependency loop I described above. If we allow that interdependency via ContainedResource, it took like 15 minutes to build the resources :(

from fhir.

Doing a little more research, I printed the (non FHIR compliant) JSON representation of the protobuf that worked with Any, and got this:

PB JSON:

full_url {
  value: "urn:uuid:c5112836-2038-4ae6-a9b5-89ce3cdff627"
}
resource {
  parameters {
    parameter {
      name {
        value: "nested-resource"
      }
      resource {
        type_url: "type.googleapis.com/google.fhir.r4.core.Parameters"
      }
    }
  }
}

FHIR JSON:
{
  "fullUrl": "urn:uuid:c5112836-2038-4ae6-a9b5-89ce3cdff627",
  "resource": {
    "resourceType": "Parameters",
    "parameter": [
      {
        "name": "nested-resource",
        "resource":
      }
    ]
  }
}

I was able to go directly from FHIR JSON to protobuf and back just fine, and the resource type looks like this instead:

PB JSON:

full_url {
  value: "urn:uuid:a22007a5-3fe1-4fc8-b16c-19bd3ebac92e"
}
resource {
  parameters {
    parameter {
      name {
        value: "nested-resource"
      }
      resource {
        type_url: "type.googleapis.com/google.fhir.r4.core.ContainedResource"
        value: "\262\006\000"
      }
    }
  }
}

FHIR JSON:
{
  "fullUrl": "urn:uuid:a22007a5-3fe1-4fc8-b16c-19bd3ebac92e",
  "resource": {
    "resourceType": "Parameters",
    "parameter": [
      {
        "name": "nested-resource",
        "resource": {
          "resourceType": "Parameters",

        }
      }
    ]
  }
}

code snippet:

def convert_from_json_to_pb_and_back():
    entry_dict = {
        "fullUrl": f"urn:uuid:{uuid4()}",
        "resource": {
            "resourceType": "Parameters",
            "parameter": [
                {"name": "nested-resource", "resource": {"resourceType": "Parameters"}}
            ],
        },
    }

    entry_json = json.dumps(entry_dict)
    entry_pb = json_format.json_fhir_string_to_proto(entry_json, Bundle.Entry)

    print(entry_pb)
    print(json_format.pretty_print_fhir_to_json_string(entry_pb))

So even though the original protobuf seemed to work OK, it looks like it should have been a ContainedResource packed into the Any. I'm not sure why it was Any though, maybe it should be ContainedResource?

from fhir.

Confirmed, packing ContainedResource worked:

raw_nested_resource = Parameters()

packed_nested_resource = AnyPB()
packed_nested_resource.Pack(ContainedResource(parameters=raw_nested_resource))

Converting packed nested resource to json:
full_url {
  value: "urn:uuid:ce0b4f84-c8bf-499f-8c0e-3e423db73169"
}
resource {
  parameters {
    parameter {
      name {
        value: "nested-resource"
      }
      resource {
        type_url: "type.googleapis.com/google.fhir.r4.core.ContainedResource"
        value: "\262\006\000"
      }
    }
  }
}

{
  "fullUrl": "urn:uuid:ce0b4f84-c8bf-499f-8c0e-3e423db73169",
  "resource": {
    "resourceType": "Parameters",
    "parameter": [
      {
        "name": "nested-resource",
        "resource": {
          "resourceType": "Parameters",

        }
      }
    ]
  }
}

This might still be considered a bug though with the protobuf types (e.g. Parameters.Parameter.resource should be ContainedResource, not Any). Let me know what you think

from fhir.

Ah, actually it's still invalid JSON - notice the trailing comma in "resourceType": "Parameters",, both in the pb->json as well as the json->pb->json conversions.

This could be because the Parameters type is empty, but it's unclear to me if that's valid or not - Parameters only requires a list of parameter, which can be empty, but is a [] still required or can it be null/undefined/not included? It's also interesting that the fhir->pb->fhir passed validation if the [] is truly required.

http://hl7.org/fhir/parameters.html

from fhir.

And with some further testing, using Parameters(parameter=[]) or { "resourceType": "Parameters", "parameter": [] } produces the same invalid JSON, without a parameter field.

from fhir.

I believe this should fix it:

diff --git a/py/google/fhir/json_format/_json_printer.py b/py/google/fhir/json_format/_json_printer.py
index ea224565..19f1e617 100644
--- a/py/google/fhir/json_format/_json_printer.py
+++ b/py/google/fhir/json_format/_json_printer.py
@@ -364,14 +364,18 @@ class JsonPrinter:
     """Prints the representation of message."""
     self.generator.open_json_object()
 
+    set_fields = msg.ListFields()
+
     # Add the resource type preamble if necessary
     if (annotation_utils.is_resource(msg) and
         self.json_format == _FhirJsonFormat.PURE):
-      self.generator.add_field('resourceType', f'"{msg.DESCRIPTOR.name}",')
-      self.generator.add_newline()
+      self.generator.add_field('resourceType', f'"{msg.DESCRIPTOR.name}"')
+
+      if len(set_fields):
+        self.generator.push(',')
+        self.generator.add_newline()
 
     # print all fields
-    set_fields = msg.ListFields()
     for (i, (set_field, value)) in enumerate(set_fields):
       if (annotation_utils.is_choice_type_field(set_field) and
           self.json_format == _FhirJsonFormat.PURE):

I hope you don't mind I didn't make a PR because wrapping my head around the test suite and getting things to pass will take a bit more time, but if there's no movement on this for a while I'll look into doing a complete PR, maybe next week.

from fhir.

Sounds good. And actually that diff should just fix the JSON parsing, but I still wonder if resource should be of type ContainedResource rather than Any... that might be a breaking change though.

from fhir.

So, short story:
In STU3, all Resource fields were typed as ContainedResources. But, since every resource has a contained field of type Resource, and protos don't have C++-like header files, this means that every resource depends on every other resource, there's a massive circular dependency, and the end result is all proto compilation is done in serial rather than in parallel, in resources.proto. This worked out OK in STU3 but was slow. In R4, it was untenable. So, we made the decision to inline ContainedResource fields as "Any" so that each individual resource can be compiled independent of other resources, and in parallel. So in general, anytime you see an Any in R4, it's a ContainedResource.

from fhir.

OK, that's interesting. So it sounds like, aside from the bug I found a fix for, there's still a bug in converting to JSON (If you give it an Any it doesn't know how to convert to a contained resource and just ends up being empty), and another one converting to proto (it converts the contained resource to ContainedResource instead of Any).

from fhir.

Ah, that's awesome, thanks for fixing that! OK, thanks for the clarification on Any vs ContainedResource - that makes sense.

from fhir.

@nickgeorge I was looking into updating to get this bug fix, but it looks like 0.6.2 is still the latest version, which still had that bug in it. Was there something holding back the release?

from fhir.

Uh sorry, we wanted to finalize some testing procedures for the pypi distribution so got delayed a bit. Shld be today if things go according to plan...

from fhir.

Thanks, no worries. I'll keep an eye out

from fhir.