Comments (15)
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.
Related Issues (20)
- Patient Id is not accepting string expecting type object
- Consider tagging submodule "go"? HOT 2
- [Go] How to parse profile HOT 3
- Unmarshaller support for R5 versions HOT 3
- Canonical FHIR Resource Enum HOT 2
- go: module source tree too large HOT 1
- R5 support
- py/ subtree accidentally deleted? HOT 1
- Idiomatic way of dealing with unknown fields? [golang] HOT 3
- No need to require backports.zoneinfo on Python 3.9+ HOT 2
- Python 3.10 + 3.11 support HOT 2
- Python: Upgrade to abseil-py v1.0.0
- [Urgent] Need help with issue using jsonformat NewMarshaller
- Missing GoalAchievementStatus code system
- Go - Task_Output encoded REST message does not match FHIR Store model HOT 4
- FHIR 4.3.0 / R4B support
- protobuf 4.21 support HOT 1
- Building under Windows using MSVC HOT 1
- FR: Exposing a list of maven deps HOT 1
- Example for Generating Protos Using the scripts seems broken
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from fhir.