Merge remote-tracking branch 'remotes/origin/master' into im_20200728_term_multi_version_support

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/atlas/points.json

@@ -259,16 +259,7 @@
 			"added": "2019-11-25"
 		},
 		{
-			"title": "Software Interoperability Lab for Asia",
+			"title": "Standards and Interoperability Lab Asia",
-			"description": "",
-			"link": "",
-			"city": "Manila, Philippines",
-			"lat": 14.6914,
-			"lon": 120.9686,
-			"added": "2019-11-25"
-		},
-		{
-			"title": "Software Interoperability Lab for Asia",
 			"description": "SIL-A maintains a suite of tools used to promote interoperability and medical informatics across Asia.",
 			"link": "http://sil-asia.org/",
 			"city": "Manila, Philippines",

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2062-avoid-db-bin-storage-deadlock.yaml

@@ -0,0 +1,5 @@
+---
+type: fix
+issue: 2062
+title: "A deadlock was fixed where the Database-backed Binary Storage service could lock the system up when running
+   under very high contention."

hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/config/DaoConfig.java

@@ -222,11 +222,6 @@ public class DaoConfig {
 	 */
 	private boolean myLastNEnabled = false;
 
-	/**
-	 * @since 5.1.0
-	 */
-	private boolean myPreloadBlobFromInputStream = false;
-
 	/**
 	 * Constructor
 	 */
@@ -2102,28 +2097,11 @@ public class DaoConfig {
 	 * </p>
 	 *
 	 * @since 5.1.0
+	 * @deprecated In 5.2.0 this setting no longer does anything
 	 */
-	public boolean isPreloadBlobFromInputStream() {
+	@Deprecated
-		return myPreloadBlobFromInputStream;
-	}
-
-	/**
-	 * <p>
-	 * This determines whether $binary-access-write operations should first load the InputStream into memory before persisting the
-	 * contents to the database. This needs to be enabled for MS SQL Server as this DB requires that the blob size be known
-	 * in advance.
-	 * </p>
-	 * <p>
-	 * Note that this setting should be enabled with caution as it can lead to significant demands on memory.
-	 * </p>
-	 * <p>
-	 * The default value for this setting is {@code false}.
-	 * </p>
-	 *
-	 * @since 5.1.0
-	 */
 	public void setPreloadBlobFromInputStream(Boolean thePreloadBlobFromInputStream) {
-		myPreloadBlobFromInputStream = thePreloadBlobFromInputStream;
+		// ignore
 	}
 
 }

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImpl.java

@@ -60,8 +60,15 @@ public class DatabaseBlobBinaryStorageSvcImpl extends BaseBinaryStorageSvcImpl {
 	private DaoConfig myDaoConfig;
 
 	@Override
-	@Transactional(Transactional.TxType.SUPPORTS)
+	@Transactional(Transactional.TxType.REQUIRED)
 	public StoredDetails storeBlob(IIdType theResourceId, String theBlobIdOrNull, String theContentType, InputStream theInputStream) throws IOException {
+
+		/*
+		 * Note on transactionality: This method used to have a propagation value of SUPPORTS and then do the actual
+		 * write in a new transaction.. I don't actually get why that was the original design, but it causes
+		 * connection pool deadlocks under load!
+		 */
+
 		Date publishedDate = new Date();
 
 		HashingInputStream hashingInputStream = createHashingInputStream(theInputStream);
@@ -77,32 +84,18 @@ public class DatabaseBlobBinaryStorageSvcImpl extends BaseBinaryStorageSvcImpl {
 
 		Session session = (Session) myEntityManager.getDelegate();
 		LobHelper lobHelper = session.getLobHelper();
-		Blob dataBlob;
-		if (myDaoConfig.isPreloadBlobFromInputStream()) {
 		byte[] loadedStream = IOUtils.toByteArray(countingInputStream);
-			dataBlob = lobHelper.createBlob(loadedStream);
+		Blob dataBlob = lobHelper.createBlob(loadedStream);
-		}  else {
-			dataBlob = lobHelper.createBlob(countingInputStream, 0);
-		}
 		entity.setBlob(dataBlob);
 
-		// Save the entity
-
-		TransactionTemplate txTemplate = new TransactionTemplate(myPlatformTransactionManager);
-		txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW);
-		txTemplate.execute(t -> {
-			myEntityManager.persist(entity);
-			return null;
-		});
-
 		// Update the entity with the final byte count and hash
 		long bytes = countingInputStream.getCount();
 		String hash = hashingInputStream.hash().toString();
-		txTemplate.execute(t -> {
+		entity.setSize((int) bytes);
-			myBinaryStorageEntityDao.setSize(id, (int) bytes);
+		entity.setHash(hash);
-			myBinaryStorageEntityDao.setHash(id, hash);
+
-			return null;
+		// Save the entity
-		});
+		myEntityManager.persist(entity);
 
 		return new StoredDetails()
 			.setBlobId(id)

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoSearchParamProvider.java

@@ -44,6 +44,10 @@ public class DaoSearchParamProvider implements ISearchParamProvider {
 
 	@Override
 	public int refreshCache(SearchParamRegistryImpl theSearchParamRegistry, long theRefreshInterval) {
-		return theSearchParamRegistry.doRefresh(theRefreshInterval);
+		int retVal = 0;
+		if (myDaoRegistry.getResourceDaoOrNull("SearchParameter") != null) {
+			retVal = theSearchParamRegistry.doRefresh(theRefreshInterval);
+		}
+		return retVal;
 	}
 }

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBinaryStorageEntityDao.java

@@ -30,14 +30,6 @@ import java.util.Optional;
 
 public interface IBinaryStorageEntityDao extends JpaRepository<BinaryStorageEntity, String> {
 
-	@Modifying
-	@Query("UPDATE BinaryStorageEntity e SET e.mySize = :blob_size WHERE e.myBlobId = :blob_id")
-	void setSize(@Param("blob_id") String theId, @Param("blob_size") int theBytes);
-
-	@Modifying
-	@Query("UPDATE BinaryStorageEntity e SET e.myHash = :blob_hash WHERE e.myBlobId = :blob_id")
-	void setHash(@Param("blob_id") String theId, @Param("blob_hash") String theHash);
-
 	@Query("SELECT e FROM BinaryStorageEntity e WHERE e.myBlobId = :blob_id AND e.myResourceId = :resource_id")
 	Optional<BinaryStorageEntity> findByIdAndResourceId(@Param("blob_id") String theBlobId, @Param("resource_id") String theResourceId);
 

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/DatabaseBlobBinaryStorageSvcImplTest.java

@@ -45,18 +45,6 @@ public class DatabaseBlobBinaryStorageSvcImplTest extends BaseJpaR4Test {
 	@Autowired
 	private DaoConfig myDaoConfig;
 
-	@BeforeEach
-	public void backupDaoConfig() {
-		defaultPreloadBlobFromInputStream = myDaoConfig.isPreloadBlobFromInputStream();
-	}
-
-	@AfterEach
-	public void restoreDaoConfig() {
-		myDaoConfig.setPreloadBlobFromInputStream(defaultPreloadBlobFromInputStream);
-	}
-
-	boolean defaultPreloadBlobFromInputStream;
-
 	@Test
 	public void testStoreAndRetrieve() throws IOException {
 
@@ -74,7 +62,7 @@ public class DatabaseBlobBinaryStorageSvcImplTest extends BaseJpaR4Test {
 
 		assertEquals(0, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size());
 		assertEquals(1, myCaptureQueriesListener.getInsertQueriesForCurrentThread().size());
-		assertEquals(2, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size());
+		assertEquals(0, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size());
 
 		myCaptureQueriesListener.clear();
 
@@ -128,7 +116,7 @@ public class DatabaseBlobBinaryStorageSvcImplTest extends BaseJpaR4Test {
 
 		assertEquals(0, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size());
 		assertEquals(1, myCaptureQueriesListener.getInsertQueriesForCurrentThread().size());
-		assertEquals(2, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size());
+		assertEquals(0, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size());
 
 		myCaptureQueriesListener.clear();
 

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/BaseTask.java

@@ -60,6 +60,14 @@ public abstract class BaseTask {
 		mySchemaVersion = theSchemaVersion;
 	}
 
+	public String getProductVersion() {
+		return myProductVersion;
+	}
+
+	public String getSchemaVersion() {
+		return mySchemaVersion;
+	}
+
 	public boolean isNoColumnShrink() {
 		return myNoColumnShrink;
 	}
@@ -137,16 +145,18 @@ public abstract class BaseTask {
 		return myConnectionProperties;
 	}
 
-	public void setConnectionProperties(DriverTypeEnum.ConnectionProperties theConnectionProperties) {
+	public BaseTask setConnectionProperties(DriverTypeEnum.ConnectionProperties theConnectionProperties) {
 		myConnectionProperties = theConnectionProperties;
+		return this;
 	}
 
 	public DriverTypeEnum getDriverType() {
 		return myDriverType;
 	}
 
-	public void setDriverType(DriverTypeEnum theDriverType) {
+	public BaseTask setDriverType(DriverTypeEnum theDriverType) {
 		myDriverType = theDriverType;
+		return this;
 	}
 
 	public abstract void validate();

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropIndexTask.java

@@ -86,9 +86,13 @@ public class DropIndexTask extends BaseTableTask {
 			@Language("SQL") String findConstraintSql = "SELECT DISTINCT constraint_name FROM user_cons_columns WHERE constraint_name = ? AND table_name = ?";
 			@Language("SQL") String dropConstraintSql = "ALTER TABLE " + getTableName() + " DROP CONSTRAINT ?";
 			findAndDropConstraint(findConstraintSql, dropConstraintSql);
-
 			findConstraintSql = "SELECT DISTINCT constraint_name FROM all_constraints WHERE index_name = ? AND table_name = ?";
 			findAndDropConstraint(findConstraintSql, dropConstraintSql);
+		} else if (getDriverType() == DriverTypeEnum.MSSQL_2012) {
+			// Legacy deletion for SQL Server unique indexes
+			@Language("SQL") String findConstraintSql = "SELECT tc.CONSTRAINT_NAME FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS AS tc WHERE tc.CONSTRAINT_NAME = ? AND tc.TABLE_NAME = ?";
+			@Language("SQL") String dropConstraintSql = "ALTER TABLE " + getTableName() + " DROP CONSTRAINT ?";
+			findAndDropConstraint(findConstraintSql, dropConstraintSql);
 		}
 
 		Set<String> indexNames = JdbcUtils.getIndexNames(getConnectionProperties(), getTableName());

hapi-fhir-jpaserver-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropTableTask.java

@@ -63,14 +63,15 @@ public class DropTableTask extends BaseTableTask {
 			}
 		}
 
+		DropIndexTask theIndexTask = new DropIndexTask(getProductVersion(), getSchemaVersion());
+		theIndexTask
+			.setTableName(getTableName())
+			.setConnectionProperties(getConnectionProperties())
+			.setDriverType(getDriverType());
 		for (String nextIndex : indexNames) {
-			List<String> sqls = DropIndexTask.createDropIndexSql(getConnectionProperties(), getTableName(), nextIndex, getDriverType());
+			theIndexTask
-			if (!sqls.isEmpty()) {
+				.setIndexName(nextIndex)
-				logInfo(ourLog, "Dropping index {} on table {} in preparation for table delete", nextIndex, getTableName());
+				.execute();
-			}
-			for (@Language("SQL") String sql : sqls) {
-				executeSql(getTableName(), sql);
-			}
 		}
 
 		logInfo(ourLog, "Dropping table: {}", getTableName());

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BinaryStorageEntity.java

@@ -89,4 +89,12 @@ public class BinaryStorageEntity {
 	public String getBlobId() {
 		return myBlobId;
 	}
+
+	public void setSize(int theSize) {
+		mySize = theSize;
+	}
+
+	public void setHash(String theHash) {
+		myHash = theHash;
+	}
 }

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/MethodUtil.java

@@ -225,6 +225,11 @@ public class MethodUtil {
 						param = new ConditionalParamBinder(theRestfulOperationTypeEnum, ((ConditionalUrlParam) nextAnnotation).supportsMultiple());
 					} else if (nextAnnotation instanceof OperationParam) {
 						Operation op = theMethod.getAnnotation(Operation.class);
+						if (op == null) {
+							throw new ConfigurationException(
+								"@OperationParam detected on method that is not annotated with @Operation: " + theMethod.toGenericString());
+						}
+
 						OperationParam operationParam = (OperationParam) nextAnnotation;
 						param = new OperationParameter(theContext, op.name(), operationParam);
 						if (isNotBlank(operationParam.typeName())) {

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/ServerInvalidDefinitionR4Test.java

@@ -1,15 +1,20 @@
 package ca.uhn.fhir.rest.server;
 
+import ca.uhn.fhir.context.ConfigurationException;
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.rest.annotation.ConditionalUrlParam;
 import ca.uhn.fhir.rest.annotation.Operation;
+import ca.uhn.fhir.rest.annotation.OperationParam;
 import ca.uhn.fhir.rest.annotation.OptionalParam;
 import ca.uhn.fhir.rest.annotation.ResourceParam;
+import ca.uhn.fhir.rest.annotation.Search;
 import ca.uhn.fhir.rest.annotation.Update;
 import ca.uhn.fhir.rest.annotation.Validate;
 import ca.uhn.fhir.rest.api.MethodOutcome;
 import ca.uhn.fhir.rest.param.TokenParam;
+import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.util.TestUtil;
+import com.google.common.collect.Lists;
 import org.hamcrest.core.StringContains;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.r4.model.Patient;
@@ -19,21 +24,19 @@ import org.junit.jupiter.api.Test;
 
 import javax.servlet.ServletException;
 
+import java.util.List;
+
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.fail;
 
-public class ServerInvalidDefinitionR4Test {
+public class ServerInvalidDefinitionR4Test extends BaseR4ServerTest {
-
-	private static FhirContext ourCtx = FhirContext.forR4();
 
 	@Test
-	public void testWrongConditionalUrlType() {
+	public void testWrongConditionalUrlType() throws Exception {
-		RestfulServer srv = new RestfulServer(ourCtx);
-		srv.setFhirContext(ourCtx);
-		srv.setResourceProviders(new UpdateWithWrongConditionalUrlType());
-
 		try {
-			srv.init();
+			startServer(new UpdateWithWrongConditionalUrlType());
 			fail();
 		} catch (ServletException e) {
 			assertThat(e.getCause().toString(), StringContains.containsString("ConfigurationException"));
@@ -43,13 +46,9 @@ public class ServerInvalidDefinitionR4Test {
 	}
 
 	@Test
-	public void testWrongResourceType() {
+	public void testWrongResourceType() throws Exception {
-		RestfulServer srv = new RestfulServer(ourCtx);
-		srv.setFhirContext(ourCtx);
-		srv.setResourceProviders(new UpdateWithWrongResourceType());
-
 		try {
-			srv.init();
+			startServer(new UpdateWithWrongResourceType());
 			fail();
 		} catch (ServletException e) {
 			assertThat(e.getCause().toString(), StringContains.containsString("ConfigurationException"));
@@ -59,13 +58,9 @@ public class ServerInvalidDefinitionR4Test {
 	}
 
 	@Test
-	public void testWrongValidateModeType() {
+	public void testWrongValidateModeType() throws Exception {
-		RestfulServer srv = new RestfulServer(ourCtx);
-		srv.setFhirContext(ourCtx);
-		srv.setResourceProviders(new ValidateWithWrongModeType());
-
 		try {
-			srv.init();
+			startServer(new ValidateWithWrongModeType());
 			fail();
 		} catch (ServletException e) {
 			assertThat(e.getCause().toString(), StringContains.containsString("ConfigurationException"));
@@ -74,13 +69,9 @@ public class ServerInvalidDefinitionR4Test {
 	}
 
 	@Test
-	public void testWrongValidateProfileType() {
+	public void testWrongValidateProfileType() throws Exception {
-		RestfulServer srv = new RestfulServer(ourCtx);
-		srv.setFhirContext(ourCtx);
-		srv.setResourceProviders(new ValidateWithWrongProfileType());
-
 		try {
-			srv.init();
+			startServer(new ValidateWithWrongProfileType());
 			fail();
 		} catch (ServletException e) {
 			assertThat(e.getCause().toString(), StringContains.containsString("ConfigurationException"));
@@ -89,7 +80,7 @@ public class ServerInvalidDefinitionR4Test {
 	}
 
 	@Test
-	public void testWrongParameterAnnotationOnOperation() {
+	public void testWrongParameterAnnotationOnOperation() throws Exception {
 		class MyProvider {
 
 			@Operation(name = "foo")
@@ -99,18 +90,42 @@ public class ServerInvalidDefinitionR4Test {
 
 		}
 
-		RestfulServer srv = new RestfulServer(ourCtx);
-		srv.setFhirContext(ourCtx);
-		srv.registerProvider(new MyProvider());
-
 		try {
-			srv.init();
+			startServer(new MyProvider());
 			fail();
 		} catch (ServletException e) {
 			assertThat(e.getCause().toString(), StringContains.containsString("Failure scanning class MyProvider: Illegal method parameter annotation @OptionalParam on method: public ca.uhn.fhir.rest.api.MethodOutcome ca.uhn.fhir.rest.server.ServerInvalidDefinitionR4Test$1MyProvider.update(org.hl7.fhir.r4.model.StringType)"));
 		}
 	}
 
+	/**
+	 * @OperationParam on a search method
+	 * <p>
+	 * See #2063
+	 */
+	@Test
+	public void testOperationParamOnASearchMethod() throws Exception {
+
+		class MyProvider extends ServerMethodSelectionR4Test.MyBaseProvider {
+			@Search
+			public List<IBaseResource> search(
+				@OptionalParam(name = "name") StringType theName,
+				@OperationParam(name = "name2") StringType theName2
+			) {
+				return Lists.newArrayList(new Patient().setActive(true).setId("Patient/123"));
+			}
+		}
+		MyProvider provider = new MyProvider();
+
+		try {
+			startServer(provider);
+			fail();
+		} catch (ServletException e) {
+			assertEquals("Failure scanning class MyProvider: @OperationParam detected on method that is not annotated with @Operation: public java.util.List<org.hl7.fhir.instance.model.api.IBaseResource> ca.uhn.fhir.rest.server.ServerInvalidDefinitionR4Test$2MyProvider.search(org.hl7.fhir.r4.model.StringType,org.hl7.fhir.r4.model.StringType)", e.getCause().getMessage());
+		}
+	}
+
+
 	public static class UpdateWithWrongConditionalUrlType implements IResourceProvider {
 
 		@Override

hapi-tinder-plugin/src/main/resources/vm/jpa_spring_beans_java.vm

@@ -51,7 +51,6 @@ public abstract class BaseJavaConfig${versionCapitalized} extends ca.uhn.fhir.jp
 
 #foreach ( $res in $resources )
 	@Bean(name="my${res.name}Dao${versionCapitalized}", autowire=Autowire.BY_NAME)
-	@Lazy
 	public
 #if ( ${versionCapitalized} == 'Dstu2' && ${res.name} == 'ValueSet' )
 		IFhirResourceDaoValueSet<ca.uhn.fhir.model.dstu2.resource.ValueSet, ca.uhn.fhir.model.dstu2.composite.CodingDt, ca.uhn.fhir.model.dstu2.composite.CodeableConceptDt>