Skip to content

Conversation

@takyyon
Copy link
Contributor

@takyyon takyyon commented Jan 31, 2026

Summary

  • Fixes bug where Data Mapper fails to load saved maps containing XML attribute bindings within loops
  • The error "Source schema node not found, or invalid custom value for key './@id'" occurred because relative attribute paths (./@AttributeName) were not being properly resolved during deserialization
  • Root cause: path was constructed as /loop/path/./@Attr instead of /loop/path/@Attr

Changes

  • Added normalization in getSourceNodeForRelativeKeyInLoop() to strip ./ prefix from ./@ attribute paths
  • Enabled previously skipped test case that validates this scenario

Test plan

  • Existing deserializer tests pass (67 tests)
  • Existing serializer tests pass (33 tests)
  • Full data-mapper-v2 test suite passes (220 tests)
  • Re-enabled skipped test "creates a looping connection w/ index variable, conditional, and relative attribute path"
  • Manual verification with customer's XSD/JSON schemas

🤖 Generated with Claude Code

When deserializing data maps containing XML attribute bindings within loops,
the relative attribute path format './@AttributeName' was not being properly
resolved back to the full schema node key. This caused "Source schema node
not found" errors when reloading saved maps.

The fix normalizes './@' prefixed paths by stripping the './' before
constructing the full schema path lookup.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 31, 2026 02:13
@github-actions
Copy link

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: fix(data-mapper-v2): resolve XML attribute paths when loading saved maps
  • Issue: None — the title is clear, follows conventional-commit-style scoping, and describes the change.
  • Recommendation: Keep as-is. (Good job.)

Commit Type

  • The PR body does not use the required PR template commit-type checkbox section.
  • Note: You should select exactly one commit type in the template. From the change, fix is the appropriate choice.
  • Recommendation: Add the Commit Type checkboxes and mark fix. Example lines to add to the top of the PR body:
    • ## Commit Type\n- [x] fix - Bug fix\n- [ ] feature\n- [ ] refactor\n- [ ] perf\n- [ ] docs\n- [ ] test\n- [ ] chore

Risk Level

  • The PR body did not include the Risk Level section of the template, and the PR currently has no risk:* label attached.
  • Assessment: Missing risk selection and missing label. Based on the code diff (4 additions, 3 deletions across 2 files — a small normalization change and re-enabling a skipped test), the advised risk is Low.
  • Recommendation: Add the Risk Level section and select Low. Also add the matching label to the PR (risk:low). Example to add to the PR body:
    • ## Risk Level\n- [x] Low - Minor changes, limited scope\n- [ ] Medium - Moderate changes, some user impact\n- [ ] High - Major changes, significant user/system impact
    • Add PR label: risk:low (so labels and body match).

What & Why

  • Current: The PR contains a clear Summary describing the bug (relative attribute paths like ./@Attr were incorrectly constructed as /loop/path/./@Attr) and the fix (strip ./ when appropriate).
  • Issue: None significant — this maps well to "What & Why".
  • Recommendation: Move or copy this Summary into the PR template's ## What & Why section to match the required template format.

Impact of Change

  • Issue: The PR is missing the Impact of Change section from the template.
  • Recommendation: Add a short Impact section. Based on the changes, suggested content you can paste:
    • ## Impact of Change\n- **Users:** No user-facing UI change; saved maps containing relative XML attribute paths will now load correctly.\n- **Developers:** Small change to data-mapper-v2 deserializer logic (MapDefinitionDeserializer). One previously skipped unit test was re-enabled.\n- **System:** No expected performance or architecture impact.

⚠️ Test Plan

  • Assessment: You included a test plan in the PR body and the code diff shows a previously skipped test was enabled. The diff contains the change in MapDefinitionDeserializer.spec.ts where it.skip was changed to it (test re-enabled), and a small implementation change in MapDefinitionDeserializer.ts.
  • Recommendation: Convert the Test Plan into the template checkbox format and clearly indicate which suites were run and where they passed. Example:
    • ## Test Plan\n- [x] Unit tests added/updated (re-enabled a previously skipped test in libs/data-mapper-v2/...)\n- [x] E2E tests added/updated (if applicable) or explain none needed\n- [x] Manual testing completed (explain manual verification steps or list the XSD/JSON used)
  • Also ensure CI status is linked or mention the CI run(s) where the 220-test suite passed.

⚠️ Contributors

  • Assessment: Not included. This section is optional but recommended if others contributed.
  • Recommendation: If others (PMs, designers, reviewers) contributed, add them under ## Contributors. If none, you can add a short note like: No additional contributors.

⚠️ Screenshots/Videos

  • Assessment: Not applicable (no UI changes). This is fine.
  • Recommendation: Leave blank or add screenshots only if the change produces a visual difference.

Summary Table

Section Status Recommendation
Title Keep as-is
Commit Type Add template commit-type and select fix
Risk Level Add template risk-level and set Low; add risk:low label
What & Why Move Summary into the ## What & Why section
Impact of Change Add Impact of Change section (Users/Developers/System)
Test Plan ⚠️ Convert to template checkboxes and include CI run info; you re-enabled a test which is visible in the diff
Contributors ⚠️ Optional — add credits if applicable
Screenshots/Videos ⚠️ Not applicable — OK to omit

Final notes

  • Advised risk level: low (the change is small: normalization of attribute paths and re-enabling a skipped test; 4 additions / 3 deletions across 2 files). The PR did not declare a risk level or include a risk:* label — please add risk:low to the PR labels and mark Low in the template.
  • Tests: The diff shows you re-enabled a skipped test (good). Please make sure CI has run and is green; link to the passing CI run in the PR description or add a short sentence "CI passed: ".

Please update the PR body to match the required template (add Commit Type and Risk Level with checkboxes and label, add Impact of Change, convert Test Plan to checkboxes with CI evidence, and optionally add Contributors). After updating, re-request review.

Thank you for the clear description and the small focused fix — once the template fields and label are added this PR should be ready to merge.


Last updated: Sat, 31 Jan 2026 02:13:52 GMT

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug where the Data Mapper v2 fails to load saved maps containing XML attribute bindings within loops. The error occurred because relative attribute paths (./@AttributeName) were not being properly resolved during deserialization, resulting in malformed paths like /loop/path/./@Attr instead of /loop/path/@Attr.

Changes:

  • Added normalization logic to strip the ./ prefix from ./@ attribute paths when resolving relative keys within loops
  • Re-enabled a previously skipped test case that validates this exact scenario

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
libs/data-mapper-v2/src/mapHandling/MapDefinitionDeserializer.ts Added path normalization in getSourceNodeForRelativeKeyInLoop() to handle ./@ attribute patterns correctly by stripping the ./ prefix before path construction
libs/data-mapper-v2/src/mapHandling/test/MapDefinitionDeserializer.spec.ts Re-enabled test case that validates looping connections with index variables, conditionals, and relative attribute paths

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants