From 796b12e33e81212bc93247a3edf160b005fa9274 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Mon, 15 Oct 2018 05:44:53 -0400 Subject: [PATCH] Fix bug in $everything processing --- .../java/ca/uhn/fhir/jpa/dao/DaoConfig.java | 14 ++--- .../ca/uhn/fhir/jpa/dao/IResultIterator.java | 3 ++ .../ca/uhn/fhir/jpa/dao/SearchBuilder.java | 51 +++++++++++++------ .../jpa/search/SearchCoordinatorSvcImpl.java | 7 +-- .../ca/uhn/fhir/jpa/config/TestR4Config.java | 1 + .../provider/r4/ResourceProviderR4Test.java | 19 +++++-- .../search/SearchCoordinatorSvcImplTest.java | 26 ++++++++++ .../BaseResourceReturningMethodBinding.java | 6 ++- 8 files changed, 95 insertions(+), 32 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoConfig.java index eee2cc4614f..c024317a7f2 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoConfig.java @@ -514,18 +514,18 @@ public class DaoConfig { /** * This may be used to optionally register server interceptors directly against the DAOs. */ - public void setInterceptors(IServerInterceptor... theInterceptor) { - setInterceptors(new ArrayList()); - if (theInterceptor != null && theInterceptor.length != 0) { - getInterceptors().addAll(Arrays.asList(theInterceptor)); - } + public void setInterceptors(List theInterceptors) { + myInterceptors = theInterceptors; } /** * This may be used to optionally register server interceptors directly against the DAOs. */ - public void setInterceptors(List theInterceptors) { - myInterceptors = theInterceptors; + public void setInterceptors(IServerInterceptor... theInterceptor) { + setInterceptors(new ArrayList()); + if (theInterceptor != null && theInterceptor.length != 0) { + getInterceptors().addAll(Arrays.asList(theInterceptor)); + } } /** diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IResultIterator.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IResultIterator.java index b479a2fa1ae..68120973c3c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IResultIterator.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/IResultIterator.java @@ -23,4 +23,7 @@ package ca.uhn.fhir.jpa.dao; import java.util.Iterator; public interface IResultIterator extends Iterator { + + int getSkippedCount(); + } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java index c26d69851be..2a259d3767e 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java @@ -9,9 +9,9 @@ package ca.uhn.fhir.jpa.dao; * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -1779,7 +1779,8 @@ public class SearchBuilder implements ISearchBuilder { } /** - * THIS SHOULD RETURN HASHSET and not just Set because we add to it later (so it can't be Collections.emptySet()) + * THIS SHOULD RETURN HASHSET and not just Set because we add to it later + * so it can't be Collections.emptySet() or some such thing */ @Override public HashSet loadIncludes(IDao theCallingDao, FhirContext theContext, EntityManager theEntityManager, Collection theMatches, Set theRevIncludes, @@ -2173,6 +2174,7 @@ public class SearchBuilder implements ISearchBuilder { private SortSpec mySort; private boolean myStillNeedToFetchIncludes; private StopWatch myStopwatch = null; + private int mySkipCount = 0; private QueryIterator() { mySort = myParams.getSort(); @@ -2212,20 +2214,26 @@ public class SearchBuilder implements ISearchBuilder { if (myPreResultsIterator != null && myPreResultsIterator.hasNext()) { while (myPreResultsIterator.hasNext()) { Long next = myPreResultsIterator.next(); - if (next != null && myPidSet.add(next)) { - myNext = next; - break; - } + if (next != null) + if (myPidSet.add(next)) { + myNext = next; + break; + } else { + mySkipCount++; + } } } if (myNext == null) { while (myResultsIterator.hasNext()) { Long next = myResultsIterator.next(); - if (next != null && myPidSet.add(next)) { - myNext = next; - break; - } + if (next != null) + if (myPidSet.add(next)) { + myNext = next; + break; + } else { + mySkipCount++; + } } } @@ -2237,10 +2245,13 @@ public class SearchBuilder implements ISearchBuilder { if (myIncludesIterator != null) { while (myIncludesIterator.hasNext()) { Long next = myIncludesIterator.next(); - if (next != null && myPidSet.add(next)) { - myNext = next; - break; - } + if (next != null) + if (myPidSet.add(next)) { + myNext = next; + break; + } else { + mySkipCount++; + } } if (myNext == null) { myNext = NO_MORE; @@ -2279,6 +2290,11 @@ public class SearchBuilder implements ISearchBuilder { Validate.isTrue(retVal != NO_MORE, "No more elements"); return retVal; } + + @Override + public int getSkippedCount() { + return mySkipCount; + } } private class UniqueIndexIterator implements IResultIterator { @@ -2315,6 +2331,11 @@ public class SearchBuilder implements ISearchBuilder { public void remove() { throw new UnsupportedOperationException(); } + + @Override + public int getSkippedCount() { + return 0; + } } private static class CountQueryIterator implements Iterator { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index d3a523d9b4e..d57ce0c2d3d 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -519,7 +519,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { }); } - private void saveUnsynced(final Iterator theResultIter) { + private void saveUnsynced(final IResultIterator theResultIter) { TransactionTemplate txTemplate = new TransactionTemplate(myManagedTxManager); txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRED); txTemplate.execute(new TransactionCallbackWithoutResult() { @@ -547,7 +547,8 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { if (theResultIter.hasNext() == false) { mySearch.setNumFound(myCountSaved); - if (myMaxResultsToFetch != null && myCountSaved < myMaxResultsToFetch) { + int loadedCountThisPass = theResultIter.getSkippedCount() + myCountSaved; + if (myMaxResultsToFetch != null && loadedCountThisPass < myMaxResultsToFetch) { mySearch.setStatus(SearchStatusEnum.FINISHED); mySearch.setTotalCount(myCountSaved); } else if (myAdditionalPrefetchThresholdsRemaining) { @@ -785,7 +786,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { /* * Construct the SQL query we'll be sending to the database */ - Iterator theResultIterator = sb.createQuery(myParams, mySearch.getUuid()); + IResultIterator theResultIterator = sb.createQuery(myParams, mySearch.getUuid()); /* * The following loop actually loads the PIDs of the resources diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java index b5ca9fe24b4..ce3510616d8 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java @@ -4,6 +4,7 @@ import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.rest.server.interceptor.RequestValidatingInterceptor; import ca.uhn.fhir.validation.ResultSeverityEnum; import net.ttddyy.dsproxy.listener.ThreadQueryCountHolder; +import net.ttddyy.dsproxy.listener.logging.SLF4JLogLevel; import net.ttddyy.dsproxy.support.ProxyDataSourceBuilder; import org.apache.commons.dbcp2.BasicDataSource; import org.springframework.context.annotation.Bean; diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java index 6c97d254af3..97ae91bf9b5 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java @@ -158,6 +158,7 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { myDaoConfig.setAllowMultipleDelete(true); ourClient.registerInterceptor(myCapturingInterceptor); + myDaoConfig.setSearchPreFetchThresholds(new DaoConfig().getPreFetchThresholds()); } @Test @@ -1633,6 +1634,8 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { */ @Test public void testEverythingWithLargeSet2() { + myDaoConfig.setSearchPreFetchThresholds(Arrays.asList(15, 30, -1)); + Patient p = new Patient(); p.setActive(true); IIdType id = ourClient.create().resource(p).execute().getId().toUnqualifiedVersionless(); @@ -1644,19 +1647,25 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { ourClient.update().resource(obs).execute(); } - Bundle responseBundle = ourClient.operation().onInstance(id).named("everything").withParameter(Parameters.class, "_count", new IntegerType(50)).useHttpGet().returnResourceType(Bundle.class) + Bundle responseBundle = ourClient + .operation() + .onInstance(id) + .named("everything") + .withParameter(Parameters.class, "_count", new IntegerType(50)) + .useHttpGet() + .returnResourceType(Bundle.class) .execute(); - TreeSet ids = new TreeSet(); + TreeSet ids = new TreeSet<>(); for (int i = 0; i < responseBundle.getEntry().size(); i++) { for (BundleEntryComponent nextEntry : responseBundle.getEntry()) { ids.add(nextEntry.getResource().getIdElement().getIdPart()); } } - ourLog.info("Have {} IDs: {}", ids.size(), ids); - BundleLinkComponent nextLink = responseBundle.getLink("next"); + ourLog.info("Have {} IDs with next link: ", ids.size(), nextLink); + while (nextLink != null) { String nextUrl = nextLink.getUrl(); responseBundle = ourClient.fetchResourceFromUrl(Bundle.class, nextUrl); @@ -1666,8 +1675,8 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { } } - ourLog.info("Have {} IDs: {}", ids.size(), ids); nextLink = responseBundle.getLink("next"); + ourLog.info("Have {} IDs with next link: ", ids.size(), nextLink); } assertThat(ids, hasItem(id.getIdPart())); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java index e7c3efe78f7..fd793f2dcf7 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java @@ -422,6 +422,10 @@ public class SearchCoordinatorSvcImplTest { return myWrap.next(); } + @Override + public int getSkippedCount() { + return myWrap.getSkippedCount(); + } } public static class ResultIterator extends BaseIterator implements IResultIterator { @@ -441,16 +445,29 @@ public class SearchCoordinatorSvcImplTest { public Long next() { return myWrap.next(); } + + @Override + public int getSkippedCount() { + return 0; + } } public static class SlowIterator extends BaseIterator implements IResultIterator { + private final IResultIterator myResultIteratorWrap; private int myDelay; private Iterator myWrap; public SlowIterator(Iterator theWrap, int theDelay) { myWrap = theWrap; myDelay = theDelay; + myResultIteratorWrap = null; + } + + public SlowIterator(IResultIterator theWrap, int theDelay) { + myWrap = theWrap; + myResultIteratorWrap = theWrap; + myDelay = theDelay; } @Override @@ -468,6 +485,15 @@ public class SearchCoordinatorSvcImplTest { return myWrap.next(); } + @Override + public int getSkippedCount() { + if (myResultIteratorWrap == null) { + return 0; + } else { + return myResultIteratorWrap.getSkippedCount(); + } + } + } @AfterClass diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java index 19b8a282812..3de8c16a612 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java @@ -215,9 +215,11 @@ public abstract class BaseResourceReturningMethodBinding extends BaseMethodBindi linkPrev = RestfulServerUtils.createPagingLink(theIncludes, serverBase, searchId, theResult.getPreviousPageId(), theRequest.getParameters(), prettyPrint, theBundleType); } } else if (searchId != null) { + int offset = theOffset + resourceList.size(); + // We're doing offset pages - if (numTotalResults == null || theOffset + numToReturn < numTotalResults) { - linkNext = (RestfulServerUtils.createPagingLink(theIncludes, serverBase, searchId, theOffset + numToReturn, numToReturn, theRequest.getParameters(), prettyPrint, theBundleType)); + if (numTotalResults == null || offset < numTotalResults) { + linkNext = (RestfulServerUtils.createPagingLink(theIncludes, serverBase, searchId, offset, numToReturn, theRequest.getParameters(), prettyPrint, theBundleType)); } if (theOffset > 0) { int start = Math.max(0, theOffset - theLimit);