[WIP] fix(DataMapper): Add VariableItem and refactor other mapping item c…#3028
Draft
igarashitm wants to merge 1 commit intoKaotoIO:mainfrom
Draft
[WIP] fix(DataMapper): Add VariableItem and refactor other mapping item c…#3028igarashitm wants to merge 1 commit intoKaotoIO:mainfrom
VariableItem and refactor other mapping item c…#3028igarashitm wants to merge 1 commit intoKaotoIO:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors datamapper model: renames ConditionItem → InstructionItem, replaces ExpressionItem with IExpressionHolder & MappingItem intersection types, adds VariableItem and ValueType, updates cloning and hierarchy, and propagates type/signature changes across UI components, services, tests, and visualization/drag-and-drop handlers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ui/src/services/mapping-serializer.service.ts (2)
13-26:⚠️ Potential issue | 🔴 CriticalRound-trip
VariableItemthrough the serializer.The new
VariableItemmodel never reaches the XSLT document:populateMapping()has noVariableItembranch, andrestoreMapping()has noxsl:variablecase. Saving or reopening a mapping tree will silently drop every variable node.Possible fix
import { ChooseItem, FieldItem, ForEachItem, IfItem, InstructionItem, MappingItem, MappingParentType, MappingTree, OtherwiseItem, + VariableItem, ValueSelector, ValueType, WhenItem, } from '../models/datamapper/mapping'; @@ } else if (mapping instanceof WhenItem) { child = MappingSerializerService.populateWhenItem(parent, mapping); } else if (mapping instanceof OtherwiseItem) { child = MappingSerializerService.populateOtherwiseItem(parent, mapping); + } else if (mapping instanceof VariableItem) { + child = MappingSerializerService.populateVariableItem(parent, mapping); } @@ + private static populateVariableItem(parent: Element, mapping: VariableItem) { + const xsltDocument = parent.ownerDocument; + const xslVariable = xsltDocument.createElementNS(NS_XSL, 'variable'); + xslVariable.setAttribute('name', mapping.name); + xslVariable.setAttribute('select', mapping.expression); + parent.appendChild(xslVariable); + return xslVariable; + } + @@ case 'otherwise': { mappingItem = new OtherwiseItem(parentMapping); break; } + case 'variable': { + const name = item.getAttribute('name'); + if (!name) return; + const variableItem = new VariableItem(parentMapping, name); + variableItem.expression = item.getAttribute('select') || ''; + mappingItem = variableItem; + break; + } case 'for-each': { const forEachItem = new ForEachItem(parentMapping); forEachItem.expression = item.getAttribute('select') || ''; mappingItem = forEachItem; break;Also applies to: 129-145, 285-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/services/mapping-serializer.service.ts` around lines 13 - 26, The serializer currently drops VariableItem nodes because populateMapping and restoreMapping lack VariableItem handling; update populateMapping to recognize MappingItem entries where item.type === 'variable' (or the VariableItem class) and serialize them into an xsl:variable element with the same attributes/children/value structure you use for other items (e.g., set name, select/content and preserve nested MappingTree if any), and update restoreMapping to handle incoming xsl:variable elements by creating a VariableItem (populate its identifier, value/children and any metadata like valueType/selector) so variables round-trip; mirror how other branches (e.g., ForEachItem/WhenItem) map attributes and children to ensure consistency and include VariableItem in any switch/if chains in both populateMapping and restoreMapping.
205-210:⚠️ Potential issue | 🟠 MajorPersist
ForEachItem.sortItemsasxsl:sortchildren.
ForEachItem.sortItemsis now part of the model, butpopulateForEachItem()only writesselectandrestoreMapping()ignoresxsl:sort. Any configured sort criteria disappear as soon as the mapping is serialized or reopened.Possible fix
private static populateForEachItem(parent: Element, mapping: ForEachItem) { const xsltDocument = parent.ownerDocument; const xslForEach = xsltDocument.createElementNS(NS_XSL, 'for-each'); xslForEach.setAttribute('select', mapping.expression); + mapping.sortItems.forEach((sortItem) => { + const xslSort = xsltDocument.createElementNS(NS_XSL, 'sort'); + xslSort.setAttribute('select', sortItem.expression); + xslSort.setAttribute('order', sortItem.order); + xslForEach.appendChild(xslSort); + }); parent.appendChild(xslForEach); return xslForEach; } @@ case 'for-each': { const forEachItem = new ForEachItem(parentMapping); forEachItem.expression = item.getAttribute('select') || ''; mappingItem = forEachItem; break; } + case 'sort': { + if (!(parentMapping instanceof ForEachItem)) return; + parentMapping.sortItems.push({ + expression: item.getAttribute('select') || '', + order: (item.getAttribute('order') as 'ascending' | 'descending') || 'ascending', + }); + return; + } case 'attribute': { if (parentField instanceof PrimitiveDocument) return; const field = MappingSerializerService.getOrCreateAttributeField(item, parentField); if (!field) return; fieldItem = field;Also applies to: 285-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/services/mapping-serializer.service.ts` around lines 205 - 210, populateForEachItem currently only emits an xsl:for-each with select and drops any ForEachItem.sortItems causing configured sort criteria to be lost; update populateForEachItem to iterate mapping.sortItems and append xsl:sort children to the created xslForEach (use xsltDocument.createElementNS(NS_XSL, 'sort') and set appropriate attributes like 'select', 'order', 'data-type' based on each SortItem), and also update the complementary restoreMapping logic to parse xsl:sort children back into ForEachItem.sortItems so serialization/deserialization round-trips sort criteria (ensure you modify both populateForEachItem and the corresponding restoreMapping/deserialization block referenced around lines ~285-338).packages/ui/src/models/datamapper/mapping.ts (1)
127-143:⚠️ Potential issue | 🟠 MajorReturn
otherwiseas optional to match the documented contract.The docblock explicitly states the fallback branch is optional, but the getter casts
find()toOtherwiseItem, which hides theundefinedpossibility. This violates TypeScript strict mode by misrepresenting the actual return type. Although existing callers handle this carefully (using&&or!!checks), the unsafe type signature creates a maintenance hazard and misleads future callers.Suggested fix
- get otherwise() { - return this.children.find((c) => c instanceof OtherwiseItem) as OtherwiseItem; + get otherwise(): OtherwiseItem | undefined { + return this.children.find((c): c is OtherwiseItem => c instanceof OtherwiseItem); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/datamapper/mapping.ts` around lines 127 - 143, The otherwise getter on ChooseItem misrepresents its return type by casting children.find(...) to OtherwiseItem even though the fallback is optional; update the ChooseItem.otherwise getter to return OtherwiseItem | undefined (remove the force-cast) so it accurately reflects that children.find(...) may return undefined, and adjust any local JSDoc or usages in the file to accept the optional OtherwiseItem where needed.
🧹 Nitpick comments (3)
packages/ui/src/services/mapping-links.service.ts (1)
32-39: Add a service-level test for the newisExpressionHolder()branch.This path now depends on duck-typing instead of the old class hierarchy, but the PR only adds guard/clone tests. A dedicated
extractMappingLinks()test for at least oneVariableItemand one non-holder item would catch silent link rendering regressions here.Also applies to: 67-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/services/mapping-links.service.ts` around lines 32 - 39, Add service-level tests that cover the new isExpressionHolder() branch so we catch silent regressions: write at least two tests invoking MappingLinksService.doExtractMappingLinks (or the public extractMappingLinks wrapper) with (1) a VariableItem instance that satisfies MappingItem and isExpressionHolder(item) and assert the returned links include the expected mapping link(s), and (2) a MappingItem-derived instance that is not an expression holder and assert it returns the expected empty or different link set; use the same sourceParameterMap/sourceBody/selectedNodeRef shapes used elsewhere in tests to replicate behavior and fail if link rendering changes.packages/ui/src/models/datamapper/mapping.test.ts (2)
56-59: Avoidas neverin theChooseItemfixture.This bypasses the real field contract, so the test can pass with an impossible object shape and won’t protect
ChooseItem.clone()if it starts reading more field metadata. Prefer a minimal typed field fixture/helper instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/datamapper/mapping.test.ts` around lines 56 - 59, The test uses an unsafe type cast ("as never") when constructing the ChooseItem fixture which bypasses the real field contract; replace this with a properly typed minimal field fixture that matches the expected Field/interface used by ChooseItem (e.g., create a small helper or inline object typed as Field) and use that instead of "as never", then call new ChooseItem(tree, field) and assert on item.clone() to ensure clone() is validated against the real field shape; reference ChooseItem and its clone() method when updating the test fixture.
39-52: Assert that cloned children are re-parented to the clone.These tests currently prove copied values, but not cloned topology. After the hierarchy refactor, a clone that leaves
child.parentpointing at the original item would still pass here and later breaknodePath/contextPathconsumers. Add an assertion likeexpect(cloned.children[0].parent).toBe(cloned).Also applies to: 67-80, 110-123, 140-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/datamapper/mapping.test.ts` around lines 39 - 52, The tests for IfItem.clone() and similar cases (e.g., the blocks around lines 67-80, 110-123, 140-153) verify copied values but not that cloned children are re-parented; update each relevant test (the one creating an IfItem, ValueSelector, calling clone()) to assert that each cloned child's parent points to the cloned item (e.g., add an assertion that cloned.children[0].parent is the cloned instance) so clone() verifies topology is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/models/datamapper/mapping.ts`:
- Around line 54-58: MappingItem.clone() deep-copies children but doesn't update
each cloned child's parent, leaving cloned descendants pointing to the original
tree and causing incorrect nodePath/contextPath; update the clone routine (in
MappingItem.clone and any other clone entry points that call doClone()) to set
cloned.children = this.children.map(c => { const cc = c.clone(); cc.parent =
cloned; return cc; }) or equivalent so each child's parent is reassigned to the
new cloned parent (ensuring any path recalculation reads the new ancestry).
Ensure the same parent-fixing pattern is applied to other clone call sites that
produce subtrees so all descendants have their parent refs updated.
In `@packages/ui/src/services/mapping.service.ts`:
- Around line 244-246: Escape regex metacharacters in oldDocumentId before
building the RegExp to avoid ReDoS and incorrect matches: compute an
escapedOldDocumentId (e.g., oldDocumentId.replace(/[.*+?^${}()|[\]\\]/g,
'\\$&')) and use it in the replacement call inside the isExpressionHolder branch
so the RegExp becomes new RegExp(`\\$${escapedOldDocumentId}\\b`, 'g') and the
replacement uses `$${newDocumentId}`; update the code references to
escapedOldDocumentId, isExpressionHolder(child), child.expression, oldDocumentId
and newDocumentId accordingly.
---
Outside diff comments:
In `@packages/ui/src/models/datamapper/mapping.ts`:
- Around line 127-143: The otherwise getter on ChooseItem misrepresents its
return type by casting children.find(...) to OtherwiseItem even though the
fallback is optional; update the ChooseItem.otherwise getter to return
OtherwiseItem | undefined (remove the force-cast) so it accurately reflects that
children.find(...) may return undefined, and adjust any local JSDoc or usages in
the file to accept the optional OtherwiseItem where needed.
In `@packages/ui/src/services/mapping-serializer.service.ts`:
- Around line 13-26: The serializer currently drops VariableItem nodes because
populateMapping and restoreMapping lack VariableItem handling; update
populateMapping to recognize MappingItem entries where item.type === 'variable'
(or the VariableItem class) and serialize them into an xsl:variable element with
the same attributes/children/value structure you use for other items (e.g., set
name, select/content and preserve nested MappingTree if any), and update
restoreMapping to handle incoming xsl:variable elements by creating a
VariableItem (populate its identifier, value/children and any metadata like
valueType/selector) so variables round-trip; mirror how other branches (e.g.,
ForEachItem/WhenItem) map attributes and children to ensure consistency and
include VariableItem in any switch/if chains in both populateMapping and
restoreMapping.
- Around line 205-210: populateForEachItem currently only emits an xsl:for-each
with select and drops any ForEachItem.sortItems causing configured sort criteria
to be lost; update populateForEachItem to iterate mapping.sortItems and append
xsl:sort children to the created xslForEach (use
xsltDocument.createElementNS(NS_XSL, 'sort') and set appropriate attributes like
'select', 'order', 'data-type' based on each SortItem), and also update the
complementary restoreMapping logic to parse xsl:sort children back into
ForEachItem.sortItems so serialization/deserialization round-trips sort criteria
(ensure you modify both populateForEachItem and the corresponding
restoreMapping/deserialization block referenced around lines ~285-338).
---
Nitpick comments:
In `@packages/ui/src/models/datamapper/mapping.test.ts`:
- Around line 56-59: The test uses an unsafe type cast ("as never") when
constructing the ChooseItem fixture which bypasses the real field contract;
replace this with a properly typed minimal field fixture that matches the
expected Field/interface used by ChooseItem (e.g., create a small helper or
inline object typed as Field) and use that instead of "as never", then call new
ChooseItem(tree, field) and assert on item.clone() to ensure clone() is
validated against the real field shape; reference ChooseItem and its clone()
method when updating the test fixture.
- Around line 39-52: The tests for IfItem.clone() and similar cases (e.g., the
blocks around lines 67-80, 110-123, 140-153) verify copied values but not that
cloned children are re-parented; update each relevant test (the one creating an
IfItem, ValueSelector, calling clone()) to assert that each cloned child's
parent points to the cloned item (e.g., add an assertion that
cloned.children[0].parent is the cloned instance) so clone() verifies topology
is preserved.
In `@packages/ui/src/services/mapping-links.service.ts`:
- Around line 32-39: Add service-level tests that cover the new
isExpressionHolder() branch so we catch silent regressions: write at least two
tests invoking MappingLinksService.doExtractMappingLinks (or the public
extractMappingLinks wrapper) with (1) a VariableItem instance that satisfies
MappingItem and isExpressionHolder(item) and assert the returned links include
the expected mapping link(s), and (2) a MappingItem-derived instance that is not
an expression holder and assert it returns the expected empty or different link
set; use the same sourceParameterMap/sourceBody/selectedNodeRef shapes used
elsewhere in tests to replicate behavior and fail if link rendering changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6123f338-856e-4c7d-a0bf-5f12b8458b0b
📒 Files selected for processing (17)
packages/ui/src/components/Document/actions/DeleteMappingItemAction.tsxpackages/ui/src/components/Document/actions/XPathEditorAction.tsxpackages/ui/src/components/Document/actions/XPathInputAction.tsxpackages/ui/src/components/XPath/XPathEditor.tsxpackages/ui/src/components/XPath/XPathEditorLayout.test.tsxpackages/ui/src/components/XPath/XPathEditorLayout.tsxpackages/ui/src/components/XPath/XPathEditorModal.test.tsxpackages/ui/src/components/XPath/XPathEditorModal.tsxpackages/ui/src/models/datamapper/mapping.test.tspackages/ui/src/models/datamapper/mapping.tspackages/ui/src/models/datamapper/visualization.tspackages/ui/src/providers/dnd/ExpressionEditorDnDHandler.tspackages/ui/src/services/mapping-links.service.tspackages/ui/src/services/mapping-serializer.service.tspackages/ui/src/services/mapping.service.tspackages/ui/src/services/visualization.service.test.tspackages/ui/src/services/visualization.service.ts
Member
Author
|
@coderabbitai |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
igarashitm
force-pushed
the
2362-2837
branch
3 times, most recently
from
e7be3b3 to
05edeb7
Compare
…lasses to follow XSLT terminology Fixes: KaotoIO#2837 Fixes: KaotoIO#2838 Add `VariableItem` and refactor other mapping items. Align class names to follow correct terminology from XSLT perspective. Currently `ConditionItem` contains `ForEachItem` but `for-each` is not a condition. Rename it to `InstructionItem` to be accurate. Also, remove `ExpressionItem` abstract class from hierarchy and introduce `ExpressionHolder` interface to properly represent the mapping item with expressions. A side effect is that we are no longer able to do `if (item instanceof ExpressionItem)`, which is replaced with the usage of `isExpressionHolder()` function. Also removed unused `isCondition` property.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/components/Document/actions/XPathInputAction.tsx (1)
22-25: Consider using the intersection type for consistency.While
IExpressionHolderis technically sufficient since this component only accessesmapping.expression, the siblingXPathEditorActioncomponent usesIExpressionHolder & MappingItem. Using the same intersection type would maintain consistency across components and reflect that all callers pass the full intersection type fromVisualizationService.getExpressionItemForNode().♻️ Suggested change for consistency
+import { IExpressionHolder, MappingItem } from '../../../models/datamapper/mapping'; -import { IExpressionHolder } from '../../../models/datamapper/mapping'; type XPathInputProps = { - mapping: IExpressionHolder; + mapping: IExpressionHolder & MappingItem; onUpdate: () => void; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Document/actions/XPathInputAction.tsx` around lines 22 - 25, Change the XPathInputProps type to use the same intersection used elsewhere so callers and future maintainers see the full shape: update the XPathInputProps definition to reference IExpressionHolder & MappingItem (instead of just IExpressionHolder), keeping the existing property names (mapping: IExpressionHolder & MappingItem; onUpdate: () => void) so XPathInputAction and XPathEditorAction remain consistent with values returned by VisualizationService.getExpressionItemForNode(); ensure no other call sites break by adjusting any narrow typings if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/components/Document/actions/XPathInputAction.tsx`:
- Around line 22-25: Change the XPathInputProps type to use the same
intersection used elsewhere so callers and future maintainers see the full
shape: update the XPathInputProps definition to reference IExpressionHolder &
MappingItem (instead of just IExpressionHolder), keeping the existing property
names (mapping: IExpressionHolder & MappingItem; onUpdate: () => void) so
XPathInputAction and XPathEditorAction remain consistent with values returned by
VisualizationService.getExpressionItemForNode(); ensure no other call sites
break by adjusting any narrow typings if necessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 970c7329-cd02-495a-b00b-2cfffb26b31e
📒 Files selected for processing (17)
packages/ui/src/components/Document/actions/DeleteMappingItemAction.tsxpackages/ui/src/components/Document/actions/XPathEditorAction.tsxpackages/ui/src/components/Document/actions/XPathInputAction.tsxpackages/ui/src/components/XPath/XPathEditor.tsxpackages/ui/src/components/XPath/XPathEditorLayout.test.tsxpackages/ui/src/components/XPath/XPathEditorLayout.tsxpackages/ui/src/components/XPath/XPathEditorModal.test.tsxpackages/ui/src/components/XPath/XPathEditorModal.tsxpackages/ui/src/models/datamapper/mapping.test.tspackages/ui/src/models/datamapper/mapping.tspackages/ui/src/models/datamapper/visualization.tspackages/ui/src/providers/dnd/ExpressionEditorDnDHandler.tspackages/ui/src/services/mapping-links.service.tspackages/ui/src/services/mapping-serializer.service.tspackages/ui/src/services/mapping.service.tspackages/ui/src/services/visualization.service.test.tspackages/ui/src/services/visualization.service.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/ui/src/components/XPath/XPathEditorLayout.test.tsx
- packages/ui/src/components/Document/actions/DeleteMappingItemAction.tsx
- packages/ui/src/services/mapping-serializer.service.ts
- packages/ui/src/components/XPath/XPathEditor.tsx
- packages/ui/src/components/Document/actions/XPathEditorAction.tsx
- packages/ui/src/services/visualization.service.ts
- packages/ui/src/models/datamapper/mapping.test.ts
- packages/ui/src/services/visualization.service.test.ts
- packages/ui/src/models/datamapper/visualization.ts
- packages/ui/src/providers/dnd/ExpressionEditorDnDHandler.ts
- packages/ui/src/components/XPath/XPathEditorLayout.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3028 +/- ##
==========================================
+ Coverage 89.73% 89.81% +0.08%
==========================================
Files 563 563
Lines 20984 21018 +34
Branches 4904 4907 +3
==========================================
+ Hits 18829 18878 +49
- Misses 2028 2138 +110
+ Partials 127 2 -125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
1 participant
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.
…lasses to follow XSLT terminology
#3026 needs to go in before this one.
Fixes: #2837
Fixes: #2838
Add
VariableItemand refactor other mapping items. Align class names to follow correct terminology from XSLT perspective. CurrentlyConditionItemcontainsForEachItembutfor-eachis not a condition. Rename it toInstructionItemto be accurate. Also, removeExpressionItemabstract class from hierarchy and introduceExpressionHolderinterface to properly represent the mapping item with expressions. A side effect is that we are no longer able to doif (item instanceof ExpressionItem), which is replaced with the usage ofisExpressionHolder()function. Also removed unusedisConditionproperty.Summary by CodeRabbit
Release Notes
Refactor
Tests