Harden $create-changelog and improve artifact-diff fidelity#1039
Merged
Conversation
|
Formatting check succeeded! |
Chris0296
approved these changes
May 28, 2026
Collaborator
Chris0296
left a comment
There was a problem hiding this comment.
Great improvements - nice work Chris!
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
Makes the
$create-changelogoperation (and the shared$artifact-diffengine it builds on) robust against real-world manifests and produces a cleaner, less noisy diff. Two threads of work: a series of null-safety fixes that were each blocking the operation on progressively more complex inputs, and diff-quality improvements that remove spurious and redundant entries from the result.Robustness (NPE fixes)
Each surfaced after the prior was unblocked, all on the diff/changelog path:
HapiArtifactDiffProcessor.extensionEquals- dereferencedextension.getValue()unconditionally, NPE-ing on complex extensions that carry only nested sub-extensions and no top-level value (e.g. thepackage-sourceextension emitted by$data-requirements). Now null-guards both sides and falls back toExtension.equalsDeep.Page.addInsertOperation/addDeleteOperation/addReplaceOperation- dereferencedoldData/newData, which are legitimately null for pages representing a resource that exists on only one side of the diff (insert-only or delete-only). Now each side is guarded independently.Null parameter names - HAPI's
Parameters.getParameter(String)NPEs when the list contains a component with a null name (which arises when a malformed canonical parses to null and is stored back on a component). Added a null-safehasParameterNamedhelper and switched the remaining.getName().equals(...)/.equalsIgnoreCase(...)call sites in both processors to literal-first form.Diff fidelity
Set-match contained expansion parameters - the contained
Parameters.parameter[]array is order-independent, butFhirPatch.diffcompares arrays positionally, so a reordered manifest produced a cascade of meaninglessreplaceops pairing unrelated entries by index. Extended the existing match-and-reorder machinery (already used forrelatedArtifact,compose.include,expansion.contains) to expansion parameters, matching by name + version-stripped URL. A pure reorder now produces no ops; a genuine version bump produces a single replace; add/remove produce clean insert/delete.Skip unchanged dependencies -
handleSourceMatchesrecursed into and re-fetched every matched dependency even when the source and target referenced the identical version-pinned canonical (the same immutable artifact). Now short-circuits when the full canonicals are equal — no wasted fetch, no spurious sub-diff entry.Suppress futile retrieval-failure placeholders - when a recursive sub-diff couldn't resolve a side (commonly external code systems like LOINC/SNOMED/ICD/CPT that are never stored as full resources), the processor baked a
"Target/Source could not be retrieved"string into the diff payload. These were both futile (external systems can't be content-diffed) and redundant (any reference-level change is already reported as arelatedArtifactoperation). Now logged at debug instead of polluting the output.