From b2c4f7b384bbbbed7d50be4c6b7a32be47945bfb Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 4 Dec 2024 18:18:28 -0500 Subject: [PATCH 1/9] Backport #6460 to 7.6.0 --- .../fhir/jpa/dao/TransactionProcessor.java | 154 ++++++++++++++---- .../fhir/jpa/dao/index/IdHelperService.java | 62 +++---- .../r4/FhirResourceDaoR4QueryCountTest.java | 75 ++++++++- .../jpa/dao/BaseTransactionProcessor.java | 24 +++ .../dao/index/DaoResourceLinkResolver.java | 3 +- .../CircularQueueCaptureQueriesListener.java | 35 ++++ 6 files changed, 288 insertions(+), 65 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java index 86d9f727906e..fa76749f2381 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java @@ -30,21 +30,23 @@ import ca.uhn.fhir.jpa.model.dao.JpaPid; import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken; import ca.uhn.fhir.jpa.model.entity.StorageSettings; -import ca.uhn.fhir.jpa.partition.IRequestPartitionHelperSvc; import ca.uhn.fhir.jpa.searchparam.MatchUrlService; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; -import ca.uhn.fhir.jpa.util.QueryChunker; import ca.uhn.fhir.model.api.IQueryParameterType; import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.api.server.storage.TransactionDetails; import ca.uhn.fhir.rest.param.TokenParam; +import ca.uhn.fhir.util.FhirTerser; import ca.uhn.fhir.util.ResourceReferenceInfo; import ca.uhn.fhir.util.StopWatch; +import ca.uhn.fhir.util.TaskChunker; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ListMultimap; +import com.google.common.collect.MultimapBuilder; import jakarta.annotation.Nullable; import jakarta.persistence.EntityManager; +import jakarta.persistence.FlushModeType; import jakarta.persistence.PersistenceContext; import jakarta.persistence.PersistenceContextType; import jakarta.persistence.PersistenceException; @@ -83,6 +85,7 @@ public class TransactionProcessor extends BaseTransactionProcessor { public static final Pattern SINGLE_PARAMETER_MATCH_URL_PATTERN = Pattern.compile("^[^?]+[?][a-z0-9-]+=[^&,]+$"); private static final Logger ourLog = LoggerFactory.getLogger(TransactionProcessor.class); + public static final int CONDITIONAL_URL_FETCH_CHUNK_SIZE = 100; @Autowired private ApplicationContext myApplicationContext; @@ -108,9 +111,6 @@ public class TransactionProcessor extends BaseTransactionProcessor { @Autowired private MatchUrlService myMatchUrlService; - @Autowired - private IRequestPartitionHelperSvc myRequestPartitionSvc; - public void setEntityManagerForUnitTest(EntityManager theEntityManager) { myEntityManager = theEntityManager; } @@ -146,25 +146,51 @@ protected EntriesToProcessMap doTransactionWriteOperations( List theEntries, StopWatch theTransactionStopWatch) { - ITransactionProcessorVersionAdapter versionAdapter = getVersionAdapter(); - RequestPartitionId requestPartitionId = - super.determineRequestPartitionIdForWriteEntries(theRequest, theEntries); + /* + * We temporarily set the flush mode for the duration of the DB transaction + * from the default of AUTO to the temporary value of COMMIT here. We do this + * because in AUTO mode, if any SQL SELECTs are required during the + * processing of an individual transaction entry, the server will flush the + * pending INSERTs/UPDATEs to the database before executing the SELECT. + * This hurts performance since we don't get the benefit of batching those + * write operations as much as possible. The tradeoff here is that we + * could theoretically have transaction operations which try to read + * data previously written in the same transaction, and they won't see it. + * This shouldn't actually be an issue anyhow - we pre-fetch conditional + * URLs and reference targets at the start of the transaction. But this + * tradeoff still feels worth it, since the most common use of transactions + * is for fast writing of data. + * + * Note that it's probably not necessary to reset it back, it should + * automatically go back to the default value after the transaction but + * we reset it just to be safe. + */ + FlushModeType initialFlushMode = myEntityManager.getFlushMode(); + try { + myEntityManager.setFlushMode(FlushModeType.COMMIT); - if (requestPartitionId != null) { - preFetch(theTransactionDetails, theEntries, versionAdapter, requestPartitionId); - } + ITransactionProcessorVersionAdapter versionAdapter = getVersionAdapter(); + RequestPartitionId requestPartitionId = + super.determineRequestPartitionIdForWriteEntries(theRequest, theEntries); - return super.doTransactionWriteOperations( - theRequest, - theActionName, - theTransactionDetails, - theAllIds, - theIdSubstitutions, - theIdToPersistedOutcome, - theResponse, - theOriginalRequestOrder, - theEntries, - theTransactionStopWatch); + if (requestPartitionId != null) { + preFetch(theTransactionDetails, theEntries, versionAdapter, requestPartitionId); + } + + return super.doTransactionWriteOperations( + theRequest, + theActionName, + theTransactionDetails, + theAllIds, + theIdSubstitutions, + theIdToPersistedOutcome, + theResponse, + theOriginalRequestOrder, + theEntries, + theTransactionStopWatch); + } finally { + myEntityManager.setFlushMode(initialFlushMode); + } } private void preFetch( @@ -199,24 +225,60 @@ private void preFetchResourcesById( RequestPartitionId theRequestPartitionId, Set foundIds, List idsToPreFetch) { - List idsToPreResolve = new ArrayList<>(); + + FhirTerser terser = myFhirContext.newTerser(); + + Set idsToPreResolve = new HashSet<>(); + Set idsToPreResolveIdOnly = new HashSet<>(); + for (IBase nextEntry : theEntries) { IBaseResource resource = theVersionAdapter.getResource(nextEntry); if (resource != null) { String verb = theVersionAdapter.getEntryRequestVerb(myFhirContext, nextEntry); + + /* + * Pre-fetch any resources that are potentially being directly updated by ID + */ if ("PUT".equals(verb) || "PATCH".equals(verb)) { String requestUrl = theVersionAdapter.getEntryRequestUrl(nextEntry); - if (countMatches(requestUrl, '/') == 1 && countMatches(requestUrl, '?') == 0) { + if (countMatches(requestUrl, '?') == 0) { IIdType id = myFhirContext.getVersion().newIdType(); id.setValue(requestUrl); - idsToPreResolve.add(id); + IIdType unqualifiedVersionless = id.toUnqualifiedVersionless(); + idsToPreResolve.add(unqualifiedVersionless); + } + } + + /* + * Pre-fetch any resources that are referred to directly by ID (don't replace + * the TRUE flag with FALSE in case we're updating a resource but also + * pointing to that resource elsewhere in the bundle) + */ + if ("PUT".equals(verb) || "POST".equals(verb)) { + for (ResourceReferenceInfo referenceInfo : terser.getAllResourceReferences(resource)) { + IIdType reference = referenceInfo.getResourceReference().getReferenceElement(); + if (reference != null + && !reference.isLocal() + && !reference.isUuid() + && reference.hasResourceType() + && reference.hasIdPart() + && !reference.getValue().contains("?")) { + idsToPreResolveIdOnly.add(reference.toUnqualifiedVersionless()); + } } } } } - List outcome = - myIdHelperService.resolveResourcePersistentIdsWithCache(theRequestPartitionId, idsToPreResolve).stream() - .collect(Collectors.toList()); + + idsToPreResolveIdOnly.removeAll(idsToPreResolve); + + /* + * Pre-resolve any IDs that we'll need to prefetch entirely (this applies to + * resources that are being updated by the transaction itself, so we need to + * load everything about the resource) + */ + List outcome = myIdHelperService.resolveResourcePersistentIdsWithCache( + theRequestPartitionId, List.copyOf(idsToPreResolve)); for (JpaPid next : outcome) { foundIds.add( next.getAssociatedResourceId().toUnqualifiedVersionless().getValue()); @@ -231,6 +293,38 @@ private void preFetchResourcesById( theTransactionDetails.addResolvedResourceId(next.toUnqualifiedVersionless(), null); } } + + /* + * Pre-resolve any IDs that are references to other resources outside of the + * transaction bundle. These ones don't need to be fully loaded since we're not + * updating them, we just need to verify that they exist and aren't deleted + * so that we know we can link against them. Doing this here means that + * we're not resolving them one-by-one. + */ + ListMultimap typeToIds = + MultimapBuilder.hashKeys().arrayListValues().build(); + for (IIdType next : idsToPreResolveIdOnly) { + typeToIds.put(next.getResourceType(), next.getIdPart()); + } + for (String resourceType : typeToIds.keySet()) { + List ids = typeToIds.get(resourceType); + Map resolvedIds = + myIdHelperService.resolveResourcePersistentIds(theRequestPartitionId, resourceType, ids, true); + for (Map.Entry entries : resolvedIds.entrySet()) { + IIdType id = myFhirContext.getVersion().newIdType(); + id.setValue(resourceType + "/" + entries.getKey()); + JpaPid pid = entries.getValue(); + pid.setAssociatedResourceId(id); + theTransactionDetails.addResolvedResourceId(id, pid); + } + } + } + + @Override + protected void handleVerbChangeInTransactionWriteOperations() { + super.handleVerbChangeInTransactionWriteOperations(); + + myEntityManager.flush(); } private void preFetchConditionalUrls( @@ -274,10 +368,10 @@ private void preFetchConditionalUrls( } } - new QueryChunker() + new TaskChunker() .chunk( searchParameterMapsToResolve, - 100, + CONDITIONAL_URL_FETCH_CHUNK_SIZE, map -> preFetchSearchParameterMaps( theTransactionDetails, theRequestPartitionId, map, idsToPreFetch)); } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index 9e81e87f598c..2dd2af9b4346 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -59,7 +59,6 @@ import org.hl7.fhir.instance.model.api.IAnyResource; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; -import org.hl7.fhir.r4.model.IdType; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.stereotype.Service; import org.springframework.transaction.support.TransactionSynchronizationManager; @@ -214,36 +213,43 @@ public Map resolveResourcePersistentIds( Validate.isTrue(!theIds.isEmpty(), "theIds must not be empty"); Map retVals = new HashMap<>(); - for (String id : theIds) { - JpaPid retVal; - if (!idRequiresForcedId(id)) { + + ArrayList remainingIds = new ArrayList<>(); + for (Iterator iter = theIds.iterator(); iter.hasNext(); ) { + String nextId = iter.next(); + IIdType nextIdType = myFhirCtx.getVersion().newIdType(); + nextIdType.setValue(theResourceType + "/" + nextId); + + if (!theExcludeDeleted && !idRequiresForcedId(nextId)) { // is already a PID - retVal = JpaPid.fromId(Long.parseLong(id)); - retVals.put(id, retVal); - } else { - // is a forced id - // we must resolve! - if (myStorageSettings.isDeleteEnabled()) { - retVal = resolveResourceIdentity(theRequestPartitionId, theResourceType, id, theExcludeDeleted) - .getPersistentId(); - retVals.put(id, retVal); - } else { - // fetch from cache... adding to cache if not available - String key = toForcedIdToPidKey(theRequestPartitionId, theResourceType, id); - retVal = myMemoryCacheService.getThenPutAfterCommit( - MemoryCacheService.CacheEnum.FORCED_ID_TO_PID, key, t -> { - List ids = Collections.singletonList(new IdType(theResourceType, id)); - // fetches from cache using a function that checks cache first... - List resolvedIds = - resolveResourcePersistentIdsWithCache(theRequestPartitionId, ids); - if (resolvedIds.isEmpty()) { - throw new ResourceNotFoundException(Msg.code(1100) + ids.get(0)); - } - return resolvedIds.get(0); - }); - retVals.put(id, retVal); + JpaPid pid = JpaPid.fromId(Long.parseLong(nextId)); + pid.setAssociatedResourceId(nextIdType); + retVals.put(nextId, pid); + continue; + } else if (!myStorageSettings.isDeleteEnabled()) { + String key = toForcedIdToPidKey(theRequestPartitionId, theResourceType, nextId); + JpaPid pid = myMemoryCacheService.getIfPresent(MemoryCacheService.CacheEnum.FORCED_ID_TO_PID, key); + if (pid != null) { + pid.setAssociatedResourceId(nextIdType); + retVals.put(nextId, pid); + continue; } } + + remainingIds.add(nextIdType); + } + + if (!remainingIds.isEmpty()) { + List output = new ArrayList<>(); + doResolvePersistentIds(theRequestPartitionId, remainingIds, output); + for (JpaPid next : output) { + assert next.getAssociatedResourceId() != null; + retVals.put(next.getAssociatedResourceId().getIdPart(), next); + } + + if (output.size() < remainingIds.size()) { + throw new ResourceNotFoundException(""); + } } return retVals; diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java index ba9a31d12cb5..07751579bbb9 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java @@ -100,6 +100,7 @@ import java.util.Collections; import java.util.Date; import java.util.List; +import java.util.UUID; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -248,6 +249,68 @@ public void testExpungeAllVersionsWithTagsDeletesRow() { } + @Test + public void testTransactionWithManyResourceLinks() { + // Setup + List patientIds = new ArrayList<>(); + List orgIds = new ArrayList<>(); + List coverageIds = new ArrayList<>(); + AtomicInteger counter = new AtomicInteger(0); + for (int i = 0; i < 10; i++) { + // Server-assigned numeric ID + coverageIds.add(createResource("Coverage", withStatus("active"))); + // Client-assigned string ID + patientIds.add(createPatient(withId(UUID.randomUUID().toString()), withActiveTrue())); + orgIds.add(createOrganization(withId(UUID.randomUUID().toString()), withName("FOO"))); + } + + Supplier creator = () -> { + int nextCount = counter.getAndIncrement(); + assert nextCount < coverageIds.size(); + + ExplanationOfBenefit retVal = new ExplanationOfBenefit(); + retVal.getMeta().addTag().setSystem("http://foo").setCode(Integer.toString(nextCount % 3)); + retVal.setId(UUID.randomUUID().toString()); + retVal.setPatient(new Reference(patientIds.get(nextCount))); + retVal.setInsurer(new Reference(orgIds.get(nextCount))); + retVal.addInsurance().setCoverage(new Reference(coverageIds.get(nextCount))); + return retVal; + }; + + Supplier transactionCreator = () -> { + BundleBuilder bb = new BundleBuilder(myFhirContext); + bb.addTransactionUpdateEntry(creator.get()); + bb.addTransactionUpdateEntry(creator.get()); + bb.addTransactionUpdateEntry(creator.get()); + bb.addTransactionUpdateEntry(creator.get()); + bb.addTransactionUpdateEntry(creator.get()); + return bb.getBundleTyped(); + }; + + // Test + myCaptureQueriesListener.clear(); + mySystemDao.transaction(mySrd, transactionCreator.get()); + myCaptureQueriesListener.logInsertQueries(); + myCaptureQueriesListener.logSelectQueries(); + + // Verify + assertEquals(33, myCaptureQueriesListener.countInsertQueries()); + assertEquals(7, myCaptureQueriesListener.countSelectQueries()); + assertEquals(0, myCaptureQueriesListener.countUpdateQueries()); + assertEquals(0, myCaptureQueriesListener.countDeleteQueries()); + + // Test again + myCaptureQueriesListener.clear(); + mySystemDao.transaction(mySrd, transactionCreator.get()); + + // Verify again + assertEquals(30, myCaptureQueriesListener.countInsertQueries()); + assertEquals(4, myCaptureQueriesListener.countSelectQueries()); + assertEquals(0, myCaptureQueriesListener.countUpdateQueries()); + assertEquals(0, myCaptureQueriesListener.countDeleteQueries()); + assertEquals(0, myCaptureQueriesListener.countInsertQueriesRepeated()); + } + /* * See the class javadoc before changing the counts in this test! */ @@ -2793,15 +2856,15 @@ public void testTransactionWithMultiplePreExistingReferences_ForcedId_DeletesDis Bundle output = mySystemDao.transaction(mySrd, input); ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(output)); - // Lookup the two existing IDs to make sure they are legit + // No lookups expected because deletes are disabled + myCaptureQueriesListener.logSelectQueriesForCurrentThread(); - assertEquals(2, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); + assertEquals(0, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); assertEquals(10, myCaptureQueriesListener.countInsertQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countDeleteQueriesForCurrentThread()); - // Do the same a second time - Deletes are enabled so we expect to have to resolve the - // targets again to make sure they weren't deleted + // Do the same a second time input = new Bundle(); @@ -2866,7 +2929,7 @@ public void testTransactionWithMultiplePreExistingReferences_Numeric_DeletesDisa // Lookup the two existing IDs to make sure they are legit myCaptureQueriesListener.logSelectQueriesForCurrentThread(); - assertEquals(0, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); + assertEquals(2, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); assertEquals(10, myCaptureQueriesListener.countInsertQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countDeleteQueriesForCurrentThread()); @@ -3164,7 +3227,7 @@ public void testTransaction_ComboParamIndexesInUse() { mySystemDao.transaction(mySrd, inputBundle); assertEquals(21, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size()); assertEquals(0, myCaptureQueriesListener.getUpdateQueriesForCurrentThread().size()); - assertEquals(78, myCaptureQueriesListener.getInsertQueriesForCurrentThread().size()); + assertEquals(7, myCaptureQueriesListener.getInsertQueriesForCurrentThread().size()); assertEquals(0, myCaptureQueriesListener.getDeleteQueriesForCurrentThread().size()); // Now run the transaction again - It should not need too many SELECTs diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index d4f96ae6372e..6418ee1adf26 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -1133,6 +1133,7 @@ protected EntriesToProcessMap doTransactionWriteOperations( * Loop through the request and process any entries of type * PUT, POST or DELETE */ + String previousVerb = null; for (int i = 0; i < theEntries.size(); i++) { if (i % 250 == 0) { ourLog.debug("Processed {} non-GET entries out of {} in transaction", i, theEntries.size()); @@ -1144,6 +1145,11 @@ protected EntriesToProcessMap doTransactionWriteOperations( IIdType nextResourceId = getNextResourceIdFromBaseResource(res, nextReqEntry, theAllIds); String verb = myVersionAdapter.getEntryRequestVerb(myContext, nextReqEntry); + if (previousVerb != null && !previousVerb.equals(verb)) { + handleVerbChangeInTransactionWriteOperations(); + } + previousVerb = verb; + String resourceType = res != null ? myContext.getResourceType(res) : null; Integer order = theOriginalRequestOrder.get(nextReqEntry); IBase nextRespEntry = @@ -1402,6 +1408,8 @@ protected EntriesToProcessMap doTransactionWriteOperations( theTransactionStopWatch.endCurrentTask(); } + postTransactionProcess(theTransactionDetails); + /* * Make sure that there are no conflicts from deletions. E.g. we can't delete something * if something else has a reference to it.. Unless the thing that has a reference to it @@ -1502,6 +1510,22 @@ protected EntriesToProcessMap doTransactionWriteOperations( } } + /** + * Subclasses may override this in order to invoke specific operations when + * we're finished handling all the write entries in the transaction bundle + * with a given verb. + */ + protected void handleVerbChangeInTransactionWriteOperations() { + // nothing + } + + /** + * Implement to handle post transaction processing + */ + protected void postTransactionProcess(TransactionDetails theTransactionDetails) { + // nothing + } + /** * Check for if a resource id should be matched in a conditional update * If the FHIR version is older than R4, it follows the old specifications and does not match diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoResourceLinkResolver.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoResourceLinkResolver.java index 9434e1269368..6992f6114b3c 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoResourceLinkResolver.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/index/DaoResourceLinkResolver.java @@ -110,7 +110,8 @@ public IResourceLookup findTargetResource( T persistentId = null; if (theTransactionDetails != null) { - T resolvedResourceId = (T) theTransactionDetails.getResolvedResourceId(targetResourceId); + IIdType unqualifiedVersionless = targetResourceId.toUnqualifiedVersionless(); + T resolvedResourceId = (T) theTransactionDetails.getResolvedResourceId(unqualifiedVersionless); if (resolvedResourceId != null && resolvedResourceId.getId() != null && resolvedResourceId.getAssociatedResourceId() != null) { diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/util/CircularQueueCaptureQueriesListener.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/util/CircularQueueCaptureQueriesListener.java index 33f73f60e7a4..84cb85d04a48 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/util/CircularQueueCaptureQueriesListener.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/util/CircularQueueCaptureQueriesListener.java @@ -32,9 +32,11 @@ import java.util.Arrays; import java.util.Collections; import java.util.Date; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Queue; +import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -187,6 +189,13 @@ public List getInsertQueries() { return getQueriesStartingWith("insert"); } + /** + * Returns all INSERT queries executed on the current thread - Index 0 is oldest + */ + public List getInsertQueries(Predicate theFilter) { + return getQueriesStartingWith("insert").stream().filter(theFilter).collect(Collectors.toList()); + } + /** * Returns all UPDATE queries executed on the current thread - Index 0 is oldest */ @@ -402,6 +411,17 @@ public int countInsertQueries() { return countQueries(getInsertQueries()); } + /** + * This method returns a count of insert queries which were issued more than + * once. If a query was issued twice with batched parameters, that does not + * count as a repeated query, but if it is issued as two separate insert + * statements with a single set of parameters this counts as a repeated + * query. + */ + public int countInsertQueriesRepeated() { + return getInsertQueries(new DuplicateCheckingPredicate()).size(); + } + public int countUpdateQueries() { return countQueries(getUpdateQueries()); } @@ -467,4 +487,19 @@ static String formatQueryAsSql(SqlQuery theQuery, boolean inlineParams, boolean b.append("\n"); return b.toString(); } + + /** + * Create a new instance of this each time you use it + */ + private static class DuplicateCheckingPredicate implements Predicate { + private final Set mySeenSql = new HashSet<>(); + + @Override + public boolean test(SqlQuery theSqlQuery) { + String sql = theSqlQuery.getSql(false, false); + boolean retVal = !mySeenSql.add(sql); + ourLog.trace("SQL duplicate[{}]: {}", retVal, sql); + return retVal; + } + } } From e1b4e08dacc117fc8cea1882dc4067444bd3a571 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 4 Dec 2024 18:25:20 -0500 Subject: [PATCH 2/9] Add changelog --- .../7_6_1/6538-improve-transaction-performance.yaml | 7 +++++++ .../java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_1/6538-improve-transaction-performance.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_1/6538-improve-transaction-performance.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_1/6538-improve-transaction-performance.yaml new file mode 100644 index 000000000000..705b36ee6596 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_1/6538-improve-transaction-performance.yaml @@ -0,0 +1,7 @@ +--- +type: perf +issue: 6538 +title: "In HAPI FHIR 8.0.0, transaction processing has been significantly improved thanks + to ticket [#6460](https://github.com/hapifhir/hapi-fhir/pull/6460). This enhancement + has been partially backported to the 7.6.x release line in order to provide partial improvement + prior to the release of HAPI FHIR 8.0.0." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index 2dd2af9b4346..87b29081a5ee 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -248,7 +248,7 @@ public Map resolveResourcePersistentIds( } if (output.size() < remainingIds.size()) { - throw new ResourceNotFoundException(""); + throw new ResourceNotFoundException(Msg.code(1100) + remainingIds.get(0)); } } From e815ce0aee901c17c1176fe5134d03ae9cc3c6dc Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 5 Dec 2024 06:08:05 -0500 Subject: [PATCH 3/9] Test fixes --- .../fhir/jpa/dao/TransactionProcessor.java | 39 +++++++++++++---- .../fhir/jpa/dao/index/IdHelperService.java | 3 +- .../jpa/dao/index/IdHelperServiceTest.java | 43 ++++++++++--------- .../jpa/dao/dstu2/FhirSystemDaoDstu2Test.java | 2 +- ...temProviderTransactionSearchDstu2Test.java | 2 + .../dao/dstu3/FhirResourceDaoDstu3Test.java | 2 +- .../jpa/dao/dstu3/FhirSystemDaoDstu3Test.java | 6 +-- .../fhir/jpa/dao/r4/FhirSystemDaoR4Test.java | 6 +-- .../jpa/dao/BaseTransactionProcessor.java | 2 +- 9 files changed, 65 insertions(+), 40 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java index fa76749f2381..ae25f0e28474 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java @@ -36,6 +36,7 @@ import ca.uhn.fhir.rest.api.server.RequestDetails; import ca.uhn.fhir.rest.api.server.storage.TransactionDetails; import ca.uhn.fhir.rest.param.TokenParam; +import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import ca.uhn.fhir.util.FhirTerser; import ca.uhn.fhir.util.ResourceReferenceInfo; import ca.uhn.fhir.util.StopWatch; @@ -165,9 +166,25 @@ protected EntriesToProcessMap doTransactionWriteOperations( * automatically go back to the default value after the transaction but * we reset it just to be safe. */ + FlushModeType flushMode = FlushModeType.COMMIT; + for (IBase entry : theEntries) { + IBaseResource res = myVersionAdapter.getResource(entry); + if (res != null) { + String type = myFhirContext.getResourceType(res); + // These types write additional tables during the entity write + if ("ValueSet".equals(type) + || "CodeSystem".equals(type) + || "Subscription".equals(type) + || "ConceptMap".equals(type)) { + flushMode = FlushModeType.AUTO; + break; + } + } + } + FlushModeType initialFlushMode = myEntityManager.getFlushMode(); try { - myEntityManager.setFlushMode(FlushModeType.COMMIT); + myEntityManager.setFlushMode(flushMode); ITransactionProcessorVersionAdapter versionAdapter = getVersionAdapter(); RequestPartitionId requestPartitionId = @@ -308,14 +325,18 @@ private void preFetchResourcesById( } for (String resourceType : typeToIds.keySet()) { List ids = typeToIds.get(resourceType); - Map resolvedIds = - myIdHelperService.resolveResourcePersistentIds(theRequestPartitionId, resourceType, ids, true); - for (Map.Entry entries : resolvedIds.entrySet()) { - IIdType id = myFhirContext.getVersion().newIdType(); - id.setValue(resourceType + "/" + entries.getKey()); - JpaPid pid = entries.getValue(); - pid.setAssociatedResourceId(id); - theTransactionDetails.addResolvedResourceId(id, pid); + try { + Map resolvedIds = + myIdHelperService.resolveResourcePersistentIds(theRequestPartitionId, resourceType, ids, true); + for (Map.Entry entries : resolvedIds.entrySet()) { + IIdType id = myFhirContext.getVersion().newIdType(); + id.setValue(resourceType + "/" + entries.getKey()); + JpaPid pid = entries.getValue(); + pid.setAssociatedResourceId(id); + theTransactionDetails.addResolvedResourceId(id, pid); + } + } catch (ResourceNotFoundException e) { + ourLog.debug("Ignoring not found exception, it will be surfaced later in the processing", e); } } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index 87b29081a5ee..6d21fe2d8ad3 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -248,7 +248,8 @@ public Map resolveResourcePersistentIds( } if (output.size() < remainingIds.size()) { - throw new ResourceNotFoundException(Msg.code(1100) + remainingIds.get(0)); + throw new ResourceNotFoundException( + Msg.code(1100) + "Resource " + remainingIds.get(0) + " is not known"); } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java index a96a38325b3c..1d49fe69f196 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java @@ -11,12 +11,15 @@ import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import jakarta.persistence.EntityManager; import jakarta.persistence.Tuple; +import jakarta.persistence.TypedQuery; +import jakarta.persistence.criteria.CriteriaQuery; import jakarta.persistence.criteria.Path; import jakarta.persistence.criteria.Root; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Answers; import org.mockito.ArgumentMatchers; import org.mockito.InjectMocks; import org.mockito.Mock; @@ -30,6 +33,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -60,13 +64,19 @@ public class IdHelperServiceTest { @Mock private MemoryCacheService myMemoryCacheService; - @Mock + @Mock(answer = Answers.RETURNS_DEEP_STUBS) private EntityManager myEntityManager; @Mock private PartitionSettings myPartitionSettings; - @BeforeEach + @Mock + private TypedQuery myQuery; + + @Mock + private Tuple myTuple; + + @BeforeEach void setUp() { myHelperSvc.setDontCheckActiveTransactionForUnitTest(true); @@ -90,9 +100,11 @@ public void testResolveResourcePersistentIds() { // configure mock behaviour when(myStorageSettings.isDeleteEnabled()).thenReturn(true); + when(myEntityManager.createQuery(any(CriteriaQuery.class))).thenReturn(myQuery); + when(myQuery.getResultList()).thenReturn(List.of()); final ResourceNotFoundException resourceNotFoundException = assertThrows(ResourceNotFoundException.class, () -> myHelperSvc.resolveResourcePersistentIds(requestPartitionId, resourceType, ids, theExcludeDeleted)); - assertEquals("HAPI-2001: Resource Patient/123 is not known", resourceNotFoundException.getMessage()); + assertEquals("HAPI-1100: Resource Patient/123 is not known", resourceNotFoundException.getMessage()); } @Test @@ -111,29 +123,18 @@ public void testResolveResourcePersistentIdsDeleteFalse() { // configure mock behaviour when(myStorageSettings.isDeleteEnabled()).thenReturn(false); + when(myStorageSettings.isDeleteEnabled()).thenReturn(true); + when(myEntityManager.createQuery(any(CriteriaQuery.class))).thenReturn(myQuery); + when(myQuery.getResultList()).thenReturn(List.of(myTuple)); + when(myTuple.get(eq(0), eq(Long.class))).thenReturn(id); + when(myTuple.get(eq(1), eq(String.class))).thenReturn(resourceType); + when(myTuple.get(eq(2), eq(String.class))).thenReturn(Long.toString(id)); Map actualIds = myHelperSvc.resolveResourcePersistentIds(requestPartitionId, resourceType, ids, theExcludeDeleted); //verifyResult assertFalse(actualIds.isEmpty()); - assertNull(actualIds.get(ids.get(0))); + assertEquals(JpaPid.fromId(123L), actualIds.get(ids.get(0))); } - - private Root getMockedFrom() { - @SuppressWarnings("unchecked") - Path path = mock(Path.class); - @SuppressWarnings("unchecked") - Root from = mock(Root.class); - when(from.get(ArgumentMatchers.any())).thenReturn(path); - return from; - } - - private List getMockedTupleList(Long idNumber, String resourceType, String id) { - Tuple tuple = mock(Tuple.class); - when(tuple.get(eq(0), eq(Long.class))).thenReturn(idNumber); - when(tuple.get(eq(1), eq(String.class))).thenReturn(resourceType); - when(tuple.get(eq(2), eq(String.class))).thenReturn(id); - return List.of(tuple); - } } diff --git a/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirSystemDaoDstu2Test.java b/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirSystemDaoDstu2Test.java index 68210181c193..aa8a34a08c17 100644 --- a/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirSystemDaoDstu2Test.java +++ b/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/dao/dstu2/FhirSystemDaoDstu2Test.java @@ -178,7 +178,7 @@ public void testTransactionBatchWithFailingRead() { // Bundle.entry[2] is failed read response assertEquals(OperationOutcome.class, resp.getEntry().get(1).getResource().getClass()); assertEquals(IssueSeverityEnum.ERROR, ((OperationOutcome) resp.getEntry().get(1).getResource()).getIssue().get(0).getSeverityElement().getValueAsEnum()); - assertEquals(Msg.code(2001) + "Resource Patient/THIS_ID_DOESNT_EXIST is not known", ((OperationOutcome) resp.getEntry().get(1).getResource()).getIssue().get(0).getDiagnostics()); + assertEquals(Msg.code(1100) + "Resource Patient/THIS_ID_DOESNT_EXIST is not known", ((OperationOutcome) resp.getEntry().get(1).getResource()).getIssue().get(0).getDiagnostics()); assertEquals("404 Not Found", resp.getEntry().get(1).getResponse().getStatus()); // Check POST diff --git a/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/provider/SystemProviderTransactionSearchDstu2Test.java b/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/provider/SystemProviderTransactionSearchDstu2Test.java index be41667846ec..9d4975859a25 100644 --- a/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/provider/SystemProviderTransactionSearchDstu2Test.java +++ b/hapi-fhir-jpaserver-test-dstu2/src/test/java/ca/uhn/fhir/jpa/provider/SystemProviderTransactionSearchDstu2Test.java @@ -28,6 +28,7 @@ import org.junit.jupiter.api.Test; import java.util.ArrayList; +import java.util.Comparator; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -201,6 +202,7 @@ public void testBatchWithManyGets() { @Test public void testTransactionWithGetHardLimitLargeSynchronous() { List ids = create20Patients(); + ids.sort(Comparator.naturalOrder()); Bundle input = new Bundle(); input.setType(BundleTypeEnum.TRANSACTION); diff --git a/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3Test.java b/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3Test.java index f193c55af7c2..3993f64ec36e 100644 --- a/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3Test.java +++ b/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirResourceDaoDstu3Test.java @@ -1675,7 +1675,7 @@ public void testHistoryWithInvalidId() throws Exception { myPatientDao.history(new IdType("Patient/FOOFOOFOO"), null, null, null, mySrd); fail(""); } catch (ResourceNotFoundException e) { - assertEquals(Msg.code(2001) + "Resource Patient/FOOFOOFOO is not known", e.getMessage()); + assertEquals(Msg.code(1100) + "Resource Patient/FOOFOOFOO is not known", e.getMessage()); } } diff --git a/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java b/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java index 501e8049b602..c1b50c6dcf4f 100644 --- a/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java +++ b/hapi-fhir-jpaserver-test-dstu3/src/test/java/ca/uhn/fhir/jpa/dao/dstu3/FhirSystemDaoDstu3Test.java @@ -284,7 +284,7 @@ public void testBatchCreateWithBadRead() { OperationOutcome oo = (OperationOutcome) response.getEntry().get(1).getResponse().getOutcome(); ourLog.debug(myFhirContext.newXmlParser().setPrettyPrint(true).encodeResourceToString(oo)); assertEquals(IssueSeverity.ERROR, oo.getIssue().get(0).getSeverity()); - assertEquals(Msg.code(2001) + "Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); + assertEquals(Msg.code(1100) + "Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); } @Test @@ -658,7 +658,7 @@ public void testTransactionBatchWithFailingRead() { Resource oo = resp.getEntry().get(1).getResponse().getOutcome(); assertEquals(OperationOutcome.class, oo.getClass()); assertEquals(IssueSeverity.ERROR, ((OperationOutcome) oo).getIssue().get(0).getSeverityElement().getValue()); - assertEquals(Msg.code(2001) + "Resource Patient/THIS_ID_DOESNT_EXIST is not known", ((OperationOutcome) oo).getIssue().get(0).getDiagnostics()); + assertEquals(Msg.code(1100) + "Resource Patient/THIS_ID_DOESNT_EXIST is not known", ((OperationOutcome) oo).getIssue().get(0).getDiagnostics()); assertEquals("404 Not Found", resp.getEntry().get(1).getResponse().getStatus()); // Check POST @@ -969,7 +969,7 @@ public void testTransactionCreateWithBadRead() { OperationOutcome oo = (OperationOutcome) response.getEntry().get(1).getResponse().getOutcome(); ourLog.debug(myFhirContext.newXmlParser().setPrettyPrint(true).encodeResourceToString(oo)); assertEquals(IssueSeverity.ERROR, oo.getIssue().get(0).getSeverity()); - assertEquals(Msg.code(2001) + "Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); + assertEquals(Msg.code(1100) + "Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); } @Test diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java index f8dde4f63ad5..c19aec5d7e90 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSystemDaoR4Test.java @@ -364,7 +364,7 @@ public void testBatchCreateWithBadRead() { OperationOutcome oo = (OperationOutcome) response.getEntry().get(1).getResponse().getOutcome(); ourLog.debug(myFhirContext.newXmlParser().setPrettyPrint(true).encodeResourceToString(oo)); assertEquals(IssueSeverity.ERROR, oo.getIssue().get(0).getSeverity()); - assertEquals(Msg.code(2001) + "Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); + assertEquals(Msg.code(1100) + "Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); } @Test @@ -881,7 +881,7 @@ public void testTransactionBatchWithFailingRead() { Resource oo = resp.getEntry().get(1).getResponse().getOutcome(); assertEquals(OperationOutcome.class, oo.getClass()); assertEquals(IssueSeverity.ERROR, ((OperationOutcome) oo).getIssue().get(0).getSeverityElement().getValue()); - assertEquals(Msg.code(2001) + "Resource Patient/THIS_ID_DOESNT_EXIST is not known", ((OperationOutcome) oo).getIssue().get(0).getDiagnostics()); + assertEquals(Msg.code(1100) + "Resource Patient/THIS_ID_DOESNT_EXIST is not known", ((OperationOutcome) oo).getIssue().get(0).getDiagnostics()); assertEquals("404 Not Found", resp.getEntry().get(1).getResponse().getStatus()); // Check POST @@ -2118,7 +2118,7 @@ public void testTransactionCreateWithBadRead() { OperationOutcome oo = (OperationOutcome) response.getEntry().get(1).getResponse().getOutcome(); ourLog.debug(myFhirContext.newXmlParser().setPrettyPrint(true).encodeResourceToString(oo)); assertEquals(IssueSeverity.ERROR, oo.getIssue().get(0).getSeverity()); - assertEquals(Msg.code(2001) + "Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); + assertEquals(Msg.code(1100) + "Resource Patient/BABABABA is not known", oo.getIssue().get(0).getDiagnostics()); } @Test diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java index 6418ee1adf26..f2aaf2161061 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseTransactionProcessor.java @@ -158,7 +158,7 @@ public abstract class BaseTransactionProcessor { @Autowired @SuppressWarnings("rawtypes") - private ITransactionProcessorVersionAdapter myVersionAdapter; + protected ITransactionProcessorVersionAdapter myVersionAdapter; @Autowired private DaoRegistry myDaoRegistry; From a566a59aeb76c114da44028b13f9456095674bc2 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 5 Dec 2024 07:46:34 -0500 Subject: [PATCH 4/9] Test fixes --- .../jpa/dao/index/IdHelperServiceTest.java | 1 - .../jpa/dao/index/IdHelperServiceTest.java | 56 ++++++++++++------- .../stresstest/GiantTransactionPerfTest.java | 4 +- .../r5/FhirSystemDaoTransactionR5Test.java | 18 +++--- 4 files changed, 46 insertions(+), 33 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java index 1d49fe69f196..86104716ab58 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java @@ -123,7 +123,6 @@ public void testResolveResourcePersistentIdsDeleteFalse() { // configure mock behaviour when(myStorageSettings.isDeleteEnabled()).thenReturn(false); - when(myStorageSettings.isDeleteEnabled()).thenReturn(true); when(myEntityManager.createQuery(any(CriteriaQuery.class))).thenReturn(myQuery); when(myQuery.getResultList()).thenReturn(List.of(myTuple)); when(myTuple.get(eq(0), eq(Long.class))).thenReturn(id); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java index fa0870d4e67a..f1b2d9b2eb3e 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/index/IdHelperServiceTest.java @@ -1,5 +1,6 @@ package ca.uhn.fhir.jpa.dao.index; +import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.interceptor.model.RequestPartitionId; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; import ca.uhn.fhir.jpa.dao.data.IResourceTableDao; @@ -7,13 +8,20 @@ import ca.uhn.fhir.jpa.model.cross.IResourceLookup; import ca.uhn.fhir.jpa.model.dao.JpaPid; import ca.uhn.fhir.jpa.util.MemoryCacheService; +import jakarta.persistence.EntityManager; +import jakarta.persistence.Tuple; +import jakarta.persistence.TypedQuery; +import jakarta.persistence.criteria.CriteriaQuery; +import org.hibernate.sql.results.internal.TupleImpl; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Answers; import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; +import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; import java.util.ArrayList; @@ -31,6 +39,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -48,6 +57,18 @@ public class IdHelperServiceTest { @Mock private PartitionSettings myPartitionSettings; + @Mock(answer = Answers.RETURNS_DEEP_STUBS) + private EntityManager myEntityManager; + + @Mock + private TypedQuery myQuery; + + @Mock + private Tuple myTuple; + + @Spy + private FhirContext myFhirContext = FhirContext.forR4Cached(); + @InjectMocks private IdHelperService myHelperService; @@ -84,29 +105,23 @@ public void resolveResourcePersistentIds_withForcedIdsAndDeleteEnabled_returnsMa patientIdsToResolve.add("BLUE"); Object[] redView = new Object[] { - "Patient", 123l, - "RED", - new Date(), - null, - null + "Patient", + "RED" }; Object[] blueView = new Object[] { - "Patient", 456l, - "BLUE", - new Date(), - null, - null + "Patient", + "BLUE" }; // when - when(myStorageSettings.isDeleteEnabled()) - .thenReturn(true); - when(myResourceTableDao.findAndResolveByForcedIdWithNoType(Mockito.anyString(), - Mockito.anyList(), Mockito.anyBoolean())) - .thenReturn(Collections.singletonList(redView)) - .thenReturn(Collections.singletonList(blueView)); + when(myStorageSettings.isDeleteEnabled()).thenReturn(true); + when(myEntityManager.createQuery(any(CriteriaQuery.class))).thenReturn(myQuery); + when(myQuery.getResultList()).thenReturn(List.of( + new TupleImpl(null, redView), + new TupleImpl(null, blueView) + )); // test Map map = myHelperService.resolveResourcePersistentIds( @@ -132,9 +147,8 @@ public void resolveResourcePersistenIds_withForcedIdAndDeleteDisabled_returnsMap JpaPid blue = JpaPid.fromIdAndVersion(456L, 456L); // we will pretend the lookup value is in the cache - when(myMemoryCacheService.getThenPutAfterCommit(any(MemoryCacheService.CacheEnum.class), - Mockito.anyString(), - any(Function.class))) + when(myMemoryCacheService.getIfPresent(any(MemoryCacheService.CacheEnum.class), + Mockito.anyString())) .thenReturn(red) .thenReturn(blue); @@ -186,7 +200,7 @@ public void testResolveResourcePersistentIds_mapDefaultFunctionality(){ JpaPid resourcePersistentId1 = JpaPid.fromId(1L); JpaPid resourcePersistentId2 = JpaPid.fromId(2L); JpaPid resourcePersistentId3 = JpaPid.fromId(3L); - when(myMemoryCacheService.getThenPutAfterCommit(any(), any(), any())) + when(myMemoryCacheService.getIfPresent(any(), any())) .thenReturn(resourcePersistentId1) .thenReturn(resourcePersistentId2) .thenReturn(resourcePersistentId3); @@ -206,7 +220,7 @@ public void testResolveResourcePersistentIds_resourcePidDefaultFunctionality(){ JpaPid jpaPid1 = JpaPid.fromId(id); when(myStorageSettings.getResourceClientIdStrategy()).thenReturn(JpaStorageSettings.ClientIdStrategyEnum.ANY); - when(myMemoryCacheService.getThenPutAfterCommit(any(), any(), any())).thenReturn(jpaPid1); + when(myMemoryCacheService.getIfPresent(any(), any())).thenReturn(jpaPid1); JpaPid result = myHelperService.resolveResourcePersistentIds(partitionId, resourceType, id.toString()); assertEquals(id, result.getId()); } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/stresstest/GiantTransactionPerfTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/stresstest/GiantTransactionPerfTest.java index ceadc2c1709b..9138ed0095d0 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/stresstest/GiantTransactionPerfTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/stresstest/GiantTransactionPerfTest.java @@ -632,12 +632,12 @@ public void flush() { @Override public FlushModeType getFlushMode() { - throw new UnsupportedOperationException(); + return FlushModeType.AUTO; } @Override public void setFlushMode(FlushModeType flushMode) { - throw new UnsupportedOperationException(); + // ignore } @Override diff --git a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirSystemDaoTransactionR5Test.java b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirSystemDaoTransactionR5Test.java index 4cf1ea982c56..4dac81321649 100644 --- a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirSystemDaoTransactionR5Test.java +++ b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/FhirSystemDaoTransactionR5Test.java @@ -363,21 +363,21 @@ public void testComplexConditionalUpdate(String theName, boolean theTargetAlread myCaptureQueriesListener.logSelectQueries(); if (theTargetAlreadyExists) { if (theResourceChanges) { - assertEquals(6, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); + assertEquals(5, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); assertEquals(1, myCaptureQueriesListener.countInsertQueriesForCurrentThread()); assertEquals(1, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); } else { - assertEquals(6, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); + assertEquals(5, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countInsertQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); } } else { if (theResourceChanges) { - assertEquals(2, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); + assertEquals(1, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); assertEquals(7, myCaptureQueriesListener.countInsertQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); } else { - assertEquals(2, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); + assertEquals(1, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); assertEquals(7, myCaptureQueriesListener.countInsertQueriesForCurrentThread()); assertEquals(0, myCaptureQueriesListener.countUpdateQueriesForCurrentThread()); } @@ -505,11 +505,11 @@ public void createBundle_withConditionalDeleteAndConditionalUpdateOnSameResource ourLog.info(myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome)); assertNull(outcome.getEntry().get(0).getResponse().getLocation()); assertEquals("204 No Content", outcome.getEntry().get(0).getResponse().getStatus()); - assertThat(outcome.getEntry().get(1).getResponse().getLocation()).endsWith("_history/3"); + assertThat(outcome.getEntry().get(1).getResponse().getLocation()).endsWith("_history/4"); assertEquals("201 Created", outcome.getEntry().get(1).getResponse().getStatus()); actual = myPatientDao.read(resourceId, mySrd); - assertEquals("3", actual.getIdElement().getVersionIdPart()); + assertEquals("4", actual.getIdElement().getVersionIdPart()); assertEquals("http://foo", actual.getIdentifierFirstRep().getSystem()); assertEquals("http://tag", actual.getMeta().getTagFirstRep().getSystem()); } @@ -655,13 +655,13 @@ public void testDeleteAndUpdateOnSameResource() { outcome = mySystemDao.transaction(mySrd, createBundleWithDeleteAndUpdateOnSameResource(myFhirContext)); ourLog.info(myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome)); - assertEquals("Patient/P/_history/3", outcome.getEntry().get(0).getResponse().getLocation()); + assertEquals("Patient/P/_history/4", outcome.getEntry().get(0).getResponse().getLocation()); assertEquals("204 No Content", outcome.getEntry().get(0).getResponse().getStatus()); - assertEquals("Patient/P/_history/3", outcome.getEntry().get(1).getResponse().getLocation()); + assertEquals("Patient/P/_history/4", outcome.getEntry().get(1).getResponse().getLocation()); assertEquals("201 Created", outcome.getEntry().get(1).getResponse().getStatus()); actual = myPatientDao.read(resourceId, mySrd); - assertEquals("3", actual.getIdElement().getVersionIdPart()); + assertEquals("4", actual.getIdElement().getVersionIdPart()); assertEquals("http://foo", actual.getIdentifierFirstRep().getSystem()); assertEquals("http://tag", actual.getMeta().getTagFirstRep().getSystem()); From 6dc39b72bd71e75069b576ad994d825bc943129f Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 5 Dec 2024 17:40:05 -0500 Subject: [PATCH 5/9] Test fixes --- .../fhir/jpa/dao/index/IdHelperService.java | 28 ++++++++++++--- ...MdmProviderMergeGoldenResourcesR4Test.java | 8 ++--- .../provider/MdmProviderQueryLinkR4Test.java | 2 +- .../r4/FhirResourceDaoR4SearchNoFtTest.java | 36 +++++++++---------- .../FhirResourceDaoR4SearchOptimizedTest.java | 16 ++++----- .../dao/r4/FhirResourceDaoR4ValueSetTest.java | 2 +- .../jpa/dao/r4/PartitioningSqlR4Test.java | 29 ++++++--------- 7 files changed, 64 insertions(+), 57 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index 6d21fe2d8ad3..8c3942be96b4 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -178,13 +178,17 @@ public IResourceLookup resolveResourceIdentity( * 1. There are two resources with the exact same resource type and forced id * 2. The unique constraint on this column-pair has been dropped */ - String msg = myFhirCtx.getLocalizer().getMessage(IdHelperService.class, "nonUniqueForcedId"); - throw new PreconditionFailedException(Msg.code(1099) + msg); + throwNonUniqueForcedId(); } return matches.get(resourceIdToUse).get(0); } + private void throwNonUniqueForcedId() { + String msg = myFhirCtx.getLocalizer().getMessage(IdHelperService.class, "nonUniqueForcedId"); + throw new PreconditionFailedException(Msg.code(1099) + msg); + } + /** * Returns a mapping of Id -> IResourcePersistentId. * If any resource is not found, it will throw ResourceNotFound exception (and no map will be returned) @@ -241,7 +245,7 @@ public Map resolveResourcePersistentIds( if (!remainingIds.isEmpty()) { List output = new ArrayList<>(); - doResolvePersistentIds(theRequestPartitionId, remainingIds, output); + doResolvePersistentIds(theRequestPartitionId, remainingIds, output, theExcludeDeleted); for (JpaPid next : output) { assert next.getAssociatedResourceId() != null; retVals.put(next.getAssociatedResourceId().getIdPart(), next); @@ -370,14 +374,17 @@ public List resolveResourcePersistentIdsWithCache( .chunk( idsToCheck, SearchBuilder.getMaximumPageSize() / 2, - ids -> doResolvePersistentIds(theRequestPartitionId, ids, retVal)); + ids -> doResolvePersistentIds(theRequestPartitionId, ids, retVal, false)); } return retVal; } private void doResolvePersistentIds( - RequestPartitionId theRequestPartitionId, List theIds, List theOutputListToPopulate) { + RequestPartitionId theRequestPartitionId, + List theIds, + List theOutputListToPopulate, + boolean theExcludeDeleted) { CriteriaBuilder cb = myEntityManager.getCriteriaBuilder(); CriteriaQuery criteriaQuery = cb.createTupleQuery(); Root from = criteriaQuery.from(ResourceTable.class); @@ -403,6 +410,11 @@ private void doResolvePersistentIds( Predicate idCriteria = cb.equal(from.get("myFhirId"), next.getIdPart()); andPredicates.add(idCriteria); getOptionalPartitionPredicate(theRequestPartitionId, cb, from).ifPresent(andPredicates::add); + + if (theExcludeDeleted) { + cb.isNull(from.get("myDeleted")); + } + predicates.add(cb.and(andPredicates.toArray(EMPTY_PREDICATE_ARRAY))); } @@ -411,16 +423,22 @@ private void doResolvePersistentIds( TypedQuery query = myEntityManager.createQuery(criteriaQuery); List results = query.getResultList(); + Set foundIds = new HashSet<>(); for (Tuple nextId : results) { // Check if the nextId has a resource ID. It may have a null resource ID if a commit is still pending. Long resourceId = nextId.get(0, Long.class); String resourceType = nextId.get(1, String.class); String forcedId = nextId.get(2, String.class); if (resourceId != null) { + JpaPid jpaPid = JpaPid.fromId(resourceId); populateAssociatedResourceId(resourceType, forcedId, jpaPid); theOutputListToPopulate.add(jpaPid); + if (!foundIds.add(resourceType + "/" + forcedId)) { + throwNonUniqueForcedId(); + } + String key = toForcedIdToPidKey(theRequestPartitionId, resourceType, forcedId); myMemoryCacheService.putAfterCommit(MemoryCacheService.CacheEnum.FORCED_ID_TO_PID, key, jpaPid); } diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderMergeGoldenResourcesR4Test.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderMergeGoldenResourcesR4Test.java index 9fc1051558b7..7b1d368f21b1 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderMergeGoldenResourcesR4Test.java +++ b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderMergeGoldenResourcesR4Test.java @@ -241,28 +241,28 @@ public void testBadParams() { myMdmProvider.mergeGoldenResources(new StringType("Patient/abc"), myToGoldenPatientId, null, myRequestDetails); fail(); } catch (ResourceNotFoundException e) { - assertEquals(Msg.code(2001) + "Resource Patient/abc is not known", e.getMessage()); + assertEquals(Msg.code(1100) + "Resource Patient/abc is not known", e.getMessage()); } try { myMdmProvider.mergeGoldenResources(new StringType("Patient/abc"), myToGoldenPatientId, null, myRequestDetails); fail(); } catch (ResourceNotFoundException e) { - assertEquals(Msg.code(2001) + "Resource Patient/abc is not known", e.getMessage()); + assertEquals(Msg.code(1100) + "Resource Patient/abc is not known", e.getMessage()); } try { myMdmProvider.mergeGoldenResources(new StringType("Organization/abc"), myToGoldenPatientId, null, myRequestDetails); fail(); } catch (ResourceNotFoundException e) { - assertEquals(Msg.code(2001) + "Resource Organization/abc is not known", e.getMessage()); + assertEquals(Msg.code(1100) + "Resource Organization/abc is not known", e.getMessage()); } try { myMdmProvider.mergeGoldenResources(myFromGoldenPatientId, new StringType("Patient/abc"), null, myRequestDetails); fail(); } catch (ResourceNotFoundException e) { - assertEquals(Msg.code(2001) + "Resource Patient/abc is not known", e.getMessage()); + assertEquals(Msg.code(1100) + "Resource Patient/abc is not known", e.getMessage()); } } } diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderQueryLinkR4Test.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderQueryLinkR4Test.java index a384caa9683d..6d39bb640ff0 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderQueryLinkR4Test.java +++ b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/provider/MdmProviderQueryLinkR4Test.java @@ -470,7 +470,7 @@ public void testNotDuplicateBadId() { myMdmProvider.notDuplicate(myGoldenResource1Id, new StringType("Patient/notAnId123"), myRequestDetails); fail(); } catch (ResourceNotFoundException e) { - assertEquals(Msg.code(2001) + "Resource Patient/notAnId123 is not known", e.getMessage()); + assertEquals(Msg.code(1100) + "Resource Patient/notAnId123 is not known", e.getMessage()); } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java index 2331b0f8a41e..1f57ee235692 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java @@ -1733,37 +1733,38 @@ public void testSearchByIdParamInverse() { @Test public void testSearchByIdParam_QueryIsMinimal() { + IIdType id = createPatient(withActiveTrue()).toUnqualifiedVersionless(); + // With only an _id parameter { SearchParameterMap params = new SearchParameterMap(); params.setLoadSynchronous(true); - params.add(PARAM_ID, new StringParam("DiagnosticReport/123")); + params.add(PARAM_ID, new StringParam(id.getValue())); myCaptureQueriesListener.clear(); - myDiagnosticReportDao.search(params).size(); + myPatientDao.search(params).size(); List selectQueries = myCaptureQueriesListener.getSelectQueriesForCurrentThread(); - assertThat(selectQueries).hasSize(1); + assertThat(selectQueries).hasSize(3); - String sqlQuery = selectQueries.get(0).getSql(true, true).toLowerCase(); + String sqlQuery = selectQueries.get(1).getSql(true, true).toLowerCase(); ourLog.info("SQL Query:\n{}", sqlQuery); - assertThat(countMatches(sqlQuery, "res_id = '123'")).as(sqlQuery).isEqualTo(1); + assertThat(countMatches(sqlQuery, "res_id = '" + id.getIdPart() + "'")).as(sqlQuery).isEqualTo(1); assertThat(countMatches(sqlQuery, "join")).as(sqlQuery).isEqualTo(0); - assertThat(countMatches(sqlQuery, "res_type = 'diagnosticreport'")).as(sqlQuery).isEqualTo(1); - assertThat(countMatches(sqlQuery, "res_deleted_at is null")).as(sqlQuery).isEqualTo(1); + assertThat(countMatches(sqlQuery, "res_type = 'patient'")).as(sqlQuery).isEqualTo(1); } // With an _id parameter and a standard search param { SearchParameterMap params = new SearchParameterMap(); params.setLoadSynchronous(true); - params.add(PARAM_ID, new StringParam("DiagnosticReport/123")); - params.add("code", new TokenParam("foo", "bar")); + params.add(PARAM_ID, new StringParam(id.getValue())); + params.add("identifier", new TokenParam("foo", "bar")); myCaptureQueriesListener.clear(); - myDiagnosticReportDao.search(params).size(); + myPatientDao.search(params).sizeOrThrowNpe(); List selectQueries = myCaptureQueriesListener.getSelectQueriesForCurrentThread(); - assertThat(selectQueries).hasSize(1); + assertThat(selectQueries).hasSize(2); - String sqlQuery = selectQueries.get(0).getSql(true, true).toLowerCase(); + String sqlQuery = selectQueries.get(1).getSql(true, true).toLowerCase(); ourLog.info("SQL Query:\n{}", sqlQuery); - assertThat(countMatches(sqlQuery, "res_id = '123'")).as(sqlQuery).isEqualTo(1); + assertThat(countMatches(sqlQuery, "res_id = '" + id.getIdPart() + "'")).as(sqlQuery).isEqualTo(1); assertThat(countMatches(sqlQuery, "join")).as(sqlQuery).isEqualTo(1); assertThat(countMatches(sqlQuery, "hash_sys_and_value")).as(sqlQuery).isEqualTo(1); assertThat(countMatches(sqlQuery, "res_type = 'diagnosticreport")).as(sqlQuery).isEqualTo(0); // could be 0 @@ -1780,14 +1781,13 @@ public void testSearchByIdParamAndOtherSearchParam_QueryIsMinimal() { myCaptureQueriesListener.clear(); myDiagnosticReportDao.search(params).size(); List selectQueries = myCaptureQueriesListener.getSelectQueriesForCurrentThread(); - assertThat(selectQueries).hasSize(1); + assertThat(selectQueries).hasSize(2); - String sqlQuery = selectQueries.get(0).getSql(true, true).toLowerCase(); + String sqlQuery = selectQueries.get(1).getSql(true, true).toLowerCase(); ourLog.info("SQL Query:\n{}", sqlQuery); - assertThat(countMatches(sqlQuery, "res_id = '123'")).as(sqlQuery).isEqualTo(1); + assertThat(countMatches(sqlQuery, "fhir_id='123'")).as(sqlQuery).isEqualTo(1); assertThat(countMatches(sqlQuery, "join")).as(sqlQuery).isEqualTo(0); - assertThat(countMatches(sqlQuery, "res_type = 'diagnosticreport'")).as(sqlQuery).isEqualTo(1); - assertThat(countMatches(sqlQuery, "res_deleted_at is null")).as(sqlQuery).isEqualTo(1); + assertThat(countMatches(sqlQuery, "res_type='diagnosticreport'")).as(sqlQuery).isEqualTo(1); } @Test diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java index e03d3725746c..d24fa4a49990 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java @@ -889,7 +889,7 @@ public void testSearchOnIdAndReference_SearchById() { String selectQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.res_type='observation'")).as(selectQuery).isEqualTo(1); - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.fhir_id in ('a')")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.fhir_id='a'")).as(selectQuery).isEqualTo(1); selectQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(1).getSql(true, false); assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "select t1.res_id from hfj_resource t1")).as(selectQuery).isEqualTo(1); @@ -907,10 +907,9 @@ public void testSearchOnIdAndReference_SearchById() { assertThat(outcome.getResources(0, 999)).hasSize(2); myCaptureQueriesListener.logSelectQueriesForCurrentThread(); String selectQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(1).getSql(true, false); - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "select t0.res_id from hfj_resource t0")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), " from hfj_resource rt1_0 where")).as(selectQuery).isEqualTo(1); // Because we included a non-forced ID, we need to verify the type - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "t0.res_type = 'observation'")).as(selectQuery).isEqualTo(1); - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "t0.res_deleted_at is null")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.res_type='observation'")).as(selectQuery).isEqualTo(1); } // Delete the resource - There should only be one search performed because deleted resources will @@ -928,10 +927,10 @@ public void testSearchOnIdAndReference_SearchById() { assertThat(outcome.getResources(0, 999)).isEmpty(); myCaptureQueriesListener.logSelectQueriesForCurrentThread(); - assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread()).hasSize(1); + assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread()).hasSize(3); String selectQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.res_type='observation'")).as(selectQuery).isEqualTo(1); - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.fhir_id in ('a')")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.fhir_id='a'")).as(selectQuery).isEqualTo(1); } // Search by ID where at least one ID is a numeric ID @@ -944,10 +943,9 @@ public void testSearchOnIdAndReference_SearchById() { assertThat(outcome.getResources(0, 999)).isEmpty(); myCaptureQueriesListener.logSelectQueriesForCurrentThread(); String selectQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(1).getSql(true, false); - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "select t0.res_id from hfj_resource t0")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), " from hfj_resource rt1_0 where")).as(selectQuery).isEqualTo(1); // Because we included a non-forced ID, we need to verify the type - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "t0.res_type = 'observation'")).as(selectQuery).isEqualTo(1); - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "t0.res_deleted_at is null")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.res_type='observation'")).as(selectQuery).isEqualTo(1); } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ValueSetTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ValueSetTest.java index 7b5e063681c0..6c4b8249ba01 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ValueSetTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ValueSetTest.java @@ -448,7 +448,7 @@ public void testExpandById_UnknownId() { myValueSetDao.expand(new IdType("http://foo"), null, mySrd); fail(); } catch (ResourceNotFoundException e) { - assertEquals("HAPI-2001: Resource ValueSet/foo is not known", e.getMessage()); + assertEquals("HAPI-1100: Resource ValueSet/foo is not known", e.getMessage()); } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java index c2ecd85a7ff9..22b2b04241e2 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java @@ -1153,7 +1153,7 @@ public void testRead_ForcedId_SpecificPartition() { myPatientDao.read(patientIdNull, mySrd).getIdElement().toUnqualifiedVersionless(); fail(); } catch (ResourceNotFoundException e) { - assertThat(e.getMessage()).matches(Msg.code(2001) + "Resource Patient/NULL is not known"); + assertThat(e.getMessage()).matches(Msg.code(1100) + "Resource Patient/NULL is not known"); } // Read in wrong Partition @@ -1162,7 +1162,7 @@ public void testRead_ForcedId_SpecificPartition() { myPatientDao.read(patientId2, mySrd).getIdElement().toUnqualifiedVersionless(); fail(); } catch (ResourceNotFoundException e) { - assertThat(e.getMessage()).matches(Msg.code(2001) + "Resource Patient/TWO is not known"); + assertThat(e.getMessage()).matches(Msg.code(1100) + "Resource Patient/TWO is not known"); } // Read in wrong Partition @@ -1171,7 +1171,7 @@ public void testRead_ForcedId_SpecificPartition() { myPatientDao.read(patientId1, mySrd).getIdElement().toUnqualifiedVersionless(); fail(); } catch (ResourceNotFoundException e) { - assertThat(e.getMessage()).matches(Msg.code(2001) + "Resource Patient/ONE is not known"); + assertThat(e.getMessage()).matches(Msg.code(1100) + "Resource Patient/ONE is not known"); } // Read in correct Partition @@ -1198,7 +1198,7 @@ public void testRead_ForcedId_DefaultPartition() { myPatientDao.read(patientId1, mySrd).getIdElement().toUnqualifiedVersionless(); fail(); } catch (ResourceNotFoundException e) { - assertThat(e.getMessage()).matches(Msg.code(2001) + "Resource Patient/ONE is not known"); + assertThat(e.getMessage()).matches(Msg.code(1100) + "Resource Patient/ONE is not known"); } // Read in wrong Partition @@ -1207,7 +1207,7 @@ public void testRead_ForcedId_DefaultPartition() { myPatientDao.read(patientId2, mySrd).getIdElement().toUnqualifiedVersionless(); fail(); } catch (ResourceNotFoundException e) { - assertThat(e.getMessage()).matches(Msg.code(2001) + "Resource Patient/TWO is not known"); + assertThat(e.getMessage()).matches(Msg.code(1100) + "Resource Patient/TWO is not known"); } } @@ -1273,8 +1273,7 @@ public void testSearch_IdParamOnly_PidId_SpecificPartition() { ourLog.info("Search SQL:\n{}", searchSql); // Only the read columns should be used, no criteria use partition - assertThat(searchSql).as(searchSql).contains("PARTITION_ID = '1'"); - assertThat(StringUtils.countMatches(searchSql, "PARTITION_ID")).as(searchSql).isEqualTo(2); + assertThat(searchSql).as(searchSql).contains("PARTITION_ID='1'"); } // Read in null Partition @@ -1325,8 +1324,7 @@ public void testSearch_IdParamSecond_PidId_SpecificPartition() { ourLog.info("Search SQL:\n{}", searchSql); // Only the read columns should be used, no criteria use partition - assertThat(searchSql).as(searchSql).contains("PARTITION_ID = '1'"); - assertThat(StringUtils.countMatches(searchSql, "PARTITION_ID")).as(searchSql).isEqualTo(3); // If this switches to 2 that would be fine + assertThat(searchSql).as(searchSql).contains("PARTITION_ID='1'"); } // Read in null Partition @@ -1380,9 +1378,7 @@ public void testSearch_IdParamOnly_ForcedId_SpecificPartition() { ourLog.info("Search SQL:\n{}", searchSql); // Only the read columns should be used, no criteria use partition - assertThat(searchSql).as(searchSql).contains("PARTITION_ID IN ('1')"); - assertThat(StringUtils.countMatches(searchSql, "PARTITION_ID,")).as(searchSql).isEqualTo(1); - assertThat(StringUtils.countMatches(searchSql, "PARTITION_DATE")).as(searchSql).isEqualTo(1); + assertThat(searchSql).as(searchSql).contains("PARTITION_ID='1'"); } // Read in null Partition @@ -1434,12 +1430,9 @@ public void testSearch_IdParamSecond_ForcedId_SpecificPartition() { // First SQL resolves the forced ID String searchSql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false).toUpperCase(); ourLog.info("Search SQL:\n{}", searchSql); - assertThat(searchSql).as(searchSql).contains("PARTITION_ID IN ('1')"); - assertThat(StringUtils.countMatches(searchSql, "PARTITION_ID,")).as(searchSql).isEqualTo(1); - assertThat(StringUtils.countMatches(searchSql, "PARTITION_DATE")).as(searchSql).isEqualTo(1); + assertThat(searchSql).as(searchSql).contains("PARTITION_ID='1'"); // Second SQL performs the search - searchSql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(1).getSql(true, false).toUpperCase(); ourLog.info("Search SQL:\n{}", searchSql); assertThat(searchSql).as(searchSql).contains("PARTITION_ID = '1'"); @@ -2096,9 +2089,7 @@ public void testSearch_HasParam_SearchOnePartition() { String searchSql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); ourLog.info("Search SQL:\n{}", searchSql); - assertThat(searchSql).as(searchSql).contains("PARTITION_ID in ('1')"); - assertThat(StringUtils.countMatches(searchSql, "PARTITION_ID,")).as(searchSql).isEqualTo(1); - assertThat(StringUtils.countMatches(searchSql, "PARTITION_DATE")).as(searchSql).isEqualTo(1); + assertThat(searchSql).as(searchSql).contains("PARTITION_ID='1'"); } From ffa086032a1562058a5535a5d92fc330fba102c7 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 5 Dec 2024 19:08:57 -0500 Subject: [PATCH 6/9] Add support for #6508 --- .../6538-improve-validation-performance.yaml | 7 + .../mdm/batch2/clear/MdmClearStepTest.java | 2 +- .../r4/FhirResourceDaoR4QueryCountTest.java | 194 ++++++++++++++++++ .../support/CachingValidationSupport.java | 7 - ...teTerminologyServiceValidationSupport.java | 7 + 5 files changed, 209 insertions(+), 8 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_1/6538-improve-validation-performance.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_1/6538-improve-validation-performance.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_1/6538-improve-validation-performance.yaml new file mode 100644 index 000000000000..2e85cd2cd520 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_1/6538-improve-validation-performance.yaml @@ -0,0 +1,7 @@ +--- +type: perf +issue: 6538 +title: "In HAPI FHIR 8.0.0, validation processing has been significantly improved thanks + to ticket [#6508](https://github.com/hapifhir/hapi-fhir/pull/6508). This enhancement + has been partially backported to the 7.6.x release line in order to provide partial improvement + prior to the release of HAPI FHIR 8.0.0." diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/mdm/batch2/clear/MdmClearStepTest.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/mdm/batch2/clear/MdmClearStepTest.java index 0ee2a64191a0..49d4b485b551 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/mdm/batch2/clear/MdmClearStepTest.java +++ b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/mdm/batch2/clear/MdmClearStepTest.java @@ -103,7 +103,7 @@ public void testGoldenResourceGetsExpunged() { assertPatientExists(myGoldenId); fail("Resource cannot be found"); } catch (ResourceNotFoundException e) { - assertEquals("HAPI-2001: Resource " + myGoldenId + " is not known", e.getMessage()); + assertEquals("HAPI-1100: Resource " + myGoldenId + " is not known", e.getMessage()); } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java index 07751579bbb9..b32344284024 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java @@ -12,6 +12,8 @@ import ca.uhn.fhir.batch2.model.JobInstance; import ca.uhn.fhir.batch2.model.WorkChunk; import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.context.support.ConceptValidationOptions; +import ca.uhn.fhir.context.support.IValidationSupport; import ca.uhn.fhir.context.support.ValidationSupportContext; import ca.uhn.fhir.context.support.ValueSetExpansionOptions; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; @@ -35,8 +37,11 @@ import ca.uhn.fhir.jpa.term.TermReadSvcImpl; import ca.uhn.fhir.jpa.test.util.SubscriptionTestUtil; import ca.uhn.fhir.jpa.util.SqlQuery; +import ca.uhn.fhir.rest.api.EncodingEnum; +import ca.uhn.fhir.rest.api.MethodOutcome; import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.api.SortSpec; +import ca.uhn.fhir.rest.api.ValidationModeEnum; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.param.ReferenceParam; @@ -63,12 +68,14 @@ import org.hl7.fhir.r4.model.Coverage; import org.hl7.fhir.r4.model.DateTimeType; import org.hl7.fhir.r4.model.Encounter; +import org.hl7.fhir.r4.model.Enumerations; import org.hl7.fhir.r4.model.ExplanationOfBenefit; import org.hl7.fhir.r4.model.Group; import org.hl7.fhir.r4.model.IdType; import org.hl7.fhir.r4.model.Location; import org.hl7.fhir.r4.model.Narrative; import org.hl7.fhir.r4.model.Observation; +import org.hl7.fhir.r4.model.OperationOutcome; import org.hl7.fhir.r4.model.Parameters; import org.hl7.fhir.r4.model.Patient; import org.hl7.fhir.r4.model.Practitioner; @@ -79,6 +86,7 @@ import org.hl7.fhir.r4.model.Reference; import org.hl7.fhir.r4.model.ServiceRequest; import org.hl7.fhir.r4.model.StringType; +import org.hl7.fhir.r4.model.StructureDefinition; import org.hl7.fhir.r4.model.Subscription; import org.hl7.fhir.r4.model.ValueSet; import org.junit.jupiter.api.AfterEach; @@ -90,6 +98,7 @@ 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 org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Slice; @@ -3717,6 +3726,191 @@ public void testDeleteResource_WithMassIngestionMode_enabled() { }); } + @Test + public void testFetchStructureDefinition_BuiltIn() { + + // First pass with an empty cache + myValidationSupport.invalidateCaches(); + myCaptureQueriesListener.clear(); + assertNotNull(myValidationSupport.fetchStructureDefinition("http://hl7.org/fhir/StructureDefinition/Patient")); + + assertEquals(0, myCaptureQueriesListener.countSelectQueries()); + + // Again (should use cache) + myCaptureQueriesListener.clear(); + assertNotNull(myValidationSupport.fetchStructureDefinition("http://hl7.org/fhir/StructureDefinition/Patient")); + + assertEquals(0, myCaptureQueriesListener.countSelectQueries()); + } + + @Test + public void testFetchStructureDefinition_StoredInRepository() { + + StructureDefinition sd = new StructureDefinition(); + sd.setUrl("http://foo"); + myStructureDefinitionDao.create(sd, mySrd); + + // First pass with an empty cache + myValidationSupport.invalidateCaches(); + myCaptureQueriesListener.clear(); + assertNotNull(myValidationSupport.fetchStructureDefinition("http://foo")); + + assertEquals(2, myCaptureQueriesListener.countSelectQueries()); + + // Again (should use cache) + myCaptureQueriesListener.clear(); + assertNotNull(myValidationSupport.fetchStructureDefinition("http://foo")); + + assertEquals(0, myCaptureQueriesListener.countSelectQueries()); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testValidateResource(boolean theStoredInRepository) { + Patient resource = new Patient(); + resource.setGender(Enumerations.AdministrativeGender.MALE); + resource.getText().setStatus(Narrative.NarrativeStatus.GENERATED).setDivAsString("
hello
"); + String encoded; + + IIdType id = null; + if (theStoredInRepository) { + id = myPatientDao.create(resource, mySrd).getId(); + resource = null; + encoded = null; + } else { + resource.setId("A"); + encoded = myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(resource); + } + + myCaptureQueriesListener.clear(); + ValidationModeEnum mode = ValidationModeEnum.UPDATE; + MethodOutcome outcome = myPatientDao.validate(resource, id, encoded, EncodingEnum.JSON, mode, null, mySrd); + assertThat(((OperationOutcome)outcome.getOperationOutcome()).getIssueFirstRep().getDiagnostics()).contains("No issues detected"); + myCaptureQueriesListener.logSelectQueries(); + if (theStoredInRepository) { + assertEquals(8, myCaptureQueriesListener.countSelectQueries()); + } else { + assertEquals(6, myCaptureQueriesListener.countSelectQueries()); + } + + // Again (should use caches) + myCaptureQueriesListener.clear(); + outcome = myPatientDao.validate(resource, id, encoded, EncodingEnum.JSON, mode, null, mySrd); + assertThat(((OperationOutcome)outcome.getOperationOutcome()).getIssueFirstRep().getDiagnostics()).contains("No issues detected"); + if (theStoredInRepository) { + assertEquals(2, myCaptureQueriesListener.countSelectQueries()); + } else { + assertEquals(0, myCaptureQueriesListener.countSelectQueries()); + } + } + + + + @Test + public void testValidateCode_BuiltIn() { + + // First pass with an empty cache + myValidationSupport.invalidateCaches(); + myCaptureQueriesListener.clear(); + ValidationSupportContext ctx = new ValidationSupportContext(myValidationSupport); + ConceptValidationOptions options = new ConceptValidationOptions(); + String vsUrl = "http://hl7.org/fhir/ValueSet/marital-status"; + String csUrl = "http://terminology.hl7.org/CodeSystem/v3-MaritalStatus"; + String code = "I"; + String code2 = "A"; + assertTrue(myValidationSupport.validateCode(ctx, options, csUrl, code, null, vsUrl).isOk()); + + assertEquals(1, myCaptureQueriesListener.countSelectQueries()); + + // Again (should use cache) + myCaptureQueriesListener.clear(); + assertTrue(myValidationSupport.validateCode(ctx, options, csUrl, code, null, vsUrl).isOk()); + assertEquals(0, myCaptureQueriesListener.countSelectQueries()); + + // Different code (should use cache) + myCaptureQueriesListener.clear(); + assertTrue(myValidationSupport.validateCode(ctx, options, csUrl, code2, null, vsUrl).isOk()); + assertEquals(1, myCaptureQueriesListener.countSelectQueries()); + } + + @Test + public void testValidateCode_StoredInRepository() { + String vsUrl = "http://vs"; + String csUrl = "http://cs"; + String code = "A"; + String code2 = "B"; + + CodeSystem cs = new CodeSystem(); + cs.setUrl(csUrl); + cs.setStatus(Enumerations.PublicationStatus.ACTIVE); + cs.setContent(CodeSystem.CodeSystemContentMode.COMPLETE); + cs.addConcept().setCode(code); + cs.addConcept().setCode(code2); + myCodeSystemDao.create(cs, mySrd); + + ValueSet vs = new ValueSet(); + vs.setUrl(vsUrl); + vs.setStatus(Enumerations.PublicationStatus.ACTIVE); + vs.getCompose().addInclude().setSystem(csUrl); + myValueSetDao.create(vs, mySrd); + IValidationSupport.CodeValidationResult result; + + // First pass with an empty cache + myValidationSupport.invalidateCaches(); + myCaptureQueriesListener.clear(); + ValidationSupportContext ctx = new ValidationSupportContext(myValidationSupport); + ConceptValidationOptions options = new ConceptValidationOptions(); + result = myValidationSupport.validateCode(ctx, options, csUrl, code, null, vsUrl); + assertNotNull(result); + assertTrue(result.isOk()); + assertThat(result.getMessage()).isNull(); + + assertEquals(6, myCaptureQueriesListener.countSelectQueries()); + myCaptureQueriesListener.logSelectQueries(); + + // Again (should use cache) + myCaptureQueriesListener.clear(); + result = myValidationSupport.validateCode(ctx, options, csUrl, code, null, vsUrl); + assertNotNull(result); + assertTrue(result.isOk()); + assertThat(result.getMessage()).isNull(); + assertEquals(0, myCaptureQueriesListener.countSelectQueries()); + + // Different code (should use cache) + myCaptureQueriesListener.clear(); + result = myValidationSupport.validateCode(ctx, options, csUrl, code2, null, vsUrl); + assertNotNull(result); + assertTrue(result.isOk()); + assertEquals(2, myCaptureQueriesListener.countSelectQueries()); + + // Now pre-expand the VS and try again (should use disk because we're fetching from pre-expansion) + myTermSvc.preExpandDeferredValueSetsToTerminologyTables(); + myCaptureQueriesListener.clear(); + result = myValidationSupport.validateCode(ctx, options, csUrl, code, null, vsUrl); + assertNotNull(result); + assertTrue(result.isOk()); + assertThat(result.getMessage()).contains("expansion that was pre-calculated"); + assertEquals(4, myCaptureQueriesListener.countSelectQueries()); + + // Same code (should use cache) + myCaptureQueriesListener.clear(); + result = myValidationSupport.validateCode(ctx, options, csUrl, code, null, vsUrl); + assertNotNull(result); + assertTrue(result.isOk()); + assertThat(result.getMessage()).contains("expansion that was pre-calculated"); + assertEquals(0, myCaptureQueriesListener.countSelectQueries()); + + // Different code (should use cache) + myCaptureQueriesListener.clear(); + result = myValidationSupport.validateCode(ctx, options, csUrl, code2, null, vsUrl); + assertNotNull(result); + assertTrue(result.isOk()); + assertThat(result.getMessage()).contains("expansion that was pre-calculated"); + assertEquals(4, myCaptureQueriesListener.countSelectQueries()); + + } + + private void assertQueryCount(int theExpectedSelectCount, int theExpectedUpdateCount, int theExpectedInsertCount, int theExpectedDeleteCount) { assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread()).hasSize(theExpectedSelectCount); diff --git a/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/CachingValidationSupport.java b/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/CachingValidationSupport.java index 7edb7effd39a..f61b849e9483 100644 --- a/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/CachingValidationSupport.java +++ b/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/CachingValidationSupport.java @@ -254,13 +254,6 @@ private T loadFromCache(Cache theCache, S theKey, Function result = (Optional) theCache.get(theKey, loaderWrapper); assert result != null; - // UGH! Animal sniffer :( - if (!result.isPresent()) { - ourLog.debug( - "Invalidating cache entry for key: {} since the result of the underlying query is empty", theKey); - theCache.invalidate(theKey); - } - return result.orElse(null); } diff --git a/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/RemoteTerminologyServiceValidationSupport.java b/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/RemoteTerminologyServiceValidationSupport.java index 398eacc52a24..992ba1ed944a 100644 --- a/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/RemoteTerminologyServiceValidationSupport.java +++ b/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/support/RemoteTerminologyServiceValidationSupport.java @@ -138,6 +138,9 @@ public IBaseResource fetchCodeSystem(String theSystem) { */ @Nullable private IBaseResource fetchCodeSystem(String theSystem, @Nullable SummaryEnum theSummaryParam) { + if (isBlank(theSystem)) { + return null; + } IGenericClient client = provideClient(); Class bundleType = myCtx.getResourceDefinition("Bundle").getImplementingClass(IBaseBundle.class); @@ -512,6 +515,10 @@ public IBaseResource fetchValueSet(String theValueSetUrl) { */ @Nullable private IBaseResource fetchValueSet(String theValueSetUrl, SummaryEnum theSummaryParam) { + if (isBlank(theValueSetUrl)) { + return null; + } + IGenericClient client = provideClient(); Class bundleType = myCtx.getResourceDefinition("Bundle").getImplementingClass(IBaseBundle.class); From bf75f29e0c330521c2eb9270a45e357958ed7418 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 5 Dec 2024 19:26:14 -0500 Subject: [PATCH 7/9] More test fixes --- .../builder/predicate/ResourceIdPredicateBuilder.java | 4 +++- .../fhir/jpa/bulk/export/ExpandResourcesStepJpaTest.java | 1 + .../ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java | 2 +- .../interceptor/PatientIdPartitionInterceptorTest.java | 8 ++++---- .../jpa/provider/r4/ResourceProviderRevIncludeTest.java | 2 +- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceIdPredicateBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceIdPredicateBuilder.java index 2c2c5bed1604..bb0e38cd4ee3 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceIdPredicateBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceIdPredicateBuilder.java @@ -85,7 +85,9 @@ public Condition createPredicateResourceId( } haveValue = true; try { - boolean excludeDeleted = true; + // These get added to a query that includes a DELETED_AT IS NULL so it's + // not actually critical to avoid deleted IDs here + boolean excludeDeleted = false; JpaPid pid = myIdHelperService.resolveResourcePersistentIds( theRequestPartitionId, theResourceName, valueAsId.getIdPart(), excludeDeleted); orPids.add(pid); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/export/ExpandResourcesStepJpaTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/export/ExpandResourcesStepJpaTest.java index 7595fa9f7679..045f0634ce06 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/export/ExpandResourcesStepJpaTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/export/ExpandResourcesStepJpaTest.java @@ -96,6 +96,7 @@ public void testBulkExportExpandResourcesStep(JpaStorageSettings.TagStorageModeE assertThat(expandedResourceList.getStringifiedResources().get(1)).contains("{\"system\":\"http://static\",\"version\":\"1\",\"code\":\"tag\",\"userSelected\":true}"); // Verify query counts + myCaptureQueriesListener.logSelectQueries(); assertEquals(theExpectedSelectQueries, myCaptureQueriesListener.countSelectQueries()); assertEquals(0, myCaptureQueriesListener.countInsertQueries()); assertEquals(0, myCaptureQueriesListener.countUpdateQueries()); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java index c220b635433f..018ddf753523 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java @@ -2178,7 +2178,7 @@ public void testHistoryWithInvalidId() { myPatientDao.history(new IdType("Patient/FOOFOOFOO"), null, null, null, mySrd); fail(); } catch (ResourceNotFoundException e) { - assertEquals(Msg.code(2001) + "Resource Patient/FOOFOOFOO is not known", e.getMessage()); + assertEquals(Msg.code(1100) + "Resource Patient/FOOFOOFOO is not known", e.getMessage()); } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/interceptor/PatientIdPartitionInterceptorTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/interceptor/PatientIdPartitionInterceptorTest.java index c2a49a3f6a5e..37d0880909ae 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/interceptor/PatientIdPartitionInterceptorTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/interceptor/PatientIdPartitionInterceptorTest.java @@ -189,7 +189,7 @@ public void testReadPatient_Good() { assertTrue(patient.getActive()); myCaptureQueriesListener.logSelectQueries(); assertThat(myCaptureQueriesListener.getSelectQueries()).hasSize(3); - assertThat(myCaptureQueriesListener.getSelectQueries().get(0).getSql(false, false)).contains("rt1_0.PARTITION_ID in (?)"); + assertThat(myCaptureQueriesListener.getSelectQueries().get(0).getSql(false, false)).contains("rt1_0.PARTITION_ID="); assertThat(myCaptureQueriesListener.getSelectQueries().get(1).getSql(false, false)).contains("where rt1_0.PARTITION_ID=? and rt1_0.RES_ID=?"); } @@ -224,7 +224,7 @@ public void testReadPatientHistory_Good() { List selectQueriesForCurrentThread = myCaptureQueriesListener.getSelectQueriesForCurrentThread(); assertEquals(4, selectQueriesForCurrentThread.size()); - assertThat(selectQueriesForCurrentThread.get(0).getSql(false, false)).contains("PARTITION_ID in (?)"); + assertThat(selectQueriesForCurrentThread.get(0).getSql(false, false)).contains("PARTITION_ID=?"); assertThat(selectQueriesForCurrentThread.get(1).getSql(false, false)).contains("PARTITION_ID="); } @@ -238,7 +238,7 @@ public void testSearchPatient_Good() { assertEquals(1, outcome.size()); myCaptureQueriesListener.logSelectQueries(); assertThat(myCaptureQueriesListener.getSelectQueries()).hasSize(3); - assertThat(myCaptureQueriesListener.getSelectQueries().get(0).getSql(false, false)).contains("rt1_0.PARTITION_ID in (?)"); + assertThat(myCaptureQueriesListener.getSelectQueries().get(0).getSql(false, false)).contains("rt1_0.PARTITION_ID=?"); assertThat(myCaptureQueriesListener.getSelectQueries().get(1).getSql(false, false)).contains("t0.PARTITION_ID = ?"); } @@ -353,7 +353,7 @@ public void testHistory_Instance() { myCaptureQueriesListener.logSelectQueries(); assertEquals(2, outcome.size()); assertThat(myCaptureQueriesListener.getSelectQueries()).hasSize(3); - assertThat(myCaptureQueriesListener.getSelectQueries().get(0).getSql(false, false)).contains("PARTITION_ID in "); + assertThat(myCaptureQueriesListener.getSelectQueries().get(0).getSql(false, false)).contains("PARTITION_ID="); assertThat(myCaptureQueriesListener.getSelectQueries().get(1).getSql(false, false)).contains("PARTITION_ID="); } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderRevIncludeTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderRevIncludeTest.java index d531251f6617..3fc959800178 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderRevIncludeTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderRevIncludeTest.java @@ -145,7 +145,7 @@ public void testRevincludeIterate() { //Ensure that the revincludes are included in the query list of the sql trace. //TODO GGG/KHS reduce this to something less than 6 by smarter iterating and getting the resource types earlier when needed. - assertThat(sqlCapturingInterceptor.getQueryList()).hasSize(5); + assertThat(sqlCapturingInterceptor.getQueryList()).hasSize(6); myInterceptorRegistry.unregisterInterceptor(sqlCapturingInterceptor); } From 1501265818ef97ba75826619a04400891ff5d1ad Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 6 Dec 2024 05:52:22 -0500 Subject: [PATCH 8/9] Test fixes --- .../predicate/ResourceIdPredicateBuilder.java | 2 +- .../r4/FhirResourceDaoR4SearchNoFtTest.java | 20 +++++++++---------- .../FhirResourceDaoR4SearchOptimizedTest.java | 10 +++++----- .../dao/r4/FhirResourceDaoR4ValidateTest.java | 3 +++ .../jpa/dao/r4/PartitioningSqlR4Test.java | 4 ++-- .../provider/r4/ResourceProviderR4Test.java | 2 ++ ...rceProviderR4ValueSetNoVerCSNoVerTest.java | 2 +- .../r4/ResourceProviderRevIncludeTest.java | 2 +- 8 files changed, 24 insertions(+), 21 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceIdPredicateBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceIdPredicateBuilder.java index bb0e38cd4ee3..24b9a44d1270 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceIdPredicateBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceIdPredicateBuilder.java @@ -124,7 +124,7 @@ public Condition createPredicateResourceId( List resourceIds = JpaPid.toLongList(allOrPids); if (theSourceJoinColumn == null) { - BaseJoiningPredicateBuilder queryRootTable = super.getOrCreateQueryRootTable(!allIdsAreForcedIds); + BaseJoiningPredicateBuilder queryRootTable = super.getOrCreateQueryRootTable(true); Condition predicate; switch (operation) { default: diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java index 1f57ee235692..a925b8ce6e63 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java @@ -1743,13 +1743,12 @@ public void testSearchByIdParam_QueryIsMinimal() { myCaptureQueriesListener.clear(); myPatientDao.search(params).size(); List selectQueries = myCaptureQueriesListener.getSelectQueriesForCurrentThread(); - assertThat(selectQueries).hasSize(3); + assertThat(selectQueries).hasSize(2); - String sqlQuery = selectQueries.get(1).getSql(true, true).toLowerCase(); + String sqlQuery = selectQueries.get(0).getSql(true, true).toLowerCase(); ourLog.info("SQL Query:\n{}", sqlQuery); assertThat(countMatches(sqlQuery, "res_id = '" + id.getIdPart() + "'")).as(sqlQuery).isEqualTo(1); - assertThat(countMatches(sqlQuery, "join")).as(sqlQuery).isEqualTo(0); - assertThat(countMatches(sqlQuery, "res_type = 'patient'")).as(sqlQuery).isEqualTo(1); + assertThat(countMatches(sqlQuery, "res_deleted_at is null")).as(sqlQuery).isEqualTo(1); } // With an _id parameter and a standard search param { @@ -1760,9 +1759,9 @@ public void testSearchByIdParam_QueryIsMinimal() { myCaptureQueriesListener.clear(); myPatientDao.search(params).sizeOrThrowNpe(); List selectQueries = myCaptureQueriesListener.getSelectQueriesForCurrentThread(); - assertThat(selectQueries).hasSize(2); + assertThat(selectQueries).hasSize(1); - String sqlQuery = selectQueries.get(1).getSql(true, true).toLowerCase(); + String sqlQuery = selectQueries.get(0).getSql(true, true).toLowerCase(); ourLog.info("SQL Query:\n{}", sqlQuery); assertThat(countMatches(sqlQuery, "res_id = '" + id.getIdPart() + "'")).as(sqlQuery).isEqualTo(1); assertThat(countMatches(sqlQuery, "join")).as(sqlQuery).isEqualTo(1); @@ -1781,13 +1780,12 @@ public void testSearchByIdParamAndOtherSearchParam_QueryIsMinimal() { myCaptureQueriesListener.clear(); myDiagnosticReportDao.search(params).size(); List selectQueries = myCaptureQueriesListener.getSelectQueriesForCurrentThread(); - assertThat(selectQueries).hasSize(2); + assertThat(selectQueries).hasSize(1); - String sqlQuery = selectQueries.get(1).getSql(true, true).toLowerCase(); + String sqlQuery = selectQueries.get(0).getSql(true, true).toLowerCase(); ourLog.info("SQL Query:\n{}", sqlQuery); - assertThat(countMatches(sqlQuery, "fhir_id='123'")).as(sqlQuery).isEqualTo(1); - assertThat(countMatches(sqlQuery, "join")).as(sqlQuery).isEqualTo(0); - assertThat(countMatches(sqlQuery, "res_type='diagnosticreport'")).as(sqlQuery).isEqualTo(1); + assertThat(countMatches(sqlQuery, "res_id = '123'")).as(sqlQuery).isEqualTo(1); + assertThat(countMatches(sqlQuery, "res_deleted_at is null")).as(sqlQuery).isEqualTo(1); } @Test diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java index d24fa4a49990..ff72de42721a 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java @@ -907,9 +907,9 @@ public void testSearchOnIdAndReference_SearchById() { assertThat(outcome.getResources(0, 999)).hasSize(2); myCaptureQueriesListener.logSelectQueriesForCurrentThread(); String selectQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(1).getSql(true, false); - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), " from hfj_resource rt1_0 where")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), " from hfj_resource t0 where")).as(selectQuery).isEqualTo(1); // Because we included a non-forced ID, we need to verify the type - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.res_type='observation'")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "t0.res_type = 'observation'")).as(selectQuery).isEqualTo(1); } // Delete the resource - There should only be one search performed because deleted resources will @@ -927,7 +927,7 @@ public void testSearchOnIdAndReference_SearchById() { assertThat(outcome.getResources(0, 999)).isEmpty(); myCaptureQueriesListener.logSelectQueriesForCurrentThread(); - assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread()).hasSize(3); + assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread()).hasSize(2); String selectQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.res_type='observation'")).as(selectQuery).isEqualTo(1); assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.fhir_id='a'")).as(selectQuery).isEqualTo(1); @@ -943,9 +943,9 @@ public void testSearchOnIdAndReference_SearchById() { assertThat(outcome.getResources(0, 999)).isEmpty(); myCaptureQueriesListener.logSelectQueriesForCurrentThread(); String selectQuery = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(1).getSql(true, false); - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), " from hfj_resource rt1_0 where")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), " from hfj_resource t0 where")).as(selectQuery).isEqualTo(1); // Because we included a non-forced ID, we need to verify the type - assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "rt1_0.res_type='observation'")).as(selectQuery).isEqualTo(1); + assertThat(StringUtils.countMatches(selectQuery.toLowerCase(), "t0.res_type = 'observation'")).as(selectQuery).isEqualTo(1); } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ValidateTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ValidateTest.java index 65eb5737e2b0..c19a0fb5cd33 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ValidateTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ValidateTest.java @@ -2230,6 +2230,9 @@ public void validatePatientRealUrlThenCreateStructDefThenValidatePatientWithReal assertTrue(outcomePatientValidateInitial.contains(I18nConstants.VALIDATION_VAL_PROFILE_UNKNOWN_NOT_POLICY)); createStructureDefinitionInDao(); + // Simulate a cache timeout + myValidationSupport.invalidateCaches(); + // execute final String outcomePatientValidateAfterStructDef = validate(PATIENT_WITH_REAL_URL); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java index 22b2b04241e2..227281b6587b 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java @@ -1273,7 +1273,7 @@ public void testSearch_IdParamOnly_PidId_SpecificPartition() { ourLog.info("Search SQL:\n{}", searchSql); // Only the read columns should be used, no criteria use partition - assertThat(searchSql).as(searchSql).contains("PARTITION_ID='1'"); + assertThat(searchSql).as(searchSql).contains("PARTITION_ID = '1'"); } // Read in null Partition @@ -1324,7 +1324,7 @@ public void testSearch_IdParamSecond_PidId_SpecificPartition() { ourLog.info("Search SQL:\n{}", searchSql); // Only the read columns should be used, no criteria use partition - assertThat(searchSql).as(searchSql).contains("PARTITION_ID='1'"); + assertThat(searchSql).as(searchSql).contains("PARTITION_ID = '1'"); } // Read in null Partition diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java index c9ee6d81e3ad..9d8c48c1162d 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java @@ -3787,10 +3787,12 @@ public void testSearchByIdForDeletedResourceWithClientAssignedId() { assertNull(outcome.getResource()); // Search + myCaptureQueriesListener.clear(); Bundle search2 = (Bundle) myClient.search() .forResource(Patient.class) .where(Patient.RES_ID.exactly().identifier(patientId)) .execute(); + myCaptureQueriesListener.logSelectQueries(); assertTrue(CollectionUtils.isEmpty(search2.getEntry())); assertEquals(0, search2.getTotal()); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4ValueSetNoVerCSNoVerTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4ValueSetNoVerCSNoVerTest.java index 97ac4db89c86..4a2c5afb01b1 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4ValueSetNoVerCSNoVerTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4ValueSetNoVerCSNoVerTest.java @@ -1421,7 +1421,7 @@ public void testInvalidatePrecalculatedExpansion_NonExistent() { .execute(); fail(); } catch (ResourceNotFoundException e) { - assertEquals("HTTP 404 Not Found: " + Msg.code(2001) + "Resource ValueSet/FOO is not known", e.getMessage()); + assertEquals("HTTP 404 Not Found: " + Msg.code(1100) + "Resource ValueSet/FOO is not known", e.getMessage()); } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderRevIncludeTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderRevIncludeTest.java index 3fc959800178..d531251f6617 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderRevIncludeTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderRevIncludeTest.java @@ -145,7 +145,7 @@ public void testRevincludeIterate() { //Ensure that the revincludes are included in the query list of the sql trace. //TODO GGG/KHS reduce this to something less than 6 by smarter iterating and getting the resource types earlier when needed. - assertThat(sqlCapturingInterceptor.getQueryList()).hasSize(6); + assertThat(sqlCapturingInterceptor.getQueryList()).hasSize(5); myInterceptorRegistry.unregisterInterceptor(sqlCapturingInterceptor); } From 73e6fb82c0a19fc0f523d1cb4b1244dd6f56776d Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 6 Dec 2024 10:42:40 -0500 Subject: [PATCH 9/9] Test fix --- .../r4/ConsentInterceptorResourceProviderR4IT.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ConsentInterceptorResourceProviderR4IT.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ConsentInterceptorResourceProviderR4IT.java index e75e3047b825..2f842fd3781a 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ConsentInterceptorResourceProviderR4IT.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ConsentInterceptorResourceProviderR4IT.java @@ -960,7 +960,12 @@ public ConsentOutcome canSeeResource(RequestDetails theRequestDetails, IBaseReso String fhirType = theResource.fhirType(); IFhirResourceDao dao = myDaoRegistry.getResourceDao(fhirType); String currentTransactionName = TransactionSynchronizationManager.getCurrentTransactionName(); - dao.read(theResource.getIdElement()); + try { + dao.read(theResource.getIdElement()); + } catch (ResourceNotFoundException e) { + // this is expected, the resource isn't saved in the DB yet. But it shouldn't + // be an NPE, only a ResourceNotFoundException + } return ConsentOutcome.PROCEED; }