Skip to content

Commit 1d3c6af

Browse files
jamesagnewtadgh
jamesagnew
authored and
tadgh
committed
Fix deletes in Patient compartment mode (#7474)
* Fix deletes in Patient compartment mode * Add changelog * Address review comment
1 parent c091184 commit 1d3c6af

13 files changed

Lines changed: 171 additions & 93 deletions

File tree

‎hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_8_0/7474-enable-delete-on-patient-id-compartment-mode.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
type: fix
3+
issue: 7474
4+
title: "FHIR deletes do not always work when the PatientIdPartitionInterceptor is in use. This has been corrected."

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/JpaConfig.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,7 @@
144144
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceHistoryPredicateBuilder;
145145
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceHistoryProvenancePredicateBuilder;
146146
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceIdPredicateBuilder;
147+
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceLinkForHasParameterPredicateBuilder;
147148
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceLinkPredicateBuilder;
148149
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceTablePredicateBuilder;
149150
import ca.uhn.fhir.jpa.search.builder.predicate.SearchParamPresentPredicateBuilder;
@@ -773,11 +774,18 @@ public QuantityNormalizedPredicateBuilder newQuantityNormalizedPredicateBuilder(
773774
return new QuantityNormalizedPredicateBuilder(theSearchBuilder);
774775
}
775776

776-
@Bean
777+
@Bean("newResourceLinkPredicateBuilder")
777778
@Scope("prototype")
778779
public ResourceLinkPredicateBuilder newResourceLinkPredicateBuilder(
779-
QueryStack theQueryStack, SearchQueryBuilder theSearchBuilder, boolean theReversed) {
780-
return new ResourceLinkPredicateBuilder(theQueryStack, theSearchBuilder, theReversed);
780+
QueryStack theQueryStack, SearchQueryBuilder theSearchBuilder) {
781+
return new ResourceLinkPredicateBuilder(theQueryStack, theSearchBuilder);
782+
}
783+
784+
@Bean("newHasLinkPredicateBuilder")
785+
@Scope("prototype")
786+
public ResourceLinkForHasParameterPredicateBuilder newHasLinkPredicateBuilder(
787+
QueryStack theQueryStack, SearchQueryBuilder theSearchBuilder) {
788+
return new ResourceLinkForHasParameterPredicateBuilder(theQueryStack, theSearchBuilder);
781789
}
782790

783791
@Bean

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -780,21 +780,27 @@ public DaoMethodOutcome delete(IIdType theId, RequestDetails theRequestDetails)
780780
validateIdPresentForDelete(theId);
781781
validateDeleteEnabled();
782782

783-
return myTransactionService.execute(theRequestDetails, transactionDetails, tx -> {
784-
DeleteConflictList deleteConflicts = new DeleteConflictList();
785-
if (isNotBlank(theId.getValue())) {
786-
deleteConflicts.setResourceIdMarkedForDeletion(theId);
787-
}
783+
RequestPartitionId requestPartitionId = myRequestPartitionHelperService.determineReadPartitionForRequestForRead(
784+
theRequestDetails, getResourceName(), theId);
785+
786+
return myTransactionService
787+
.withRequest(theRequestDetails)
788+
.withRequestPartitionId(requestPartitionId)
789+
.execute(tx -> {
790+
DeleteConflictList deleteConflicts = new DeleteConflictList();
791+
if (isNotBlank(theId.getValue())) {
792+
deleteConflicts.setResourceIdMarkedForDeletion(theId);
793+
}
788794

789-
StopWatch w = new StopWatch();
795+
StopWatch w = new StopWatch();
790796

791-
DaoMethodOutcome retVal = delete(theId, deleteConflicts, theRequestDetails, transactionDetails);
797+
DaoMethodOutcome retVal = delete(theId, deleteConflicts, theRequestDetails, transactionDetails);
792798

793-
DeleteConflictUtil.validateDeleteConflictsEmptyOrThrowException(getContext(), deleteConflicts);
799+
DeleteConflictUtil.validateDeleteConflictsEmptyOrThrowException(getContext(), deleteConflicts);
794800

795-
ourLog.debug("Processed delete on {} in {}ms", theId.getValue(), w.getMillisAndRestart());
796-
return retVal;
797-
});
801+
ourLog.debug("Processed delete on {} in {}ms", theId.getValue(), w.getMillisAndRestart());
802+
return retVal;
803+
});
798804
}
799805

800806
@Override

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1219,7 +1219,7 @@ private Condition createPredicateHas(
12191219
}
12201220

12211221
ResourceLinkPredicateBuilder resourceLinkTableJoin =
1222-
mySqlBuilder.addReferencePredicateBuilderReversed(this, theSourceJoinColumn);
1222+
mySqlBuilder.addResourceLinkForHasParameterPredicateBuilderReversed(this, theSourceJoinColumn);
12231223

12241224
List<String> paths = resourceLinkTableJoin.createResourceLinkPaths(
12251225
targetResourceType, paramReference, new ArrayList<>());

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkForHasParameterPredicateBuilder.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
package ca.uhn.fhir.jpa.search.builder.predicate;
2+
3+
import ca.uhn.fhir.jpa.search.builder.QueryStack;
4+
import ca.uhn.fhir.jpa.search.builder.sql.SearchQueryBuilder;
5+
import com.healthmarketscience.sqlbuilder.dbspec.basic.DbColumn;
6+
7+
/**
8+
* This is a specialization on {@link ResourceLinkPredicateBuilder} which is a predicate builder for
9+
* the {@link ca.uhn.fhir.jpa.model.entity.ResourceLink HFJ_RES_LINK} table. That builder assumes a
10+
* forward reference (i.e. on the source columns) whereas this builder assumes a reverse reference
11+
* (i.e. on the target columns which are needed for the <code>_has</code> search parameter).
12+
*/
13+
public class ResourceLinkForHasParameterPredicateBuilder extends ResourceLinkPredicateBuilder {
14+
15+
/**
16+
* Constructor
17+
*/
18+
public ResourceLinkForHasParameterPredicateBuilder(
19+
QueryStack theQueryStack, SearchQueryBuilder theSearchSqlBuilder) {
20+
super(theQueryStack, theSearchSqlBuilder);
21+
}
22+
23+
@Override
24+
public DbColumn getResourceTypeColumn() {
25+
return myColumnTargetResourceType;
26+
}
27+
28+
@Override
29+
public DbColumn getPartitionIdColumn() {
30+
return myColumnTargetPartitionId;
31+
}
32+
33+
@Override
34+
public DbColumn getResourceIdColumn() {
35+
return myColumnTargetResourceId;
36+
}
37+
}

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/ResourceLinkPredicateBuilder.java‎

Lines changed: 17 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -109,14 +109,12 @@ public class ResourceLinkPredicateBuilder extends BaseJoiningPredicateBuilder im
109109
private static final Pattern MODIFIER_REPLACE_PATTERN = Pattern.compile(".*:");
110110
private final DbColumn myColumnSrcType;
111111
private final DbColumn myColumnSrcPath;
112-
private final DbColumn myColumnTargetResourceId;
112+
protected final DbColumn myColumnTargetResourceId;
113113
private final DbColumn myColumnTargetResourceUrl;
114114
private final DbColumn myColumnSrcResourceId;
115-
private final DbColumn myColumnTargetResourceType;
115+
protected final DbColumn myColumnTargetResourceType;
116116
private final QueryStack myQueryStack;
117-
private final boolean myReversed;
118-
119-
private final DbColumn myColumnTargetPartitionId;
117+
protected final DbColumn myColumnTargetPartitionId;
120118
private final DbColumn myColumnSrcPartitionId;
121119

122120
@Autowired
@@ -146,8 +144,7 @@ public class ResourceLinkPredicateBuilder extends BaseJoiningPredicateBuilder im
146144
/**
147145
* Constructor
148146
*/
149-
public ResourceLinkPredicateBuilder(
150-
QueryStack theQueryStack, SearchQueryBuilder theSearchSqlBuilder, boolean theReversed) {
147+
public ResourceLinkPredicateBuilder(QueryStack theQueryStack, SearchQueryBuilder theSearchSqlBuilder) {
151148
super(theSearchSqlBuilder, theSearchSqlBuilder.addTable("HFJ_RES_LINK"));
152149
myColumnSrcResourceId = getTable().addColumn("SRC_RESOURCE_ID");
153150
myColumnSrcPartitionId = getTable().addColumn("PARTITION_ID");
@@ -158,17 +155,22 @@ public ResourceLinkPredicateBuilder(
158155
myColumnTargetResourceUrl = getTable().addColumn("TARGET_RESOURCE_URL");
159156
myColumnTargetResourceType = getTable().addColumn("TARGET_RESOURCE_TYPE");
160157

161-
myReversed = theReversed;
162158
myQueryStack = theQueryStack;
163159
}
164160

165161
@Override
166162
public DbColumn getResourceTypeColumn() {
167-
if (myReversed) {
168-
return myColumnTargetResourceType;
169-
} else {
170-
return myColumnSrcType;
171-
}
163+
return myColumnSrcType;
164+
}
165+
166+
@Override
167+
public DbColumn getPartitionIdColumn() {
168+
return myColumnSrcPartitionId;
169+
}
170+
171+
@Override
172+
public DbColumn getResourceIdColumn() {
173+
return myColumnSrcResourceId;
172174
}
173175

174176
public DbColumn getColumnSourcePath() {
@@ -201,27 +203,10 @@ public DbColumn[] getJoinColumns() {
201203
return super.getJoinColumns();
202204
}
203205

204-
public DbColumn getColumnSrcResourceId() {
205-
return myColumnSrcResourceId;
206-
}
207-
208-
public DbColumn getColumnSrcPartitionId() {
209-
return myColumnSrcPartitionId;
210-
}
211-
212206
public DbColumn getColumnTargetResourceType() {
213207
return myColumnTargetResourceType;
214208
}
215209

216-
@Override
217-
public DbColumn getResourceIdColumn() {
218-
if (myReversed) {
219-
return myColumnTargetResourceId;
220-
} else {
221-
return myColumnSrcResourceId;
222-
}
223-
}
224-
225210
@Nullable
226211
public Condition createPredicate(
227212
RequestDetails theRequest,
@@ -272,8 +257,7 @@ public Condition createPredicate(
272257
for (int orIdx = 0; orIdx < theReferenceOrParamList.size(); orIdx++) {
273258
IQueryParameterType nextOr = theReferenceOrParamList.get(orIdx);
274259

275-
if (nextOr instanceof ReferenceParam) {
276-
ReferenceParam ref = (ReferenceParam) nextOr;
260+
if (nextOr instanceof ReferenceParam ref) {
277261

278262
if (isBlank(ref.getChain())) {
279263

@@ -325,12 +309,7 @@ public Condition createPredicate(
325309
}
326310

327311
List<String> pathsToMatch = createResourceLinkPaths(theResourceType, theParamName, theQualifiers);
328-
boolean inverse;
329-
if ((theOperation == null) || (theOperation == SearchFilterParser.CompareOperation.eq)) {
330-
inverse = false;
331-
} else {
332-
inverse = true;
333-
}
312+
boolean inverse = (theOperation != null) && (theOperation != SearchFilterParser.CompareOperation.eq);
334313

335314
List<JpaPid> pids = myIdHelperService.resolveResourcePids(
336315
requestPartitionId,

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/PredicateBuilderFactory.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,10 +53,10 @@ public static ICanMakeMissingParamPredicate createPredicateBuilderForParamType(
5353
case URI:
5454
return createUriPredicateBuilder(theBuilder);
5555
case REFERENCE:
56-
return createReferencePredicateBuilder(theQueryStack, theBuilder);
57-
case HAS:
56+
return createResourceLinkPredicateBuilder(theQueryStack, theBuilder);
5857
case SPECIAL:
5958
return createCoordsPredicateBuilder(theBuilder);
59+
case HAS:
6060
case COMPOSITE:
6161
default:
6262
throw new InternalErrorException(Msg.code(2593) + "Invalid param type " + theParamType.name());
@@ -91,8 +91,8 @@ private static UriPredicateBuilder createUriPredicateBuilder(SearchQueryBuilder
9191
return theBuilder.getSqlBuilderFactory().uriIndexTable(theBuilder);
9292
}
9393

94-
private static ResourceLinkPredicateBuilder createReferencePredicateBuilder(
94+
private static ResourceLinkPredicateBuilder createResourceLinkPredicateBuilder(
9595
QueryStack theQueryStack, SearchQueryBuilder theBuilder) {
96-
return theBuilder.getSqlBuilderFactory().referenceIndexTable(theQueryStack, theBuilder, false);
96+
return theBuilder.getSqlBuilderFactory().resourceLinkIndexTable(theQueryStack, theBuilder);
9797
}
9898
}

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryBuilder.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceHistoryPredicateBuilder;
3939
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceHistoryProvenancePredicateBuilder;
4040
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceIdPredicateBuilder;
41+
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceLinkForHasParameterPredicateBuilder;
4142
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceLinkPredicateBuilder;
4243
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceTablePredicateBuilder;
4344
import ca.uhn.fhir.jpa.search.builder.predicate.SearchParamPresentPredicateBuilder;
@@ -322,16 +323,17 @@ public ResourceLinkPredicateBuilder addReferencePredicateBuilder(
322323
* Create a predicate builder for selecting on a REFERENCE search parameter
323324
*/
324325
public ResourceLinkPredicateBuilder createReferencePredicateBuilder(QueryStack theQueryStack) {
325-
return mySqlBuilderFactory.referenceIndexTable(theQueryStack, this, false);
326+
return mySqlBuilderFactory.resourceLinkIndexTable(theQueryStack, this);
326327
}
327328

328329
/**
329330
* Add and return a predicate builder (or a root query if no root query exists yet) for selecting on a resource link where the
330331
* source and target are reversed. This is used for _has queries.
331332
*/
332-
public ResourceLinkPredicateBuilder addReferencePredicateBuilderReversed(
333+
public ResourceLinkPredicateBuilder addResourceLinkForHasParameterPredicateBuilderReversed(
333334
QueryStack theQueryStack, DbColumn[] theSourceJoinColumn) {
334-
ResourceLinkPredicateBuilder retVal = mySqlBuilderFactory.referenceIndexTable(theQueryStack, this, true);
335+
ResourceLinkForHasParameterPredicateBuilder retVal =
336+
mySqlBuilderFactory.resourceLinkForHasParameterIndexTable(theQueryStack, this);
335337
addTable(retVal, theSourceJoinColumn);
336338
return retVal;
337339
}

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SqlObjectFactory.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceHistoryPredicateBuilder;
3131
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceHistoryProvenancePredicateBuilder;
3232
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceIdPredicateBuilder;
33+
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceLinkForHasParameterPredicateBuilder;
3334
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceLinkPredicateBuilder;
3435
import ca.uhn.fhir.jpa.search.builder.predicate.ResourceTablePredicateBuilder;
3536
import ca.uhn.fhir.jpa.search.builder.predicate.SearchParamPresentPredicateBuilder;
@@ -78,10 +79,16 @@ public QuantityNormalizedPredicateBuilder quantityNormalizedIndexTable(SearchQue
7879
return myApplicationContext.getBean(QuantityNormalizedPredicateBuilder.class, theSearchSqlBuilder);
7980
}
8081

81-
public ResourceLinkPredicateBuilder referenceIndexTable(
82-
QueryStack theQueryStack, SearchQueryBuilder theSearchSqlBuilder, boolean theReversed) {
82+
public ResourceLinkPredicateBuilder resourceLinkIndexTable(
83+
QueryStack theQueryStack, SearchQueryBuilder theSearchSqlBuilder) {
84+
return (ResourceLinkPredicateBuilder)
85+
myApplicationContext.getBean("newResourceLinkPredicateBuilder", theQueryStack, theSearchSqlBuilder);
86+
}
87+
88+
public ResourceLinkForHasParameterPredicateBuilder resourceLinkForHasParameterIndexTable(
89+
QueryStack theQueryStack, SearchQueryBuilder theSearchSqlBuilder) {
8390
return myApplicationContext.getBean(
84-
ResourceLinkPredicateBuilder.class, theQueryStack, theSearchSqlBuilder, theReversed);
91+
ResourceLinkForHasParameterPredicateBuilder.class, theQueryStack, theSearchSqlBuilder);
8592
}
8693

8794
public ResourceTablePredicateBuilder resourceTable(

‎hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/interceptor/PatientCompartmentEnforcingInterceptorTest.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
44
import ca.uhn.fhir.jpa.model.config.PartitionSettings;
5+
import ca.uhn.fhir.jpa.model.util.JpaConstants;
56
import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test;
67
import ca.uhn.fhir.jpa.searchparam.extractor.ISearchParamExtractor;
78
import ca.uhn.fhir.rest.api.PatchTypeEnum;
@@ -22,6 +23,7 @@
2223
import org.junit.jupiter.params.ParameterizedTest;
2324
import org.junit.jupiter.params.provider.CsvSource;
2425
import org.springframework.beans.factory.annotation.Autowired;
26+
import org.springframework.test.context.TestPropertySource;
2527

2628
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2729
import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -98,7 +100,7 @@ public void testUpdateResource_whenCrossingPatientCompartment_throws(boolean the
98100

99101
assertThatThrownBy(() -> myObservationDao.update(obs, new SystemRequestDetails()))
100102
.isInstanceOf(PreconditionFailedException.class)
101-
.hasMessageContaining("HAPI-2476: Resource compartment changed. Was a referenced Patient changed?");
103+
.hasMessageContaining("HAPI-2476: Resource compartment for Observation/O changed. Was a referenced Patient changed?");
102104
}
103105

104106
@ParameterizedTest
@@ -175,7 +177,7 @@ public void testCompartmentEnforcement_EncounterUpdate() {
175177
IBaseParameters patch = pb.build();
176178
assertThatThrownBy(() -> myEncounterDao.patch(new IdType("Encounter/E"), null, PatchTypeEnum.FHIR_PATCH_JSON, null, patch, newSrd()))
177179
.isInstanceOf(PreconditionFailedException.class)
178-
.hasMessageContaining("HAPI-2476: Resource compartment changed. Was a referenced Patient changed?");
180+
.hasMessageContaining("HAPI-2476: Resource compartment for Encounter/E changed. Was a referenced Patient changed?");
179181

180182
// Verify
181183
Encounter actual = myEncounterDao.read(new IdType("E"), mySrd);

0 commit comments

Comments
 (0)