diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6582-drop-tag-fk-constraint.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6582-drop-tag-fk-constraint.yaml new file mode 100644 index 00000000000..40d7e536fea --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6582-drop-tag-fk-constraint.yaml @@ -0,0 +1,4 @@ +--- +type: perf +issue: 6582 +title: "Under heavy load, a foreign key constraint in the Tag Definition table (used for Tags, Security Labels, and Profile Definitions) can cause serious slowdowns when writing large numbers of resources (particularly if many resources contain the same tags/labels, or if the resources are being written individually or in smaller batches). This has been corrected. Also, a foreign key constraint on the Resource Link table has been dropped. This will significantly improve performance when writing resource collections with many links to resources not also in the same Bundle." diff --git a/hapi-fhir-jpa-hibernate-services/src/main/java/ca/uhn/hapi/fhir/sql/hibernatesvc/DatabasePartitionModeIdFilteringMappingContributor.java b/hapi-fhir-jpa-hibernate-services/src/main/java/ca/uhn/hapi/fhir/sql/hibernatesvc/DatabasePartitionModeIdFilteringMappingContributor.java index c99f5b223c0..76e9c4a4760 100644 --- a/hapi-fhir-jpa-hibernate-services/src/main/java/ca/uhn/hapi/fhir/sql/hibernatesvc/DatabasePartitionModeIdFilteringMappingContributor.java +++ b/hapi-fhir-jpa-hibernate-services/src/main/java/ca/uhn/hapi/fhir/sql/hibernatesvc/DatabasePartitionModeIdFilteringMappingContributor.java @@ -192,6 +192,15 @@ private void removePartitionedIdColumnsFromMetadata( theClassLoaderService, theMetadata, foreignKey, table, nextEntry.getKey()); } + for (Property property : entityPersistentClass.getProperties()) { + Value value = property.getValue(); + if (value instanceof ToOne) { + ToOne toOne = (ToOne) value; + filterPropertiesFromToOneRelationship( + theClassLoaderService, theMetadata, table, entityPersistentClass.getClassName(), toOne); + } + } + for (UniqueKey uniqueKey : table.getUniqueKeys().values()) { // Adjust UniqueKey constraints, which are uniqueness // constraints automatically generated to support FKs on @@ -346,22 +355,9 @@ private void filterPartitionedIdsFromLocalFks( Value value = theForeignKey.getColumn(0).getValue(); if (value instanceof ToOne) { ToOne manyToOne = (ToOne) value; - - String targetTableName = theMetadata - .getEntityBindingMap() - .get(manyToOne.getReferencedEntityName()) - .getTable() - .getName(); - Class entityType = getType(theClassLoaderService, theEntityTypeName); - String propertyName = manyToOne.getPropertyName(); - Set columnNamesToRemoveFromFks = - determineFilteredColumnNamesInForeignKey(entityType, propertyName, targetTableName); - - removeColumns(manyToOne.getColumns(), t1 -> columnNamesToRemoveFromFks.contains(t1.getName())); + Set columnNamesToRemoveFromFks = filterPropertiesFromToOneRelationship( + theClassLoaderService, theMetadata, theTable, theEntityTypeName, manyToOne); removeColumns(theForeignKey.getColumns(), t1 -> columnNamesToRemoveFromFks.contains(t1.getName())); - - columnNamesToRemoveFromFks.forEach(t -> myQualifiedIdRemovedColumnNames.add(theTable.getName() + "#" + t)); - } else { theForeignKey @@ -371,6 +367,29 @@ private void filterPartitionedIdsFromLocalFks( } } + @Nonnull + private Set filterPropertiesFromToOneRelationship( + ClassLoaderService theClassLoaderService, + InFlightMetadataCollector theMetadata, + Table theTable, + String theEntityTypeName, + ToOne manyToOne) { + String targetTableName = theMetadata + .getEntityBindingMap() + .get(manyToOne.getReferencedEntityName()) + .getTable() + .getName(); + Class entityType = getType(theClassLoaderService, theEntityTypeName); + String propertyName = manyToOne.getPropertyName(); + Set columnNamesToRemoveFromFks = + determineFilteredColumnNamesInForeignKey(entityType, propertyName, targetTableName); + + removeColumns(manyToOne.getColumns(), t1 -> columnNamesToRemoveFromFks.contains(t1.getName())); + + columnNamesToRemoveFromFks.forEach(t -> myQualifiedIdRemovedColumnNames.add(theTable.getName() + "#" + t)); + return columnNamesToRemoveFromFks; + } + private void filterPartitionedIdsFromUniqueConstraints(UniqueKey uniqueKey, Table table) { uniqueKey .getColumns() diff --git a/hapi-fhir-jpaserver-base/pom.xml b/hapi-fhir-jpaserver-base/pom.xml index ef1baaf98f1..9bb16507004 100644 --- a/hapi-fhir-jpaserver-base/pom.xml +++ b/hapi-fhir-jpaserver-base/pom.xml @@ -563,10 +563,14 @@ ca.uhn.fhir.jpa.model.dialect.HapiFhirOracleDialect oracle.sql + + add constraint FK_RESLINK_TARGET ca.uhn.fhir.jpa.model.dialect.HapiFhirSQLServerDialect sqlserver.sql + + add constraint FK_RESLINK_TARGET ca.uhn.fhir.jpa.model.dialect.HapiFhirCockroachDialect @@ -580,6 +584,8 @@ ca.uhn.fhir.jpa.model.dialect.HapiFhirPostgresDialect postgres.sql classpath:ca/uhn/fhir/jpa/docs/database/hapifhirpostgres94-init01.sql + + add constraint FK_RESLINK_TARGET diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java index 24454a1847f..c8368475588 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java @@ -671,17 +671,17 @@ private boolean updateTags( allResourceTagsNewAndOldFromTheEntity.forEach(tag -> { - // Don't keep duplicate tags - if (!allTagDefinitionsPresent.add(tag.getTag())) { + // Don't keep duplicate tags or tags with a missing definition + TagDefinition tagDefinition = tag.getTag(); + if (tagDefinition == null || !allTagDefinitionsPresent.add(tagDefinition)) { theEntity.getTags().remove(tag); } // Drop any tags that have been removed - if (!allResourceTagsFromTheResource.contains(tag)) { + if (tagDefinition != null && !allResourceTagsFromTheResource.contains(tag)) { if (shouldDroppedTagBeRemovedOnUpdate(theRequest, tag)) { theEntity.getTags().remove(tag); - } else if (HapiExtensions.EXT_SUBSCRIPTION_MATCHING_STRATEGY.equals( - tag.getTag().getSystem())) { + } else if (HapiExtensions.EXT_SUBSCRIPTION_MATCHING_STRATEGY.equals(tagDefinition.getSystem())) { theEntity.getTags().remove(tag); } } @@ -766,6 +766,9 @@ public BaseHasResource readEntity(IIdType theValueId, RequestDetails theRequest) * @return Returns true if the tag should be removed */ protected boolean shouldDroppedTagBeRemovedOnUpdate(RequestDetails theRequest, ResourceTag theTag) { + if (theTag.getTag() == null) { + return true; + } Set metaSnapshotModeTokens = null; diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java index 125bf6f4f7f..2dcaead5238 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java @@ -490,7 +490,19 @@ private R populateResourceMetadataRi( res.getMeta().getTag().clear(); res.getMeta().getProfile().clear(); res.getMeta().getSecurity().clear(); + + boolean haveWarnedForMissingTag = false; for (BaseTag next : theTagList) { + if (next.getTag() == null) { + if (!haveWarnedForMissingTag) { + ourLog.warn( + "Tag definition HFJ_TAG_DEF#{} is missing, returned Resource.meta may not be complete", + next.getTagId()); + haveWarnedForMissingTag = true; + } + continue; + } + switch (next.getTag().getTagType()) { case PROFILE: res.getMeta().addProfile(next.getTag().getCode()); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java index 29c6aba1567..a7fb1f09509 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java @@ -379,6 +379,29 @@ protected void init780() { .nullable() .withType(ColumnTypeEnum.TINYINT) .heavyweightSkipByDefault(); + + /* + * These two constraints were the last two that we had that used + * hibernate-generated names. Yay! + */ + version.onTable("HFJ_RES_TAG").dropForeignKey("20250115.10", "FKbfcjbaftmiwr3rxkwsy23vneo", "HFJ_TAG_DEF"); + version.onTable("HFJ_HISTORY_TAG").dropForeignKey("20250115.20", "FKtderym7awj6q8iq5c51xv4ndw", "HFJ_TAG_DEF"); + + /* + * This migration drops a constraint from ResourceLink#myTargetResource. Not having this + * constraint is a significant performance improvement in some cases, and the column is + * nullable anyhow already because we also have the possibility of having a logical reference + * instead of a hard one. We still keep the constraint present on the ResourceLink + * entity for two reasons: + * 1. We want to leave it in place on H2 to ensure that it helps to catch any bugs. + * 2. We can't drop it as of 2025-01-16 because of this Hibernate bug: + * https://hibernate.atlassian.net/browse/HHH-19046 + */ + version.onTable("HFJ_RES_LINK") + .dropForeignKey("20250115.30", "FK_RESLINK_TARGET", "HFJ_RESOURCE") + .runEvenDuringSchemaInitialization() + .onlyAppliesToPlatforms( + DriverTypeEnum.POSTGRES_9_4, DriverTypeEnum.MSSQL_2012, DriverTypeEnum.ORACLE_12C); } protected void init780_afterPartitionChanges() { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/entity/GeneratedSchemaTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/entity/GeneratedSchemaTest.java index a41126af2a6..df9698fe3ee 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/entity/GeneratedSchemaTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/entity/GeneratedSchemaTest.java @@ -1,13 +1,11 @@ package ca.uhn.fhir.jpa.entity; -import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; -import ca.uhn.fhir.jpa.model.dialect.HapiFhirH2Dialect; import ca.uhn.fhir.util.ClasspathUtil; import jakarta.annotation.Nonnull; import org.apache.commons.lang3.StringUtils; -import org.hibernate.search.mapper.pojo.common.annotation.Param; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; import java.util.Arrays; @@ -37,21 +35,13 @@ public void testVerifyLongVarcharColumnDefinition() { validateLongVarcharDatatype("sqlserver.sql", "varchar(max)"); } - private static void validateLongVarcharDatatype(String schemaName, String expectedDatatype) { - String schema = ClasspathUtil.loadResource("ca/uhn/hapi/fhir/jpa/docs/database/nonpartitioned/" + - "" + schemaName); - String[] lines = StringUtils.split(schema, '\n'); - String resTextVc = Arrays.stream(lines).filter(t -> t.contains("RES_TEXT_VC ")).findFirst().orElseThrow(); - assertThat(resTextVc).as("Wrong type in " + schemaName).contains("RES_TEXT_VC " + expectedDatatype); - } - @Test void testVerifyPksNonPartitioned() { String file = "ca/uhn/hapi/fhir/jpa/docs/database/nonpartitioned/postgres.sql"; List lines = loadSchemaLines(file); List fks = lines.stream().filter(t -> t.startsWith("alter table if exists HFJ_RES_VER add constraint FK_RESOURCE_HISTORY_RESOURCE ")).toList(); - assertEquals(1, fks.size(), ()-> String.join("\n", lines)); + assertEquals(1, fks.size(), () -> String.join("\n", lines)); assertThat(fks.get(0)).contains("foreign key (RES_ID)"); } @@ -61,10 +51,34 @@ void testVerifyPksPartitioned() { List lines = loadSchemaLines(file); List fks = lines.stream().filter(t -> t.startsWith("alter table if exists HFJ_RES_VER add constraint FK_RESOURCE_HISTORY_RESOURCE ")).toList(); - assertEquals(1, fks.size(), ()-> String.join("\n", lines)); + assertEquals(1, fks.size(), () -> String.join("\n", lines)); assertThat(fks.get(0)).contains("foreign key (RES_ID, PARTITION_ID)"); } + @ParameterizedTest + @CsvSource({ + "ca/uhn/hapi/fhir/jpa/docs/database/partitioned/postgres.sql , false", + "ca/uhn/hapi/fhir/jpa/docs/database/partitioned/sqlserver.sql , false", + "ca/uhn/hapi/fhir/jpa/docs/database/partitioned/oracle.sql , false", + "ca/uhn/hapi/fhir/jpa/docs/database/partitioned/h2.sql , true" + }) + void verifyNoResourceLinkTargetFkConstraint(String theFileName, boolean theShouldContainConstraint) { + String file = ClasspathUtil.loadResource(theFileName); + if (theShouldContainConstraint) { + assertThat(file).contains("FK_RESLINK_TARGET"); + } else { + assertThat(file).doesNotContain("FK_RESLINK_TARGET"); + } + } + + private static void validateLongVarcharDatatype(String schemaName, String expectedDatatype) { + String schema = ClasspathUtil.loadResource("ca/uhn/hapi/fhir/jpa/docs/database/nonpartitioned/" + + "" + schemaName); + String[] lines = StringUtils.split(schema, '\n'); + String resTextVc = Arrays.stream(lines).filter(t -> t.contains("RES_TEXT_VC ")).findFirst().orElseThrow(); + assertThat(resTextVc).as("Wrong type in " + schemaName).contains("RES_TEXT_VC " + expectedDatatype); + } + @Nonnull private static List loadSchemaLines(String file) { String schema = ClasspathUtil.loadResource(file); diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BaseTag.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BaseTag.java index 5c502fd35a3..d945be797f1 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BaseTag.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/BaseTag.java @@ -19,10 +19,15 @@ */ package ca.uhn.fhir.jpa.model.entity; +import jakarta.annotation.Nullable; import jakarta.persistence.Column; +import jakarta.persistence.ConstraintMode; +import jakarta.persistence.ForeignKey; import jakarta.persistence.JoinColumn; import jakarta.persistence.ManyToOne; import jakarta.persistence.MappedSuperclass; +import org.hibernate.annotations.NotFound; +import org.hibernate.annotations.NotFoundAction; import java.io.Serializable; @@ -31,9 +36,15 @@ public abstract class BaseTag extends BasePartitionable implements Serializable private static final long serialVersionUID = 1L; - // many baseTags -> one tag definition + /** + * Every tag has a reference to the tag definition. Note that this field + * must not have a FK constraint! In this case, Postgres (and maybe others) + * are horribly slow writing to the table if there's an FK constraint. + * See https://pganalyze.com/blog/5mins-postgres-multiXact-ids-foreign-keys-performance + */ @ManyToOne(cascade = {}) - @JoinColumn(name = "TAG_ID", nullable = false) + @JoinColumn(name = "TAG_ID", nullable = false, foreignKey = @ForeignKey(ConstraintMode.NO_CONSTRAINT)) + @NotFound(action = NotFoundAction.IGNORE) private TagDefinition myTag; @Column(name = "TAG_ID", insertable = false, updatable = false) @@ -43,6 +54,12 @@ public Long getTagId() { return myTagId; } + /** + * This can be null if the tag definition has been deleted. This + * should never happen unless someone has manually messed with + * the database, but it could happen. + */ + @Nullable public TagDefinition getTag() { return myTag; } diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceHistoryTag.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceHistoryTag.java index 3467a68afe5..64ddda7eba9 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceHistoryTag.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceHistoryTag.java @@ -154,12 +154,12 @@ public JpaPid getResourcePid() { public String toString() { ToStringBuilder b = new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE); b.append("id", getId()); - if (getPartitionId() != null) { + if (getPartitionId().getPartitionId() != null) { b.append("partId", getPartitionId().getPartitionId()); } b.append("versionId", myResourceHistoryPid); b.append("resId", getResourceId()); - b.append("tag", getTag().getId()); + b.append("tag", getTagId()); return b.build(); } } diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceLink.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceLink.java index 0a7dee4b630..3978970e091 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceLink.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceLink.java @@ -124,6 +124,12 @@ public class ResourceLink extends BaseResourceIndex { insertable = false, updatable = false), }, + /* + * TODO: We need to drop this constraint because it affects performance in pretty + * terrible ways on a lot of platforms. But a Hibernate bug present in Hibernate 6.6.4 + * makes it impossible. + * See: https://hibernate.atlassian.net/browse/HHH-19046 + */ foreignKey = @ForeignKey(name = FK_RESLINK_TARGET)) private ResourceTable myTargetResource; diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceTag.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceTag.java index bda0dd2a84f..29ff08da828 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceTag.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceTag.java @@ -175,7 +175,7 @@ public String toString() { b.append("partition", getPartitionId().getPartitionId()); } b.append("resId", getResourceId()); - b.append("tag", getTag().getId()); + b.append("tag", getTagId()); return b.build(); } } diff --git a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/DeletedDatabaseRowsR5Test.java b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/DeletedDatabaseRowsR5Test.java new file mode 100644 index 00000000000..8516d309cab --- /dev/null +++ b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/DeletedDatabaseRowsR5Test.java @@ -0,0 +1,172 @@ +package ca.uhn.fhir.jpa.dao.r5; + +import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; +import ca.uhn.fhir.jpa.api.dao.IDao; +import ca.uhn.fhir.jpa.dao.JpaStorageResourceParser; +import ca.uhn.fhir.jpa.model.dao.JpaPid; +import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable; +import ca.uhn.fhir.jpa.model.entity.ResourceLink; +import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; +import ca.uhn.fhir.jpa.test.BaseJpaTest; +import ca.uhn.fhir.model.api.Include; +import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; +import ca.uhn.test.util.LogbackTestExtension; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.spi.ILoggingEvent; +import org.hl7.fhir.r5.model.Coding; +import org.hl7.fhir.r5.model.IdType; +import org.hl7.fhir.r5.model.Organization; +import org.hl7.fhir.r5.model.Patient; +import org.hl7.fhir.r5.model.Reference; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class DeletedDatabaseRowsR5Test extends BaseJpaR5Test{ + + @RegisterExtension + private LogbackTestExtension myLogbackTestExtension = new LogbackTestExtension(JpaStorageResourceParser.class, Level.WARN); + + + /** + * We have removed the FK constraint from {@link ca.uhn.fhir.jpa.model.entity.ResourceTag} + * to {@link ca.uhn.fhir.jpa.model.entity.TagDefinition} so it's possible that tag + * definitions get deleted. This test makes sure we act sanely when that happens. + */ + @ParameterizedTest + @CsvSource(value = { + "NON_VERSIONED, read", + "NON_VERSIONED, vread", + "NON_VERSIONED, search", + "NON_VERSIONED, history", + "NON_VERSIONED, update", + "VERSIONED, read", + "VERSIONED, vread", + "VERSIONED, search", + "VERSIONED, history", + "VERSIONED, update" + }) + public void testTagDefinitionDeleted(JpaStorageSettings.TagStorageModeEnum theTagStorageMode, String theOperation) { + // Setup + myStorageSettings.setTagStorageMode(theTagStorageMode); + + Patient patient = new Patient(); + patient.setId("Patient/A"); + patient.getMeta().addProfile("http://profile0"); + patient.getMeta().addProfile("http://profile1"); + patient.getMeta().addTag("http://tag", "tag0", "display0"); + patient.getMeta().addTag("http://tag", "tag1", "display1"); + patient.getMeta().addSecurity(new Coding("http://security", "security0", "display0")); + patient.getMeta().addSecurity(new Coding("http://security", "security1", "display1")); + myPatientDao.update(patient, mySrd); + + patient.setActive(false); + myPatientDao.update(patient, mySrd); + + // Test + + runInTransaction(() -> { + assertEquals(6, myTagDefinitionDao.count()); + myTagDefinitionDao.deleteAll(); + assertEquals(0, myTagDefinitionDao.count()); + }); + Patient actualPatient; + switch (theOperation) { + case "read": + actualPatient = myPatientDao.read(new IdType("Patient/A"), mySrd); + break; + case "vread": + actualPatient = myPatientDao.read(new IdType("Patient/A/_history/1"), mySrd); + break; + case "search": + actualPatient = (Patient) myPatientDao.search(SearchParameterMap.newSynchronous(), mySrd).getResources(0, 1).get(0); + break; + case "history": + actualPatient = (Patient) mySystemDao.history(null, null, null, mySrd).getResources(0, 1).get(0); + break; + case "update": + Patient updatePatient = new Patient(); + updatePatient.setId("Patient/A"); + updatePatient.setActive(true); + actualPatient = (Patient) myPatientDao.update(updatePatient, mySrd).getResource(); + break; + default: + throw new IllegalArgumentException("Unknown operation: " + theOperation); + } + + // Verify + + assertEquals(0, actualPatient.getMeta().getProfile().size()); + assertEquals(0, actualPatient.getMeta().getTag().size()); + assertEquals(0, actualPatient.getMeta().getSecurity().size()); + + List logEvents = myLogbackTestExtension.getLogEvents(t -> t.getMessage().contains("Tag definition HFJ_TAG_DEF#{} is missing")); + assertThat(logEvents).as(() -> myLogbackTestExtension.getLogEvents().toString()).hasSize(1); + } + + /** + * We don't use a FK constraint on {@link ResourceLink#getTargetResource()} for performance + * reasons so this can be nulled out even if a value is present. This is a test to make sure + * we behave if the target row is deleted. + * We actually do keep this constraint present on H2 in order to make sure that unit tests + * catch any issues which would accidentally try to drop data. So we need to temporarily + * remove that constraint in order for the test to work. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testReferenceWithTargetExpunged(boolean theSynchronous) { + // Setup + runInTransaction(()->{ + myEntityManager.createNativeQuery("alter table HFJ_RES_LINK drop constraint FK_RESLINK_TARGET").executeUpdate(); + }); + try { + Organization org = new Organization(); + org.setId("Organization/O"); + JpaPid orgPid = IDao.RESOURCE_PID.get(myOrganizationDao.update(org, mySrd).getResource()); + + Patient patient = new Patient(); + patient.setId("Patient/P"); + patient.setManagingOrganization(new Reference("Organization/O")); + myPatientDao.update(patient, mySrd); + + // Delete the Organization resource + runInTransaction(() -> { + List historyEntities = myResourceHistoryTableDao.findAllVersionsForResourceIdInOrder(orgPid.toFk()); + myResourceHistoryTableDao.deleteAll(historyEntities); + }); + runInTransaction(() -> { + myResourceTableDao.deleteByPid(orgPid); + }); + + // Make sure that the Org was deleted + assertThrows(ResourceNotFoundException.class, () -> myOrganizationDao.read(new IdType("Organization/O"), mySrd)); + + // Test + SearchParameterMap params = new SearchParameterMap().addInclude(new Include("*")); + params.setLoadSynchronous(theSynchronous); + IBundleProvider result = myPatientDao.search(params, mySrd); + + // Verify + List values = toUnqualifiedVersionlessIdValues(result); + assertThat(values).asList().containsExactly("Patient/P"); + } finally { + runInTransaction(() -> { + myResourceHistoryTableDao.deleteAll(); + myResourceLinkDao.deleteAll(); + myResourceIndexedSearchParamTokenDao.deleteAll(); + myResourceTableDao.deleteAll(); + // Restore the index we dropped at the start of the test + myEntityManager.createNativeQuery("alter table if exists HFJ_RES_LINK add constraint FK_RESLINK_TARGET foreign key (TARGET_RESOURCE_ID) references HFJ_RESOURCE").executeUpdate(); + }); + } + } + +} diff --git a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/search/reindex/InstanceReindexServiceImplR5Test.java b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/search/reindex/InstanceReindexServiceImplR5Test.java index b348c446bb3..a69451365b5 100644 --- a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/search/reindex/InstanceReindexServiceImplR5Test.java +++ b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/search/reindex/InstanceReindexServiceImplR5Test.java @@ -183,11 +183,13 @@ public void testDryRunTypes_ResourceLink_WithUrl() { IIdType id = createObservation(withSubject("Patient/A")); runInTransaction(() -> { - assertEquals(2, myEntityManager.createNativeQuery("update HFJ_RES_LINK set TARGET_RESOURCE_ID = null").executeUpdate()); + assertEquals(2, myEntityManager.createNativeQuery("update HFJ_RES_LINK set (TARGET_RESOURCE_ID,TARGET_RES_PARTITION_ID) = (null, null)").executeUpdate()); assertEquals(2, myEntityManager.createNativeQuery("update HFJ_RES_LINK set TARGET_RESOURCE_URL = 'http://foo'").executeUpdate()); assertEquals(2, myEntityManager.createNativeQuery("update HFJ_RES_LINK set TARGET_RESOURCE_VERSION = 1").executeUpdate()); }); + myEntityManager.clear(); + Parameters outcome = (Parameters) mySvc.reindexDryRun(new SystemRequestDetails(), id, null); ourLog.info("Output:{}", myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome)); diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java index 2bbee486af5..79274cec578 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java @@ -276,7 +276,8 @@ public static ColumnType getColumnType( } /** - * Retrieve all index names + * Retrieve all index names. The returned names will be in upper case + * always. */ public static Set getForeignKeys( DriverTypeEnum.ConnectionProperties theConnectionProperties, @@ -588,9 +589,9 @@ public static boolean isColumnNullable( } } - private static String massageIdentifier(DatabaseMetaData theMetadata, String theCatalog) throws SQLException { - String retVal = theCatalog; - if (theCatalog == null) { + public static String massageIdentifier(DatabaseMetaData theMetadata, String theIdentifier) throws SQLException { + String retVal = theIdentifier; + if (theIdentifier == null) { return null; } else if (theMetadata.storesLowerCaseIdentifiers()) { retVal = retVal.toLowerCase(); diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java index 40fd601ba61..909a547067d 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/taskdef/DropForeignKeyTask.java @@ -26,12 +26,15 @@ import org.apache.commons.lang3.Validate; import org.apache.commons.lang3.builder.EqualsBuilder; import org.apache.commons.lang3.builder.HashCodeBuilder; +import org.intellij.lang.annotations.Language; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.sql.Connection; import java.sql.SQLException; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.Set; import static org.apache.commons.lang3.StringUtils.isNotBlank; @@ -92,14 +95,18 @@ public void validate() { public void doExecute() throws SQLException { Set existing = JdbcUtils.getForeignKeys(getConnectionProperties(), myParentTableName, getTableName()); - if (!existing.contains(myConstraintName)) { + if (!existing.contains(myConstraintName.toUpperCase(Locale.US))) { logInfo(ourLog, "Don't have constraint named {} - No action performed", myConstraintName); return; } - List sqls = generateSql(getTableName(), myConstraintName, getDriverType()); + List sqlStatements; + try (Connection connection = getConnectionProperties().getDataSource().getConnection()) { + String constraintName = JdbcUtils.massageIdentifier(connection.getMetaData(), myConstraintName); + sqlStatements = generateSql(getTableName(), constraintName, getDriverType()); + } - for (String next : sqls) { + for (@Language("SQL") String next : sqlStatements) { executeSql(getTableName(), next); } } diff --git a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java index 9d3cfba3c6f..869d5921c40 100644 --- a/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java +++ b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java @@ -334,12 +334,13 @@ public void addPrimaryKey(String theVersion, String... theColumnsInOrder) { * @param theFkName the name of the foreign key * @param theParentTableName the name of the table that exports the foreign key */ - public void dropForeignKey(String theVersion, String theFkName, String theParentTableName) { + public BuilderCompleteTask dropForeignKey(String theVersion, String theFkName, String theParentTableName) { DropForeignKeyTask task = new DropForeignKeyTask(myRelease, theVersion); task.setConstraintName(theFkName); task.setTableName(getTableName()); task.setParentTableName(theParentTableName); addTask(task); + return new BuilderCompleteTask(task); } public BuilderCompleteTask renameTable(String theVersion, String theNewTableName) { diff --git a/hapi-tinder-plugin/src/main/java/ca/uhn/fhir/tinder/ddl/DdlGeneratorHibernate61.java b/hapi-tinder-plugin/src/main/java/ca/uhn/fhir/tinder/ddl/DdlGeneratorHibernate61.java index 413b0290756..dc547e2febe 100644 --- a/hapi-tinder-plugin/src/main/java/ca/uhn/fhir/tinder/ddl/DdlGeneratorHibernate61.java +++ b/hapi-tinder-plugin/src/main/java/ca/uhn/fhir/tinder/ddl/DdlGeneratorHibernate61.java @@ -1,5 +1,6 @@ package ca.uhn.fhir.tinder.ddl; +import ca.uhn.fhir.jpa.migrate.util.SqlUtil; import ca.uhn.fhir.jpa.util.ISequenceValueMassager; import ca.uhn.fhir.util.IoUtil; import ca.uhn.hapi.fhir.sql.hibernatesvc.HapiHibernateDialectSettingsService; @@ -7,6 +8,7 @@ import jakarta.persistence.Entity; import jakarta.persistence.MappedSuperclass; import org.apache.commons.io.FileUtils; +import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.Validate; import org.apache.maven.plugin.MojoFailureException; import org.apache.maven.project.MavenProject; @@ -35,6 +37,7 @@ import java.io.Closeable; import java.io.File; +import java.io.FileReader; import java.io.FileWriter; import java.io.IOException; import java.io.Writer; @@ -47,8 +50,10 @@ import java.util.ArrayList; import java.util.EnumSet; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Set; +import java.util.regex.Pattern; import static org.apache.commons.lang3.StringUtils.isNotBlank; @@ -145,6 +150,44 @@ public void generateDdl() throws MojoFailureException { schemaExport.execute(targetTypes, action, metadata, standardRegistry); writeContentsToFile(nextDialect.getAppendFile(), classLoader, outputFile); + + if (nextDialect.getDropStatementsContainingRegex() != null + && !nextDialect.getDropStatementsContainingRegex().isEmpty()) { + ourLog.info( + "Dropping statements containing regex(s): {}", nextDialect.getDropStatementsContainingRegex()); + try { + String fullFile; + try (FileReader fr = new FileReader(outputFileName, StandardCharsets.UTF_8)) { + fullFile = IOUtils.toString(fr); + } + + int count = 0; + List statements = SqlUtil.splitSqlFileIntoStatements(fullFile); + for (Iterator statementIter = statements.iterator(); statementIter.hasNext(); ) { + String statement = statementIter.next(); + if (nextDialect.getDropStatementsContainingRegex().stream() + .anyMatch(regex -> Pattern.compile(regex) + .matcher(statement) + .find())) { + statementIter.remove(); + count++; + } + } + + ourLog.info( + "Filtered {} statement(s) from file for dialect: {}", count, nextDialect.getClassName()); + + try (FileWriter fw = new FileWriter(outputFileName, StandardCharsets.UTF_8)) { + for (String statement : statements) { + fw.append(statement); + fw.append(";\n\n"); + } + } + + } catch (IOException theE) { + throw new RuntimeException(theE); + } + } } IoUtil.closeQuietly(connectionProvider); diff --git a/hapi-tinder-plugin/src/main/java/ca/uhn/fhir/tinder/ddl/GenerateDdlMojo.java b/hapi-tinder-plugin/src/main/java/ca/uhn/fhir/tinder/ddl/GenerateDdlMojo.java index 014e78429c7..6b60f73b76e 100644 --- a/hapi-tinder-plugin/src/main/java/ca/uhn/fhir/tinder/ddl/GenerateDdlMojo.java +++ b/hapi-tinder-plugin/src/main/java/ca/uhn/fhir/tinder/ddl/GenerateDdlMojo.java @@ -113,6 +113,7 @@ public static class Dialect { private String targetFileName; private String prependFile; private String appendFile; + private List dropStatementsContainingRegex; public Dialect() { super(); @@ -124,6 +125,14 @@ public Dialect(String theClassName, String theTargetFileName) { setTargetFileName(theTargetFileName); } + public List getDropStatementsContainingRegex() { + return dropStatementsContainingRegex; + } + + public void setDropStatementsContainingRegex(List theDropStatementsContainingRegex) { + dropStatementsContainingRegex = theDropStatementsContainingRegex; + } + public String getAppendFile() { return appendFile; } diff --git a/pom.xml b/pom.xml index f6ff522893e..8f90c1d0821 100644 --- a/pom.xml +++ b/pom.xml @@ -2528,6 +2528,11 @@ maven-dependency-plugin 3.8.1 + + org.apache.maven.plugins + maven-deploy-plugin + 3.1.3 + org.sonatype.plugins nexus-staging-maven-plugin @@ -3011,60 +3016,6 @@ - - - - org.apache.maven.plugins - maven-changes-plugin - ${maven_changes_version} - false - - - - changes-report - - - - - atom_1.0 - - https://github.com/hapifhir/hapi-fhir/issues/%ISSUE% - - false - - - - org.apache.maven.plugins - maven-surefire-report-plugin - ${surefire_version} - - - - failsafe-report-only - - - - - - ${project.basedir}/hapi-fhir-base/target/surefire-reports/ - ${project.basedir}/hapi-fhir-structures-dstu/target/surefire-reports/ - - ${project.basedir}/hapi-fhir-structures-dstu2/target/surefire-reports/ - - ${project.basedir}/hapi-fhir-jpaserver-base/target/surefire-reports/ - - - - - - org.apache.maven.plugins - maven-project-info-reports-plugin - 3.0.0 - false - - - - ERRORPRONE