Skip to content

Harden $create-changelog and improve artifact-diff fidelity #1039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
c-schuler merged 2 commits into from
May 28, 2026
Merged

Harden $create-changelog and improve artifact-diff fidelity #1039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42cfcc0
Harden $create-changelog and improve artifact-diff fidelity
c-schuler May 28, 2026
7264ec5
Addressing minor sonarqube issue
c-schuler May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 148 additions & 21 deletions ...-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiArtifactDiffProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
});
}
Expand All @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -554,11 +566,42 @@ private static <T> 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();
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -685,7 +759,7 @@ private static void fixRelatedArtifactExtensionDiffPaths(
for (ParametersParameterComponent parameter : parameters) {
Optional<ParametersParameterComponent> 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();
Expand All @@ -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)
Expand Down Expand Up @@ -885,7 +1012,7 @@ private static void fixDeletePathIndexesAndAddValues(
ParametersParameterComponent parameter = parameters.get(i);
Optional<ParametersParameterComponent> path = parameter.getPart().stream()
.filter(ParametersParameterComponent::hasName)
.filter(part -> part.getName().equals("path"))
.filter(part -> "path".equals(part.getName()))
.findFirst();
if (path.isPresent()) {

Expand Down Expand Up @@ -947,13 +1074,13 @@ private static void fixInsertPathIndexes(List<ParametersParameterComponent> para
int opCounter = 0;
for (ParametersParameterComponent parameter : parameters) {
Optional<ParametersParameterComponent> index = parameter.getPart().stream()
.filter(part -> part.getName().equals("index"))
.filter(part -> "index".equals(part.getName()))
.findFirst();
Optional<ParametersParameterComponent> value = parameter.getPart().stream()
.filter(part -> part.getName().equals("value"))
.filter(part -> "value".equals(part.getName()))
.findFirst();
Optional<ParametersParameterComponent> 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();
Expand Down
8 changes: 4 additions & 4 deletions ...-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/common/HapiCreateChangelogProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -267,7 +267,7 @@ private String removeBase(EncodeContextPath path) {

private Optional<String> 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())
Expand All @@ -276,7 +276,7 @@ private Optional<String> getStringParameter(Parameters.ParametersParameterCompon

private Optional<IBase> 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();
Expand Down
Loading
Loading