Skip to content

Commit

Permalink
Merge branch 'rel_8_0' into missing-search-params-with-hsearch-enabled
Browse files Browse the repository at this point in the history
  • Loading branch information
Abayomi Adebowale committed Feb 1, 2025
2 parents dba6493 + d636df3 commit 13b66d9
Show file tree
Hide file tree
Showing 4 changed files with 254 additions and 33 deletions.
1 change: 1 addition & 0 deletions hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ ca.uhn.fhir.jpa.dao.BaseTransactionProcessor.fhirPatchShouldNotUseBinaryResource
ca.uhn.fhir.jpa.patch.FhirPatch.invalidInsertIndex=Invalid insert index {0} for path {1} - Only have {2} existing entries
ca.uhn.fhir.jpa.patch.FhirPatch.invalidMoveSourceIndex=Invalid move source index {0} for path {1} - Only have {2} existing entries
ca.uhn.fhir.jpa.patch.FhirPatch.invalidMoveDestinationIndex=Invalid move destination index {0} for path {1} - Only have {2} existing entries
ca.uhn.fhir.jpa.patch.FhirPatch.noMatchingElementForPath=No element matches the specified path: {0}
ca.uhn.fhir.jpa.searchparam.extractor.BaseSearchParamExtractor.externalReferenceNotAllowed=Resource contains external reference to URL "{0}" but this server is not configured to allow external references
ca.uhn.fhir.jpa.searchparam.extractor.BaseSearchParamExtractor.failedToExtractPaths=Failed to extract values from resource using FHIRPath "{0}": {1}
ca.uhn.fhir.jpa.search.SearchCoordinatorSvcImpl.invalidInclude=Invalid {0} parameter value: "{1}". {2}
Expand Down
6 changes: 6 additions & 0 deletions ...ca/uhn/hapi/fhir/changelog/7_8_0/6656-fhir-patch-replace-fix-for-collection-elements.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
type: fix
issue: 6656
title: "Previously, FHIR patch 'replace' would fail when trying to replace a sub-element of a high cardinality element
using the the FHIR patch syntax. This has been fixed."

112 changes: 112 additions & 0 deletions hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/patch/FhirPatchApplyR4Test.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class FhirPatchApplyR4Test {

Expand Down Expand Up @@ -456,6 +457,117 @@ public void testAddToHighCardinalityFieldSetsValueIfEmpty() {
assertEquals("third-system", patient.getIdentifier().get(0).getSystem());
assertEquals("third-value", patient.getIdentifier().get(0).getValue());
}


@Test
public void testReplaceAnElementInHighCardinalityFieldByIndex() {
FhirPatch svc = new FhirPatch(ourCtx);
Patient patient = new Patient();
patient.addIdentifier().setSystem("first-system").setValue("first-value");
patient.addIdentifier().setSystem("second-system").setValue("second-value");

//Given: We create a patch to replace the second identifier
Identifier theValue = new Identifier().setSystem("third-system").setValue("third-value");
Parameters patch = new Parameters();
patch.addParameter(createPatchReplaceOperation("Patient.identifier[1]", theValue));

//When: We apply the patch
svc.apply(patient, patch);
ourLog.debug("Outcome:\n{}", ourCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(patient));

//Then: it replaces the identifier correctly.
assertThat(patient.getIdentifier()).hasSize(2);
assertThat(patient.getIdentifier().get(0).getSystem()).isEqualTo("first-system");
assertThat(patient.getIdentifier().get(0).getValue()).isEqualTo("first-value");
assertThat(patient.getIdentifier().get(1).getSystem()).isEqualTo("third-system");
assertThat(patient.getIdentifier().get(1).getValue()).isEqualTo("third-value");
}

@ParameterizedTest
@CsvSource({
"Patient.identifier.where(system='not-an-existing-system')", //filter for not existing element
"Patient.identifier[5]" // index out of bounds for the element
})
public void testReplaceAnElementInHighCardinalityField_NoMatchingElement_InvalidRequest(String thePath) {
FhirPatch svc = new FhirPatch(ourCtx);
Patient patient = new Patient();
patient.addIdentifier().setSystem("first-system").setValue("first-value");
patient.addIdentifier().setSystem("second-system").setValue("second-value");

//Given: We create a patch to replace the second identifier
Identifier theValue = new Identifier().setSystem("third-system").setValue("third-value");
Parameters patch = new Parameters();
patch.addParameter(createPatchReplaceOperation(thePath, theValue));

//When: We apply the patch, expect an InvalidRequestException
InvalidRequestException ex = assertThrows(InvalidRequestException.class, () -> svc.apply(patient, patch));
String expectedMessage = String.format("HAPI-2617: No element matches the specified path: %s", thePath);
assertThat(ex.getMessage()).isEqualTo(expectedMessage);


assertThat(patient.getIdentifier()).hasSize(2);
assertThat(patient.getIdentifier().get(0).getSystem()).isEqualTo("first-system");
assertThat(patient.getIdentifier().get(0).getValue()).isEqualTo("first-value");
assertThat(patient.getIdentifier().get(1).getSystem()).isEqualTo("second-system");
assertThat(patient.getIdentifier().get(1).getValue()).isEqualTo("second-value");
}


@Test
public void testReplaceAnElementInHighCardinalityFieldByFilter_SingleMatch() {
FhirPatch svc = new FhirPatch(ourCtx);
Patient patient = new Patient();
patient.addIdentifier().setSystem("first-system").setValue("first-value");
patient.addIdentifier().setSystem("second-system").setValue("second-value");

//Given: We create a patch to replace the second identifier
Identifier theValue = new Identifier().setSystem("third-system").setValue("third-value");
Parameters patch = new Parameters();
patch.addParameter(createPatchReplaceOperation("Patient.identifier.where(system='second-system')",
theValue));

//When: We apply the patch
svc.apply(patient, patch);
ourLog.debug("Outcome:\n{}", ourCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(patient));

//Then: it replaces the identifier correctly.
assertThat(patient.getIdentifier()).hasSize(2);
assertThat(patient.getIdentifier().get(0).getSystem()).isEqualTo("first-system");
assertThat(patient.getIdentifier().get(0).getValue()).isEqualTo("first-value");
assertThat(patient.getIdentifier().get(1).getSystem()).isEqualTo("third-system");
assertThat(patient.getIdentifier().get(1).getValue()).isEqualTo("third-value");
}

@Test
public void testReplaceElementsInHighCardinalityFieldByFilter_MultipleMatches() {
FhirPatch svc = new FhirPatch(ourCtx);
Patient patient = new Patient();
patient.addIdentifier().setSystem("existing-system1").setValue("first-value");
patient.addIdentifier().setSystem("to-be-replaced-system").setValue("second-value");
patient.addIdentifier().setSystem("existing-system2").setValue("third-value");
patient.addIdentifier().setSystem("to-be-replaced-system").setValue("fourth-value");
//Given: We create a patch to replace the second identifier
Identifier theValue = new Identifier().setSystem("new-system").setValue("new-value");
Parameters patch = new Parameters();
patch.addParameter(createPatchReplaceOperation("Patient.identifier.where(system='to-be-replaced-system')",
theValue));

//When: We apply the patch
svc.apply(patient, patch);
ourLog.debug("Outcome:\n{}", ourCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(patient));

//Then: it replaces identifiers correctly.
assertThat(patient.getIdentifier()).hasSize(4);
assertThat(patient.getIdentifier().get(0).getSystem()).isEqualTo("existing-system1");
assertThat(patient.getIdentifier().get(0).getValue()).isEqualTo("first-value");
assertThat(patient.getIdentifier().get(1).getSystem()).isEqualTo("new-system");
assertThat(patient.getIdentifier().get(1).getValue()).isEqualTo("new-value");
assertThat(patient.getIdentifier().get(2).getSystem()).isEqualTo("existing-system2");
assertThat(patient.getIdentifier().get(2).getValue()).isEqualTo("third-value");
assertThat(patient.getIdentifier().get(3).getSystem()).isEqualTo("new-system");
assertThat(patient.getIdentifier().get(3).getValue()).isEqualTo("new-value");
}

@Test
public void testReplaceToHighCardinalityFieldRemovesAllAndSetsValue() {
FhirPatch svc = new FhirPatch(ourCtx);
Expand Down
168 changes: 135 additions & 33 deletions hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/patch/FhirPatch.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,34 +172,20 @@ private void handleDeleteOperation(IBaseResource theResource, IBase theParameter
String path = ParametersUtil.getParameterPartValueAsString(myContext, theParameters, PARAMETER_PATH);
path = defaultString(path);

String containingPath;
String elementName;

if (path.endsWith(")")) {
// This is probably a filter, so we're probably dealing with a list
int filterArgsIndex = path.lastIndexOf('('); // Let's hope there aren't nested parentheses
int lastDotIndex = path.lastIndexOf(
'.', filterArgsIndex); // There might be a dot inside the parentheses, so look to the left of that
int secondLastDotIndex = path.lastIndexOf('.', lastDotIndex - 1);
containingPath = path.substring(0, secondLastDotIndex);
elementName = path.substring(secondLastDotIndex + 1, lastDotIndex);
} else if (path.endsWith("]")) {
// This is almost definitely a list
int openBracketIndex = path.lastIndexOf('[');
int lastDotIndex = path.lastIndexOf('.', openBracketIndex);
containingPath = path.substring(0, lastDotIndex);
elementName = path.substring(lastDotIndex + 1, openBracketIndex);
} else {
containingPath = path;
elementName = null;
}
ParsedPath parsedPath = ParsedPath.parse(path);
List<IBase> containingElements = myContext
.newFhirPath()
.evaluate(
theResource,
parsedPath.getEndsWithAFilterOrIndex() ? parsedPath.getContainingPath() : path,
IBase.class);

List<IBase> containingElements = myContext.newFhirPath().evaluate(theResource, containingPath, IBase.class);
for (IBase nextElement : containingElements) {
if (elementName == null) {
deleteSingleElement(nextElement);
if (parsedPath.getEndsWithAFilterOrIndex()) {
// if the path ends with a filter or index, we must be dealing with a list
deleteFromList(theResource, nextElement, parsedPath.getLastElementName(), path);
} else {
deleteFromList(theResource, nextElement, elementName, path);
deleteSingleElement(nextElement);
}
}
}
Expand Down Expand Up @@ -227,18 +213,51 @@ private void handleReplaceOperation(IBaseResource theResource, IBase theParamete
String path = ParametersUtil.getParameterPartValueAsString(myContext, theParameters, PARAMETER_PATH);
path = defaultString(path);

int lastDot = path.lastIndexOf(".");
String containingPath = path.substring(0, lastDot);
String elementName = path.substring(lastDot + 1);
ParsedPath parsedPath = ParsedPath.parse(path);

List<IBase> containingElements = myContext.newFhirPath().evaluate(theResource, containingPath, IBase.class);
for (IBase nextElement : containingElements) {
List<IBase> containingElements =
myContext.newFhirPath().evaluate(theResource, parsedPath.getContainingPath(), IBase.class);
for (IBase containingElement : containingElements) {

ChildDefinition childDefinition = findChildDefinition(nextElement, elementName);
ChildDefinition childDefinition = findChildDefinition(containingElement, parsedPath.getLastElementName());
IBase newValue = getNewValue(theParameters, containingElement, childDefinition);
if (parsedPath.getEndsWithAFilterOrIndex()) {
// if the path ends with a filter or index, we must be dealing with a list
replaceInList(newValue, theResource, containingElement, childDefinition, path);
} else {
childDefinition.getChildDef().getMutator().setValue(containingElement, newValue);
}
}
}

IBase newValue = getNewValue(theParameters, nextElement, childDefinition);
private void replaceInList(
IBase theNewValue,
IBaseResource theResource,
IBase theContainingElement,
ChildDefinition theChildDefinitionForTheList,
String theFullReplacePath) {

List<IBase> existingValues = new ArrayList<>(
theChildDefinitionForTheList.getChildDef().getAccessor().getValues(theContainingElement));
List<IBase> valuesToReplace = myContext.newFhirPath().evaluate(theResource, theFullReplacePath, IBase.class);
if (valuesToReplace.isEmpty()) {
String msg = myContext
.getLocalizer()
.getMessage(FhirPatch.class, "noMatchingElementForPath", theFullReplacePath);
throw new InvalidRequestException(Msg.code(2617) + msg);
}

childDefinition.getChildDef().getMutator().setValue(nextElement, newValue);
BaseRuntimeChildDefinition.IMutator listMutator =
theChildDefinitionForTheList.getChildDef().getMutator();
// clear the whole list first, then reconstruct it in the loop below replacing the values that need to be
// replaced
listMutator.setValue(theContainingElement, null);
for (IBase existingValue : existingValues) {
if (valuesToReplace.contains(existingValue)) {
listMutator.addValue(theContainingElement, theNewValue);
} else {
listMutator.addValue(theContainingElement, existingValue);
}
}
}

Expand Down Expand Up @@ -667,4 +686,87 @@ public BaseRuntimeElementDefinition<?> getChildElement() {
return myChildElement;
}
}

/**
* This class helps parse a FHIR path into its component parts for easier patch operation processing.
* It has 3 components:
* - The last element name, which is the last element in the path (not including any list index or filter)
* - The containing path, which is the prefix of the path up to the last element
* - A flag indicating whether the path has a filter or index on the last element of the path, which indicates
* that the path we are dealing is probably for a list element.
* Examples:
* 1. For path "Patient.identifier[2].system",
* - the lastElementName is "system",
* - the containingPath is "Patient.identifier[2]",
* - and endsWithAFilterOrIndex flag is false
*
* 2. For path "Patient.identifier[2]" or for path "Patient.identifier.where('system'='sys1')"
* - the lastElementName is "identifier",
* - the containingPath is "Patient",
* - and the endsWithAFilterOrIndex is true
*/
protected static class ParsedPath {
private final String myLastElementName;
private final String myContainingPath;
private final boolean myEndsWithAFilterOrIndex;

public ParsedPath(String theLastElementName, String theContainingPath, boolean theEndsWithAFilterOrIndex) {
myLastElementName = theLastElementName;
myContainingPath = theContainingPath;
myEndsWithAFilterOrIndex = theEndsWithAFilterOrIndex;
}

/**
* returns the last element of the path
*/
public String getLastElementName() {
return myLastElementName;
}

/**
* Returns the prefix of the path up to the last FHIR resource element
*/
public String getContainingPath() {
return myContainingPath;
}

/**
* Returns whether the path has a filter or index on the last element of the path, which indicates
* that the path we are dealing is probably a list element.
*/
public boolean getEndsWithAFilterOrIndex() {
return myEndsWithAFilterOrIndex;
}

public static ParsedPath parse(String path) {
String containingPath;
String elementName;
boolean endsWithAFilterOrIndex = false;

if (path.endsWith(")")) {
// This is probably a filter, so we're probably dealing with a list
endsWithAFilterOrIndex = true;
int filterArgsIndex = path.lastIndexOf('('); // Let's hope there aren't nested parentheses
int lastDotIndex = path.lastIndexOf(
'.',
filterArgsIndex); // There might be a dot inside the parentheses, so look to the left of that
int secondLastDotIndex = path.lastIndexOf('.', lastDotIndex - 1);
containingPath = path.substring(0, secondLastDotIndex);
elementName = path.substring(secondLastDotIndex + 1, lastDotIndex);
} else if (path.endsWith("]")) {
// This is almost definitely a list
endsWithAFilterOrIndex = true;
int openBracketIndex = path.lastIndexOf('[');
int lastDotIndex = path.lastIndexOf('.', openBracketIndex);
containingPath = path.substring(0, lastDotIndex);
elementName = path.substring(lastDotIndex + 1, openBracketIndex);
} else {
int lastDot = path.lastIndexOf(".");
containingPath = path.substring(0, lastDot);
elementName = path.substring(lastDot + 1);
}

return new ParsedPath(elementName, containingPath, endsWithAFilterOrIndex);
}
}
}

0 comments on commit 13b66d9

Please sign in to comment.