Skip to content

feat(mdl): DROP <document> IF EXISTS (#1190) - #1192

Closed
tgolembiewski wants to merge 1 commit into
mendixlabs:mainfrom
tgolembiewski:feature/1190-drop-if-exists
Closed

tgolembiewski wants to merge 1 commit into
mendixlabs:mainfrom
tgolembiewski:feature/1190-drop-if-exists

Conversation

@tgolembiewski

Copy link
Copy Markdown
Contributor

Closes #1190

Opened alongside the issue so the change can be judged concretely. I am happy to rework it or drop it if you would rather go another way.

What

IF EXISTS on the document DROPs that scripts actually use: ENTITY, ASSOCIATION, ENUMERATION, CONSTANT, MICROFLOW, NANOFLOW, PAGE, LAYOUT, SNIPPET, MENU, JAVA ACTION and IMAGE COLLECTION. When the document is missing, exec prints <kind> <name> does not exist, skipping and goes on to the next statement. Without IF EXISTS, behaviour is unchanged.

  • Grammar: ifExists? after the keyword. It is the same rule DROP USER ROLE and ALTER ENTITY … DROP ATTRIBUTE already use, so there is no new spelling.
  • AST: the twelve statements embed a small DropIfExists struct, which implements MissingSkipper.
  • Executor: one place, Registry.Dispatch, rather than twelve handlers. For a MissingSkipper written with IF EXISTS, a NotFoundError for the named document, or for its module, becomes the notice. Any other error still fails the statement, including a not-found for some other element.

Every one of the twelve handlers already returns mdlerrors.NewNotFound for a missing document, so no handler changed. DROP MENU was added to stmtDropInfo so its name is known; that function is otherwise unchanged.

The other ~30 alternatives in dropStatement (OData, REST, mappings, agents, …) can take the same clause the same way. I left them out to keep this reviewable.

Tests

  • TestDropDocument_IfExistsIsParsed: each of the twelve kinds, with and without IF EXISTS.
  • TestSkipMissingIfAsked: skips the named document or its module; does not skip without IF EXISTS, another document's not-found, a non-not-found error, or success.
  • I disabled the dispatch skip and the visitor flag in turn, and each time its test failed.
  • mdl-examples/bug-tests/drop-document-if-exists.mdl: executed twice against the same 11.12.1 project, with no error on the second run and mx check 0 errors.
  • I ran the whole of push-test.yml locally on macOS. All green.

A script that dropped a document ran once: the second run stopped at the DROP
and skipped every later statement. ENTITY, ASSOCIATION, ENUMERATION, CONSTANT,
MICROFLOW, NANOFLOW, PAGE, LAYOUT, SNIPPET, MENU, JAVA ACTION and IMAGE
COLLECTION now take IF EXISTS, the same ifExists rule DROP USER ROLE uses.
Registry.Dispatch turns a not-found for the named document (or its module)
into "<kind> <name> does not exist, skipping"; any other error still fails.
@github-actions

Copy link
Copy Markdown

AI Code Review

Critical Issues

  • None found

Moderate Issues

  • None found

Minor Issues

  • None found

What Looks Good

  • MDL syntax design: The IF EXISTS clause follows existing patterns (used in DROP USER ROLE and ALTER ENTITY … DROP ATTRIBUTE), reads as English ("DROP ENTITY IF EXISTS Module.Name"), uses qualified names, and maintains consistent CRUD verb usage. The colon/AS distinction is respected (no property definitions or name mappings involved here).
  • Full-stack consistency: The feature is properly wired through the entire pipeline:
    • Grammar: ifExists? added to document DROP rules in MDLParser.g4
    • AST: DropIfExists struct added and embedded in all twelve document DROP statement types
    • Visitor: ExitDropStatement updated to set DropIfExists based on parser context
    • Executor: Centralized skipMissingIfAsked handler in registry.go correctly skips only missing documents/modules for statements with IF EXISTS
    • LSP: No changes needed as feature doesn't add formatting/diagnostics/navigation; grammar update will propagate via regeneration
    • DESCRIBE roundtrip: Verified by re-runnable test script (drop-document-if-exists.mdl) executing twice against same project with mx check passing
  • Test coverage: Comprehensive tests include:
    • Parsing verification for all twelve document types (with/without IF EXISTS)
    • Executor behavior tests (skips only named document/module missing, preserves other errors)
    • Integration test via bug-test script confirming re-runnability
    • No time.Sleep used for synchronization
  • Code quality:
    • Consistent application of existing DropIfExists pattern across all statement types
    • Minimal, focused changes (no unrelated modifications)
    • Proper error handling preserves non-missing errors and non-not-found errors
    • stmtDropInfo updated for DROP MENU to ensure correct messaging
  • Scope & atomicity: PR is strictly focused on adding IF EXISTS to document DROP statements (no extra features or refactors)
  • Documentation:
    • CHANGELOG entry added
    • Syntax page updated with clear explanation and examples
    • Skill file updated to reflect new best practice for layout scripts

Recommendation


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

@tgolembiewski

Copy link
Copy Markdown
Contributor Author

Closing: IF EXISTS on every document-level DROP landed in main through #1193 (ako#531), with the same central NotFoundError skip in Registry.Dispatch and across all 35 kinds rather than the 12 here. Thank you, @ako!

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.

DROP <document> IF EXISTS, so a script that removes a document can run twice

1 participant