Skip to content

Fix silent drops of conditional expressions with keyword-named functions - #108

Merged
ako merged 4 commits into
mainfrom
claude/mxcli-issues-ovfoxk
Aug 7, 2026
Merged

Fix silent drops of conditional expressions with keyword-named functions#108
ako merged 4 commits into
mainfrom
claude/mxcli-issues-ovfoxk

Conversation

@ako

@ako ako commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Summary

This PR fixes three related issues where MDL statements were silently dropped during parsing or writing, leaving no diagnostic trace:

Key Changes

Grammar & Parser (Issue mendixlabs#852)

  • mdl/grammar/domains/MDLPage.g4: Replaced enumerated xpathFunctionName rule with xpathWord (negated token set) plus NOT, allowing any function name including keywords. Added comprehensive documentation explaining why the grammar accepts any name and lets mxbuild adjudicate semantics.
  • mdl/visitor/visitor_conditional_visibility_test.go: Added TestConditionalVisibility_KeywordFunctionNames covering trim(), length(), find(), count(), empty(), and non-keyword functions to prevent regression.

Validation (Issue mendixlabs#852)

  • mdl/executor/validate_widgets.go: Added validateConsumableConditional() function implementing MDL-WIDGET19 rule to detect unparseable conditional expressions that would be silently dropped on write.
  • mdl/executor/validate_widgets_test.go: Added TestValidateStaticWidget_UnconsumableConditional covering both flagged violations and valid static forms.

Page Mutation (Issue mendixlabs#851)

  • mdl/backend/pagemutator/mutator.go: Fixed setWidgetConditionalSettingMut() to write Attribute: "" (empty string) instead of Attribute: nil. The Attribute field is a BY_NAME AttributeIdentifier where unset must be "", not null; a null causes Studio Pro to fail with StorageLoadException.
  • mdl/backend/pagemutator/mutator_test.go: Added TestSetWidgetConditionalSetting_AttributeIsEmptyString with helper functions to verify the BSON structure matches CREATE path behavior.

Microflow Writing (Issue mendixlabs#850)

  • mdl/backend/modelsdk/microflow_write.go: Added missing *microflows.DownloadFileAction case in microflowActionToGen() to write download file actions to BSON instead of falling through to default: return nil.
  • mdl/backend/modelsdk/microflow_downloadfile_test.go: Added TestMicroflowRoundTrip_DownloadFile round-trip test covering both ShowInBrowser variants to catch write-side regressions.

Documentation

  • .claude/skills/mendix/create-page.md: Added examples of keyword-named function calls in conditionals and a comparison table showing the different function sets for widget expressions vs. XPath constraints.
  • mdl-examples/bug-tests/: Added three new test scripts:
    • 850-download-file-action.mdl: Verifies download file actions survive round-trip
    • 851-alter-page-conditional-attribute.mdl: Compares CREATE vs. ALTER conditional settings
    • 852-conditional-keyword-functions.mdl: Comprehensive test of keyword-named functions with runtime verification page

Notable Implementation Details

  • The grammar fix is deliberately permissive: it accepts any function name and lets mxbuild adjudicate semantics via CE0117, because xpathConstraint serves two contexts (widget expressions vs. XPath) with different valid function sets.
  • xpathWord is a negated token set, so it self-maintains as the lexer grows new keywords—an enumerated list would silently reacquire this bug.
  • MDL-WIDGET19 is a general guard for future silent

https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8

claude and others added 4 commits August 7, 2026 16:48
`alter page … set Editable = [expr]` (and `set Visible`) produced a project
Studio Pro refused to open:

    StorageLoadException: Conditional editability settings has an invalid
    value '' for property Attribute

`Attribute` on Forms$Conditional{Visibility,Editability}Settings is a BY_NAME
AttributeIdentifier, so its unset value is the empty string, not null. The
CREATE path already encodes it that way — codec.RegisterTypeDefaults with
EmptyStringFields: {"Attribute"} in mdl/backend/modelsdk/widget_write.go,
whose comment records this exact StorageLoadException from mendixlabs#627. The ALTER
path builds the node by hand in setWidgetConditionalSettingMut and never got
the same treatment, so a widget authored through ALTER was unloadable while
the identical widget authored through CREATE was fine.

Neither `mxcli check` nor `mx check` inspects the stored value, so both
reported success on the broken project.

SourceVariable stays nil: it is a BY_ID reference, where null is the absent
value. "null is wrong here" is per-field, not a blanket rule.

Verified by dumping both encodings (`mxcli bson dump --type page`): a
CREATE-authored and an ALTER-authored widget now produce identical key sets
and values for both settings types.

Fixes mendixlabs#851

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8
…ssions

`visible: [trim($currentObject/Slug) != '']` and `[length(...) > 0]` silently
dropped the entire property. `xpathFunctionName` enumerated only IDENTIFIER,
HYPHENATED_ID, NOT, TRUE, FALSE and CONTAINS, so a call to a function whose
name is also an MDL lexer keyword never matched xpathFunctionCall. The
enclosing `[...]` then failed to parse as an xpathConstraint and matched the
generic property-value alternative instead, so the visitor set `Visible` to an
array rather than `VisibleIf`, and the builder — which reads only a bool or a
string from that slot — never fired.

Nothing reported it: `mxcli check` passed, `mx check` passed, and a dropped
Visible defaults to "always visible", so the only symptom was a widget that
should have been hidden appearing in the running app. toUpperCase/isMatch (plain
IDENTIFIERs) and contains (enumerated) were unaffected, which is what made the
failure look arbitrary.

Define the rule as `xpathWord | NOT` instead. xpathWord is the negated token set
already used for name parts, so it self-maintains as the lexer gains keywords —
an enumerated list would reacquire this bug with the next function name promoted
to a token. This cannot swallow a path: xpathFunctionCall requires a following
LPAREN and no xpathStepValue may be followed by one, so bare `empty` still
parses as a word, which is what keeps `[Name = empty]` working. NOT is spelled
out because xpathWord excludes it.

The grammar deliberately does not enumerate a valid function set, because
xpathConstraint serves two contexts with different ones: `Visible:`/`Editable:`
is a Mendix client expression (trim/length/toUpperCase/find), while a datasource
`where` is real XPath (contains/starts-with/ends-with/string-length/not, where
`length()` means list length, the aggregates are Java-API-only, and empty/NULL
are keywords rather than calls). One rule cannot encode both, so mxbuild
adjudicates — it reports a wrong-context call as CE0117 against the real
version's rules, which no table here could track.

Also add MDL-WIDGET19 as the general guard, per the issue's request that
encoding failures be loud: a `Visible`/`Editable` value that is neither routed
to VisibleIf/EditableIf nor a recognized static form is the residue signature of
any conditional the visitor could not build. It now fails `check` instead of
disappearing on write. Verified by reverting the grammar fix and confirming the
rule fires on the reported MDL.

Verification, on a blank Mendix 11.6.6 app via `mxcli run --local` + Playwright
(the repro script carries the Bug852.Verify page for this; Slug is three spaces
so trim() changes the outcome):

  pre-fix   all 5 markers render — TRIM_HIDDEN and LEN_HIDDEN should not
  post-fix  only the 3 that should be visible render

`mx check` reports 0 errors both ways, so this could only be confirmed in a
browser. XPath regression checked too: `[Name = empty]`, `[Name = NULL]`,
not(), contains(), starts-with() and string-length() all still parse and check
clean.

Behaviour change worth noting: `count(…)` and `empty(…)` are also keyword tokens
and now parse, but they are not client-expression functions, so mxbuild now
reports CE0117 where it previously saw nothing — the property having been
dropped before reaching it. Loud beats silent, but anyone who had written one
will newly see an error.

Fixes mendixlabs#852

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8
`download file $Doc;` was accepted by the grammar, the visitor, the flow
builder and `mxcli check`, and `mxcli exec` reported "Created microflow" — but
microflowActionToGen had no *microflows.DownloadFileAction case, so the action
hit `default: return nil` and the enclosing ActionActivity was serialized with
no Action at all. DESCRIBE then rendered "-- Empty action" and `mx check`
failed with CE0008 "No action defined."

The statement passed every stage that reports something and disappeared at the
one that reports nothing. This is the same silent-drop mechanism as the
microflowObjectToGen default branch in mendixlabs#791.

Add the case, setting FileDocumentVariableName, ShowFileInBrowser and
ErrorHandlingType (Rollback default). The storage key is ShowFileInBrowser, not
ShowInBrowser; the gen setter binds the right one (legacy's
parseDownloadFileAction reads the wrong key — a latent legacy bug the existing
reader test documents).

Tested at the round trip rather than the reader. A reader-only test cannot
catch this class: it starts from BSON the writer never had to produce, which is
why TestActionFromGen_DownloadFile stayed green the whole time. The new test
asserts the round-tripped ActionActivity has a non-nil Action — the CE0008
shape itself — and covers both the plain and `show in browser` forms.

Fixes mendixlabs#850

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013uQvFDd5R4eNqqita59jM8
@ako
ako merged commit b254d1b into main Aug 7, 2026
3 checks passed
ako pushed a commit that referenced this pull request Aug 7, 2026
Brings the branch up to date with main (PRs #107, #108 and the commits
behind them) so PR #109 merges cleanly and CI runs against the current base.

No conflicts. The fix-issue.md symptom table merged via the union driver as
intended — both this branch's rows and main's survive, with no duplicates.
Full suite green on the merged tree.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018hifgRSawfaRWXS44YKtSJ
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.

2 participants