Fix #91 - Unable to add more than two extensions with unknown orders to a custom resource

hapi-fhir-base/src/main/java/ca/uhn/fhir/context/ModelScanner.java

@@ -117,8 +117,7 @@ class ModelScanner {
 		init(null, new HashSet<Class<? extends IBase>>(theResourceTypes));
 	}
 
-	ModelScanner(FhirContext theContext, Map<Class<? extends IBase>, BaseRuntimeElementDefinition<?>> theExistingDefinitions, Collection<Class<? extends IElement>> theResourceTypes)
-			throws ConfigurationException {
+	ModelScanner(FhirContext theContext, Map<Class<? extends IBase>, BaseRuntimeElementDefinition<?>> theExistingDefinitions, Collection<Class<? extends IElement>> theResourceTypes) throws ConfigurationException {
 		myContext = theContext;
 		Set<Class<? extends IBase>> 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<? extends IBaseResource> resClass = (Class<? extends IBaseResource>) theClass;
@@ -293,8 +291,7 @@ class ModelScanner {
 				Class<? extends IPrimitiveType<?>> resClass = (Class<? extends IPrimitiveType<?>>) 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<? extends IResourceBlock> blockClass = (Class<? extends IResourceBlock>) 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<? extends IBase> theClass, Set<String> elementNames, TreeMap<Integer, BaseRuntimeDeclaredChildDefinition> theOrderToElementDef,
-			TreeMap<Integer, BaseRuntimeDeclaredChildDefinition> theOrderToExtensionDef) {
+	private void scanCompositeElementForChildren(Class<? extends IBase> theClass, Set<String> elementNames, TreeMap<Integer, BaseRuntimeDeclaredChildDefinition> theOrderToElementDef, TreeMap<Integer, BaseRuntimeDeclaredChildDefinition> 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<Class<? extends IBase>> choiceTypes = new ArrayList<Class<? extends IBase>>();
@@ -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<? extends IBase> et = (Class<? extends IBase>) 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<? extends IElement>) nextElementType);
@@ -537,8 +535,7 @@ class ModelScanner {
 				List<Class<? extends IBaseResource>> refTypesList = new ArrayList<Class<? extends IBaseResource>>();
 				for (Class<? extends IElement> 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<? extends IBaseResource>) 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<? extends IBase> blockDef = (Class<? extends IBase>) 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<? extends ICodeEnum> 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 extends Annotation> T pullAnnotation(AnnotatedElement theTarget, Class<T> 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);

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());

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)

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;
+	}
+
+}

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(
+			"<extension url=\"urn:ex1\"><valueBoolean value=\"true\"/></extension>",
+			"<extension url=\"urn:ex2\"><valueString value=\"val2\"/></extension>",
+			"<extension url=\"urn:ex3\"><valueDate value=\"20110102\"/></extension>"
+			)));
+		//@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() {
 

src/changes/changes.xml

@@ -77,6 +77,12 @@
 				incorrect behaviour. Thanks to Mochaholic for reporting! 
 			</action>
 		</release>
+			<action type="fix" issue="91" due-to="andyhuang91">
+				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! 
+			</action>
 		<release version="0.8" date="2014-Dec-17">
 			<action type="add">
 				<![CDATA[<b>API CHANGE:</b>]]> The "FHIR structures" for DSTU1 (the classes which model the