Fix DESCRIBE PAGE / ALTER PAGE column name mismatch for DataGrid2 columns (#116)#298
Fix DESCRIBE PAGE / ALTER PAGE column name mismatch for DataGrid2 columns (#116)#298yscraft wants to merge 2 commits intomendixlabs:mainfrom
Conversation
deriveColumnNameBson skipped the intermediate TextTemplate.Template
document when reading DataGrid2 column headers, so caption-only columns
always fell back to col{N} instead of the name DESCRIBE PAGE shows.
sanitizeColumnName also diverged from deriveColumnName on edge cases
(leading/trailing underscores, all-special-char captions).
Fix: traverse TextTemplate → Template → Items (with a nil fallback for
older BSON), rewrite sanitizeColumnName to match deriveColumnName
exactly (returns "" for all-special-char input so caller uses col{N}),
and add 5 regression tests covering attribute binding, caption fallback,
all-special-char caption, index fallback, and sanitization trimming.
AI Code ReviewCritical IssuesNone Moderate IssuesNone Minor IssuesNone What Looks Good
RecommendationApprove the PR. The fix correctly resolves issue #116 with appropriate test coverage and minimal, focused changes. No further action needed. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
ako
left a comment
ako
left a comment
There was a problem hiding this comment.
Code Review — PR #298
Overview: Fixes a DESCRIBE/ALTER mismatch for DataGrid2 column names. Two independent bugs correctly identified and fixed. The unit tests are solid. One design concern before merging.
What's good
- Root cause analysis is correct — the PR description accurately explains both bugs (H1: wrong BSON traversal path, H2: sanitization divergence)
sanitizeColumnNamerewrite is a direct copy of the DESCRIBE-side logic — guaranteed to match- The empty-string gate (
if name := sanitizeColumnName(caption); name != "") correctly enables thecol{N}fallback for all-special-char captions - 5 unit tests cover attribute binding, caption path, all-special-char fallback, index fallback, and sanitization trimming
One concern: the template = tmpl fallback may reintroduce a divergence
The H1 fix adds a defensive fallback:
template := dGetDoc(tmpl, "Template")
if template == nil {
template = tmpl // fallback: some versions store Items directly
}The DESCRIBE side (extractTextContent in cmd_pages_describe_output.go) does not have this fallback:
template, ok := content["Template"].(map[string]any)
if !ok {
return "" // no Template → no caption
}If Template is nil, DESCRIBE returns "" and falls through to col{N}. ALTER with the fallback would instead read Items from tmpl directly and derive a name — producing a different result than DESCRIBE.
The stated goal is for ALTER and DESCRIBE to agree. The safest fix is to match DESCRIBE exactly and drop the fallback:
template := dGetDoc(tmpl, "Template")
if template == nil {
break // no Template → no caption, match DESCRIBE behaviour
}There's also no test for the fallback path, which suggests there's no known Mendix version that actually uses it.
Missing: bug-test MDL script
Per the PR checklist in CLAUDE.md:
Every bug fix should include an MDL test script in
mdl-examples/bug-tests/that reproduces the issue, so the fix can be verified in Studio Pro if applicable.
mdl-examples/bug-tests/116-datagrid2-column-name-mismatch.mdl is missing. A script that creates a DataGrid2 with a caption-only column and verifies DESCRIBE PAGE roundtrips the column name correctly would cover this.
Minor
makePropTypeID/propKeyhelpers in the test file are clear and well-namedTestDeriveColumnNameBson_CaptionFallbackdirectly exercises the previously broken path — good regression anchor
Summary: Approve after dropping the template = tmpl fallback (match DESCRIBE exactly) and adding a bug-tests/116-*.mdl script. The core fix is correct.
…t script
Drop the `template = tmpl` fallback in deriveColumnNameBson — if Template
is nil, DESCRIBE returns "" and falls to col{N}, so ALTER must do the same.
The fallback had no test coverage and would have reintroduced a divergence.
Add mdl-examples/bug-tests/116-datagrid2-column-name-mismatch.mdl per the
PR checklist: covers attribute-bound, caption-only, and all-special-char
column name roundtrip.
|
Thanks for the thorough review, @ako. Both points addressed: 1. Dropped the 2. Added All 5 unit tests still pass. |
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove - This PR correctly fixes the DESCRIBE/ALTER PAGE column name mismatch bug with appropriate tests and minimal, focused changes. The implementation now ensures ALTER PAGE uses the same column name derivation logic as DESCRIBE PAGE, resolving the inconsistency. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
2 participants
Summary
Fixes #116.
DESCRIBE PAGEandALTER PAGEderived DataGrid2 column names independently with two bugs that caused mismatches:deriveColumnNameBsonreadTextTemplate → Itemsdirectly, skipping the intermediateTemplatedocument.DESCRIBEcorrectly traversesTextTemplate → Template → Items. Result: caption-only columns always fell back tocol1,col2, … in ALTER regardless of what DESCRIBE showed.sanitizeColumnNamedidn't trim leading/trailing underscores and didn't fall through tocol{N}for all-special-char captions, unlikederiveColumnNameon the DESCRIBE side.Changes
mdl/executor/cmd_alter_page.goderiveColumnNameBson— traverseTextTemplate → Template → Items(nil-fallback for edge cases); gatesanitizeColumnNameresult so empty string falls through tocol{N}sanitizeColumnName— rewritten to matchderiveColumnNameexactly:strings.Map+strings.TrimFunc, returns""when result is all underscoresmdl/executor/alter_page_test.gocol{N}, index fallback, sanitization trimmingTest plan
go test ./mdl/executor/... -run TestDeriveColumnNameBson|TestSanitizeColumnName— all 5 passgo test ./mdl/executor/...— full suite passes, no regressions🤖 Generated with Claude Code