Skip to content

fix: preserve mapping result range cardinality#372

Open
hjotha wants to merge 5 commits intomendixlabs:mainfrom
hjotha:submit/rest-result-mapping-range-cardinality
Open

fix: preserve mapping result range cardinality#372
hjotha wants to merge 5 commits intomendixlabs:mainfrom
hjotha:submit/rest-result-mapping-range-cardinality

Conversation

@hjotha
Copy link
Copy Markdown
Contributor

@hjotha hjotha commented Apr 27, 2026

Summary

  • store ForceSingleOccurrence separately from result cardinality
  • parse Range.SingleObject and object variable type as object-valued mapping signals
  • write REST and XML import mapping result metadata without collapsing the two fields

Closes #369. Part of #332.

Validation

  • make build
  • make lint-go
  • make test

hjotha pushed a commit to hjotha/mxcli that referenced this pull request Apr 27, 2026
Adds an MDL script under mdl-examples/bug-tests/ exercising an
`import from mapping` action that uses the same
ResultHandlingMapping BSON shape that mendixlabs#372 splits apart. Pure-MDL
authoring cannot reproduce the divergent Force/Range flag split
(no Studio Pro REST client setup), so the script is a positive
control: `mx check` must continue to report 0 errors.

The negative case (Force vs Range divergence) is covered by the
parser/writer Go tests in sdk/mpr/parser_microflow_test.go.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Apr 28, 2026
@ako
Copy link
Copy Markdown
Collaborator

ako commented Apr 29, 2026

Cant merge due to conflicts

hjothamendix and others added 2 commits April 30, 2026 09:50
Symptom: REST and import-mapping result handling can change cardinality metadata during roundtrip when ForceSingleOccurrence and Range.SingleObject differ.

Root cause: ResultHandlingMapping had only one SingleObject flag, so parsing collapsed ForceSingleOccurrence into result cardinality and writing forced both BSON fields to the same value.

Fix: store ForceSingleOccurrence separately, parse Range.SingleObject and ObjectType as result-cardinality signals, and write ForceSingleOccurrence independently from Range.SingleObject for REST and XML import mappings.

Tests: add parser/writer coverage for object-valued mapping results with ForceSingleOccurrence=false. make build, make lint-go, and make test pass.
Adds an MDL script under mdl-examples/bug-tests/ exercising an
`import from mapping` action that uses the same
ResultHandlingMapping BSON shape that mendixlabs#372 splits apart. Pure-MDL
authoring cannot reproduce the divergent Force/Range flag split
(no Studio Pro REST client setup), so the script is a positive
control: `mx check` must continue to report 0 errors.

The negative case (Force vs Range divergence) is covered by the
parser/writer Go tests in sdk/mpr/parser_microflow_test.go.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@hjotha hjotha force-pushed the submit/rest-result-mapping-range-cardinality branch from c1492c6 to c799107 Compare April 30, 2026 07:51
@github-actions
Copy link
Copy Markdown

AI Code Review

Critical Issues

  • None found

Moderate Issues

  • None found

Minor Issues

  • XML parser asymmetry: The XML import action parser includes a fallback (if !handling.SingleObject { handling.SingleObject = forceSingleOccurrence }) that the REST parser lacks. While likely necessary for backward compatibility with older MPR files, this creates inconsistent handling between REST and XML import mappings. No test covers the scenario where Range is absent in XML mappings to verify this fallback behaves correctly.
  • Test script classification: The new test script in mdl-examples/bug-tests/ is a positive control (valid MDL that should work) rather than a bug reproduction test. While acceptable as documentation, bug-test directory conventionally contains scripts that reproduce the issue pre-fix. Consider moving to doctype-tests/ if it's purely a validity check.

What Looks Good

  • Precise fix: Changes are surgically focused on separating ForceSingleOccurrence and SingleObject in ResultHandlingMapping across parser, writer, and tests.
  • Comprehensive unit tests: Added three targeted test cases in sdk/mpr/parser_microflow_test.go verifying:
    • Correct parsing of split fields (REST)
    • Proper serialization preserving ForceSingleOccurrence separately (REST)
    • XML import action preserving both fields independently
  • Valid positive control: The bug-test script demonstrates a valid import mapping microflow that exercises the fixed codepath and validates with mx check.
  • No syntax changes: Since this is an internal BSON fix without new MDL features, correctly avoids unnecessary grammar/AST/visitor/executor changes.
  • Atomic scope: PR addresses exactly one issue (conflated mapping metadata) with cohesive changes across related files.

Recommendation

Approve. The fix correctly resolves the cardinality conflation bug with adequate test coverage. Minor concerns about XML parser asymmetry are mitigated by existing test coverage and backward compatibility needs. The bug-test script serves its purpose as a validity check despite directory naming. No blocker issues identified.


Automated review via OpenRouter (Nemotron Super 120B) — workflow source

Symptom: REST calls that returned import mappings could rename an explicit
assignment such as $Items to the mapped entity name during exec, leaving later
statements that still referenced $Items invalid.

Root cause: the REST mapping builder always overwrote OutputVariable with the
result entity short name, even when the MDL assignment already supplied a
variable.

Fix: only derive the entity-name fallback when the statement has no explicit
output variable.

Tests: add a synthetic builder regression that asserts both the action output
and ResultHandlingMapping.ResultVariable preserve the authored name.
@ako ako added this to the v0.8.0 milestone Apr 30, 2026
…pping-range-cardinality

# Conflicts:
#	sdk/mpr/parser_microflow_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve mapping result range cardinality during roundtrip

3 participants