Fix #380 - Correct cardinality for OperationDefinitions

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/annotation/OperationParam.java

@@ -87,7 +87,7 @@ public @interface OperationParam {
 	 * repetitions. See {@link #MAX_DEFAULT} for a description of the default
 	 * behaviour.
 	 */
-	int max() default MAX_UNLIMITED;
+	int max() default MAX_DEFAULT;
 
 	
 }

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/client/GenericClient.java

@@ -1775,7 +1775,11 @@ public class GenericClient extends BaseClient implements IGenericClient {
 		@Override
 		public IReadExecutable withId(String theId) {
 			Validate.notBlank(theId, "The ID can not be blank");
-			myId = new IdDt(myType.getName(), theId);
+			if (theId.indexOf('/') == -1) {
+				myId = new IdDt(myType.getName(), theId);
+			} else {
+				myId = new IdDt(theId);
+			}
 			return this;
 		}
 

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/gclient/IReadTyped.java

@@ -25,10 +25,26 @@ import org.hl7.fhir.instance.model.api.IIdType;
 
 public interface IReadTyped<T extends IBaseResource> {
 
+	/**
+	 * Perform a search by resource ID
+	 * 
+	 * @param theId The resource ID, e.g. "123"
+	 */
 	IReadExecutable<T> withId(String theId);
 
+	/**
+	 * Perform a search by resource ID and version
+	 * 
+	 * @param theId The resource ID, e.g. "123"
+	 * @param theVersion The resource version, eg. "5"
+	 */
 	IReadExecutable<T> withIdAndVersion(String theId, String theVersion);
 
+	/**
+	 * Perform a search by resource ID
+	 * 
+	 * @param theId The resource ID, e.g. "123"
+	 */
 	IReadExecutable<T> withId(Long theId);
 	
 	/**

hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/OperationServerDstu2Test.java

@@ -1,6 +1,11 @@
 package ca.uhn.fhir.rest.server;
 
-import static org.hamcrest.Matchers.*;
+import static org.hamcrest.Matchers.blankOrNullString;
+import static org.hamcrest.Matchers.containsInRelativeOrder;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.startsWith;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
@@ -33,10 +38,11 @@ import ca.uhn.fhir.model.api.IResource;
 import ca.uhn.fhir.model.dstu2.composite.MoneyDt;
 import ca.uhn.fhir.model.dstu2.resource.Bundle;
 import ca.uhn.fhir.model.dstu2.resource.Conformance;
-import ca.uhn.fhir.model.dstu2.resource.OperationDefinition;
 import ca.uhn.fhir.model.dstu2.resource.Conformance.RestOperation;
+import ca.uhn.fhir.model.dstu2.resource.OperationDefinition;
 import ca.uhn.fhir.model.dstu2.resource.Parameters;
 import ca.uhn.fhir.model.dstu2.resource.Patient;
+import ca.uhn.fhir.model.dstu2.valueset.OperationParameterUseEnum;
 import ca.uhn.fhir.model.primitive.DateTimeDt;
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.model.primitive.IntegerDt;
@@ -65,6 +71,7 @@ public class OperationServerDstu2Test {
 	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(OperationServerDstu2Test.class);
 	private static int ourPort;
 	private static Server ourServer;
+	private IGenericClient myFhirClient;
 
 	@Before
 	public void before() {
@@ -75,22 +82,23 @@ public class OperationServerDstu2Test {
 		ourLastParamMoney1 = null;
 		ourLastId = null;
 		ourLastMethod = "";
+		
+		myFhirClient = ourCtx.newRestfulGenericClient("http://localhost:" + ourPort);
 	}
 
 	@Test
 	public void testConformance() throws Exception {
-		IGenericClient client = ourCtx.newRestfulGenericClient("http://localhost:" + ourPort);
 		LoggingInterceptor loggingInterceptor = new LoggingInterceptor();
 		loggingInterceptor.setLogResponseBody(true);
-		client.registerInterceptor(loggingInterceptor);
+		myFhirClient.registerInterceptor(loggingInterceptor);
 
-		Conformance p = client.fetchConformance().ofType(Conformance.class).prettyPrint().execute();
+		Conformance p = myFhirClient.fetchConformance().ofType(Conformance.class).prettyPrint().execute();
 		List<RestOperation> ops = p.getRest().get(0).getOperation();
 		assertThat(ops.size(), greaterThan(1));
 		assertNull(ops.get(0).getDefinition().getReference().getBaseUrl());
 		assertThat(ops.get(0).getDefinition().getReference().getValue(), startsWith("OperationDefinition/"));
 
-		OperationDefinition def = client.read().resource(OperationDefinition.class).withId(ops.get(0).getDefinition().getReference()).execute();
+		OperationDefinition def = myFhirClient.read().resource(OperationDefinition.class).withId(ops.get(0).getDefinition().getReference()).execute();
 		assertThat(def.getCode(), not(blankOrNullString()));
 
 		List<String> opNames = toOpNames(ops);
@@ -99,6 +107,44 @@ public class OperationServerDstu2Test {
 		assertEquals("OperationDefinition/OP_TYPE", ops.get(opNames.indexOf("OP_TYPE")).getDefinition().getReference().getValue());
 	}
 
+	/**
+	 * See #380
+	 */
+	@Test
+	public void testOperationDefinition() {
+		OperationDefinition def = myFhirClient.read().resource(OperationDefinition.class).withId("OperationDefinition/OP_TYPE").execute();
+		
+		ourLog.info(ourCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(def));
+		
+//		@OperationParam(name="PARAM1") StringType theParam1,
+//		@OperationParam(name="PARAM2") Patient theParam2,
+//		@OperationParam(name="PARAM3", min=2, max=5) List<StringType> theParam3,
+//		@OperationParam(name="PARAM4", min=1) List<StringType> theParam4,
+
+		assertEquals(4, def.getParameter().size());
+		assertEquals("PARAM1", def.getParameter().get(0).getName());
+		assertEquals(OperationParameterUseEnum.IN.getCode(), def.getParameter().get(0).getUse());
+		assertEquals(0, def.getParameter().get(0).getMin().intValue());
+		assertEquals("1", def.getParameter().get(0).getMax());
+		
+		assertEquals("PARAM2", def.getParameter().get(1).getName());
+		assertEquals(OperationParameterUseEnum.IN.getCode(), def.getParameter().get(1).getUse());
+		assertEquals(0, def.getParameter().get(1).getMin().intValue());
+		assertEquals("1", def.getParameter().get(1).getMax());
+		
+		assertEquals("PARAM3", def.getParameter().get(2).getName());
+		assertEquals(OperationParameterUseEnum.IN.getCode(), def.getParameter().get(2).getUse());
+		assertEquals(2, def.getParameter().get(2).getMin().intValue());
+		assertEquals("5", def.getParameter().get(2).getMax());
+		
+		assertEquals("PARAM4", def.getParameter().get(3).getName());
+		assertEquals(OperationParameterUseEnum.IN.getCode(), def.getParameter().get(3).getUse());
+		assertEquals(1, def.getParameter().get(3).getMin().intValue());
+		assertEquals("*", def.getParameter().get(3).getMax());
+		
+	}
+
+	
 	private List<String> toOpNames(List<RestOperation> theOps) {
 		ArrayList<String> retVal = new ArrayList<String>();
 		for (RestOperation next : theOps) {
@@ -613,7 +659,9 @@ public class OperationServerDstu2Test {
 		@Operation(name="$OP_TYPE", idempotent=true)
 		public Parameters opType(
 				@OperationParam(name="PARAM1") StringDt theParam1,
-				@OperationParam(name="PARAM2") Patient theParam2
+				@OperationParam(name="PARAM2") Patient theParam2,
+				@OperationParam(name="PARAM3", min=2, max=5) List<StringDt> theParam3,
+				@OperationParam(name="PARAM4", min=1) List<StringDt> theParam4
 				) {
 			//@formatter:on
 

hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/hapi/rest/server/ServerConformanceProvider.java

@@ -289,7 +289,7 @@ public class ServerConformanceProvider implements IServerConformanceProvider<Con
 						String opName = myOperationBindingToName.get(methodBinding);
 						if (operationNames.add(opName)) {
 							// Only add each operation (by name) once
-							rest.addOperation().setName(opName).setDefinition(new Reference(readOperationDefinition(new IdType(opName))));
+							rest.addOperation().setName(opName).setDefinition(new Reference("OperationDefinition/" + opName));
 						}
 					}
 
@@ -324,7 +324,7 @@ public class ServerConformanceProvider implements IServerConformanceProvider<Con
 						String opName = myOperationBindingToName.get(methodBinding);
 						if (operationNames.add(opName)) {
 							ourLog.info("Found bound operation: {}", opName);
-							rest.addOperation().setName(opName).setDefinition(new Reference(readOperationDefinition(new IdType(opName))));
+							rest.addOperation().setName(opName).setDefinition(new Reference("OperationDefinition/" + opName));
 						}
 					}
 				}

hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/OperationServerDstu3Test.java

@@ -1,10 +1,8 @@
 package ca.uhn.fhir.rest.server;
 
-import static org.hamcrest.Matchers.blankOrNullString;
 import static org.hamcrest.Matchers.containsInRelativeOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.greaterThan;
-import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.startsWith;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
@@ -35,6 +33,7 @@ import org.hl7.fhir.dstu3.model.IdType;
 import org.hl7.fhir.dstu3.model.IntegerType;
 import org.hl7.fhir.dstu3.model.Money;
 import org.hl7.fhir.dstu3.model.OperationDefinition;
+import org.hl7.fhir.dstu3.model.OperationDefinition.OperationParameterUse;
 import org.hl7.fhir.dstu3.model.Parameters;
 import org.hl7.fhir.dstu3.model.Patient;
 import org.hl7.fhir.dstu3.model.StringType;
@@ -69,6 +68,7 @@ public class OperationServerDstu3Test {
 	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(OperationServerDstu3Test.class);
 	private static int ourPort;
 	private static Server ourServer;
+	private IGenericClient myFhirClient;
 
 	@Before
 	public void before() {
@@ -79,27 +79,66 @@ public class OperationServerDstu3Test {
 		ourLastParamMoney1 = null;
 		ourLastId = null;
 		ourLastMethod = "";
+
+		myFhirClient = ourCtx.newRestfulGenericClient("http://localhost:" + ourPort);
 	}
 
 
 	@Test
 	public void testConformance() throws Exception {
-		IGenericClient client = ourCtx.newRestfulGenericClient("http://localhost:" + ourPort);
 		LoggingInterceptor loggingInterceptor = new LoggingInterceptor();
 		loggingInterceptor.setLogResponseBody(true);
-		client.registerInterceptor(loggingInterceptor);
+		myFhirClient.registerInterceptor(loggingInterceptor);
 
-		Conformance p = client.fetchConformance().ofType(Conformance.class).prettyPrint().execute();
+		Conformance p = myFhirClient.fetchConformance().ofType(Conformance.class).prettyPrint().execute();
 		List<ConformanceRestOperationComponent> ops = p.getRest().get(0).getOperation();
 		assertThat(ops.size(), greaterThan(1));
 
 		List<String> opNames = toOpNames(ops);
 		assertThat(opNames, containsInRelativeOrder("OP_TYPE"));
 		
-		OperationDefinition def = (OperationDefinition) ops.get(opNames.indexOf("OP_TYPE")).getDefinition().getResource();
+//		OperationDefinition def = (OperationDefinition) ops.get(opNames.indexOf("OP_TYPE")).getDefinition().getResource();
+		OperationDefinition def = myFhirClient.read().resource(OperationDefinition.class).withId(ops.get(opNames.indexOf("OP_TYPE")).getDefinition().getReferenceElement()).execute();
 		assertEquals("OP_TYPE", def.getCode());
 	}
 	
+	/**
+	 * See #380
+	 */
+	@Test
+	public void testOperationDefinition() {
+		OperationDefinition def = myFhirClient.read().resource(OperationDefinition.class).withId("OperationDefinition/OP_TYPE").execute();
+		
+		ourLog.info(ourCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(def));
+		
+//		@OperationParam(name="PARAM1") StringType theParam1,
+//		@OperationParam(name="PARAM2") Patient theParam2,
+//		@OperationParam(name="PARAM3", min=2, max=5) List<StringType> theParam3,
+//		@OperationParam(name="PARAM4", min=1) List<StringType> theParam4,
+
+		assertEquals(4, def.getParameter().size());
+		assertEquals("PARAM1", def.getParameter().get(0).getName());
+		assertEquals(OperationParameterUse.IN, def.getParameter().get(0).getUse());
+		assertEquals(0, def.getParameter().get(0).getMin());
+		assertEquals("1", def.getParameter().get(0).getMax());
+		
+		assertEquals("PARAM2", def.getParameter().get(1).getName());
+		assertEquals(OperationParameterUse.IN, def.getParameter().get(1).getUse());
+		assertEquals(0, def.getParameter().get(1).getMin());
+		assertEquals("1", def.getParameter().get(1).getMax());
+		
+		assertEquals("PARAM3", def.getParameter().get(2).getName());
+		assertEquals(OperationParameterUse.IN, def.getParameter().get(2).getUse());
+		assertEquals(2, def.getParameter().get(2).getMin());
+		assertEquals("5", def.getParameter().get(2).getMax());
+		
+		assertEquals("PARAM4", def.getParameter().get(3).getName());
+		assertEquals(OperationParameterUse.IN, def.getParameter().get(3).getUse());
+		assertEquals(1, def.getParameter().get(3).getMin());
+		assertEquals("*", def.getParameter().get(3).getMax());
+		
+	}
+
 	private List<String> toOpNames(List<ConformanceRestOperationComponent> theOps) {
 		ArrayList<String> retVal = new ArrayList<String>();
 		for (ConformanceRestOperationComponent next : theOps) {
@@ -530,6 +569,7 @@ public class OperationServerDstu3Test {
 
 	}
 
+	
 	public static void main(String[] theValue) {
 		Parameters p = new Parameters();
 		p.addParameter().setName("start").setValue(new DateTimeType("2001-01-02"));
@@ -617,7 +657,9 @@ public class OperationServerDstu3Test {
 		@Operation(name="$OP_TYPE", idempotent=true)
 		public Parameters opType(
 				@OperationParam(name="PARAM1") StringType theParam1,
-				@OperationParam(name="PARAM2") Patient theParam2
+				@OperationParam(name="PARAM2") Patient theParam2,
+				@OperationParam(name="PARAM3", min=2, max=5) List<StringType> theParam3,
+				@OperationParam(name="PARAM4", min=1) List<StringType> theParam4
 				) {
 			//@formatter:on
 

src/changes/changes.xml

@@ -324,6 +324,14 @@
 				server conformance statement should not include the $ prefix in the operation
 				name or code. Thanks to Dion McMurtrie for reporting!
 			</action>
+			<action type="fix" issue="380">
+				OperationDefinition resources generated automatically by the server for operations
+				that are defined within resource/plain providers incorrectly stated that
+				the maximum cardinality was "*" for non-collection types with no explicit
+				maximum stated, which is not the behaviour that the JavaDoc on the
+				<![CDATA[@OperationParam]] annotation describes. Thanks to Michael Lawley
+				for reporting!
+			</action>
 		</release>
 		<release version="1.5" date="2016-04-20">
 			<action type="fix" issue="339">