New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extension for contained resource Reference does not serialize to JSON or XML properly. #327

Closed
fw060 opened this Issue Apr 6, 2016 · 13 comments

Comments

Projects
None yet
2 participants
@fw060

fw060 commented Apr 6, 2016

See the following test code:

@ResourceDef(name="Patient")
public class TestPatient extends Patient
{

    @Child(name = "testCondition")
    @Extension(url="testCondition", definedLocally=true, isModifier=false)
    private List<Reference> testConditions = null;

    public void setCondition(List<Reference> ref)
    {
        this.testConditions = ref;
    }

    public List<Reference> getConditions()
    {
        return this.testConditions;
    }
}

PatientProvider class:

    @Read()
    public Patient read(@IdParam IdType theId) {


        TestPatient patient = new TestPatient();
        patient.setBirthDate(new Date());

        List<Reference> conditions = new ArrayList<>();
        conditions.add(new Reference(new Condition()));
        patient.setCondition(conditions);


        return patient;
    }


Test result is :

{
  "resourceType": "Patient",
  "extension": [
    {
      "url": "testCondition",
      "valueReference": {}
    }
  ],
  "contained": [
    {
      "resourceType": "Condition",
      "id": "1"
    }
  ],
  "birthDate": "2016-04-06"
}
@fw060

This comment has been minimized.

Show comment
Hide comment
@fw060

fw060 Apr 6, 2016

The following code fixed it, but the code can use more optimization. The code seems try to change valueResourceReference to valueResource, but now it should be just "valueReference" with DSTU2 and DSTU3. If this is no longer necessary, the code can be simplified, but I don't have the full context.

diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeChildDeclaredExtensionDefinition.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeChildDeclaredExtensionDefinition.java
index 60a2786..20a5c74 100644
--- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeChildDeclaredExtensionDefinition.java
+++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeChildDeclaredExtensionDefinition.java
@@ -159,12 +159,12 @@ public class RuntimeChildDeclaredExtensionDefinition extends BaseRuntimeDeclared
                BaseRuntimeElementDefinition<?> elementDef = theClassToElementDefinitions.get(myChildType);
                if (elementDef instanceof RuntimePrimitiveDatatypeDefinition || elementDef instanceof RuntimeCompositeDatatypeDefinition) {
                        myDatatypeChildName = "value" + elementDef.getName().substring(0, 1).toUpperCase() + elementDef.getName().substring(1);
-                       if ("valueResourceReference".equals(myDatatypeChildName)) {
+                       if ("valueReference".equals(myDatatypeChildName)) {
                                // Per one of the examples here: http://hl7.org/implement/standards/fhir/extensibility.html#extension
                                myDatatypeChildName = "valueResource";
                                List<Class<? extends IBaseResource>> types = new ArrayList<Class<? extends IBaseResource>>();
                                types.add(IResource.class);
-                               myChildDef = new RuntimeResourceReferenceDefinition("valueResource", types, false);
+                               myChildDef = new RuntimeResourceReferenceDefinition("valueReference", types, false);
                        } else {
                                myChildDef = elementDef;
                        }

Test result is:

{
  "resourceType": "Patient",
  "extension": [
    {
      "url": "testCondition",
      "valueResource": {
        "reference": "#1"
      }
    }
  ],
  "contained": [
    {
      "resourceType": "Condition",
      "id": "1"
    }
  ],
  "birthDate": "2016-04-06"
}

fw060 commented Apr 6, 2016

The following code fixed it, but the code can use more optimization. The code seems try to change valueResourceReference to valueResource, but now it should be just "valueReference" with DSTU2 and DSTU3. If this is no longer necessary, the code can be simplified, but I don't have the full context.

diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeChildDeclaredExtensionDefinition.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeChildDeclaredExtensionDefinition.java
index 60a2786..20a5c74 100644
--- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeChildDeclaredExtensionDefinition.java
+++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/RuntimeChildDeclaredExtensionDefinition.java
@@ -159,12 +159,12 @@ public class RuntimeChildDeclaredExtensionDefinition extends BaseRuntimeDeclared
                BaseRuntimeElementDefinition<?> elementDef = theClassToElementDefinitions.get(myChildType);
                if (elementDef instanceof RuntimePrimitiveDatatypeDefinition || elementDef instanceof RuntimeCompositeDatatypeDefinition) {
                        myDatatypeChildName = "value" + elementDef.getName().substring(0, 1).toUpperCase() + elementDef.getName().substring(1);
-                       if ("valueResourceReference".equals(myDatatypeChildName)) {
+                       if ("valueReference".equals(myDatatypeChildName)) {
                                // Per one of the examples here: http://hl7.org/implement/standards/fhir/extensibility.html#extension
                                myDatatypeChildName = "valueResource";
                                List<Class<? extends IBaseResource>> types = new ArrayList<Class<? extends IBaseResource>>();
                                types.add(IResource.class);
-                               myChildDef = new RuntimeResourceReferenceDefinition("valueResource", types, false);
+                               myChildDef = new RuntimeResourceReferenceDefinition("valueReference", types, false);
                        } else {
                                myChildDef = elementDef;
                        }

Test result is:

{
  "resourceType": "Patient",
  "extension": [
    {
      "url": "testCondition",
      "valueResource": {
        "reference": "#1"
      }
    }
  ],
  "contained": [
    {
      "resourceType": "Condition",
      "id": "1"
    }
  ],
  "birthDate": "2016-04-06"
}
@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Apr 14, 2016

Owner

This was fixed as a result of a recent checkin, and should work correctly as a part of 1.5. Are you able to verify using a 1.5 snapshot build?

Owner

jamesagnew commented Apr 14, 2016

This was fixed as a result of a recent checkin, and should work correctly as a part of 1.5. Are you able to verify using a 1.5 snapshot build?

@fw060

This comment has been minimized.

Show comment
Hide comment
@fw060

fw060 Apr 25, 2016

Sorry I wasn't verify this yet. We are not fully committed to use 1.5 yet. Which check in are you referring to?

Thanks,
Fei

fw060 commented Apr 25, 2016

Sorry I wasn't verify this yet. We are not fully committed to use 1.5 yet. Which check in are you referring to?

Thanks,
Fei

@fw060

This comment has been minimized.

Show comment
Hide comment
@fw060

fw060 Oct 12, 2016

We upgraded to 1.6 and unfortunately this is still not fixed.

fw060 commented Oct 12, 2016

We upgraded to 1.6 and unfortunately this is still not fixed.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Oct 12, 2016

Owner

Hi Fei,

Is it possible you have an old version of HAPI on your classpath?

We have your test case embedded basically verbatim in a unit test and it seems to pass. See XmlParserDstu3Test.java's testEncodeExtensionWithContainedResource method

Do you see any obvious differences between this test and your issue?

Owner

jamesagnew commented Oct 12, 2016

Hi Fei,

Is it possible you have an old version of HAPI on your classpath?

We have your test case embedded basically verbatim in a unit test and it seems to pass. See XmlParserDstu3Test.java's testEncodeExtensionWithContainedResource method

Do you see any obvious differences between this test and your issue?

@fw060

This comment has been minimized.

Show comment
Hide comment
@fw060

fw060 Oct 14, 2016

More tests shows that XML format does work, but not JSON format. I will work on a new fix. At the same time if you have any suggestion, please let me know.

fw060 commented Oct 14, 2016

More tests shows that XML format does work, but not JSON format. I will work on a new fix. At the same time if you have any suggestion, please let me know.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Oct 14, 2016

Owner

Ahh I just tried it in JSON and I see it too

Owner

jamesagnew commented Oct 14, 2016

Ahh I just tried it in JSON and I see it too

@fw060

This comment has been minimized.

Show comment
Hide comment
@fw060

fw060 Oct 14, 2016

yes, unfortunately my original fix no longer works. I am trying to figure out the new fix, but hopefully you have some quick insight :)

fw060 commented Oct 14, 2016

yes, unfortunately my original fix no longer works. I am trying to figure out the new fix, but hopefully you have some quick insight :)

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Oct 14, 2016

Owner

Good news actually. I figured it out... Fix coming up soon :)

Owner

jamesagnew commented Oct 14, 2016

Good news actually. I figured it out... Fix coming up soon :)

@fw060

This comment has been minimized.

Show comment
Hide comment
@fw060

fw060 Oct 14, 2016

Awesome!!!

fw060 commented Oct 14, 2016

Awesome!!!

@fw060

This comment has been minimized.

Show comment
Hide comment
@fw060

fw060 Oct 14, 2016

Another related question, is there any chance we can get a patch release 1.6.1? A strategy to have a patch release would greatly help people who use the library in production. Otherwise, we have to create a customized version :(

fw060 commented Oct 14, 2016

Another related question, is there any chance we can get a patch release 1.6.1? A strategy to have a patch release would greatly help people who use the library in production. Otherwise, we have to create a customized version :(

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Oct 14, 2016

Owner

Unfortunately we just don't have the capacity to do backported patches.. The backlog of fixes and enhancements on trunk is already more than we seem to be able to keep up with these days given how fast FHIR itself is changing :(

The change will be in a single commit though, so you should be able to cherry-pick and apply it to a checkout of any arbitrary version in a fork if you need to.

Owner

jamesagnew commented Oct 14, 2016

Unfortunately we just don't have the capacity to do backported patches.. The backlog of fixes and enhancements on trunk is already more than we seem to be able to keep up with these days given how fast FHIR itself is changing :(

The change will be in a single commit though, so you should be able to cherry-pick and apply it to a checkout of any arbitrary version in a fork if you need to.

@fw060

This comment has been minimized.

Show comment
Hide comment
@fw060

fw060 Oct 17, 2016

Thanks James! The signature of preProcessValues() changed, so I have to remove the last argument, but after that, your fix works for 1.6 too!

I understand your challenge on keeping up the FHIR spec change. Hopefully STU3 will be stable soon. That would benefit everyone :)

Again, thank a lot for the great work!

fw060 commented Oct 17, 2016

Thanks James! The signature of preProcessValues() changed, so I have to remove the last argument, but after that, your fix works for 1.6 too!

I understand your challenge on keeping up the FHIR spec change. Hopefully STU3 will be stable soon. That would benefit everyone :)

Again, thank a lot for the great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment