fix: picklist values corrupted in CustomField merge#175
Merged
Conversation
… container
ObjectMergeNode iterated child properties and re-wrapped each child's
output with `{ [key]: item }`, but child nodes (e.g. TextMergeNode)
already include the property key. This caused double-wrapping.
Additionally, ObjectMergeNode never wrapped its combined output with
its own attribute name, losing the container (e.g. `valueSet`).
- Add `attribute` parameter to ObjectMergeNode constructor
- Push child results directly without re-wrapping
- Wrap combined output with `{ [this.attribute]: ... }`
- Add optional chaining for safety on ancestor/local/other access
- Pass attribute from MergeNodeFactory to ObjectMergeNode
Fixes #174
…ement OrderedKeyedArrayMergeStrategy.wrapElement() and wrapKeys() wrapped raw parsed objects directly, but fast-xml-parser XMLBuilder with preserveOrder:true requires builder format. This produced empty `<value></value>` elements in the XML output. Use toJsonArray() to convert raw objects to builder format before wrapping, consistent with how the unordered strategy handles output. Fixes #174
This is an integration test exercising the full XmlMerger pipeline, not a unit test.
- @biomejs/biome 2.4.0 → 2.4.3 - @commitlint/config-conventional 20.4.1 → 20.4.2 - @salesforce/core 8.26.0 → 8.26.2 - fast-xml-parser 5.3.6 → 5.3.7 - knip 5.83.1 → 5.84.1
yohanim
approved these changes
Feb 20, 2026
Collaborator
yohanim
left a comment
yohanim
left a comment
There was a problem hiding this comment.
If it solves the issue with all other tests ok it's fine by me stuck in travel.
We'll discuss it at a later time.
Owner
Author
Yes, it is just a fix at the marshalling level, zero change to the algo |
Owner
Author
Contributor
|
Can't test, it's one of my Cloudity colleagues that reported me the bug ^^ |
…ergeNode Avoid naming collision with the MergeContext type from ../MergeContext.ts. The local interface represents ordered merge state, not the orchestrator context.
Replace manual wrapping with the existing wrapWithRootKey utility. This deduplicates the wrapping logic and properly handles the empty output case (returns noConflict instead of raw empty array).
Each merge() returns output already wrapped with its attribute key. This implicit contract was the root cause of the double-wrapping bug.
ObjectMergeNode merges pure objects property-by-property and wraps with attribute. NestedObjectMergeNode delegates to MergeOrchestrator with full scenario detection.
…ration test Parse the merge output with XMLParser and assert on the DOM tree instead of fragile string patterns. Consolidates 5 tests into 3 focused assertions: conflict-free merge, correct tree structure, no leaked children.
When all children resolve to nothing during merge, return empty output instead of producing an empty container element like <valueSet></valueSet>.
…bjectMergeNode PropertyMergeNode better conveys it merges objects property-by-property. NestedObjectMergeNode was unused in production code (only tests) and not part of the public API, so it is removed entirely.
@biomejs/biome 2.4.3 → 2.4.4
Owner
Author
@nvuillam if your colleagues could test before we merge it is better, else if we all agree it should fix properly then let's merge and see, we'll fix further if needed |
…ry classes DefaultMergeNodeFactory and 8 TextMergeStrategy classes were exported but never imported by production code. Tests now use the public API (getTextMergeStrategy, defaultNodeFactory) instead of importing internals.
Introduce TextMergeParams for TextMergeStrategy.handle() (7 positional params to 1 object) and GapKeys for KeyedArrayMergeNode gap methods (5-6 params to 3-4 params), following the existing MergeContext pattern.
scolladon
force-pushed
the
fix/picklist-values-corrupted
branch
from
458aa74 to
e4e5625
Compare
Remove duplicated toJsonArray + rootKey wrapping logic.
Replace duplicated output.length > 0 || hasConflict guard in KeyedArrayMergeNode with shared isNonEmpty helper.
Rename installService.ts and uninstallService.ts to match the PascalCase convention used by all other source files.
Move hasSameOrder and lcs to arrayUtils, setsEqual and setsIntersect to new setUtils, filterEmptyTextNodes and buildKeyedMap to nodeUtils. Add unit tests for all.
Reduces KeyedArrayMergeNode.ts from 737 to 117 lines by moving the 523-line OrderedKeyedArrayMergeStrategy class and its supporting types to a dedicated file. Exports KeyExtractor from nodeUtils.ts to share the type definition.
Merges 10 strategy files + factory into ScenarioStrategy.ts following the TextMergeStrategy pattern. Consolidates 9 test files into ScenarioStrategy.test.ts using getScenarioStrategy() factory function. Updates MergeOrchestrator import path accordingly.
Adds 6 tests covering: multiple properties merged simultaneously, property only in local/other, property deleted in both, mixed conflict/no-conflict children propagation, and nested object recursive delegation.
Move ordered-strategy tests from KeyedArrayMergeNode.test.ts into a dedicated OrderedKeyedArrayMergeStrategy.test.ts that tests the strategy directly with an explicit keyExtractor function.
Consolidate the two standalone "Diverged orderings" tests into the gracefulMergeScenarios array as M11 and M12.
|
Published under $ sf plugins install sf-git-merge-driver@dev-175 |
Contributor
|
@JulianAubert is the one that found the bug, he'll tell you if it's fixed :) But meanwhile i think you should release a patch: can't be break more picklists that it already does :D |
Owner
Author
|
I have been able to test locally the fix 👍 |
|
Shipped in release $ sf plugins install sf-git-merge-driver@latest-rc
# Or
$ sf plugins install sf-git-merge-driver@v1.5.4💡 Enjoying sf-git-merge-driver? |
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
3 participants
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.
Explain your changes
When merging a
CustomFieldwith inline picklist values (valueSet > valueSetDefinition > value), the merge driver produced completely broken XML output. The<valueSet>container was lost,<restricted>and<valueSetDefinition>appeared directly inside<CustomField>, and all value elements were empty/corrupted.Root causes:
ObjectMergeNodedouble-wrapped children and lost its own container. It re-wrapped each child's output with{ [key]: item }, but child nodes (likeTextMergeNode) already include the property key. Additionally, it never wrapped its combined output with its own attribute name (e.g.valueSet), so the container was lost.OrderedKeyedArrayMergeStrategy.wrapElement()wrapped raw parsed objects instead of converting them to the builder format expected byfast-xml-parserwithpreserveOrder: true, producing empty<value></value>elements.Fixes:
attributeparameter toObjectMergeNode, remove double-wrapping, wrap combined output with container nametoJsonArray()inwrapElement()andwrapKeys()to convert to builder formatRefactoring included in this PR:
ObjectMergeNodetoPropertyMergeNode, removeNestedObjectMergeNodeMergeContexttoArrayMergeStateinKeyedArrayMergeNodewrapWithRootKeyinPropertyMergeNodeTextMergeParams,GapKeys)MergeNodeinterfaceDESIGN.mdwithPropertyMergeNoderenameOrderedKeyedArrayMergeStrategyto its own file (~523 lines out ofKeyedArrayMergeNode.ts)ScenarioStrategyclasses into a single-file strategy map (followingTextMergeStrategypattern)PropertyMergeNodeunit tests (6 new tests)OrderedKeyedArrayMergeStrategytest file with parameterized merge scenariosDoes this close any currently open issues?
closes #174
Any particular element that can be tested locally
Merge a
CustomFieldXML with inline picklistvalueSetwhere one branch modifiesrestrictedand removes values while the other branch is unchanged. The output should preserve the<valueSet>container, take the modified<restricted>value, and correctly render remaining<value>elements.