honeymoose
/
hapi-fhir
mirror of https://github.com/hapifhir/hapi-fhir.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Ensure Delete Expunge removes corresponding HFJ_RES_SEARCH_URL records for affected resources (#5940) 

* Delete HFJ_RES_SEARCH_URL record when delete expunging a resource.  Add a new foreign key on RES_ID to HFJ_RESOURCE to HFJ_RES_SEARCH_URL.  Add a migration task to set up the new FK.

* Cleanup logging.

* Fix unit test failures and ensure ResourceSearchUrlEntity is created with a ResourceTable.

* Remove TODO.

* Fix more unit tests.

* Add more unit test assertions.

* Add changelog.

* Code review feedback.

* Fix unit test failures due to lazy loading lack of session.

* Reverse original code review feedback changes.
This commit is contained in:
Luke deGruchy 2024-05-22 14:54:40 -04:00 committed by GitHub
parent 97cfb6de37
commit 0fe3380c4a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 121 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/5942-delete-expunge-targeted-doesnt-delete-search-url.yaml Normal file
View File

@ -0,0 +1,7 @@
---
type: fix
issue: 5942
title: "Delete expunge targeted to resources (not everything) that were created with IfNoneExists URL followed by
recreation of said resources was failing with HAPI-0550 due to the fact that the corresponding search url
records were not being deleted.
This has been fixed."

2
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java
View File

@ -575,7 +575,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
// Pre-cache the match URL, and create an entry in the HFJ_RES_SEARCH_URL table to
// protect against concurrent writes to the same conditional URL
if (theMatchUrl != null) {
myResourceSearchUrlSvc.enforceMatchUrlResourceUniqueness(getResourceName(), theMatchUrl, jpaPid);
myResourceSearchUrlSvc.enforceMatchUrlResourceUniqueness(getResourceName(), theMatchUrl, updatedEntity);
myMatchResourceUrlService.matchUrlResolved(theTransactionDetails, getResourceName(), theMatchUrl, jpaPid);
}

1
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceTableFKProvider.java
View File

@ -72,6 +72,7 @@ public class ResourceTableFKProvider {
retval.add(new ResourceForeignKey("NPM_PACKAGE_VER_RES", "BINARY_RES_ID"));
retval.add(new ResourceForeignKey("HFJ_SUBSCRIPTION_STATS", "RES_ID"));
retval.add(new ResourceForeignKey("HFJ_RES_SEARCH_URL", "RES_ID"));
return retval;
}

14
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/migrate/tasks/HapiFhirJpaMigrationTasks.java
View File

@ -121,6 +121,20 @@ public class HapiFhirJpaMigrationTasks extends BaseMigrationTasks<VersionEnum> {
init680_Part2();
init700();
init720();
init740();
}
protected void init740() {
// Start of migrations from 7.2 to 7.4
Builder version = forVersion(VersionEnum.V7_4_0);
{
version.onTable("HFJ_RES_SEARCH_URL")
.addForeignKey("20240515.1", "FK_RES_SEARCH_URL_RESOURCE")
.toColumn("RES_ID")
.references("HFJ_RESOURCE", "RES_ID");
}
}
protected void init720() {

6
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/ResourceSearchUrlSvc.java
View File

@ -22,8 +22,8 @@ package ca.uhn.fhir.jpa.search;
import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.context.RuntimeResourceDefinition;
import ca.uhn.fhir.jpa.dao.data.IResourceSearchUrlDao;
import ca.uhn.fhir.jpa.model.dao.JpaPid;
import ca.uhn.fhir.jpa.model.entity.ResourceSearchUrlEntity;
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
import jakarta.persistence.EntityManager;
@ -84,11 +84,11 @@ public class ResourceSearchUrlSvc {
* We store a record of match urls with res_id so a db constraint can catch simultaneous creates that slip through.
*/
public void enforceMatchUrlResourceUniqueness(
String theResourceName, String theMatchUrl, JpaPid theResourcePersistentId) {
String theResourceName, String theMatchUrl, ResourceTable theResourceTable) {
String canonicalizedUrlForStorage = createCanonicalizedUrlForStorage(theResourceName, theMatchUrl);
ResourceSearchUrlEntity searchUrlEntity =
ResourceSearchUrlEntity.from(canonicalizedUrlForStorage, theResourcePersistentId.getId());
ResourceSearchUrlEntity.from(canonicalizedUrlForStorage, theResourceTable);
// calling dao.save performs a merge operation which implies a trip to
// the database to see if the resource exists. Since we don't need the check, we avoid the trip by calling
// em.persist.

30
hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceSearchUrlEntity.java
View File

@ -21,8 +21,12 @@ package ca.uhn.fhir.jpa.model.entity;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.ForeignKey;
import jakarta.persistence.Id;
import jakarta.persistence.Index;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.Table;
import jakarta.persistence.Temporal;
import jakarta.persistence.TemporalType;
@ -55,29 +59,49 @@ public class ResourceSearchUrlEntity {
@Column(name = RES_SEARCH_URL_COLUMN_NAME, length = RES_SEARCH_URL_LENGTH, nullable = false)
private String mySearchUrl;
@Column(name = "RES_ID", updatable = false, nullable = false)
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(
name = "RES_ID",
nullable = false,
updatable = false,
foreignKey = @ForeignKey(name = "FK_RES_SEARCH_URL_RESOURCE"))
private ResourceTable myResourceTable;
@Column(name = "RES_ID", updatable = false, nullable = false, insertable = false)
private Long myResourcePid;
@Column(name = "CREATED_TIME", nullable = false)
@Temporal(TemporalType.TIMESTAMP)
private Date myCreatedTime;
public static ResourceSearchUrlEntity from(String theUrl, Long theId) {
public static ResourceSearchUrlEntity from(String theUrl, ResourceTable theResourceTable) {
return new ResourceSearchUrlEntity()
.setResourcePid(theId)
.setResourceTable(theResourceTable)
.setSearchUrl(theUrl)
.setCreatedTime(new Date());
}
public Long getResourcePid() {
if (myResourcePid != null) {
return myResourcePid;
}
return myResourceTable.getResourceId();
}
public ResourceSearchUrlEntity setResourcePid(Long theResourcePid) {
myResourcePid = theResourcePid;
return this;
}
public ResourceTable getResourceTable() {
return myResourceTable;
}
public ResourceSearchUrlEntity setResourceTable(ResourceTable myResourceTable) {
this.myResourceTable = myResourceTable;
return this;
}
public Date getCreatedTime() {
return myCreatedTime;
}

35
hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ConcurrentCreateTest.java
View File

@ -7,6 +7,7 @@ import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
import ca.uhn.fhir.jpa.dao.data.IResourceSearchUrlDao;
import ca.uhn.fhir.jpa.interceptor.UserRequestRetryVersionConflictsInterceptor;
import ca.uhn.fhir.jpa.model.entity.ResourceSearchUrlEntity;
import ca.uhn.fhir.jpa.model.entity.ResourceTable;
import ca.uhn.fhir.jpa.search.ResourceSearchUrlSvc;
import ca.uhn.fhir.jpa.search.SearchUrlJobMaintenanceSvcImpl;
import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
@ -14,6 +15,7 @@ import ca.uhn.fhir.jpa.test.config.TestR4Config;
import ca.uhn.fhir.rest.api.server.RequestDetails;
import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
import ca.uhn.test.concurrency.PointcutLatch;
import jakarta.annotation.Nonnull;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.time.DateUtils;
import org.hl7.fhir.r4.model.Identifier;
@ -124,13 +126,18 @@ public class FhirResourceDaoR4ConcurrentCreateTest extends BaseJpaR4Test {
// given
long tenMinutes = 10 * DateUtils.MILLIS_PER_HOUR;
final ResourceTable resTable1 = myResourceTableDao.save(createResTable());
final ResourceTable resTable2 = myResourceTableDao.save(createResTable());
final ResourceTable resTable3 = myResourceTableDao.save(createResTable());
final ResourceTable resTable4 = myResourceTableDao.save(createResTable());
Date tooOldBy10Minutes = cutOffTimeMinus(tenMinutes);
ResourceSearchUrlEntity tooOld1 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.444", 1l).setCreatedTime(tooOldBy10Minutes);
ResourceSearchUrlEntity tooOld2 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.445", 2l).setCreatedTime(tooOldBy10Minutes);
ResourceSearchUrlEntity tooOld1 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.444", resTable1).setCreatedTime(tooOldBy10Minutes);
ResourceSearchUrlEntity tooOld2 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.445", resTable2).setCreatedTime(tooOldBy10Minutes);
Date tooNewBy10Minutes = cutOffTimePlus(tenMinutes);
ResourceSearchUrlEntity tooNew1 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.446", 3l).setCreatedTime(tooNewBy10Minutes);
ResourceSearchUrlEntity tooNew2 =ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.447", 4l).setCreatedTime(tooNewBy10Minutes);
ResourceSearchUrlEntity tooNew1 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.446", resTable3).setCreatedTime(tooNewBy10Minutes);
ResourceSearchUrlEntity tooNew2 =ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.447", resTable4).setCreatedTime(tooNewBy10Minutes);
myResourceSearchUrlDao.saveAll(asList(tooOld1, tooOld2, tooNew1, tooNew2));
@ -139,7 +146,7 @@ public class FhirResourceDaoR4ConcurrentCreateTest extends BaseJpaR4Test {
// then
List<Long> resourcesPids = getStoredResourceSearchUrlEntitiesPids();
assertThat(resourcesPids, containsInAnyOrder(3l, 4l));
assertThat(resourcesPids, containsInAnyOrder(resTable3.getResourceId(), resTable4.getResourceId()));
}
@Test
@ -155,8 +162,11 @@ public class FhirResourceDaoR4ConcurrentCreateTest extends BaseJpaR4Test {
// given
long nonExistentResourceId = 99l;
ResourceSearchUrlEntity entry1 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.444", 1l);
ResourceSearchUrlEntity entry2 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.445", 2l);
final ResourceTable resTable1 = myResourceTableDao.save(createResTable());
final ResourceTable resTable2 = myResourceTableDao.save(createResTable());
ResourceSearchUrlEntity entry1 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.444", resTable1);
ResourceSearchUrlEntity entry2 = ResourceSearchUrlEntity.from("Observation?identifier=20210427133226.445", resTable2);
myResourceSearchUrlDao.saveAll(asList(entry1, entry2));
// when
@ -165,7 +175,7 @@ public class FhirResourceDaoR4ConcurrentCreateTest extends BaseJpaR4Test {
// then
List<Long> resourcesPids = getStoredResourceSearchUrlEntitiesPids();
assertThat(resourcesPids, containsInAnyOrder(2l));
assertThat(resourcesPids, containsInAnyOrder(resTable2.getResourceId()));
}
@ -180,6 +190,15 @@ public class FhirResourceDaoR4ConcurrentCreateTest extends BaseJpaR4Test {
return new Date(offset);
}
@Nonnull
private static ResourceTable createResTable() {
final ResourceTable resourceTable = new ResourceTable();
resourceTable.setResourceType("Patient");
resourceTable.setPublished(new Date());
resourceTable.setUpdated(new Date());
return resourceTable;
}
private Date cutOffTimeMinus(long theAdjustment) {
return cutOffTimePlus(-theAdjustment);
}

39
hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4CreateTest.java
View File

@ -4,6 +4,7 @@ import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
import ca.uhn.fhir.jpa.model.dao.JpaPid;
import ca.uhn.fhir.jpa.model.entity.NormalizedQuantitySearchLevel;
import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamQuantity;
@ -48,6 +49,7 @@ import org.hl7.fhir.r4.model.Organization;
import org.hl7.fhir.r4.model.Patient;
import org.hl7.fhir.r4.model.Quantity;
import org.hl7.fhir.r4.model.Reference;
import org.hl7.fhir.r4.model.Resource;
import org.hl7.fhir.r4.model.SampledData;
import org.hl7.fhir.r4.model.SearchParameter;
import org.hl7.fhir.r4.model.StructureDefinition;
@ -60,10 +62,12 @@ import org.junit.jupiter.params.provider.ValueSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.data.domain.PageRequest;
import org.springframework.transaction.support.TransactionTemplate;
import java.io.IOException;
import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Date;
import java.util.List;
import java.util.Optional;
@ -85,7 +89,7 @@ import static org.hamcrest.Matchers.matchesPattern;
import static org.hamcrest.Matchers.notNullValue;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
@ -1294,17 +1298,18 @@ public class FhirResourceDaoR4CreateTest extends BaseJpaR4Test {
public void testConditionalCreateDependsOnFirstEntryExisting(boolean theHasQuestionMark) {
final BundleBuilder bundleBuilder = new BundleBuilder(myFhirContext);
bundleBuilder.addTransactionCreateEntry(myTask1, "urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea")
.conditional("identifier=http://tempuri.org|1");
final String firstMatchUrl = "identifier=http://tempuri.org|1";
final String secondEntryConditionalTemplate = "%sidentifier=http://tempuri.org|2&based-on=urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea";
final String secondMatchUrl = String.format(secondEntryConditionalTemplate, theHasQuestionMark ? "?" : "");
bundleBuilder.addTransactionCreateEntry(myTask1, "urn:uuid:59cda086-4763-4ef0-8e36-8c90058686ea")
.conditional(firstMatchUrl);
bundleBuilder.addTransactionCreateEntry(myTask2)
.conditional(secondMatchUrl);
final IBaseBundle requestBundle = bundleBuilder.getBundle();
assertTrue(requestBundle instanceof Bundle);
assertInstanceOf(Bundle.class, requestBundle);
final List<Bundle.BundleEntryComponent> responseEntries = sendBundleAndGetResponse(requestBundle);
@ -1324,6 +1329,30 @@ public class FhirResourceDaoR4CreateTest extends BaseJpaR4Test {
assertEquals(1, task2BasedOn.size());
final Reference task2BasedOnReference = task2BasedOn.get(0);
assertEquals(taskPostBundle1.getIdElement().toUnqualifiedVersionless().asStringValue(), task2BasedOnReference.getReference());
assertRemainingTasks(myTask1, myTask2);
deleteExpunge(myTask2);
assertRemainingTasks(myTask1);
deleteExpunge(myTask1);
assertRemainingTasks();
}
private void assertRemainingTasks(Task... theExpectedTasks) {
final List<ResourceSearchUrlEntity> searchUrlsPreDelete = myResourceSearchUrlDao.findAll();
assertEquals(theExpectedTasks.length, searchUrlsPreDelete.size());
assertEquals(Arrays.stream(theExpectedTasks).map(Resource::getIdElement).map(IdType::getIdPartAsLong).toList(),
searchUrlsPreDelete.stream().map(ResourceSearchUrlEntity::getResourcePid).toList());
}
private void deleteExpunge(Task theTask) {
final JpaPid pidOrThrowException = myIdHelperService.getPidOrThrowException(theTask);
final List<JpaPid> pidOrThrowException1 = List.of(pidOrThrowException);
final TransactionTemplate transactionTemplate = new TransactionTemplate(getTxManager());
transactionTemplate.execute(x -> myDeleteExpungeSvc.deleteExpunge(pidOrThrowException1, true, 10));
}
}

2
hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java
View File

@ -863,7 +863,7 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
assertEquals(1, myCaptureQueriesListener.countSelectQueriesForCurrentThread());
assertEquals(0, myCaptureQueriesListener.countUpdateQueriesForCurrentThread());
assertEquals(0, myCaptureQueriesListener.countInsertQueriesForCurrentThread());
assertEquals(28, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
assertEquals(29, myCaptureQueriesListener.countDeleteQueriesForCurrentThread());
assertEquals(10, outcome.getRecordsProcessed());
runInTransaction(()-> assertEquals(0, myResourceTableDao.count()));
}

6
hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/BaseJpaR4Test.java
View File

@ -34,6 +34,7 @@ import ca.uhn.fhir.jpa.api.dao.IFhirResourceDaoStructureDefinition;
import ca.uhn.fhir.jpa.api.dao.IFhirResourceDaoSubscription;
import ca.uhn.fhir.jpa.api.dao.IFhirResourceDaoValueSet;
import ca.uhn.fhir.jpa.api.dao.IFhirSystemDao;
import ca.uhn.fhir.jpa.api.svc.IDeleteExpungeSvc;
import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
import ca.uhn.fhir.jpa.api.svc.ISearchCoordinatorSvc;
import ca.uhn.fhir.jpa.binary.interceptor.BinaryStorageInterceptor;
@ -82,6 +83,7 @@ import ca.uhn.fhir.jpa.entity.TermValueSet;
import ca.uhn.fhir.jpa.entity.TermValueSetConcept;
import ca.uhn.fhir.jpa.interceptor.PerformanceTracingLoggingInterceptor;
import ca.uhn.fhir.jpa.model.config.PartitionSettings;
import ca.uhn.fhir.jpa.model.dao.JpaPid;
import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
import ca.uhn.fhir.jpa.packages.IPackageInstallerSvc;
import ca.uhn.fhir.jpa.partition.IPartitionLookupSvc;
@ -534,7 +536,7 @@ public abstract class BaseJpaR4Test extends BaseJpaTest implements ITestDataBuil
@Autowired
protected DaoRegistry myDaoRegistry;
@Autowired
protected IIdHelperService myIdHelperService;
protected IIdHelperService<JpaPid> myIdHelperService;
@Autowired
protected ValidationSettings myValidationSettings;
@Autowired
@ -554,6 +556,8 @@ public abstract class BaseJpaR4Test extends BaseJpaTest implements ITestDataBuil
private MdmStorageInterceptor myMdmStorageInterceptor;
@Autowired
protected TestDaoSearch myTestDaoSearch;
@Autowired
protected IDeleteExpungeSvc<JpaPid> myDeleteExpungeSvc;
@Autowired
protected IJobMaintenanceService myJobMaintenanceService;