fix(mergedeep): preserve array shape when comparing empty targets#1030
Open
tdabasinskas wants to merge 2 commits into
Open
Conversation
When `compareDeep` encounters an empty target and a source array, it now returns additions as an array rather than an index-keyed object. This ensures consistent behavior whether the target is empty or contains existing entries.
Previously, comparing `[]` with `[item]` would produce `additions: { 0: item }`, while comparing `[existing]` with `[existing, item]` would correctly produce `additions: [item]`. This inconsistency caused issues when applying diffs to repositories receiving their first array-based configuration.
Also ensures the `deletions` property is always included in the result object for consistency.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes MergeDeep.compareDeep so diffs for top-level array configs (e.g. rulesets) are consistent when comparing against an empty target, and ensures the compare result always includes deletions—improving the stability of plan/diff JSON consumed by dry-run reports and PR comments.
Changes:
- Adjusts the empty-target early return in
compareDeepto preserve top-level array diff shape foradditions, and to always returndeletions. - Adds unit tests covering top-level array shape consistency and presence of
deletionsin the result.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/mergeDeep.js | Fixes empty-target diff behavior to preserve top-level array shape and include deletions. |
| test/unit/lib/mergeDeep.test.js | Adds regression tests for consistent top-level array diff shape and inclusion of deletions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When comparing an empty target object against a source, the code now explicitly copies keys instead of using Object.assign. This prevents prototype pollution vectors like `__proto__` and `constructor` from being copied into the additions object. The fix mirrors the existing protection in the comparison loop and ensures that malicious keys parsed from JSON cannot pollute the prototype chain through the additions reference.
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.
MergeDeep.compareDeepreports the same kind of change in two different JSON shapes depending on whether the target is empty. Comparing rulesets (any top-level array config works the same):Two details combine to cause this. On first invocation, top-level arrays are wrapped into
{ __array: [...] }before the accumulator-type check runs, soArray.isArray(source)is always false andadditionsinitializes as{}(the array branch of that check is effectively dead code). The non-empty-target path recovers because the__arraykey is processed as a nested array and unwound before returning. But the empty-target early return doesObject.assign(additions, s)— spreading an array into a plain object produces the{ "0": ..., "1": ... }shape — and returns without the unwind. That early return also omitsdeletionsfrom its result, unlike every other return path.This hits anything consuming plan/diff JSON — dry-run reports, PR comments, or tooling parsing the NopCommand actions. A repo receiving its first ruleset reports index-keyed attribute soup instead of one added item, while the second ruleset onward reports a clean array. (Possibly related to the inconsistent-diff reports in #693).