From 15a57242aa583e8df2358947984ac0ed96a34180 Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Sat, 24 Jan 2015 07:42:14 +0100 Subject: [PATCH] Fix #91 - Unable to add more than two extensions with unknown orders to a custom resource --- .../ca/uhn/fhir/context/ModelScanner.java | 66 +++++++++---------- .../ca/uhn/fhir/parser/JsonParserTest.java | 4 +- ...ions.java => MyPatientWithExtensions.java} | 2 +- .../MyPatientWithUnorderedExtensions.java | 65 ++++++++++++++++++ .../ca/uhn/fhir/parser/XmlParserTest.java | 28 +++++++- src/changes/changes.xml | 6 ++ 6 files changed, 131 insertions(+), 40 deletions(-) rename hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/{MyObservationWithExtensions.java => MyPatientWithExtensions.java} (97%) create mode 100644 hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyPatientWithUnorderedExtensions.java diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/ModelScanner.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/ModelScanner.java index 54fb1143951..071f3f52dfb 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/ModelScanner.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/context/ModelScanner.java @@ -117,8 +117,7 @@ class ModelScanner { init(null, new HashSet>(theResourceTypes)); } - ModelScanner(FhirContext theContext, Map, BaseRuntimeElementDefinition> theExistingDefinitions, Collection> theResourceTypes) - throws ConfigurationException { + ModelScanner(FhirContext theContext, Map, BaseRuntimeElementDefinition> theExistingDefinitions, Collection> theResourceTypes) throws ConfigurationException { myContext = theContext; Set> toScan; if (theResourceTypes != null) { @@ -274,8 +273,7 @@ class ModelScanner { ResourceDef resourceDefinition = pullAnnotation(theClass, ResourceDef.class); if (resourceDefinition != null) { if (!IResource.class.isAssignableFrom(theClass)) { - throw new ConfigurationException("Resource type contains a @" + ResourceDef.class.getSimpleName() + " annotation but does not implement " + IResource.class.getCanonicalName() + ": " - + theClass.getCanonicalName()); + throw new ConfigurationException("Resource type contains a @" + ResourceDef.class.getSimpleName() + " annotation but does not implement " + IResource.class.getCanonicalName() + ": " + theClass.getCanonicalName()); } @SuppressWarnings("unchecked") Class resClass = (Class) theClass; @@ -293,8 +291,7 @@ class ModelScanner { Class> resClass = (Class>) theClass; scanPrimitiveDatatype(resClass, datatypeDefinition); } else { - throw new ConfigurationException("Resource type contains a @" + DatatypeDef.class.getSimpleName() + " annotation but does not implement " + IDatatype.class.getCanonicalName() + ": " - + theClass.getCanonicalName()); + throw new ConfigurationException("Resource type contains a @" + DatatypeDef.class.getSimpleName() + " annotation but does not implement " + IDatatype.class.getCanonicalName() + ": " + theClass.getCanonicalName()); } } @@ -305,8 +302,7 @@ class ModelScanner { Class blockClass = (Class) theClass; scanBlock(blockClass, blockDefinition); } else { - throw new ConfigurationException("Type contains a @" + Block.class.getSimpleName() + " annotation but does not implement " + IResourceBlock.class.getCanonicalName() + ": " - + theClass.getCanonicalName()); + throw new ConfigurationException("Type contains a @" + Block.class.getSimpleName() + " annotation but does not implement " + IResourceBlock.class.getCanonicalName() + ": " + theClass.getCanonicalName()); } } @@ -397,8 +393,7 @@ class ModelScanner { } @SuppressWarnings("unchecked") - private void scanCompositeElementForChildren(Class theClass, Set elementNames, TreeMap theOrderToElementDef, - TreeMap theOrderToExtensionDef) { + private void scanCompositeElementForChildren(Class theClass, Set elementNames, TreeMap theOrderToElementDef, TreeMap theOrderToExtensionDef) { int baseElementOrder = theOrderToElementDef.isEmpty() ? 0 : theOrderToElementDef.lastEntry().getKey() + 1; for (Field next : theClass.getDeclaredFields()) { @@ -440,8 +435,8 @@ class ModelScanner { } } if (order == Child.REPLACE_PARENT) { - throw new ConfigurationException("Field " + next.getName() + "' on target type " + theClass.getSimpleName() + " has order() of REPLACE_PARENT (" + Child.REPLACE_PARENT - + ") but no parent element with extension URL " + extensionAttr.url() + " could be found on type " + next.getDeclaringClass().getSimpleName()); + throw new ConfigurationException("Field " + next.getName() + "' on target type " + theClass.getSimpleName() + " has order() of REPLACE_PARENT (" + Child.REPLACE_PARENT + ") but no parent element with extension URL " + extensionAttr.url() + " could be found on type " + + next.getDeclaringClass().getSimpleName()); } } else { @@ -456,8 +451,8 @@ class ModelScanner { } } if (order == Child.REPLACE_PARENT) { - throw new ConfigurationException("Field " + next.getName() + "' on target type " + theClass.getSimpleName() + " has order() of REPLACE_PARENT (" + Child.REPLACE_PARENT - + ") but no parent element with name " + elementName + " could be found on type " + next.getDeclaringClass().getSimpleName()); + throw new ConfigurationException("Field " + next.getName() + "' on target type " + theClass.getSimpleName() + " has order() of REPLACE_PARENT (" + Child.REPLACE_PARENT + ") but no parent element with name " + elementName + " could be found on type " + + next.getDeclaringClass().getSimpleName()); } } @@ -471,11 +466,16 @@ class ModelScanner { } int min = childAnnotation.min(); int max = childAnnotation.max(); + /* - * Anything that's marked as unknown is given a new ID that is <0 so that it doesn't conflict with any given IDs and can be figured out later + * Anything that's marked as unknown is given a new ID that is <0 so that it doesn't conflict with any given + * IDs and can be figured out later */ - while (order == Child.ORDER_UNKNOWN && orderMap.containsKey(order)) { - order--; + if (order == Child.ORDER_UNKNOWN) { + order = Integer.MIN_VALUE; + while (orderMap.containsKey(order)) { + order++; + } } List> choiceTypes = new ArrayList>(); @@ -484,8 +484,7 @@ class ModelScanner { } if (orderMap.containsKey(order)) { - throw new ConfigurationException("Detected duplicate field order '" + childAnnotation.order() + "' for element named '" + elementName + "' in type '" + theClass.getCanonicalName() - + "'"); + throw new ConfigurationException("Detected duplicate field order '" + childAnnotation.order() + "' for element named '" + elementName + "' in type '" + theClass.getCanonicalName() + "'"); } if (elementNames.contains(elementName)) { @@ -524,8 +523,7 @@ class ModelScanner { * Child is an extension */ Class et = (Class) nextElementType; - RuntimeChildDeclaredExtensionDefinition def = new RuntimeChildDeclaredExtensionDefinition(next, childAnnotation, descriptionAnnotation, extensionAttr, elementName, - extensionAttr.url(), et); + RuntimeChildDeclaredExtensionDefinition def = new RuntimeChildDeclaredExtensionDefinition(next, childAnnotation, descriptionAnnotation, extensionAttr, elementName, extensionAttr.url(), et); orderMap.put(order, def); if (IElement.class.isAssignableFrom(nextElementType)) { addScanAlso((Class) nextElementType); @@ -537,8 +535,7 @@ class ModelScanner { List> refTypesList = new ArrayList>(); for (Class nextType : childAnnotation.type()) { if (IBaseResource.class.isAssignableFrom(nextType) == false) { - throw new ConfigurationException("Field '" + next.getName() + "' in class '" + next.getDeclaringClass().getCanonicalName() + "' is of type " + BaseResourceReferenceDt.class - + " but contains a non-resource type: " + nextType.getCanonicalName()); + throw new ConfigurationException("Field '" + next.getName() + "' in class '" + next.getDeclaringClass().getCanonicalName() + "' is of type " + BaseResourceReferenceDt.class + " but contains a non-resource type: " + nextType.getCanonicalName()); } refTypesList.add((Class) nextType); addScanAlso(nextType); @@ -548,7 +545,8 @@ class ModelScanner { } else if (IResourceBlock.class.isAssignableFrom(nextElementType) || BackboneElement.class.isAssignableFrom(nextElementType)) { /* - * Child is a resource block (i.e. a sub-tag within a resource) TODO: do these have a better name according to HL7? + * Child is a resource block (i.e. a sub-tag within a resource) TODO: do these have a better name + * according to HL7? */ Class blockDef = (Class) nextElementType; @@ -587,8 +585,7 @@ class ModelScanner { CodeableConceptElement concept = pullAnnotation(next, CodeableConceptElement.class); if (concept != null) { if (!ICodedDatatype.class.isAssignableFrom(nextDatatype)) { - throw new ConfigurationException("Field '" + elementName + "' in type '" + theClass.getCanonicalName() + "' is marked as @" + CodeableConceptElement.class.getCanonicalName() - + " but type is not a subtype of " + ICodedDatatype.class.getName()); + throw new ConfigurationException("Field '" + elementName + "' in type '" + theClass.getCanonicalName() + "' is marked as @" + CodeableConceptElement.class.getCanonicalName() + " but type is not a subtype of " + ICodedDatatype.class.getName()); } else { Class type = concept.type(); myScanAlsoCodeTable.add(type); @@ -607,9 +604,11 @@ class ModelScanner { } /** - * There are two implementations of all of the annotations (e.g. {@link Child} and {@link org.hl7.fhir.instance.model.annotations.Child}) since the HL7.org ones will eventually replace the HAPI - * ones. Annotations can't extend each other or implement interfaces or anything like that, so rather than duplicate all of the annotation processing code this method just creates an interface - * Proxy to simulate the HAPI annotations if the HL7.org ones are found instead. + * There are two implementations of all of the annotations (e.g. {@link Child} and + * {@link org.hl7.fhir.instance.model.annotations.Child}) since the HL7.org ones will eventually replace the HAPI + * ones. Annotations can't extend each other or implement interfaces or anything like that, so rather than duplicate + * all of the annotation processing code this method just creates an interface Proxy to simulate the HAPI + * annotations if the HL7.org ones are found instead. */ @SuppressWarnings("unchecked") private T pullAnnotation(AnnotatedElement theTarget, Class theAnnotationType) { @@ -682,8 +681,7 @@ class ModelScanner { parent = parent.getSuperclass(); } if (isBlank(resourceName)) { - throw new ConfigurationException("Resource type @" + ResourceDef.class.getSimpleName() + " annotation contains no resource name(): " + theClass.getCanonicalName() - + " - This is only allowed for types that extend other resource types "); + throw new ConfigurationException("Resource type @" + ResourceDef.class.getSimpleName() + " annotation contains no resource name(): " + theClass.getCanonicalName() + " - This is only allowed for types that extend other resource types "); } } @@ -709,8 +707,7 @@ class ModelScanner { // theClass.getCanonicalName()); } else { if (myIdToResourceDefinition.containsKey(resourceId)) { - throw new ConfigurationException("The following resource types have the same ID of '" + resourceId + "' - " + theClass.getCanonicalName() + " and " - + myIdToResourceDefinition.get(resourceId).getImplementingClass().getCanonicalName()); + throw new ConfigurationException("The following resource types have the same ID of '" + resourceId + "' - " + theClass.getCanonicalName() + " and " + myIdToResourceDefinition.get(resourceId).getImplementingClass().getCanonicalName()); } } @@ -759,8 +756,7 @@ class ModelScanner { for (String nextName : searchParam.compositeOf()) { RuntimeSearchParam param = nameToParam.get(nextName); if (param == null) { - ourLog.warn("Search parameter {}.{} declares that it is a composite with compositeOf value '{}' but that is not a valid parametr name itself. Valid values are: {}", new Object[] { - theResourceDef.getName(), searchParam.name(), nextName, nameToParam.keySet() }); + ourLog.warn("Search parameter {}.{} declares that it is a composite with compositeOf value '{}' but that is not a valid parametr name itself. Valid values are: {}", new Object[] { theResourceDef.getName(), searchParam.name(), nextName, nameToParam.keySet() }); continue; } compositeOf.add(param); diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/JsonParserTest.java b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/JsonParserTest.java index eb5e16a414f..3ee0f86d89b 100644 --- a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/JsonParserTest.java +++ b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/JsonParserTest.java @@ -1147,9 +1147,9 @@ public class JsonParserTest { @Test public void testSimpleResourceEncodeWithCustomType() throws IOException { - FhirContext fhirCtx = new FhirContext(MyObservationWithExtensions.class); + FhirContext fhirCtx = new FhirContext(MyPatientWithExtensions.class); String xmlString = IOUtils.toString(JsonParser.class.getResourceAsStream("/example-patient-general.xml"), Charset.forName("UTF-8")); - MyObservationWithExtensions obs = fhirCtx.newXmlParser().parseResource(MyObservationWithExtensions.class, xmlString); + MyPatientWithExtensions obs = fhirCtx.newXmlParser().parseResource(MyPatientWithExtensions.class, xmlString); assertEquals(0, obs.getAllUndeclaredExtensions().size()); assertEquals("aaaa", obs.getExtAtt().getContentType().getValue()); diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyObservationWithExtensions.java b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyPatientWithExtensions.java similarity index 97% rename from hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyObservationWithExtensions.java rename to hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyPatientWithExtensions.java index 5309bf2131f..eca9fb4f3b3 100644 --- a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyObservationWithExtensions.java +++ b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyPatientWithExtensions.java @@ -17,7 +17,7 @@ import ca.uhn.fhir.model.primitive.StringDt; import ca.uhn.fhir.util.ElementUtil; @ResourceDef(name="Patient") -public class MyObservationWithExtensions extends Patient { +public class MyPatientWithExtensions extends Patient { @Extension(url = "urn:patientext:att", definedLocally = false, isModifier = false) @Child(name = "extAtt", order = 0) diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyPatientWithUnorderedExtensions.java b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyPatientWithUnorderedExtensions.java new file mode 100644 index 00000000000..fadc1d2c97d --- /dev/null +++ b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/MyPatientWithUnorderedExtensions.java @@ -0,0 +1,65 @@ +package ca.uhn.fhir.parser; + +import ca.uhn.fhir.model.api.annotation.Child; +import ca.uhn.fhir.model.api.annotation.Extension; +import ca.uhn.fhir.model.api.annotation.ResourceDef; +import ca.uhn.fhir.model.dstu.resource.Patient; +import ca.uhn.fhir.model.primitive.BooleanDt; +import ca.uhn.fhir.model.primitive.DateDt; +import ca.uhn.fhir.model.primitive.StringDt; +import ca.uhn.fhir.util.ElementUtil; + +@ResourceDef(name = "Patient") +public class MyPatientWithUnorderedExtensions extends Patient { + + @Extension(url = "urn:ex1", definedLocally = false, isModifier = false) + @Child(name = "extAtt") + private BooleanDt myExtAtt1; + + @Extension(url = "urn:ex2", definedLocally = false, isModifier = false) + @Child(name = "moreExt") + private StringDt myExtAtt2; + + @Extension(url = "urn:ex3", definedLocally = false, isModifier = false) + @Child(name = "modExt") + private DateDt myExtAtt3; + + public BooleanDt getExtAtt1() { + if (myExtAtt1 == null) { + myExtAtt1 = new BooleanDt(); + } + return myExtAtt1; + } + + public StringDt getExtAtt2() { + if (myExtAtt2 == null) { + myExtAtt2 = new StringDt(); + } + return myExtAtt2; + } + + public DateDt getExtAtt3() { + if (myExtAtt3 == null) { + myExtAtt3 = new DateDt(); + } + return myExtAtt3; + } + + @Override + public boolean isEmpty() { + return super.isEmpty() && ElementUtil.isEmpty(myExtAtt1, myExtAtt3, myExtAtt2); + } + + public void setExtAtt1(BooleanDt theExtAtt1) { + myExtAtt1 = theExtAtt1; + } + + public void setExtAtt2(StringDt theExtAtt2) { + myExtAtt2 = theExtAtt2; + } + + public void setExtAtt3(DateDt theExtAtt3) { + myExtAtt3 = theExtAtt3; + } + +} diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/XmlParserTest.java b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/XmlParserTest.java index 885468d074d..e30326a7c01 100644 --- a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/XmlParserTest.java +++ b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/parser/XmlParserTest.java @@ -1438,12 +1438,35 @@ public class XmlParserTest { } + /** + * See #91 + */ + @Test + public void testCustomTypeWithUnoderedExtensions() { + MyPatientWithUnorderedExtensions pat = new MyPatientWithUnorderedExtensions(); + pat.getExtAtt1().setValue(true); + pat.getExtAtt2().setValue("val2"); + pat.getExtAtt3().setValueAsString("20110102"); + + String string = ourCtx.newXmlParser().encodeResourceToString(pat); + ourLog.info(string); + + //@formatter:off + assertThat(string, stringContainsInOrder(Arrays.asList( + "", + "", + "" + ))); + //@formatter:on + + } + @Test public void testSimpleResourceEncodeWithCustomType() throws IOException, SAXException { - FhirContext fhirCtx = new FhirContext(MyObservationWithExtensions.class); + FhirContext fhirCtx = new FhirContext(MyPatientWithExtensions.class); String xmlString = IOUtils.toString(JsonParser.class.getResourceAsStream("/example-patient-general.json"), Charset.forName("UTF-8")); - MyObservationWithExtensions obs = fhirCtx.newJsonParser().parseResource(MyObservationWithExtensions.class, xmlString); + MyPatientWithExtensions obs = fhirCtx.newJsonParser().parseResource(MyPatientWithExtensions.class, xmlString); assertEquals(0, obs.getAllUndeclaredExtensions().size()); assertEquals("aaaa", obs.getExtAtt().getContentType().getValue()); @@ -1470,6 +1493,7 @@ public class XmlParserTest { } + @Test public void testTagList() { diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 764ec1bf1e2..56883aa9ad4 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -77,6 +77,12 @@ incorrect behaviour. Thanks to Mochaholic for reporting! + + Custom/user defined resource definitions which contained more than one + child with no order defined failed to initialize properly. Thanks to + Andy Huang for reporting and figuring out where the + problem was! + API CHANGE:]]> The "FHIR structures" for DSTU1 (the classes which model the