Contained bug (#6402)

* Contained bug

* more tests

* changelog, tests, implementation

* code review

* backwards logic
This commit is contained in:
Tadgh 2024-10-25 18:09:28 -07:00 committed by GitHub
parent 60ed1fd225
commit e78a0b8742
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 209 additions and 38 deletions

View File

@ -105,7 +105,6 @@ public abstract class BaseParser implements IParser {
private static final Set<String> notEncodeForContainedResource =
new HashSet<>(Arrays.asList("security", "versionId", "lastUpdated"));
private FhirTerser.ContainedResources myContainedResources;
private boolean myEncodeElementsAppliesToChildResourcesOnly;
private final FhirContext myContext;
private Collection<String> myDontEncodeElements;
@ -183,12 +182,15 @@ public abstract class BaseParser implements IParser {
}
private String determineReferenceText(
IBaseReference theRef, CompositeChildElement theCompositeChildElement, IBaseResource theResource) {
IBaseReference theRef,
CompositeChildElement theCompositeChildElement,
IBaseResource theResource,
EncodeContext theContext) {
IIdType ref = theRef.getReferenceElement();
if (isBlank(ref.getIdPart())) {
String reference = ref.getValue();
if (theRef.getResource() != null) {
IIdType containedId = getContainedResources().getResourceId(theRef.getResource());
IIdType containedId = theContext.getContainedResources().getResourceId(theRef.getResource());
if (containedId != null && !containedId.isEmpty()) {
if (containedId.isLocal()) {
reference = containedId.getValue();
@ -262,7 +264,8 @@ public abstract class BaseParser implements IParser {
@Override
public final void encodeResourceToWriter(IBaseResource theResource, Writer theWriter)
throws IOException, DataFormatException {
EncodeContext encodeContext = new EncodeContext(this, myContext.getParserOptions());
EncodeContext encodeContext =
new EncodeContext(this, myContext.getParserOptions(), new FhirTerser.ContainedResources());
encodeResourceToWriter(theResource, theWriter, encodeContext);
}
@ -285,7 +288,8 @@ public abstract class BaseParser implements IParser {
} else if (theElement instanceof IPrimitiveType) {
theWriter.write(((IPrimitiveType<?>) theElement).getValueAsString());
} else {
EncodeContext encodeContext = new EncodeContext(this, myContext.getParserOptions());
EncodeContext encodeContext =
new EncodeContext(this, myContext.getParserOptions(), new FhirTerser.ContainedResources());
encodeToWriter(theElement, theWriter, encodeContext);
}
}
@ -404,10 +408,6 @@ public abstract class BaseParser implements IParser {
return elementId;
}
FhirTerser.ContainedResources getContainedResources() {
return myContainedResources;
}
@Override
public Set<String> getDontStripVersionsFromReferencesAtPaths() {
return myDontStripVersionsFromReferencesAtPaths;
@ -539,10 +539,11 @@ public abstract class BaseParser implements IParser {
return mySuppressNarratives;
}
protected boolean isChildContained(BaseRuntimeElementDefinition<?> childDef, boolean theIncludedResource) {
protected boolean isChildContained(
BaseRuntimeElementDefinition<?> childDef, boolean theIncludedResource, EncodeContext theContext) {
return (childDef.getChildType() == ChildTypeEnum.CONTAINED_RESOURCES
|| childDef.getChildType() == ChildTypeEnum.CONTAINED_RESOURCE_LIST)
&& getContainedResources().isEmpty() == false
&& theContext.getContainedResources().isEmpty() == false
&& theIncludedResource == false;
}
@ -788,7 +789,8 @@ public abstract class BaseParser implements IParser {
*/
if (next instanceof IBaseReference) {
IBaseReference nextRef = (IBaseReference) next;
String refText = determineReferenceText(nextRef, theCompositeChildElement, theResource);
String refText =
determineReferenceText(nextRef, theCompositeChildElement, theResource, theEncodeContext);
if (!StringUtils.equals(refText, nextRef.getReferenceElement().getValue())) {
if (retVal == theValues) {
@ -980,7 +982,7 @@ public abstract class BaseParser implements IParser {
return true;
}
protected void containResourcesInReferences(IBaseResource theResource) {
protected void containResourcesInReferences(IBaseResource theResource, EncodeContext theContext) {
/*
* If a UUID is present in Bundle.entry.fullUrl but no value is present
@ -1003,7 +1005,7 @@ public abstract class BaseParser implements IParser {
}
}
myContainedResources = getContext().newTerser().containResources(theResource);
theContext.setContainedResources(getContext().newTerser().containResources(theResource));
}
static class ChildNameAndDef {
@ -1034,8 +1036,12 @@ public abstract class BaseParser implements IParser {
private final List<EncodeContextPath> myEncodeElementPaths;
private final Set<String> myEncodeElementsAppliesToResourceTypes;
private final List<EncodeContextPath> myDontEncodeElementPaths;
private FhirTerser.ContainedResources myContainedResources;
public EncodeContext(BaseParser theParser, ParserOptions theParserOptions) {
public EncodeContext(
BaseParser theParser,
ParserOptions theParserOptions,
FhirTerser.ContainedResources theContainedResources) {
Collection<String> encodeElements = theParser.myEncodeElements;
Collection<String> dontEncodeElements = theParser.myDontEncodeElements;
if (isSummaryMode()) {
@ -1058,6 +1064,8 @@ public abstract class BaseParser implements IParser {
dontEncodeElements.stream().map(EncodeContextPath::new).collect(Collectors.toList());
}
myContainedResources = theContainedResources;
myEncodeElementsAppliesToResourceTypes =
ParserUtil.determineApplicableResourceTypesForTerserPaths(myEncodeElementPaths);
}
@ -1065,6 +1073,14 @@ public abstract class BaseParser implements IParser {
private Map<Key, List<BaseParser.CompositeChildElement>> getCompositeChildrenCache() {
return myCompositeChildrenCache;
}
public FhirTerser.ContainedResources getContainedResources() {
return myContainedResources;
}
public void setContainedResources(FhirTerser.ContainedResources theContainedResources) {
myContainedResources = theContainedResources;
}
}
protected class CompositeChildElement {

View File

@ -54,6 +54,7 @@ import ca.uhn.fhir.parser.json.JsonLikeStructure;
import ca.uhn.fhir.parser.json.jackson.JacksonStructure;
import ca.uhn.fhir.rest.api.EncodingEnum;
import ca.uhn.fhir.util.ElementUtil;
import ca.uhn.fhir.util.FhirTerser;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.Validate;
import org.apache.commons.text.WordUtils;
@ -386,12 +387,14 @@ public class JsonParser extends BaseParser implements IJsonLikeParser {
}
case CONTAINED_RESOURCE_LIST:
case CONTAINED_RESOURCES: {
List<IBaseResource> containedResources = getContainedResources().getContainedResources();
List<IBaseResource> containedResources =
theEncodeContext.getContainedResources().getContainedResources();
if (containedResources.size() > 0) {
beginArray(theEventWriter, theChildName);
for (IBaseResource next : containedResources) {
IIdType resourceId = getContainedResources().getResourceId(next);
IIdType resourceId =
theEncodeContext.getContainedResources().getResourceId(next);
String value = resourceId.getValue();
encodeResourceToJsonStreamWriter(
theResDef,
@ -554,7 +557,8 @@ public class JsonParser extends BaseParser implements IJsonLikeParser {
if (nextValue == null || nextValue.isEmpty()) {
if (nextValue instanceof BaseContainedDt) {
if (theContainedResource || getContainedResources().isEmpty()) {
if (theContainedResource
|| theEncodeContext.getContainedResources().isEmpty()) {
continue;
}
} else {
@ -838,7 +842,8 @@ public class JsonParser extends BaseParser implements IJsonLikeParser {
+ theResource.getStructureFhirVersionEnum());
}
EncodeContext encodeContext = new EncodeContext(this, getContext().getParserOptions());
EncodeContext encodeContext =
new EncodeContext(this, getContext().getParserOptions(), new FhirTerser.ContainedResources());
String resourceName = getContext().getResourceType(theResource);
encodeContext.pushPath(resourceName, true);
doEncodeResourceToJsonLikeWriter(theResource, theJsonLikeWriter, encodeContext);
@ -894,7 +899,7 @@ public class JsonParser extends BaseParser implements IJsonLikeParser {
}
if (!theContainedResource) {
containResourcesInReferences(theResource);
containResourcesInReferences(theResource, theEncodeContext);
}
RuntimeResourceDefinition resDef = getContext().getResourceDefinition(theResource);

View File

@ -191,7 +191,7 @@ public class RDFParser extends BaseParser {
}
if (!containedResource) {
containResourcesInReferences(resource);
containResourcesInReferences(resource, encodeContext);
}
if (!(resource instanceof IAnyResource)) {
@ -354,7 +354,7 @@ public class RDFParser extends BaseParser {
try {
if (element == null || element.isEmpty()) {
if (!isChildContained(childDef, includedResource)) {
if (!isChildContained(childDef, includedResource, theEncodeContext)) {
return rdfModel;
}
}

View File

@ -295,7 +295,7 @@ public class XmlParser extends BaseParser {
try {
if (theElement == null || theElement.isEmpty()) {
if (isChildContained(childDef, theIncludedResource)) {
if (isChildContained(childDef, theIncludedResource, theEncodeContext)) {
// We still want to go in..
} else {
return;
@ -359,8 +359,10 @@ public class XmlParser extends BaseParser {
* theEventWriter.writeStartElement("contained"); encodeResourceToXmlStreamWriter(next, theEventWriter, true, fixContainedResourceId(next.getId().getValue()));
* theEventWriter.writeEndElement(); }
*/
for (IBaseResource next : getContainedResources().getContainedResources()) {
IIdType resourceId = getContainedResources().getResourceId(next);
for (IBaseResource next :
theEncodeContext.getContainedResources().getContainedResources()) {
IIdType resourceId =
theEncodeContext.getContainedResources().getResourceId(next);
theEventWriter.writeStartElement("contained");
String value = resourceId.getValue();
encodeResourceToXmlStreamWriter(
@ -682,7 +684,7 @@ public class XmlParser extends BaseParser {
}
if (!theContainedResource) {
containResourcesInReferences(theResource);
containResourcesInReferences(theResource, theEncodeContext);
}
theEventWriter.writeStartElement(resDef.getName());

View File

@ -61,6 +61,7 @@ import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.IdentityHashMap;
@ -83,10 +84,23 @@ public class FhirTerser {
private static final Pattern COMPARTMENT_MATCHER_PATH =
Pattern.compile("([a-zA-Z.]+)\\.where\\(resolve\\(\\) is ([a-zA-Z]+)\\)");
private static final String USER_DATA_KEY_CONTAIN_RESOURCES_COMPLETED =
FhirTerser.class.getName() + "_CONTAIN_RESOURCES_COMPLETED";
private final FhirContext myContext;
/**
* This comparator sorts IBaseReferences, and places any that are missing an ID at the end. Those with an ID go to the front.
*/
private static final Comparator<IBaseReference> REFERENCES_WITH_IDS_FIRST =
Comparator.nullsLast(Comparator.comparing(ref -> {
if (ref.getResource() == null) return true;
if (ref.getResource().getIdElement() == null) return true;
if (ref.getResource().getIdElement().getValue() == null) return true;
return false;
}));
public FhirTerser(FhirContext theContext) {
super();
myContext = theContext;
@ -1418,6 +1432,13 @@ public class FhirTerser {
private void containResourcesForEncoding(
ContainedResources theContained, IBaseResource theResource, boolean theModifyResource) {
List<IBaseReference> allReferences = getAllPopulatedChildElementsOfType(theResource, IBaseReference.class);
// Note that we process all contained resources that have arrived here with an ID contained resources first, so
// that we don't accidentally auto-assign an ID
// which may collide with a resource we have yet to process.
// See: https://github.com/hapifhir/hapi-fhir/issues/6403
allReferences.sort(REFERENCES_WITH_IDS_FIRST);
for (IBaseReference next : allReferences) {
IBaseResource resource = next.getResource();
if (resource == null && next.getReferenceElement().isLocal()) {
@ -1437,11 +1458,11 @@ public class FhirTerser {
IBaseResource resource = next.getResource();
if (resource != null) {
if (resource.getIdElement().isEmpty() || resource.getIdElement().isLocal()) {
if (theContained.getResourceId(resource) != null) {
// Prevent infinite recursion if there are circular loops in the contained resources
IIdType id = theContained.addContained(resource);
if (id == null) {
continue;
}
IIdType id = theContained.addContained(resource);
if (theModifyResource) {
getContainedResourceList(theResource).add(resource);
next.setReference(id.getValue());
@ -1769,7 +1790,6 @@ public class FhirTerser {
public static class ContainedResources {
private long myNextContainedId = 1;
private List<IBaseResource> myResourceList;
private IdentityHashMap<IBaseResource, IIdType> myResourceToIdMap;
private Map<String, IBaseResource> myExistingIdToContainedResourceMap;
@ -1782,6 +1802,11 @@ public class FhirTerser {
}
public IIdType addContained(IBaseResource theResource) {
if (this.getResourceId(theResource) != null) {
// Prevent infinite recursion if there are circular loops in the contained resources
return null;
}
IIdType existing = getResourceToIdMap().get(theResource);
if (existing != null) {
return existing;
@ -1796,7 +1821,10 @@ public class FhirTerser {
if (substring(idPart, 0, 1).equals("#")) {
idPart = idPart.substring(1);
if (StringUtils.isNumeric(idPart)) {
myNextContainedId = Long.parseLong(idPart) + 1;
// If there is a user-supplied numeric contained ID, our auto-assigned IDs should exceed the
// largest
// client-assigned contained ID.
myNextContainedId = Math.max(myNextContainedId, Long.parseLong(idPart) + 1);
}
}
}
@ -1864,6 +1892,7 @@ public class FhirTerser {
}
public void assignIdsToContainedResources() {
// TODO JA: Dead code?
if (!getContainedResources().isEmpty()) {

View File

@ -0,0 +1,6 @@
---
type: fix
jira: SMILE-8969
title: "Fixed a rare bug in the JSON Parser, wherein client-assigned contained resource IDs could collide with server-assigned contained IDs. For example if a
resource had a client-assigned contained ID of `#2`, and a contained resource with no ID, then depending on the processing order, the parser could occasionally
provide duplicate contained resource IDs, leading to non-deterministic behaviour."

View File

@ -1,3 +1,22 @@
/*-
* #%L
* HAPI FHIR JPA Server
* %%
* Copyright (C) 2014 - 2024 Smile CDR, Inc.
* %%
* 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.
* See the License for the specific language governing permissions and
* limitations under the License.
* #L%
*/
package ca.uhn.fhir.jpa.search.builder.sql;
import com.healthmarketscience.common.util.AppendableExt;

View File

@ -1,6 +1,7 @@
package ca.uhn.fhir.jpa.dao.r4;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
import ca.uhn.fhir.rest.api.server.IBundleProvider;
@ -23,6 +24,8 @@ import org.hl7.fhir.r4.model.Reference;
import org.hl7.fhir.r4.model.ServiceRequest;
import org.hl7.fhir.r4.model.ServiceRequest.ServiceRequestIntent;
import org.hl7.fhir.r4.model.ServiceRequest.ServiceRequestStatus;
import org.hl7.fhir.r4.model.Specimen;
import org.hl7.fhir.r4.model.StringType;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

View File

@ -263,6 +263,8 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test {
myStorageSettings.setSearchPreFetchThresholds(new JpaStorageSettings().getSearchPreFetchThresholds());
}
@Test
public void testParameterWithNoValueThrowsError_InvalidChainOnCustomSearch() throws IOException {
SearchParameter searchParameter = new SearchParameter();

View File

@ -23,6 +23,7 @@ import org.hl7.fhir.r4.model.Composition;
import org.hl7.fhir.r4.model.DateTimeType;
import org.hl7.fhir.r4.model.DecimalType;
import org.hl7.fhir.r4.model.Device;
import org.hl7.fhir.r4.model.DiagnosticReport;
import org.hl7.fhir.r4.model.DocumentReference;
import org.hl7.fhir.r4.model.Encounter;
import org.hl7.fhir.r4.model.Extension;
@ -43,6 +44,7 @@ import org.hl7.fhir.r4.model.PrimitiveType;
import org.hl7.fhir.r4.model.Quantity;
import org.hl7.fhir.r4.model.QuestionnaireResponse;
import org.hl7.fhir.r4.model.Reference;
import org.hl7.fhir.r4.model.Specimen;
import org.hl7.fhir.r4.model.StringType;
import org.hl7.fhir.r4.model.Type;
import org.hl7.fhir.r4.model.codesystems.DataAbsentReason;
@ -55,6 +57,8 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import jakarta.annotation.Nonnull;
import org.testcontainers.shaded.com.trilead.ssh2.packets.PacketDisconnect;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Date;
@ -261,7 +265,81 @@ public class JsonParserR4Test extends BaseTest {
idx = encoded.indexOf("\"Medication\"", idx + 1);
assertEquals(-1, idx);
}
@Test
public void testDuplicateContainedResourcesAcrossABundleAreReplicated() {
Bundle b = new Bundle();
Specimen specimen = new Specimen();
Practitioner practitioner = new Practitioner();
DiagnosticReport report = new DiagnosticReport();
report.addSpecimen(new Reference(specimen));
b.addEntry().setResource(report).getRequest().setMethod(Bundle.HTTPVerb.POST).setUrl("/DiagnosticReport");
Observation obs = new Observation();
obs.addPerformer(new Reference(practitioner));
obs.setSpecimen(new Reference(specimen));
b.addEntry().setResource(obs).getRequest().setMethod(Bundle.HTTPVerb.POST).setUrl("/Observation");
String encoded = ourCtx.newJsonParser().setPrettyPrint(false).encodeResourceToString(b);
//Then: Diag should contain one local contained specimen
assertThat(encoded).contains("[{\"resource\":{\"resourceType\":\"DiagnosticReport\",\"contained\":[{\"resourceType\":\"Specimen\",\"id\":\"1\"}]");
//Then: Obs should contain one local contained specimen, and one local contained pract
assertThat(encoded).contains("\"resource\":{\"resourceType\":\"Observation\",\"contained\":[{\"resourceType\":\"Specimen\",\"id\":\"1\"},{\"resourceType\":\"Practitioner\",\"id\":\"2\"}]");
assertThat(encoded).contains("\"performer\":[{\"reference\":\"#2\"}],\"specimen\":{\"reference\":\"#1\"}");
//Also, reverting the operation should work too!
Bundle bundle = ourCtx.newJsonParser().parseResource(Bundle.class, encoded);
IBaseResource resource1 = ((DiagnosticReport) bundle.getEntry().get(0).getResource()).getSpecimenFirstRep().getResource();
IBaseResource resource = ((Observation) bundle.getEntry().get(1).getResource()).getSpecimen().getResource();
assertThat(resource1.getIdElement().getIdPart()).isEqualTo(resource.getIdElement().getIdPart());
assertThat(resource1).isNotSameAs(resource);
}
@Test
public void testAutoAssignedContainedCollisionOrderDependent() {
{
Specimen specimen = new Specimen();
Practitioner practitioner = new Practitioner();
DiagnosticReport report = new DiagnosticReport();
report.addSpecimen(new Reference(specimen));
Observation obs = new Observation();
//When: The practitioner (which is parsed first, has an assigned id that will collide with auto-assigned
practitioner.setId("#1");
obs.addPerformer(new Reference(practitioner));
obs.setSpecimen(new Reference(specimen));
String encoded = ourCtx.newJsonParser().setPrettyPrint(false).encodeResourceToString(obs);
assertThat(encoded).contains("\"contained\":[{\"resourceType\":\"Practitioner\",\"id\":\"1\"},{\"resourceType\":\"Specimen\",\"id\":\"2\"}]");
assertThat(encoded).contains("\"performer\":[{\"reference\":\"#1\"}]");
assertThat(encoded).contains("\"specimen\":{\"reference\":\"#2\"}}");
ourLog.info(encoded);
}
{
Specimen specimen = new Specimen();
Practitioner practitioner = new Practitioner();
DiagnosticReport report = new DiagnosticReport();
report.addSpecimen(new Reference(specimen));
Observation obs = new Observation();
//When: The specimen (which is parsed second, has an assigned id that will collide with auto-assigned practitioner
specimen.setId("#1");
obs.addPerformer(new Reference(practitioner));
obs.setSpecimen(new Reference(specimen));
String encoded = ourCtx.newJsonParser().setPrettyPrint(false).encodeResourceToString(obs);
assertThat(encoded).contains("\"contained\":[{\"resourceType\":\"Specimen\",\"id\":\"1\"},{\"resourceType\":\"Practitioner\",\"id\":\"2\"}]");
assertThat(encoded).contains("\"performer\":[{\"reference\":\"#2\"}]");
assertThat(encoded).contains("\"specimen\":{\"reference\":\"#1\"}}");
ourLog.info(encoded);
}
}
@Test

View File

@ -6,7 +6,10 @@ import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.i18n.Msg;
import ca.uhn.fhir.model.api.annotation.Block;
import ca.uhn.fhir.parser.DataFormatException;
import ca.uhn.fhir.parser.IParser;
import ca.uhn.fhir.parser.JsonParser;
import com.google.common.collect.Lists;
import org.apache.jena.base.Sys;
import org.hl7.fhir.instance.model.api.IBase;
import org.hl7.fhir.instance.model.api.IBaseExtension;
import org.hl7.fhir.instance.model.api.IBaseReference;
@ -47,6 +50,8 @@ import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.shaded.com.fasterxml.jackson.core.JsonProcessingException;
import org.testcontainers.shaded.com.fasterxml.jackson.databind.ObjectMapper;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
@ -1545,23 +1550,29 @@ public class FhirTerserR4Test {
@Test
void copyingAndParsingCreatesDuplicateContainedResources() {
var input = new Library();
var library = new Library();
var params = new Parameters();
var id = "#expansion-parameters-ecr";
params.setId(id);
params.addParameter("system-version", new StringType("test2"));
var paramsExt = new Extension();
paramsExt.setUrl("test").setValue(new Reference(id));
input.addContained(params);
input.addExtension(paramsExt);
library.addContained(params);
library.addExtension(paramsExt);
final var parser = FhirContext.forR4Cached().newJsonParser();
var stringified = parser.encodeResourceToString(input);
var stringified = parser.encodeResourceToString(library);
var parsed = parser.parseResource(stringified);
var copy = ((Library) parsed).copy();
assertEquals(1, copy.getContained().size());
var stringifiedCopy = parser.encodeResourceToString(copy);
var parsedCopy = parser.parseResource(stringifiedCopy);
assertEquals(1, ((Library) parsedCopy).getContained().size());
String stringifiedCopy = FhirContext.forR4Cached().newJsonParser().encodeResourceToString(copy);
Library parsedCopy = (Library) parser.parseResource(stringifiedCopy);
assertEquals(1, parsedCopy.getContained().size());
}
/**