feat(calm-models): add implementation for metadata diff - #3014
feat(calm-models): add implementation for metadata diff#3014aamanrebello wants to merge 5 commits into
Conversation
rocketstack-matt
left a comment
rocketstack-matt
left a comment
There was a problem hiding this comment.
Three real defects in the metadata diff logic — details inline, each with a suggested fix.
| function stringifyMetadata(metadata: CalmMetadataSchema): string[] { | ||
| if (Array.isArray(metadata)) { | ||
| return metadata.map((obj: Record<string, unknown>) => JSON.stringify(obj)); | ||
| } else { | ||
| return [JSON.stringify(metadata)]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Array metadata items are matched by raw JSON.stringify, so two objects with identical content but reordered keys are reported as removed+added instead of unchanged. Every other equality check in this file (valuesEqual/normalizeValue/canonicalKey, and diffControlItem's diffArrays(..., { comparator: valuesEqual })) normalizes key order first — this should too.
| function stringifyMetadata(metadata: CalmMetadataSchema): string[] { | |
| if (Array.isArray(metadata)) { | |
| return metadata.map((obj: Record<string, unknown>) => JSON.stringify(obj)); | |
| } else { | |
| return [JSON.stringify(metadata)]; | |
| } | |
| } | |
| function stringifyMetadata(metadata: CalmMetadataSchema): string[] { | |
| if (Array.isArray(metadata)) { | |
| return metadata.map((obj: Record<string, unknown>) => JSON.stringify(normalizeValue(obj))); | |
| } else { | |
| return [JSON.stringify(normalizeValue(metadata))]; | |
| } | |
| } |
| if (valuesEqual(metadataA, metadataB)) { | ||
| result.metadataObjectsUnchanged.push(metadataA); | ||
| } else { |
There was a problem hiding this comment.
archA.metadata ?? {} at the call site means two architectures with no metadata at all both resolve to {}, and valuesEqual({}, {}) is true, so every diff between metadata-less architectures reports a phantom metadataObjectsUnchanged: [{}] instead of [] (unlike the no-controls/no-ADRs case, which reports empty results). Skipping the push when the object is empty avoids the phantom entry without needing to distinguish "caller passed {}" from "caller had no metadata" at the call site.
| if (valuesEqual(metadataA, metadataB)) { | |
| result.metadataObjectsUnchanged.push(metadataA); | |
| } else { | |
| if (valuesEqual(metadataA, metadataB)) { | |
| if (Object.keys(metadataA).length > 0) { | |
| result.metadataObjectsUnchanged.push(metadataA); | |
| } | |
| } else { |
| const result: MetadataItemDiffResult = {}; | ||
|
|
||
| Object.keys(metadataObjA).forEach((key: string) => { | ||
| if (!(key in metadataObjB)) { |
There was a problem hiding this comment.
in walks the prototype chain, not just own properties. Since CALM metadata is arbitrary key/value data, a metadata object using a key like toString or constructor will appear "present" on the other side (inherited from Object.prototype) even when actually absent, misclassifying a removal as a modification with a non-serializable Function as the value.
| if (!(key in metadataObjB)) { | |
| if (!Object.hasOwn(metadataObjB, key)) { |
| }); | ||
|
|
||
| Object.keys(metadataObjB).forEach((key: string) => { | ||
| if (!(key in metadataObjA)) { |
There was a problem hiding this comment.
Same prototype-chain issue as above, other direction.
| if (!(key in metadataObjA)) { | |
| if (!Object.hasOwn(metadataObjA, key)) { |
2 participants
Description
Same as #2732 but for metadata. Only flows left to go after this!
Type of Change
Affected Components
cli/)calm/)calm-ai/)calm-hub/)calm-hub-ui/)calm-server/)calm-widgets/)docs/)shared/)calm-plugins/vscode/)Commit Message Format ✅
Testing
Checklist