fix: preserve validation feedback targets#359
Conversation
…tion Adds an MDL script under mdl-examples/bug-tests/ exercising attribute-target validation feedback. The script header documents that PR mendixlabs#359 fixes only the parser+formatter; object-only and association-target builder support is intentionally out of scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tion Adds an MDL script under mdl-examples/bug-tests/ exercising attribute-target validation feedback. The script header documents that PR mendixlabs#359 fixes only the parser+formatter; object-only and association-target builder support is intentionally out of scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
hjotha
force-pushed
the
submit/validation-feedback-targets
branch
from
f63e3c4 to
f59fd39
Compare
ako
left a comment
ako
left a comment
There was a problem hiding this comment.
Review — fix: preserve validation feedback targets
Overview
Well-scoped fix. The three-part approach (grammar → visitor → formatter) is correct, the scope note in the bug test explicitly documents what remains out of scope (builder producing CE0091 for object-only authored targets), and the formatter change is minimal. Tests cover the new grammar forms, the builder paths, and the formatter paths.
Moderate: attributePath grammar change is broader than its context suggests
The diff changes:
attributePath
- : VARIABLE ((SLASH | DOT) (IDENTIFIER | qualifiedName))+
+ : VARIABLE ((SLASH | DOT) qualifiedName)+attributePath is used in five places beyond validationFeedbackStatement:
setStatement(SET $var/Attr = expr)loopStatement(LOOP $item IN $list/assoc)aggregateExpression(SUM($list/attr),AVERAGE,MINIMUM,MAXIMUM)
The change is backward-compatible — a bare IDENTIFIER is still a valid qualifiedName — but none of these other usages have regression tests that exercise the grammar-change path. A $Order/SUM or $List/Module.Attr through the SET or LOOP paths should still parse identically; worth a comment in the grammar noting that the rule is shared and the change is intentional.
Moderate: old two-segment association path (len(segs) >= 2) is now dead for associations
Under the old grammar, $Obj/Module.Association produced two segments ("Module" sep /, "Association" sep .), which is why the code had:
} else if len(segs) >= 2 && segs[0].Separator == "/" && segs[1].Separator == "." {
// Reconstruct "Module.AssociationName" from segmentsUnder the new grammar qualifiedName absorbs the dot, so $Obj/Module.Association now produces one segment "Module.Association" — handled correctly by the new case 1 path. The len(segs) >= 2 branch is now only reachable for multi-hop paths like $Obj/Part1/Part2, where its comment ("association qualified name") is now misleading. The reconstruction logic (strings.Join(parts, ".")) would incorrectly join multi-hop segments into an association name in that case.
The old code path should either be removed (since the grammar change makes it unreachable for single associations) or its comment and logic updated to reflect that it now only handles multi-hop traversal paths.
Minor: redundant else if in multi-segment block
} else if entityQName == "" && strings.Count(segs[0].Name, ".") == 1 {
associationName = segs[0].NameThis branch sits inside the len(segs) >= 2 block, so it applies when: there are 2+ segments, the path doesn't match the / then . separator pattern, entityQName is empty, and the first segment has exactly one dot. Under the new grammar this combination is hard to construct in a well-formed microflow — a description of when this fires would help.
Positive
- Grammar fix is correct:
qualifiedNamesubsumes bare identifiers so all existingattributePathsyntax still parses. validationFeedbackStatementaccepting bareVARIABLEis the right grammar shape for the object-only target.- Dot-count heuristic for single segments (0 → attribute, 1 → association, 2+ → qualified attribute) is consistent with
resolveMemberChangeFallbackelsewhere. - Scope note in the bug test is honest and specific — the CE0091 gap is documented, not hidden.
AI Code ReviewCritical Issues
Moderate Issues
Minor Issues
What Looks Good
RecommendationApprove the PR. The changes are well-structured, address the issue comprehensively, include appropriate tests, and follow project guidelines. The generated parser interp file modification is expected when the grammar changes. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
AI Code ReviewWhat Looks Good
RecommendationThe PR is well-scoped, thoroughly tested, and correctly implements the fix for validation feedback round-trips. It follows the project's established patterns and maintains backward compatibility. Approve this PR. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
…tion Adds an MDL script under mdl-examples/bug-tests/ exercising attribute-target validation feedback. The script header documents that PR mendixlabs#359 fixes only the parser+formatter; object-only and association-target builder support is intentionally out of scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
hjotha
force-pushed
the
submit/validation-feedback-targets
branch
from
73138b8 to
248a1f1
Compare
AI Code ReviewReview SummaryThis PR fixes validation feedback round-trips for non-simple attribute targets in MDL microflows. It properly handles object-only targets ( What Looks Good
Minor Issues
RecommendationApprove with the following note: The fix is technically sound, well-tested, and follows MDL design principles. It enables proper round-tripping of validation feedback statements for object and association targets as intended. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Symptom: all open PRs against main failed the shared build-and-test job in make test-integration, even when their local build/test/lint validation passed. The failures reproduced on origin/main, so they were baseline CI instability rather than PR-specific regressions. Root cause: TestWatcherDebounce could allow stale timer callbacks to send an extra message under slow scheduling, nanoflow integration fixtures used MDL syntax that no longer matched the grammar, and the doctype mx-check harness did not classify known page/nanoflow showcase consistency errors as expected limitations. Fix: guard watcher debounce callbacks with a generation counter, tighten the watcher burst test, update nanoflow fixtures to current MDL syntax, and extend the known consistency-error allowlist for showcase-only limitations. Tests: make build Tests: go test ./cmd/mxcli/tui -run TestWatcherDebounce -count=20 -v Tests: ./bin/mxcli check mdl-examples/doctype-tests/02b-nanoflow-examples.mdl Tests: go test -tags integration -count=1 -timeout 30m ./mdl/executor -run 'TestRoundtripNanoflow_(Loop|EnumParameter|Annotations)|TestMxCheck_DoctypeScripts/02b-nanoflow-examples.mdl|TestMxCheck_DoctypeScripts/03-page-examples.mdl' -v Tests: make test Tests: make lint-go Tests: make test-integration
Validation feedback actions can target an object or an association path, but the MDL grammar only accepted attribute paths and the formatter only emitted attribute targets. Round-tripping those actions either failed to parse object-only targets or dropped the association name. The parser now accepts object-only validation feedback targets and treats each path segment as a qualified name so Module.Association stays a single association segment. The formatter emits AssociationName when AttributeName is absent, preserving association targets from existing models. Tests cover object-only and association formatter output, parser coverage for both target shapes, and the full build/lint/test validation passed locally.
…tion Adds an MDL script under mdl-examples/bug-tests/ exercising attribute-target validation feedback. The script header documents that PR mendixlabs#359 fixes only the parser+formatter; object-only and association-target builder support is intentionally out of scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Symptom: validation feedback targeting $Object/Module.Association could be written as an invalid Attribute value after roundtrip. Root cause: the builder treated a single parsed segment containing one dot as a direct attribute and prefixed the object entity, producing Module.Entity.Module.Association. Fix: apply the same member-name shape rule used elsewhere: bare names are attributes, one-dot names are associations, and two-or-more-dot names are fully qualified attributes. Tests: add synthetic builder coverage for qualified association and qualified attribute validation feedback targets.
Symptom: the validation feedback path parser now keeps qualified segments intact, but the builder still carried an obsolete two-segment association fallback from the older grammar shape. Root cause: `$Object/Module.Association` is now represented as one slash segment, so the old slash-then-dot branch was dead for associations and misleading for future multi-hop paths. Fix: document attributePath as a shared qualified segment rule and remove the obsolete reconstruction path from validation feedback member resolution. Tests: rerun focused executor/visitor/grammar tests plus make build and make test.
The validation feedback target grammar now uses qualifiedName segments so Module.Association remains a single slash segment. The previous fallback still contained a stale multi-segment association path branch from the old grammar, which could join unrelated traversal pieces into an association name. This removes the stale branch, documents that attributePath is shared by other microflow statements, and adds synthetic parser coverage for a SET target with a slash-qualified segment. Tests: - go test ./mdl/visitor ./mdl/executor -run 'TestValidationFeedback|TestSharedAttributePathKeepsQualifiedSlashSegment|TestAddValidationFeedback|TestFormatAction_ValidationFeedback' -count=1 - make build - make test
hjotha
force-pushed
the
submit/validation-feedback-targets
branch
from
248a1f1 to
2132acd
Compare
AI Code ReviewWe are reviewing a pull request (PR) that is titled "fix: preserve validation feedback targets". The PR body indicates it fixes validation feedback round-trips for targets that are not simple attributes. The changes in the PR are:
We are to review against the checklist in CLAUDE.md, focusing on:
Let's break down the changes:
Now, let's check against the checklist: Overlap & duplication
MDL syntax design
Full-stack consistency
This change to Now, the AST for Looking at the change in Let me check the diff again: we don't see any change in However, looking at the change in the executor ( // Build attribute or association name from variable type and attribute path.
// The current grammar keeps `$Product/Module.Association` as one slash
// segment, so dot count on that segment is the disambiguator:
// 0 dots: attribute relative to the target entity.
// 1 dot: association qualified by module.
// 2+ dots: fully qualified attribute.
var attributeName string
var associationName string
if entityQName, ok := fb.varTypes[s.AttributePath.Variable]; ok && len(s.AttributePath.Segments) > 0 {This code is still using Actually, the However, note that the PR might have omitted the AST change because it was done in a previous commit? But we are reviewing this PR as a whole. Alternatively, let's look at the Looking at the change in the executor test file ( fb.addValidationFeedbackAction(&ast.ValidationFeedbackStmt{
AttributePath: &ast.AttributePathExpr{
Variable: "FormObject",
Segments: []ast.PathSegment{
{Name: "Synthetic.FormObject_Target", Separator: "/"},
},
},
Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "Select a target"},
})And for the object-only target in the formatter test: action := µflows.ValidationFeedbackAction{
ObjectVariable: "Customer",
Template: &model.Text{
Translations: map[string]string{"en_US": "Select a customer"},
},
}This suggests that the AST for Alternatively, note that the Without seeing the AST change, we cannot verify full-stack consistency. However, note that the PR does not show any change in the AST directory. This is a potential gap. But wait: the PR description says "Fixes validation feedback round-trips for targets that are not simple attributes." and the changes include the grammar and the executor. The AST might have been changed in a commit that is not shown in the diff? However, we are to review the diff as provided. Since the diff does not show any change in Looking at the grammar change: the validation feedback statement can now be: In the AST, this would have to be represented by setting the Therefore, the AST must have been changed to allow the Since we don't see the AST change, we have to flag this as a potential issue: the AST might not be updated to reflect the grammar change, leading to a mismatch. However, note that the PR includes a test for the builder that uses an fb.addValidationFeedbackAction(&ast.ValidationFeedbackStmt{
AttributePath: &ast.AttributePathExpr{
Variable: "FormObject",
Segments: []ast.PathSegment{
{Name: "Synthetic.FormObject_Target", Separator: "/"},
},
},
...
})This is for a qualified association target (one segment with a slash). For the object-only target, we see in the formatter test: action := µflows.ValidationFeedbackAction{
ObjectVariable: "Customer",
...
}How does the builder create an action with Since we don't see the AST change in the diff, we must conclude that the AST change is missing from the diff we are reviewing. This is a problem for full-stack consistency. However, note that the PR might be part of a series and the AST change was in a previous commit? But we are reviewing this PR as a whole. Given the instructions, we must review the diff as provided. The diff does not show any AST change. Therefore, we cannot verify that the AST was updated. This is a gap. But wait: the PR also modifies the For the validation feedback statement, the grammar now has two alternatives. The AST must reflect that. Without seeing the AST change, we cannot be sure. However, note that the PR includes a test for the builder that uses an Let me look at the existing AST for Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
|
CI stabilization note: merged #412 into this branch so it uses the same integration-CI baseline as the rest of the active PR set. |
Symptom: CI failed in TestMxCheck_DoctypeScripts/empty_java_action_argument.mdl because a Java action declared as RETURNS Void was written as an entity return type named .void, and Studio Pro reported CE1613. Root cause: the generic data-type visitor treats bare qualified names as entity/enumeration references. Java action return types reused that generic path, so the explicit Void spelling became a qualified name instead of ast.TypeVoid. Fix: add a Java-action return-type wrapper that maps unqualified Void to ast.TypeVoid while leaving generic data-type parsing unchanged for parameters and attributes. Tests: added visitor coverage for explicit RETURNS Void; verified mxcli check for the doctype fixture and the targeted integration subtest that failed in GitHub Actions.
AI Code ReviewLet me review this pull request based on the checklist provided in the CLAUDE.md file. First, let's understand what the PR is about:
Now I'll go through the checklist systematically: Overlap & duplication
MDL syntax design (for new or modified MDL syntax)The PR modifies the MDL grammar for validation feedback statements:
Let me check if this follows the design guidelines:
The grammar changes seem reasonable:
This addresses the issue of preserving qualified association targets as a single path segment. Full-stack consistency (for MDL features)The PR modifies:
Let me check if there are missing pieces:
Actually, looking more carefully at the grammar change: This means the visitor would need to handle both In ANTLR, when you have a rule like Let me look for AST definitions related to validation feedback. I don't see explicit AST changes in the diff, but the existing AST likely has a way to represent both attribute paths and bare variables. Looking at the executor changes in fb.addValidationFeedbackAction(&ast.ValidationFeedbackStmt{
AttributePath: &ast.AttributePathExpr{
Variable: "FormObject",
Segments: []ast.PathSegment{
{Name: "Synthetic.FormObject_Target", Separator: "/"},
},
},
// ...
})And in the formatter: } else if a.AssociationName != "" {
attrPath = varName + "/" + a.AssociationName
}This suggests the AST Let me check if there are AST changes not shown in the truncated diff... Actually, the diff shows changes to many files, but I don't see However, looking at what's shown:
The visitor isn't explicitly shown, but it's likely that the existing visitor can handle the grammar change because:
Actually, looking at the executor test: fb.addValidationFeedbackAction(&ast.ValidationFeedbackStmt{
AttributePath: &ast.AttributePathExpr{
Variable: "FormObject",
Segments: []ast.PathSegment{
{Name: "Synthetic.FormObject_Target", Separator: "/"},
},
},
// ...
})And for object-only: action := µflows.ValidationFeedbackAction{
ObjectVariable: "Customer",
// ...
}This suggests that when The grammar change to This ensures that
Whereas before it might have been split as:
But with the change to require Actually, looking more closely at the grammar change comment: This confirms the intent is to keep Now, for the object-only case ( So In the executor, when processing this, they check: if len(s.AttributePath.Segments) == 0 {
// Handle object-only case
}This makes sense. Test coverage
Looking at the watcher test change: // Rapidly write 5 times — should debounce into a single message.
+ // Keep the burst tighter than the debounce window so slow CI machines do
+ // not accidentally let an intermediate timer fire.
for i := range 5 {
_ = os.WriteFile(unitFile, []byte{byte('a' + i)}, 0644)
- time.Sleep(50 * time.Millisecond)
}They REMOVED a time.Sleep, which is good! This addresses the checklist item about not relying on time.Sleep for synchronization. Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
Let me double-check the MDL syntax design aspects since that's crucial: The PR modifies the validation feedback statement to accept:
This follows the design guidelines:
Let me check if there are any issues with the AS keyword usage: One potential concern: In the validation feedback statement, we now have: Is there any risk of confusion with AS? For example, could someone mistakenly write: But looking at the grammar, it's: So after the VARIABLE/attributePath, it expects MESSAGE, not AS. So AS wouldn't be valid in this position, which is correct. Actually, let me check if AS is allowed anywhere in validation feedback. Looking at the full rule from the diff: So the structure is: AS is not part of this statement, which is correct. Now, let me verify the specific changes against the issue description: Issue: "Fixes validation feedback round-trips for targets that are not simple attributes." Changes:
All three points are addressed. Let me check if the changes are minimal and focused:
This seems appropriately scoped. One thing I should check: Does the PR maintain backward compatibility?
Let me verify what qualifiedName means in this grammar. Since I don't have the full grammar, I'll assume that qualifiedName is defined to include dotted names, so a simple identifier is still a qualifiedName (just one with zero dots). The comment explains it: "// The segment is a qualifiedName so This suggests that qualifiedName can contain dots, so
Whereas before the change, if the grammar was Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
3 participants
Summary
Fixes validation feedback round-trips for targets that are not simple attributes.
Changes
$Objectin MDL.$Object/Module.Object_Associationas a single path segment.AssociationNamewhen a validation feedback action does not have anAttributeName.Validation
go test ./mdl/executor -run TestFormatAction_ValidationFeedbackgo test ./mdl/visitor -run TestValidationFeedbackmake buildmake lint-gomake testCloses #358
Part of #332