From 77cbeb33c41c742c5404811b5bdf2db5e32fb55f Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 2 Jan 2025 18:39:25 -0500 Subject: [PATCH 1/9] Drop tag definition constraint --- .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 3 + .../jpa/dao/JpaStorageResourceParser.java | 10 ++ .../tasks/HapiFhirJpaMigrationTasks.java | 10 ++ .../ca/uhn/fhir/jpa/model/entity/BaseTag.java | 14 ++- .../jpa/dao/r4/FhirResourceDaoR4TagsTest.java | 102 +++++++++++++++--- 5 files changed, 121 insertions(+), 18 deletions(-) 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 b7627bf88be5..aede003ad5af 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 @@ -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 125bf6f4f7f4..6d0243351cea 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,17 @@ 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 29c6aba1567d..7aca59e6cbc5 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,16 @@ 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("20250102.10", "FKbfcjbaftmiwr3rxkwsy23vneo", "HFJ_TAG_DEF"); + version.onTable("HFJ_HISTORY_TAG") + .dropForeignKey("20250102.20", "FKtderym7awj6q8iq5c51xv4ndw", "HFJ_TAG_DEF"); + } protected void init780_afterPartitionChanges() { 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 5c502fd35a32..a45c8166e6fb 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 @@ -20,9 +20,13 @@ package ca.uhn.fhir.jpa.model.entity; 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 +35,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) diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java index f67f5af08ebe..4fa956204e77 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java @@ -2,12 +2,17 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; +import ca.uhn.fhir.jpa.dao.JpaStorageResourceParser; import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.gclient.TokenClientParam; import ca.uhn.fhir.util.BundleBuilder; +import ca.uhn.test.util.LogbackTestExtension; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.spi.ILoggingEvent; +import jakarta.annotation.Nonnull; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.Bundle; import org.hl7.fhir.r4.model.Enumerations; @@ -17,8 +22,10 @@ import org.hl7.fhir.r4.model.SearchParameter; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; -import jakarta.annotation.Nonnull; import java.util.List; import java.util.stream.Collectors; @@ -32,6 +39,9 @@ public class FhirResourceDaoR4TagsTest extends BaseResourceProviderR4Test { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoR4TagsTest.class); + @RegisterExtension + private LogbackTestExtension myLogbackTestExtension = new LogbackTestExtension(JpaStorageResourceParser.class, Level.WARN); + @Override @AfterEach public final void after() throws Exception { @@ -182,6 +192,66 @@ public void testDeleteResourceWithTags_NonVersionedTags_InTransaction() { } + /** + * 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 + @ValueSource(strings = {"read", "vread", "search", "history", "update"}) + public void testTagDefinitionDeleted(String theOperation) { + // Setup + 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("http://security", "security0", "display0"); + patient.getMeta().addSecurity("http://security", "security1", "display1"); + 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); + } + + /** * Make sure tags are preserved */ @@ -510,21 +580,6 @@ public void testInlineTags_Search_Security() { validatePatientSearchResultsForInlineTags(outcome); } - @Nonnull - public static SearchParameter createSecuritySearchParameter(FhirContext fhirContext) { - SearchParameter searchParameter = new SearchParameter(); - searchParameter.setId("SearchParameter/resource-security"); - for (String next : fhirContext.getResourceTypes().stream().sorted().collect(Collectors.toList())) { - searchParameter.addBase(next); - } - searchParameter.setStatus(Enumerations.PublicationStatus.ACTIVE); - searchParameter.setType(Enumerations.SearchParamType.TOKEN); - searchParameter.setCode("_security"); - searchParameter.setName("Security"); - searchParameter.setExpression("meta.security"); - return searchParameter; - } - private void validatePatientSearchResultsForInlineTags(Bundle outcome) { Patient patient; patient = (Patient) outcome.getEntry().get(0).getResource(); @@ -599,6 +654,21 @@ private void initializeVersioned() { assertEquals("2", myPatientDao.update(patient, mySrd).getId().getVersionIdPart()); } + @Nonnull + public static SearchParameter createSecuritySearchParameter(FhirContext fhirContext) { + SearchParameter searchParameter = new SearchParameter(); + searchParameter.setId("SearchParameter/resource-security"); + for (String next : fhirContext.getResourceTypes().stream().sorted().collect(Collectors.toList())) { + searchParameter.addBase(next); + } + searchParameter.setStatus(Enumerations.PublicationStatus.ACTIVE); + searchParameter.setType(Enumerations.SearchParamType.TOKEN); + searchParameter.setCode("_security"); + searchParameter.setName("Security"); + searchParameter.setExpression("meta.security"); + return searchParameter; + } + @Nonnull static List toTags(Patient patient) { return toTags(patient.getMeta()); From 1b39807d2b6638b1246c8fa38f33ba07fd4ae75b Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 2 Jan 2025 18:44:43 -0500 Subject: [PATCH 2/9] Add changelog --- .../7_8_0/6582-drop-tag-fk-constraint.yaml | 4 ++++ .../jpa/dao/r4/FhirResourceDaoR4TagsTest.java | 21 +++++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6582-drop-tag-fk-constraint.yaml 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 000000000000..e866502b2ee9 --- /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." diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java index 4fa956204e77..49bfea138789 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java @@ -24,6 +24,7 @@ import org.junit.jupiter.api.Test; 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; @@ -198,9 +199,22 @@ public void testDeleteResourceWithTags_NonVersionedTags_InTransaction() { * definitions get deleted. This test makes sure we act sanely when that happens. */ @ParameterizedTest - @ValueSource(strings = {"read", "vread", "search", "history", "update"}) - public void testTagDefinitionDeleted(String theOperation) { + @CsvSource(value = { + "NONVERSIONED, read", + "NONVERSIONED, vread", + "NONVERSIONED, search", + "NONVERSIONED, history", + "NONVERSIONED, 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"); @@ -211,6 +225,9 @@ public void testTagDefinitionDeleted(String theOperation) { patient.getMeta().addSecurity("http://security", "security1", "display1"); myPatientDao.update(patient, mySrd); + patient.setActive(false); + myPatientDao.update(patient, mySrd); + // Test runInTransaction(() -> { assertEquals(6, myTagDefinitionDao.count()); From db60adf6fda8629e61dbeacb435b8b360c5d2efd Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 3 Jan 2025 05:55:49 -0500 Subject: [PATCH 3/9] Add tests --- .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 9 ++++---- .../ca/uhn/fhir/jpa/model/entity/BaseTag.java | 7 +++++++ .../jpa/model/entity/ResourceHistoryTag.java | 4 ++-- .../fhir/jpa/model/entity/ResourceTag.java | 2 +- .../jpa/dao/r4/FhirResourceDaoR4TagsTest.java | 21 ++++++++++--------- 5 files changed, 26 insertions(+), 17 deletions(-) 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 aede003ad5af..d428715ef234 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,18 @@ 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())) { + tagDefinition.getSystem())) { theEntity.getTags().remove(tag); } } 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 a45c8166e6fb..d945be797f19 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,6 +19,7 @@ */ package ca.uhn.fhir.jpa.model.entity; +import jakarta.annotation.Nullable; import jakarta.persistence.Column; import jakarta.persistence.ConstraintMode; import jakarta.persistence.ForeignKey; @@ -53,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 3467a68afe5b..64ddda7eba9f 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/ResourceTag.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceTag.java index bda0dd2a84f9..29ff08da8281 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-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java index 49bfea138789..127c02f18871 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java @@ -200,16 +200,16 @@ public void testDeleteResourceWithTags_NonVersionedTags_InTransaction() { */ @ParameterizedTest @CsvSource(value = { - "NONVERSIONED, read", - "NONVERSIONED, vread", - "NONVERSIONED, search", - "NONVERSIONED, history", - "NONVERSIONED, update", - "VERSIONED, read", - "VERSIONED, vread", - "VERSIONED, search", - "VERSIONED, history", - "VERSIONED, update" + "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 @@ -229,6 +229,7 @@ public void testTagDefinitionDeleted(JpaStorageSettings.TagStorageModeEnum theTa myPatientDao.update(patient, mySrd); // Test + runInTransaction(() -> { assertEquals(6, myTagDefinitionDao.count()); myTagDefinitionDao.deleteAll(); From c21646c6fa8d8915add7d320c926dc25a1303909 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 3 Jan 2025 06:00:54 -0500 Subject: [PATCH 4/9] Add tests --- .../src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 3 +-- .../java/ca/uhn/fhir/jpa/dao/JpaStorageResourceParser.java | 4 +++- .../fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java | 7 ++----- pom.xml | 5 +++++ 4 files changed, 11 insertions(+), 8 deletions(-) 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 d428715ef234..974c316924c8 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 @@ -681,8 +681,7 @@ private boolean updateTags( if (tagDefinition != null && !allResourceTagsFromTheResource.contains(tag)) { if (shouldDroppedTagBeRemovedOnUpdate(theRequest, tag)) { theEntity.getTags().remove(tag); - } else if (HapiExtensions.EXT_SUBSCRIPTION_MATCHING_STRATEGY.equals( - tagDefinition.getSystem())) { + } else if (HapiExtensions.EXT_SUBSCRIPTION_MATCHING_STRATEGY.equals(tagDefinition.getSystem())) { theEntity.getTags().remove(tag); } } 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 6d0243351cea..2dcaead52381 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 @@ -495,7 +495,9 @@ private R populateResourceMetadataRi( 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()); + ourLog.warn( + "Tag definition HFJ_TAG_DEF#{} is missing, returned Resource.meta may not be complete", + next.getTagId()); haveWarnedForMissingTag = true; } continue; 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 7aca59e6cbc5..194271cae167 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 @@ -384,11 +384,8 @@ protected void init780() { * These two constraints were the last two that we had that used * hibernate-generated names. Yay! */ - version.onTable("HFJ_RES_TAG") - .dropForeignKey("20250102.10", "FKbfcjbaftmiwr3rxkwsy23vneo", "HFJ_TAG_DEF"); - version.onTable("HFJ_HISTORY_TAG") - .dropForeignKey("20250102.20", "FKtderym7awj6q8iq5c51xv4ndw", "HFJ_TAG_DEF"); - + version.onTable("HFJ_RES_TAG").dropForeignKey("20250102.10", "FKbfcjbaftmiwr3rxkwsy23vneo", "HFJ_TAG_DEF"); + version.onTable("HFJ_HISTORY_TAG").dropForeignKey("20250102.20", "FKtderym7awj6q8iq5c51xv4ndw", "HFJ_TAG_DEF"); } protected void init780_afterPartitionChanges() { diff --git a/pom.xml b/pom.xml index ba100ec13a25..d98e7703af08 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 From a2a29319156331b981b493aee18e427d282250b9 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 3 Jan 2025 08:15:11 -0500 Subject: [PATCH 5/9] Test fix --- .../java/ca/uhn/fhir/jpa/migrate/JdbcUtils.java | 9 +++++---- .../jpa/migrate/taskdef/DropForeignKeyTask.java | 13 ++++++++++--- 2 files changed, 15 insertions(+), 7 deletions(-) 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 2bbee486af56..79274cec5784 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 40fd601ba619..909a547067d5 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); } } From a86bd730501a8eeee60f73ba1bcfebfa357e0eb2 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 15 Jan 2025 17:54:25 -0500 Subject: [PATCH 6/9] Also drop resource link target FK --- .../7_8_0/6582-drop-tag-fk-constraint.yaml | 2 +- ...tionModeIdFilteringMappingContributor.java | 43 +++-- .../tasks/HapiFhirJpaMigrationTasks.java | 6 +- .../fhir/jpa/model/entity/ResourceLink.java | 4 +- .../jpa/dao/r4/FhirResourceDaoR4TagsTest.java | 120 ++----------- .../jpa/dao/r5/DeletedDatabaseRowsR5Test.java | 159 ++++++++++++++++++ 6 files changed, 210 insertions(+), 124 deletions(-) create mode 100644 hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/DeletedDatabaseRowsR5Test.java 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 index e866502b2ee9..40d7e536fea6 100644 --- 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 @@ -1,4 +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." +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 c99f5b223c02..2002ff4c5676 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,8 @@ 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 +366,24 @@ 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/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 194271cae167..735ef048b670 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 @@ -384,8 +384,10 @@ protected void init780() { * These two constraints were the last two that we had that used * hibernate-generated names. Yay! */ - version.onTable("HFJ_RES_TAG").dropForeignKey("20250102.10", "FKbfcjbaftmiwr3rxkwsy23vneo", "HFJ_TAG_DEF"); - version.onTable("HFJ_HISTORY_TAG").dropForeignKey("20250102.20", "FKtderym7awj6q8iq5c51xv4ndw", "HFJ_TAG_DEF"); + version.onTable("HFJ_RES_TAG").dropForeignKey("20250115.10", "FKbfcjbaftmiwr3rxkwsy23vneo", "HFJ_TAG_DEF"); + version.onTable("HFJ_HISTORY_TAG").dropForeignKey("20250115.20", "FKtderym7awj6q8iq5c51xv4ndw", "HFJ_TAG_DEF"); + + version.onTable("HFJ_RES_LINK").dropForeignKey("20250115.30", "FK_RESLINK_TARGET", "HFJ_RESOURCE"); } protected void init780_afterPartitionChanges() { 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 0a7dee4b6307..58a1f501bf82 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 @@ -21,6 +21,7 @@ import ca.uhn.fhir.jpa.model.dao.JpaPid; import jakarta.persistence.Column; +import jakarta.persistence.ConstraintMode; import jakarta.persistence.Entity; import jakarta.persistence.FetchType; import jakarta.persistence.ForeignKey; @@ -68,7 +69,6 @@ public class ResourceLink extends BaseResourceIndex { private static final long serialVersionUID = 1L; public static final String TARGET_RES_PARTITION_ID = "TARGET_RES_PARTITION_ID"; public static final String TARGET_RESOURCE_ID = "TARGET_RESOURCE_ID"; - public static final String FK_RESLINK_TARGET = "FK_RESLINK_TARGET"; @GenericGenerator(name = "SEQ_RESLINK_ID", type = ca.uhn.fhir.jpa.model.dialect.HapiSequenceStyleGenerator.class) @GeneratedValue(strategy = GenerationType.AUTO, generator = "SEQ_RESLINK_ID") @@ -124,7 +124,7 @@ public class ResourceLink extends BaseResourceIndex { insertable = false, updatable = false), }, - foreignKey = @ForeignKey(name = FK_RESLINK_TARGET)) + foreignKey = @ForeignKey(ConstraintMode.NO_CONSTRAINT)) private ResourceTable myTargetResource; @Transient diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java index 127c02f18871..f67f5af08ebe 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4TagsTest.java @@ -2,17 +2,12 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; -import ca.uhn.fhir.jpa.dao.JpaStorageResourceParser; import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.gclient.TokenClientParam; import ca.uhn.fhir.util.BundleBuilder; -import ca.uhn.test.util.LogbackTestExtension; -import ch.qos.logback.classic.Level; -import ch.qos.logback.classic.spi.ILoggingEvent; -import jakarta.annotation.Nonnull; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.Bundle; import org.hl7.fhir.r4.model.Enumerations; @@ -22,11 +17,8 @@ import org.hl7.fhir.r4.model.SearchParameter; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; -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 jakarta.annotation.Nonnull; import java.util.List; import java.util.stream.Collectors; @@ -40,9 +32,6 @@ public class FhirResourceDaoR4TagsTest extends BaseResourceProviderR4Test { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoR4TagsTest.class); - @RegisterExtension - private LogbackTestExtension myLogbackTestExtension = new LogbackTestExtension(JpaStorageResourceParser.class, Level.WARN); - @Override @AfterEach public final void after() throws Exception { @@ -193,83 +182,6 @@ public void testDeleteResourceWithTags_NonVersionedTags_InTransaction() { } - /** - * 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("http://security", "security0", "display0"); - patient.getMeta().addSecurity("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); - } - - /** * Make sure tags are preserved */ @@ -598,6 +510,21 @@ public void testInlineTags_Search_Security() { validatePatientSearchResultsForInlineTags(outcome); } + @Nonnull + public static SearchParameter createSecuritySearchParameter(FhirContext fhirContext) { + SearchParameter searchParameter = new SearchParameter(); + searchParameter.setId("SearchParameter/resource-security"); + for (String next : fhirContext.getResourceTypes().stream().sorted().collect(Collectors.toList())) { + searchParameter.addBase(next); + } + searchParameter.setStatus(Enumerations.PublicationStatus.ACTIVE); + searchParameter.setType(Enumerations.SearchParamType.TOKEN); + searchParameter.setCode("_security"); + searchParameter.setName("Security"); + searchParameter.setExpression("meta.security"); + return searchParameter; + } + private void validatePatientSearchResultsForInlineTags(Bundle outcome) { Patient patient; patient = (Patient) outcome.getEntry().get(0).getResource(); @@ -672,21 +599,6 @@ private void initializeVersioned() { assertEquals("2", myPatientDao.update(patient, mySrd).getId().getVersionIdPart()); } - @Nonnull - public static SearchParameter createSecuritySearchParameter(FhirContext fhirContext) { - SearchParameter searchParameter = new SearchParameter(); - searchParameter.setId("SearchParameter/resource-security"); - for (String next : fhirContext.getResourceTypes().stream().sorted().collect(Collectors.toList())) { - searchParameter.addBase(next); - } - searchParameter.setStatus(Enumerations.PublicationStatus.ACTIVE); - searchParameter.setType(Enumerations.SearchParamType.TOKEN); - searchParameter.setCode("_security"); - searchParameter.setName("Security"); - searchParameter.setExpression("meta.security"); - return searchParameter; - } - @Nonnull static List toTags(Patient patient) { return toTags(patient.getMeta()); 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 000000000000..e5a26d12a19b --- /dev/null +++ b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/DeletedDatabaseRowsR5Test.java @@ -0,0 +1,159 @@ +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.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); + } + + /** + * There is no FK constraint on {@link ResourceLink#getTargetResource()} 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. + */ + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testReferenceWithTargetExpunged(boolean theSynchronous) { + // Test + 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); + }); + + logAllResources(); + logAllResourceVersions(); + + 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"); + + } + +} From 7a18de1284c13ee345dc6d123dee5a373a56ef34 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 15 Jan 2025 17:54:54 -0500 Subject: [PATCH 7/9] Spotless --- ...PartitionModeIdFilteringMappingContributor.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) 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 2002ff4c5676..76e9c4a47606 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 @@ -196,11 +196,11 @@ private void removePartitionedIdColumnsFromMetadata( Value value = property.getValue(); if (value instanceof ToOne) { ToOne toOne = (ToOne) value; - filterPropertiesFromToOneRelationship(theClassLoaderService, theMetadata, table, entityPersistentClass.getClassName(), toOne); + 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 @@ -355,7 +355,8 @@ private void filterPartitionedIdsFromLocalFks( Value value = theForeignKey.getColumn(0).getValue(); if (value instanceof ToOne) { ToOne manyToOne = (ToOne) value; - Set columnNamesToRemoveFromFks = filterPropertiesFromToOneRelationship(theClassLoaderService, theMetadata, theTable, theEntityTypeName, manyToOne); + Set columnNamesToRemoveFromFks = filterPropertiesFromToOneRelationship( + theClassLoaderService, theMetadata, theTable, theEntityTypeName, manyToOne); removeColumns(theForeignKey.getColumns(), t1 -> columnNamesToRemoveFromFks.contains(t1.getName())); } else { @@ -367,7 +368,12 @@ private void filterPartitionedIdsFromLocalFks( } @Nonnull - private Set filterPropertiesFromToOneRelationship(ClassLoaderService theClassLoaderService, InFlightMetadataCollector theMetadata, Table theTable, String theEntityTypeName, ToOne manyToOne) { + private Set filterPropertiesFromToOneRelationship( + ClassLoaderService theClassLoaderService, + InFlightMetadataCollector theMetadata, + Table theTable, + String theEntityTypeName, + ToOne manyToOne) { String targetTableName = theMetadata .getEntityBindingMap() .get(manyToOne.getReferencedEntityName()) From b06c0e10624717d1aaea8da15044b2bc0f6c75d7 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 16 Jan 2025 08:39:45 -0500 Subject: [PATCH 8/9] Only drop HFJ_RES_LINK target FK when we're on Postgres/MSSQL/Oracle mode --- .../tasks/HapiFhirJpaMigrationTasks.java | 16 +++++- .../fhir/jpa/model/entity/ResourceLink.java | 10 +++- .../InstanceReindexServiceImplR5Test.java | 4 +- .../fhir/jpa/migrate/tasks/api/Builder.java | 3 +- pom.xml | 54 ------------------- 5 files changed, 28 insertions(+), 59 deletions(-) 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 735ef048b670..a7fb1f09509d 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 @@ -387,7 +387,21 @@ protected void init780() { version.onTable("HFJ_RES_TAG").dropForeignKey("20250115.10", "FKbfcjbaftmiwr3rxkwsy23vneo", "HFJ_TAG_DEF"); version.onTable("HFJ_HISTORY_TAG").dropForeignKey("20250115.20", "FKtderym7awj6q8iq5c51xv4ndw", "HFJ_TAG_DEF"); - version.onTable("HFJ_RES_LINK").dropForeignKey("20250115.30", "FK_RESLINK_TARGET", "HFJ_RESOURCE"); + /* + * 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-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 58a1f501bf82..3978970e0914 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 @@ -21,7 +21,6 @@ import ca.uhn.fhir.jpa.model.dao.JpaPid; import jakarta.persistence.Column; -import jakarta.persistence.ConstraintMode; import jakarta.persistence.Entity; import jakarta.persistence.FetchType; import jakarta.persistence.ForeignKey; @@ -69,6 +68,7 @@ public class ResourceLink extends BaseResourceIndex { private static final long serialVersionUID = 1L; public static final String TARGET_RES_PARTITION_ID = "TARGET_RES_PARTITION_ID"; public static final String TARGET_RESOURCE_ID = "TARGET_RESOURCE_ID"; + public static final String FK_RESLINK_TARGET = "FK_RESLINK_TARGET"; @GenericGenerator(name = "SEQ_RESLINK_ID", type = ca.uhn.fhir.jpa.model.dialect.HapiSequenceStyleGenerator.class) @GeneratedValue(strategy = GenerationType.AUTO, generator = "SEQ_RESLINK_ID") @@ -124,7 +124,13 @@ public class ResourceLink extends BaseResourceIndex { insertable = false, updatable = false), }, - foreignKey = @ForeignKey(ConstraintMode.NO_CONSTRAINT)) + /* + * 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; @Transient 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 b348c446bb3b..a69451365b5b 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/tasks/api/Builder.java b/hapi-fhir-sql-migrate/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/api/Builder.java index 9d3cfba3c6f1..869d5921c40d 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/pom.xml b/pom.xml index 70b34d882441..8f90c1d08211 100644 --- a/pom.xml +++ b/pom.xml @@ -3016,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 From b5debcfbef32ffd834ffc25609438d8c55a4b3ca Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 16 Jan 2025 09:30:27 -0500 Subject: [PATCH 9/9] Cleanup and test fixes --- hapi-fhir-jpaserver-base/pom.xml | 6 ++ .../fhir/jpa/entity/GeneratedSchemaTest.java | 40 ++++++--- .../jpa/dao/r5/DeletedDatabaseRowsR5Test.java | 85 +++++++++++-------- .../tinder/ddl/DdlGeneratorHibernate61.java | 43 ++++++++++ .../uhn/fhir/tinder/ddl/GenerateDdlMojo.java | 9 ++ 5 files changed, 134 insertions(+), 49 deletions(-) diff --git a/hapi-fhir-jpaserver-base/pom.xml b/hapi-fhir-jpaserver-base/pom.xml index ef1baaf98f19..9bb165070040 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/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 a41126af2a6d..df9698fe3ee2 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-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 index e5a26d12a19b..8516d309cab3 100644 --- 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 @@ -7,6 +7,7 @@ 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; @@ -112,48 +113,60 @@ public void testTagDefinitionDeleted(JpaStorageSettings.TagStorageModeEnum theTa } /** - * There is no FK constraint on {@link ResourceLink#getTargetResource()} 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 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) { - // Test - 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); - }); - - logAllResources(); - logAllResourceVersions(); - + // Setup runInTransaction(()->{ - myResourceTableDao.deleteByPid(orgPid); + myEntityManager.createNativeQuery("alter table HFJ_RES_LINK drop constraint FK_RESLINK_TARGET").executeUpdate(); }); - - // 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"); - + 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-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 413b02907568..dc547e2febe0 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 014e78429c75..6b60f73b76ef 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; }