From 42cfcc0620131ccc13f6dcd4770a1e154bed5be9 Mon Sep 17 00:00:00 2001 From: c-schuler Date: Thu, 28 May 2026 12:40:42 -0600 Subject: [PATCH 1/2] Harden $create-changelog and improve artifact-diff fidelity --- .../common/HapiArtifactDiffProcessor.java | 169 ++++++++++++-- .../common/HapiCreateChangelogProcessor.java | 8 +- .../common/HapiArtifactDiffProcessorTest.java | 218 ++++++++++++++++++ .../cqf/fhir/cr/crmi/changelog/Page.java | 22 +- .../cqf/fhir/cr/crmi/changelog/PageTest.java | 72 ++++++ 5 files changed, 460 insertions(+), 29 deletions(-) create mode 100644 cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/crmi/changelog/PageTest.java diff --git a/cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessor.java b/cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessor.java index 223e7b9d2..867a1253b 100644 --- a/cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessor.java +++ b/cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessor.java @@ -31,6 +31,7 @@ import org.hl7.fhir.r4.model.Parameters; import org.hl7.fhir.r4.model.Parameters.ParametersParameterComponent; import org.hl7.fhir.r4.model.PlanDefinition; +import org.hl7.fhir.r4.model.Reference; import org.hl7.fhir.r4.model.RelatedArtifact; import org.hl7.fhir.r4.model.StringType; import org.hl7.fhir.r4.model.Type; @@ -192,8 +193,14 @@ private static void handleSourceMatches( combinedReferenceList.getSourceMatches().get(i).getResource(); var targetCanonical = combinedReferenceList.getTargetMatches().get(i).getResource(); + // Identical version-pinned canonicals reference the same immutable artifact — there + // is nothing to diff, so skip the resolve/recurse entirely. This avoids both wasted + // fetches and spurious "could not be retrieved" entries for unchanged dependencies. + if (Objects.equals(sourceCanonical, targetCanonical)) { + continue; + } boolean diffNotAlreadyComputedAndPresent = - baseDiff.getParameter(Canonicals.getUrl(targetCanonical)) == null; + !hasParameterNamed(baseDiff, Canonicals.getUrl(targetCanonical)); if (diffNotAlreadyComputedAndPresent) { var source = checkOrUpdateResourceCache( sourceCanonical, cache, repository, true, ctx, terminologyEndpoint, compareExecutable); @@ -227,14 +234,19 @@ private static void handleSourceMatches( terminologyEndpoint); }, () -> { + // A side couldn't be resolved (commonly external code systems + // like LOINC/SNOMED that aren't stored as full resources). There + // is no sub-diff to add, and any reference-level change is already + // captured in the relatedArtifact operations — so log rather than + // emitting a diagnostic string into the diff payload. if (target == null) { - var component = baseDiff.addParameter(); - component.setName(Canonicals.getUrl(sourceCanonical)); - component.setValue(new StringType("Target could not be retrieved")); + logger.debug( + "Target resource could not be retrieved for {}; skipping sub-diff", + sourceCanonical); } else if (source == null) { - var component = baseDiff.addParameter(); - component.setName(Canonicals.getUrl(targetCanonical)); - component.setValue(new StringType("Source could not be retrieved")); + logger.debug( + "Source resource could not be retrieved for {}; skipping sub-diff", + targetCanonical); } }); } @@ -255,7 +267,7 @@ private static void handleInsertions( for (var addition : combinedReferenceList.getInsertions()) { if (addition.hasResource()) { boolean diffNotAlreadyComputedAndPresent = - baseDiff.getParameter(Canonicals.getUrl(addition.getResource())) == null; + !hasParameterNamed(baseDiff, Canonicals.getUrl(addition.getResource())); if (diffNotAlreadyComputedAndPresent) { var targetResource = checkOrUpdateResourceCache( addition.getResource(), @@ -297,7 +309,7 @@ private static void handleDeletions( for (var deletion : combinedReferenceList.getDeletions()) { if (deletion.hasResource()) { boolean diffNotAlreadyComputedAndPresent = - baseDiff.getParameter(Canonicals.getUrl(deletion.getResource())) == null; + !hasParameterNamed(baseDiff, Canonicals.getUrl(deletion.getResource())); if (diffNotAlreadyComputedAndPresent) { var sourceResource = checkOrUpdateResourceCache( deletion.getResource(), @@ -554,11 +566,42 @@ private static boolean compareSourceAndTarget(T targetObj, T sourceObj) { return valueSetContainsEquals(sourceVSECC, targetVSECC); } else if (sourceObj instanceof Extension sourceExt && targetObj instanceof Extension targetExt) { return extensionEquals(sourceExt, targetExt); + } else if (sourceObj instanceof ParametersParameterComponent sourcePpc + && targetObj instanceof ParametersParameterComponent targetPpc) { + return parametersParameterEquals(sourcePpc, targetPpc); } else { return false; } } + /** + * Identity match for expansion-parameter entries (contained Parameters.parameter[]). Two entries + * are the "same" entry when they share a name and — for canonical-valued params like + * system-version / canonicalVersion — the same version-stripped URL. This lets reordering produce + * no operations while a genuine version bump on the same system surfaces as a single replace. For + * non-canonical-valued params (booleans, codes, language tags) it falls back to deep equality. + */ + private static boolean parametersParameterEquals( + ParametersParameterComponent source, ParametersParameterComponent target) { + if (!Objects.equals(source.getName(), target.getName())) { + return false; + } + var sourceUrl = canonicalValueUrl(source); + var targetUrl = canonicalValueUrl(target); + if (sourceUrl != null && targetUrl != null) { + return sourceUrl.equals(targetUrl); + } + return source.equalsDeep(target); + } + + private static String canonicalValueUrl(ParametersParameterComponent p) { + if (p.getValue() instanceof IPrimitiveType primitive) { + var value = primitive.getValueAsString(); + return value == null ? null : Canonicals.getUrl(value); + } + return null; + } + private static boolean relatedArtifactEquals(RelatedArtifact ref1, RelatedArtifact ref2) { return Objects.equals(Canonicals.getUrl(ref1.getResource()), Canonicals.getUrl(ref2.getResource())) && ref1.getType() == ref2.getType(); @@ -590,23 +633,47 @@ private static boolean valueSetContainsEquals( return ref1.getSystem().equals(ref2.getSystem()) && ref1.getCode().equals(ref2.getCode()); } + /** + * Null-safe variant of {@link Parameters#getParameter(String)} membership check. HAPI's + * built-in iterates components and calls {@code component.getName().equals(name)} which NPEs + * if any component in the list has a null name — a state that occurs here whenever a malformed + * relatedArtifact URL passes through {@link Canonicals#getUrl(String)} and the resulting null + * is stored on a component (see e.g. {@code handleSourceMatches} fall-through paths). + */ + private static boolean hasParameterNamed(Parameters params, String name) { + if (params == null || name == null) { + return false; + } + return params.getParameter().stream() + .filter(ParametersParameterComponent::hasName) + .anyMatch(p -> name.equals(p.getName())); + } + private static boolean extensionEquals(Extension source, Extension target) { - if (!source.getValue().getClass().equals(target.getValue().getClass())) { + var sourceValue = source.getValue(); + var targetValue = target.getValue(); + // Complex extensions (e.g. package-source) carry no top-level value — only + // nested sub-extensions. Fall back to deep equality (which compares URLs + + // nested extensions) rather than dereferencing a null value. + if (sourceValue == null || targetValue == null) { + return sourceValue == null && targetValue == null && source.equalsDeep(target); + } + if (!sourceValue.getClass().equals(targetValue.getClass())) { return false; } - if (source.getValue() instanceof IPrimitiveType sourcePrimitive) { - return (sourcePrimitive.getValue()).equals((((IPrimitiveType) target.getValue()).getValue())); - } else if (source.getValue() instanceof CodeableConcept sourceCodableConcept) { + if (sourceValue instanceof IPrimitiveType sourcePrimitive) { + return (sourcePrimitive.getValue()).equals(((IPrimitiveType) targetValue).getValue()); + } else if (sourceValue instanceof CodeableConcept sourceCodableConcept) { return sourceCodableConcept .getCodingFirstRep() .getSystem() - .equals(((CodeableConcept) target.getValue()) + .equals(((CodeableConcept) targetValue) .getCodingFirstRep() .getSystem()) - && ((CodeableConcept) source.getValue()) + && sourceCodableConcept .getCodingFirstRep() .getCode() - .equals(((CodeableConcept) target.getValue()) + .equals(((CodeableConcept) targetValue) .getCodingFirstRep() .getCode()); } else { @@ -651,6 +718,13 @@ private static Parameters diffWithExtensions( updateSource.setRelatedArtifact(processedRelatedArtifacts.getSourceMatches()); updateTarget.setRelatedArtifact(processedRelatedArtifacts.getTargetMatches()); + // Align order-independent contained expansion parameters before the positional diff, so a + // reordered-but-unchanged parameter doesn't surface as a spurious replace. Matched entries + // end up at the same indices on both sides; source-only entries (deletions) and target-only + // entries (insertions) are appended last. + reorderContainedExpansionParameters( + (MetadataResource) updateSource.get(), (MetadataResource) updateTarget.get()); + // get the diff of the RelatedArtifacts without Extensions var updateOperations = (Parameters) patch.diff( removeRelatedArtifactExtensions(updateSource), removeRelatedArtifactExtensions(updateTarget)); @@ -685,7 +759,7 @@ private static void fixRelatedArtifactExtensionDiffPaths( for (ParametersParameterComponent parameter : parameters) { Optional path = parameter.getPart().stream() .filter(ParametersParameterComponent::hasName) - .filter(part -> part.getName().equals("path")) + .filter(part -> "path".equals(part.getName())) .findFirst(); if (path.isPresent()) { var pathString = ((StringType) path.get().getValue()).getValue(); @@ -696,6 +770,59 @@ private static void fixRelatedArtifactExtensionDiffPaths( } } + /** + * Reorders the contained expansion-parameters ({@code Parameters.parameter[]}) on both sides so + * matched entries occupy the same indices before the positional {@code patch.diff}. Without this, + * a manifest whose expansion parameters are merely reordered yields a cascade of meaningless + * replace operations pairing unrelated entries by position. Mutates the working copies in place, + * consistent with the existing relatedArtifact reordering. + */ + private static void reorderContainedExpansionParameters( + MetadataResource sourceLibrary, MetadataResource targetLibrary) { + var sourceParams = getContainedExpansionParameters(sourceLibrary); + var targetParams = getContainedExpansionParameters(targetLibrary); + if (sourceParams == null || targetParams == null) { + return; + } + var processed = extractAdditionsAndDeletions( + sourceParams.getParameter(), targetParams.getParameter(), ParametersParameterComponent.class); + sourceParams.setParameter( + new ArrayList<>(Stream.concat(processed.getSourceMatches().stream(), processed.getDeletions().stream()) + .toList())); + targetParams.setParameter( + new ArrayList<>(Stream.concat(processed.getTargetMatches().stream(), processed.getInsertions().stream()) + .toList())); + } + + /** + * Resolves the contained Parameters carrying expansion parameters. Prefers the contained resource + * referenced by the {@code cqf-expansionParameters} extension; falls back to the first contained + * Parameters. Returns null when there is none. + */ + private static Parameters getContainedExpansionParameters(MetadataResource resource) { + if (resource == null) { + return null; + } + var containedParams = resource.getContained().stream() + .filter(Parameters.class::isInstance) + .map(Parameters.class::cast) + .toList(); + if (containedParams.isEmpty()) { + return null; + } + var ext = resource.getExtensionByUrl(Constants.CQF_EXPANSION_PARAMETERS); + if (ext != null && ext.getValue() instanceof Reference reference && reference.getReference() != null) { + var localId = reference.getReference().replace("#", ""); + var match = containedParams.stream() + .filter(p -> localId.equals(p.getIdPart())) + .findFirst(); + if (match.isPresent()) { + return match.get(); + } + } + return containedParams.get(0); + } + private static IDomainResource removeRelatedArtifactExtensions(IKnowledgeArtifactAdapter resource) { var retval = resource.copy(); IAdapterFactory.forFhirVersion(FhirVersionEnum.R4) @@ -885,7 +1012,7 @@ private static void fixDeletePathIndexesAndAddValues( ParametersParameterComponent parameter = parameters.get(i); Optional path = parameter.getPart().stream() .filter(ParametersParameterComponent::hasName) - .filter(part -> part.getName().equals("path")) + .filter(part -> "path".equals(part.getName())) .findFirst(); if (path.isPresent()) { @@ -947,13 +1074,13 @@ private static void fixInsertPathIndexes(List para int opCounter = 0; for (ParametersParameterComponent parameter : parameters) { Optional index = parameter.getPart().stream() - .filter(part -> part.getName().equals("index")) + .filter(part -> "index".equals(part.getName())) .findFirst(); Optional value = parameter.getPart().stream() - .filter(part -> part.getName().equals("value")) + .filter(part -> "value".equals(part.getName())) .findFirst(); Optional path = parameter.getPart().stream() - .filter(part -> part.getName().equals("path")) + .filter(part -> "path".equals(part.getName())) .findFirst(); if (path.isPresent()) { var pathString = ((StringType) path.get().getValue()).getValue(); diff --git a/cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiCreateChangelogProcessor.java b/cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiCreateChangelogProcessor.java index f732c2282..8a0a98275 100644 --- a/cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiCreateChangelogProcessor.java +++ b/cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiCreateChangelogProcessor.java @@ -222,12 +222,12 @@ private void processChange( MetadataResource sourceResource, Page page) { if (change.hasName() - && !change.getName().equals("operation") + && !"operation".equals(change.getName()) && change.hasResource() && change.getResource() instanceof Parameters parameters) { // Nested Parameters objects get recursively processed processChanges(parameters.getParameter(), changelog, cache, change.getName()); - } else if (change.getName().equals("operation")) { + } else if ("operation".equals(change.getName())) { // 1) For each operation get the relevant parameters var type = getStringParameter(change, "type") .orElseThrow(() -> new UnprocessableEntityException( @@ -267,7 +267,7 @@ private String removeBase(EncodeContextPath path) { private Optional getStringParameter(Parameters.ParametersParameterComponent part, String name) { return part.getPart().stream() - .filter(p -> p.getName().equalsIgnoreCase(name)) + .filter(p -> name.equalsIgnoreCase(p.getName())) .filter(p -> p.getValue() instanceof IPrimitiveType) .map(p -> (IPrimitiveType) p.getValue()) .map(s -> (String) s.getValue()) @@ -276,7 +276,7 @@ private Optional getStringParameter(Parameters.ParametersParameterCompon private Optional getParameter(Parameters.ParametersParameterComponent part, String name) { return part.getPart().stream() - .filter(p -> p.getName().equalsIgnoreCase(name)) + .filter(p -> name.equalsIgnoreCase(p.getName())) .filter(ParametersParameterComponent::hasValue) .map(p -> (IBase) p.getValue()) .findAny(); diff --git a/cqf-fhir-cr-hapi/src/test/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java b/cqf-fhir-cr-hapi/src/test/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java index 01f81a1f7..f7346e058 100644 --- a/cqf-fhir-cr-hapi/src/test/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java +++ b/cqf-fhir-cr-hapi/src/test/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java @@ -1,6 +1,8 @@ package org.opencds.cqf.fhir.cr.hapi.common; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.util.ClasspathUtil; @@ -9,6 +11,7 @@ import java.util.stream.Collectors; import org.hl7.fhir.r4.model.Bundle; import org.hl7.fhir.r4.model.CodeType; +import org.hl7.fhir.r4.model.Extension; import org.hl7.fhir.r4.model.Library; import org.hl7.fhir.r4.model.Parameters; import org.hl7.fhir.r4.model.StringType; @@ -144,4 +147,219 @@ private List getOperationsByType( .equals(type))) .collect(Collectors.toList()); } + + /** + * Regression: complex extensions (e.g. the {@code package-source} extension emitted by + * {@code $data-requirements}) have no top-level value — only nested sub-extensions. The + * earlier {@code extensionEquals} implementation dereferenced {@code extension.getValue()} + * directly and NPE'd on these, blowing up {@code $create-changelog}. The fix falls back to + * {@link Extension#equalsDeep} when either side has no top-level value. + */ + @Test + void extensionEquals_complexExtensions_withNoTopLevelValue() throws Exception { + var method = + HapiArtifactDiffProcessor.class.getDeclaredMethod("extensionEquals", Extension.class, Extension.class); + method.setAccessible(true); + + var sourceComplex = buildPackageSourceExtension("hl7.fhir.us.core", "6.1.0"); + var targetComplex = buildPackageSourceExtension("hl7.fhir.us.core", "6.1.0"); + + assertTrue( + (boolean) method.invoke(null, sourceComplex, targetComplex), + "two structurally identical complex extensions should be equal"); + + var differingComplex = buildPackageSourceExtension("hl7.fhir.us.core", "7.0.0"); + assertFalse( + (boolean) method.invoke(null, sourceComplex, differingComplex), + "complex extensions differing in a nested value should not be equal"); + + var simpleExtension = + new Extension("http://example.org/StructureDefinition/simple", new StringType("anything")); + assertFalse( + (boolean) method.invoke(null, sourceComplex, simpleExtension), + "complex extension and value-bearing extension should not be equal"); + assertFalse( + (boolean) method.invoke(null, simpleExtension, sourceComplex), + "value-bearing extension and complex extension should not be equal"); + } + + /** + * Regression: HAPI's {@code Parameters.getParameter(String)} NPEs when iterating a list that + * contains a component with a null name. That state arises in this processor whenever a + * malformed relatedArtifact resource URL parses to {@code null} via {@code Canonicals.getUrl} + * and is stored back on a component, breaking subsequent lookups by name. The null-safe + * {@code hasParameterNamed} helper guards both null names in the list and a null search key. + */ + @Test + void hasParameterNamed_isNullSafe() throws Exception { + var method = + HapiArtifactDiffProcessor.class.getDeclaredMethod("hasParameterNamed", Parameters.class, String.class); + method.setAccessible(true); + + var params = new Parameters(); + params.addParameter().setName("http://example.org/foo"); // named + params.addParameter().setValue(new StringType("orphan")); // no name set + + assertTrue((boolean) method.invoke(null, params, "http://example.org/foo")); + assertFalse((boolean) method.invoke(null, params, "http://example.org/bar")); + assertFalse((boolean) method.invoke(null, params, (String) null), "null search key should never match"); + assertFalse( + (boolean) method.invoke(null, (Parameters) null, "anything"), + "null parameters collection should not throw"); + } + + private static Extension buildPackageSourceExtension(String packageId, String version) { + var ext = new Extension("http://hl7.org/fhir/StructureDefinition/package-source"); + ext.addExtension("packageId", new StringType(packageId)); + ext.addExtension("version", new StringType(version)); + return ext; + } + + /** + * Regression: contained expansion parameters (Library.contained[0].parameter[]) are an + * order-independent set, but FhirPatch.diff compares arrays positionally. A manifest whose + * expansion params are merely reordered previously produced a cascade of meaningless replace + * operations pairing unrelated entries by index. With set-based matching + reordering, a pure + * reorder yields no parameter-path operations. + */ + @Test + void artifactDiff_reorderedExpansionParams_produceNoOps() { + var processor = new HapiArtifactDiffProcessor(new InMemoryFhirRepository(FhirContext.forR4())); + var source = manifestWithExpansionParams("1.0.0", "http://loinc.org|2.81", "http://snomed.info/sct|2024"); + var target = manifestWithExpansionParams("1.0.0", "http://snomed.info/sct|2024", "http://loinc.org|2.81"); + + var diff = (Parameters) processor.getArtifactDiff(source, target, true, true, null, null); + + assertTrue( + operationsWithPathContaining(diff, "parameter").isEmpty(), + "reordered-but-unchanged expansion params should produce no operations"); + } + + @Test + void artifactDiff_versionBumpOnSameSystem_producesSingleReplace() { + var processor = new HapiArtifactDiffProcessor(new InMemoryFhirRepository(FhirContext.forR4())); + var source = manifestWithExpansionParams("1.0.0", "http://loinc.org|2.81"); + var target = manifestWithExpansionParams("1.0.0", "http://loinc.org|2.82"); + + var diff = (Parameters) processor.getArtifactDiff(source, target, true, true, null, null); + + var paramOps = operationsWithPathContaining(diff, "parameter"); + assertEquals(1, paramOps.size(), "a same-system version bump should be a single replace"); + assertEquals( + "replace", + ((CodeType) paramOps.get(0).getPart().stream() + .filter(p -> "type".equals(p.getName())) + .findFirst() + .orElseThrow() + .getValue()) + .getCode()); + } + + @Test + void artifactDiff_addedExpansionParam_producesInsert() { + var processor = new HapiArtifactDiffProcessor(new InMemoryFhirRepository(FhirContext.forR4())); + var source = manifestWithExpansionParams("1.0.0", "http://loinc.org|2.81"); + var target = manifestWithExpansionParams("1.0.0", "http://loinc.org|2.81", "http://snomed.info/sct|2024"); + + var diff = (Parameters) processor.getArtifactDiff(source, target, true, true, null, null); + + var paramOps = operationsWithPathContaining(diff, "parameter"); + assertFalse(paramOps.isEmpty(), "an added expansion param should produce at least one operation"); + assertTrue( + paramOps.stream() + .anyMatch(op -> op.getPart().stream() + .anyMatch(p -> "type".equals(p.getName()) + && p.getValue() instanceof CodeType ct + && "insert".equals(ct.getCode()))), + "the added param should surface as an insert"); + } + + /** + * An unchanged dependency (identical version-pinned canonical on both sides) is the same + * immutable artifact — the recursion short-circuits, so there is no sub-diff, no relatedArtifact + * operation, and (since the placeholder was removed) no retrieval-failure entry. + */ + @Test + void artifactDiff_unchangedDependency_producesNoEntries() { + var processor = new HapiArtifactDiffProcessor(new InMemoryFhirRepository(FhirContext.forR4())); + var ig = "http://hl7.org/fhir/us/core/ImplementationGuide/hl7.fhir.us.core|6.1.0"; + var source = manifestWithComposedOf("1.0.0", ig); + var target = manifestWithComposedOf("1.0.0", ig); + + var diff = (Parameters) processor.getArtifactDiff(source, target, true, true, null, null); + + assertFalse(hasRetrievalFailureEntry(diff), "unchanged dependency should not emit a retrieval-failure entry"); + assertTrue( + operationsWithPathContaining(diff, "relatedArtifact").isEmpty(), + "unchanged dependency should produce no relatedArtifact operation"); + } + + /** + * A dependency whose version changed (e.g. a pin removed by the data-requirements stub fix) is + * reported once, at the reference level, as a relatedArtifact replace. The recursive sub-diff + * can't resolve the (external) resource, but that no longer pollutes the payload with a + * placeholder — the change is carried solely by the relatedArtifact operation. + */ + @Test + void artifactDiff_versionChangedDependency_reportedAtReferenceLevelOnly() { + var processor = new HapiArtifactDiffProcessor(new InMemoryFhirRepository(FhirContext.forR4())); + var source = manifestWithComposedOf("1.0.0", "http://loinc.org|2.82"); + var target = manifestWithComposedOf("1.0.0", "http://loinc.org"); + + var diff = (Parameters) processor.getArtifactDiff(source, target, true, true, null, null); + + assertFalse( + hasRetrievalFailureEntry(diff), + "unresolvable external code system should not emit a retrieval-failure entry"); + assertFalse( + operationsWithPathContaining(diff, "relatedArtifact").isEmpty(), + "the version change should still be reported as a relatedArtifact operation"); + } + + private static boolean hasRetrievalFailureEntry(Parameters diff) { + return diff.getParameter().stream() + .anyMatch(p -> p.getValue() instanceof StringType st + && st.getValue() != null + && st.getValue().contains("could not be retrieved")); + } + + private static Library manifestWithComposedOf(String version, String composedOfCanonical) { + var lib = new Library(); + lib.setId("Library/test-manifest"); + lib.setUrl("http://example.org/Library/test-manifest"); + lib.setVersion(version); + lib.addRelatedArtifact() + .setType(org.hl7.fhir.r4.model.RelatedArtifact.RelatedArtifactType.COMPOSEDOF) + .setResource(composedOfCanonical); + return lib; + } + + private static List operationsWithPathContaining( + Parameters diff, String pathFragment) { + return diff.getParameter().stream() + .filter(p -> "operation".equals(p.getName())) + .filter(op -> op.getPart().stream() + .anyMatch(part -> "path".equals(part.getName()) + && part.getValue() instanceof StringType st + && st.getValue() != null + && st.getValue().contains(pathFragment))) + .collect(Collectors.toList()); + } + + private static Library manifestWithExpansionParams(String version, String... systemVersions) { + var lib = new Library(); + lib.setId("Library/test-manifest"); + lib.setUrl("http://example.org/Library/test-manifest"); + lib.setVersion(version); + var params = new Parameters(); + params.setId("expansion-parameters"); + for (var sv : systemVersions) { + params.addParameter("system-version", new org.hl7.fhir.r4.model.UriType(sv)); + } + lib.addContained(params); + lib.addExtension( + "http://hl7.org/fhir/StructureDefinition/cqf-expansionParameters", + new org.hl7.fhir.r4.model.Reference("#expansion-parameters")); + return lib; + } } diff --git a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/crmi/changelog/Page.java b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/crmi/changelog/Page.java index 6525d9936..4d074b122 100644 --- a/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/crmi/changelog/Page.java +++ b/cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/crmi/changelog/Page.java @@ -63,21 +63,35 @@ void addInsertOperation(String type, String path, Object currentValue) { if (!type.equals(ChangeLog.INSERT)) { throw new UnprocessableEntityException(ChangeLog.WRONG_TYPE); } - this.newData.addOperation(type, path, currentValue, null); + // newData can be null when the page represents a deleted-only resource (source had it, + // target doesn't). An insert against the missing side has no meaningful target. + if (this.newData != null) { + this.newData.addOperation(type, path, currentValue, null); + } } void addDeleteOperation(String type, String path, Object originalValue) { if (!type.equals(ChangeLog.DELETE)) { throw new UnprocessableEntityException(ChangeLog.WRONG_TYPE); } - this.oldData.addOperation(type, path, null, originalValue); + // oldData can be null when the page represents an inserted-only resource (target has it, + // source didn't). A delete against the missing side has no meaningful source. + if (this.oldData != null) { + this.oldData.addOperation(type, path, null, originalValue); + } } void addReplaceOperation(String type, String path, Object currentValue, Object originalValue) { if (!type.equals(ChangeLog.REPLACE)) { throw new UnprocessableEntityException(ChangeLog.WRONG_TYPE); } - this.oldData.addOperation(type, path, currentValue, null); - this.newData.addOperation(type, path, null, originalValue); + // Either side may be null when the page represents a resource that exists on only one + // side of the diff (insert-only or delete-only). Record what we have rather than NPE. + if (this.oldData != null) { + this.oldData.addOperation(type, path, currentValue, null); + } + if (this.newData != null) { + this.newData.addOperation(type, path, null, originalValue); + } } } diff --git a/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/crmi/changelog/PageTest.java b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/crmi/changelog/PageTest.java new file mode 100644 index 000000000..8248d985c --- /dev/null +++ b/cqf-fhir-cr/src/test/java/org/opencds/cqf/fhir/cr/crmi/changelog/PageTest.java @@ -0,0 +1,72 @@ +package org.opencds.cqf.fhir.cr.crmi.changelog; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import org.junit.jupiter.api.Test; + +class PageTest { + + private static PageBase newPageBase() { + return new PageBase("title", "id", "1.0.0", "name", "http://example.org/X", "Library"); + } + + /** + * Regression: pages created for resources that exist on only one side of a manifest diff + * (inserted-only or deleted-only) legitimately have a null oldData or newData. Prior to + * the null-guard, any REPLACE / INSERT / DELETE op routed against the missing side would + * NPE inside {@code PageBase.addOperation} and abort {@code $create-changelog}. + */ + @Test + void addReplaceOperation_withNullOldData_recordsOnNewDataOnly() { + var newData = newPageBase(); + var page = new Page<>("http://example.org/X|2.0.0", null, newData); + + assertDoesNotThrow(() -> page.addOperation(ChangeLog.REPLACE, "url", "newUrl", "oldUrl")); + + // newData side recorded the operation; oldData side was null so nothing to record. + assertEquals(ChangeLog.REPLACE, newData.getUrl().getOperation().getType()); + } + + @Test + void addReplaceOperation_withNullNewData_recordsOnOldDataOnly() { + var oldData = newPageBase(); + var page = new Page<>("http://example.org/X|1.0.0", oldData, null); + + assertDoesNotThrow(() -> page.addOperation(ChangeLog.REPLACE, "url", "newUrl", "oldUrl")); + + assertEquals(ChangeLog.REPLACE, oldData.getUrl().getOperation().getType()); + } + + @Test + void addInsertOperation_withNullNewData_isNoOp() { + var oldData = newPageBase(); + var page = new Page<>("http://example.org/X|1.0.0", oldData, null); + + assertDoesNotThrow(() -> page.addOperation(ChangeLog.INSERT, "version", "9.9.9", null)); + + // oldData isn't touched by an insert; nothing should have changed on either side. + assertNull(oldData.getVersion().getOperation()); + } + + @Test + void addDeleteOperation_withNullOldData_isNoOp() { + var newData = newPageBase(); + var page = new Page<>("http://example.org/X|2.0.0", null, newData); + + assertDoesNotThrow(() -> page.addOperation(ChangeLog.DELETE, "version", null, "1.0.0")); + + // newData isn't touched by a delete; nothing should have changed on either side. + assertNull(newData.getVersion().getOperation()); + } + + @Test + void addOperation_unknownTypeStillThrows() { + var page = new Page<>("http://example.org/X", newPageBase(), newPageBase()); + + assertThrows(UnprocessableEntityException.class, () -> page.addOperation("not-a-real-type", "url", "a", "b")); + } +} From 7264ec58612510106ca5065f5161d203a7741b5d Mon Sep 17 00:00:00 2001 From: c-schuler Date: Thu, 28 May 2026 15:16:19 -0600 Subject: [PATCH 2/2] Addressing minor sonarqube issue --- .../cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cqf-fhir-cr-hapi/src/test/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java b/cqf-fhir-cr-hapi/src/test/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java index f7346e058..956d5ba51 100644 --- a/cqf-fhir-cr-hapi/src/test/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java +++ b/cqf-fhir-cr-hapi/src/test/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessorTest.java @@ -343,7 +343,7 @@ private static List operationsWithPathC && part.getValue() instanceof StringType st && st.getValue() != null && st.getValue().contains(pathFragment))) - .collect(Collectors.toList()); + .toList(); } private static Library manifestWithExpansionParams(String version, String... systemVersions) {