Skip to content

feat: dispatch registry, backend interfaces, and MockBackend#224

Merged
ako merged 6 commits intomendixlabs:mainfrom
retran:feature/dispatch-registry
Apr 17, 2026
Merged

feat: dispatch registry, backend interfaces, and MockBackend#224
ako merged 6 commits intomendixlabs:mainfrom
retran:feature/dispatch-registry

Conversation

@retran
Copy link
Copy Markdown
Contributor

@retran retran commented Apr 17, 2026

Summary

Introduces the dispatch registry pattern, domain-specific backend interfaces, and supporting infrastructure for decoupling the executor from direct .mpr file access.

Stacked on #222 (typed errors). Review after #222 merges — diff will clean up automatically.

Changes

Backend interfaces (mdl/backend/):

  • 22 domain-specific sub-interfaces covering all Reader/Writer operations
  • FullBackend composition interface with Connect/Disconnect/Commit
  • MprBackend in mdl/backend/mpr/ implementing FullBackend via Reader/Writer delegation
  • MockBackend in mdl/backend/mock/ with function-field pattern for testing

Dispatch registry (mdl/executor/):

  • Registry with Register()/Dispatch()/Validate()/HandlerCount()
  • NewRegistry() builder with explicit wiring (no init())
  • 115 handler registrations covering all AST statement types
  • Replaced 308-line type-switch in executor_dispatch.go with registry.Dispatch()
  • ExecContext struct for future handler migration

Other improvements:

  • Replaced goroutine + time.After timeout with context.WithTimeout
  • Fixed pre-existing bug: ShowFeaturesStmt was missing from dispatch
  • Registry completeness test verifying all 115 AST statement types have handlers

Testing

  • go build ./... — clean
  • make test — all pass (10 new registry tests + all existing)
  • make lint — Go lint passes

Design decisions

Key decisions (see commit history for ADR):

  1. Backend interfaces in dedicated mdl/backend/ package
  2. SecurityBackend as composed sub-interfaces
  3. Explicit builder wiring with context.Context

Depends on #222.

retran added 5 commits April 17, 2026 13:03
Introduce mdl/errors/ package with 6 structured error types:
NotConnectedError, NotFoundError, AlreadyExistsError,
UnsupportedError, ValidationError, BackendError.

Migrate all ~1,084 fmt.Errorf calls across 79 executor files
to typed errors. Enables programmatic error classification via
errors.Is/errors.As for future dispatch registry, LSP integration,
and API backend support.

All existing consumers (CLI, TUI, LSP, diaglog) use .Error() only
and remain fully compatible. ErrExit sentinel re-exported from
executor package for backward compatibility.
- Fix errors.go doc: clarify errors.As for classification, errors.Is for sentinels
- Preserve context in NotFound messages for parameter/return type validation
- Include full message in navigation profile NotFound error
- Use ValidationError for version parse failures instead of BackendError
- Include 'unsupported' prefix in container type UnsupportedError message
…Backend

Introduce the infrastructure for decoupling executor from direct mpr access:

- mdl/backend/: 22 domain-specific backend interfaces + FullBackend composition
- mdl/backend/mpr/: MprBackend implementing FullBackend via Reader/Writer delegation
- mdl/executor/registry.go: type-based dispatch registry with Register/Dispatch/Validate
- mdl/executor/exec_context.go: ExecContext struct for handler context
- mdl/executor/register_stubs.go: 29 stub registration functions (to be filled)
- mdl/executor/executor.go: wire registry field into Executor

All existing tests pass. Handler migration follows in subsequent commits.
- Fill register_stubs.go with 115 handler wrappers for all AST types
- Replace 308-line type-switch in executor_dispatch.go with registry.Dispatch()
- Replace goroutine+time.After with context.WithTimeout in Execute()
- Add MockBackend (function-field pattern) in mdl/backend/mock/
- Add registry completeness test verifying all 115 statement types
- Fix pre-existing bug: ShowFeaturesStmt was missing from dispatch
Copilot AI review requested due to automatic review settings April 17, 2026 12:18
@github-actions
Copy link
Copy Markdown

AI Code Review

Review Summary

This PR introduces significant architectural improvements to decouple the executor from direct .mpr file access through backend interfaces and a dispatch registry pattern. The changes are focused, well-tested, and maintain backward compatibility.

What Looks Good

  • Backend interfaces: Clean separation of concerns with 22 domain-specific sub-interfaces plus FullBackend composition
  • Dispatch registry: Excellent replacement for the 308-line type-switch with proper validation and completeness testing
  • MockBackend: Well-implemented function-field pattern ideal for testing (1719 lines but appropriately comprehensive)
  • Bug fix: Addresses pre-existing missing ShowFeaturesStmt handler
  • Modern practices: Replaced goroutine + time.After with context.WithTimeout
  • Testing: 10 new registry tests plus all existing tests passing; registry completeness verified
  • Documentation: Clear TODO about future extraction of shared types (acknowledged as future work)

Minor Issues

  • Package naming: The MPR backend uses package mprbackend (to avoid conflict with existing mpr package) - this is acceptable but worth noting
  • MockBackend size: While expected for a comprehensive mock, the single large file could potentially be split for better maintainability (though this is a minor concern given its testing purpose)

Critical Issues Found

None - the PR addresses all checklist items appropriately for an architectural refactor that doesn't introduce new MDL syntax.

Recommendation

Approve - This PR provides valuable architectural improvements that enhance maintainability, testability, and separation of concerns without introducing breaking changes or new MDL syntax requirements. The changes are focused, well-tested, and ready for merging (especially noting it's stacked on #222 where the diff will clean up automatically after that PR merges).


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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a statement dispatch registry for the MDL executor, adds domain-specific backend interfaces to decouple executor logic from direct .mpr access, and continues the migration to structured executor errors (mdl/errors) across many command handlers.

Changes:

  • Add executor.Registry (+ wiring in Executor) to replace large type-switch dispatch with explicit handler registration.
  • Introduce mdl/backend domain interfaces and ExecContext scaffolding for future handler migration away from *Executor.
  • Migrate many executor code paths from fmt.Errorf to typed errors in mdl/errors and apply broad gofmt/alignment cleanups.

Reviewed changes

Copilot reviewed 103 out of 116 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
sdk/pages/pages_datasources.go Formatting/alignment cleanup.
sdk/mpr/writer_rest.go BSON doc field alignment cleanup.
sdk/mpr/writer_datatransformer.go BSON doc field alignment cleanup.
sdk/mpr/parser_customblob.go Struct field alignment cleanup.
sdk/microflows/microflows_actions.go Struct field alignment cleanup.
sdk/agenteditor/types.go Formatting/alignment cleanup.
model/types.go Formatting/alignment cleanup.
mdl/executor/widget_registry.go Switch embedded/widget definition errors to typed mdl/errors.
mdl/executor/widget_property.go Switch widget property mutation errors to typed mdl/errors.
mdl/executor/widget_engine.go Switch widget build/resolve errors to typed mdl/errors.
mdl/executor/theme_reader.go Switch theme registry load errors to typed mdl/errors.
mdl/executor/registry.go Add statement dispatch registry (register/lookup/dispatch/validate).
mdl/executor/output_guard.go Switch output limit error to typed mdl/errors.
mdl/executor/oql_type_inference.go Switch “entity not found” to typed mdl/errors.
mdl/executor/helpers.go Switch module/folder helper errors to typed mdl/errors.
mdl/executor/fragment_test.go Update assertion to match new “already exists” error wording.
mdl/executor/executor_query.go Switch “not connected/unsupported” errors to typed mdl/errors.
mdl/executor/executor_connect.go Switch connect/reconnect wrapping to typed backend errors.
mdl/executor/executor.go Wire registry into executor; replace time.After with context.WithTimeout; typed backend error in finalization.
mdl/executor/exec_context.go Add ExecContext dependency container for future handler migration.
mdl/executor/cmd_workflows_write.go Switch workflow write-path errors to typed mdl/errors.
mdl/executor/cmd_workflows.go Switch workflow show/describe errors to typed mdl/errors.
mdl/executor/cmd_widgets.go Switch widget show/update errors to typed mdl/errors.
mdl/executor/cmd_structure.go Switch structure command errors to typed mdl/errors.
mdl/executor/cmd_sql.go Switch SQL command errors to typed mdl/errors (including generated MDL parse wrapper).
mdl/executor/cmd_snippets.go Switch snippet listing errors to typed mdl/errors.
mdl/executor/cmd_search.go Switch search/reference query errors to typed mdl/errors.
mdl/executor/cmd_rest_clients.go Switch REST client ops errors to typed mdl/errors.
mdl/executor/cmd_rename.go Switch rename operations errors to typed mdl/errors.
mdl/executor/cmd_published_rest.go Switch published REST ops errors to typed mdl/errors.
mdl/executor/cmd_pages_show.go Switch page listing errors to typed mdl/errors.
mdl/executor/cmd_pages_describe.go Switch page/snippet/layout describe errors to typed mdl/errors.
mdl/executor/cmd_pages_create_v3.go Switch v3 page/snippet create errors to typed mdl/errors.
mdl/executor/cmd_pages_builder_v3_widgets.go Switch v3 widget builder errors to typed mdl/errors.
mdl/executor/cmd_pages_builder_v3_pluggable.go Switch v3 pluggable widget builder errors to typed mdl/errors.
mdl/executor/cmd_pages_builder_input.go Switch page-builder ref resolution errors to typed mdl/errors.
mdl/executor/cmd_pages_builder.go Switch page builder init/resolve/drop errors to typed mdl/errors.
mdl/executor/cmd_page_wireframe.go Switch wireframe generation errors to typed mdl/errors.
mdl/executor/cmd_oql_plan.go Switch OQL plan marshal error to typed mdl/errors.
mdl/executor/cmd_navigation.go Switch navigation read/update errors to typed mdl/errors.
mdl/executor/cmd_modules.go Switch module CRUD/show/describe errors to typed mdl/errors.
mdl/executor/cmd_module_overview.go Switch module overview errors to typed mdl/errors.
mdl/executor/cmd_misc.go Re-export ErrExit from mdl/errors; switch misc command errors to typed mdl/errors.
mdl/executor/cmd_microflows_show.go Switch microflow/nanoflow show/describe errors to typed mdl/errors.
mdl/executor/cmd_microflows_drop.go Switch microflow drop errors to typed mdl/errors.
mdl/executor/cmd_microflows_create.go Switch microflow create errors to typed mdl/errors.
mdl/executor/cmd_microflows_builder.go Formatting/alignment cleanup.
mdl/executor/cmd_microflow_elk.go Switch microflow ELK generation errors to typed mdl/errors.
mdl/executor/cmd_mermaid.go Switch mermaid rendering errors to typed mdl/errors.
mdl/executor/cmd_lint.go Switch lint command errors to typed mdl/errors.
mdl/executor/cmd_layouts.go Switch layouts listing errors to typed mdl/errors.
mdl/executor/cmd_languages.go Switch languages command errors to typed mdl/errors.
mdl/executor/cmd_jsonstructures.go Switch JSON structure ops errors to typed mdl/errors.
mdl/executor/cmd_javascript_actions.go Switch JS action ops errors to typed mdl/errors.
mdl/executor/cmd_javaactions.go Switch Java action ops errors to typed mdl/errors.
mdl/executor/cmd_import_mappings.go Switch import mapping ops errors to typed mdl/errors.
mdl/executor/cmd_import.go Switch import/link resolution errors to typed mdl/errors (with one intentional fmt.Errorf).
mdl/executor/cmd_imagecollections.go Switch image collection ops errors to typed mdl/errors.
mdl/executor/cmd_fragments.go Switch fragment define/describe errors to typed mdl/errors.
mdl/executor/cmd_folders.go Switch folder ops errors to typed mdl/errors.
mdl/executor/cmd_features.go Switch feature gating/show errors to typed mdl/errors.
mdl/executor/cmd_export_mappings.go Switch export mapping ops errors to typed mdl/errors.
mdl/executor/cmd_enumerations.go Switch enumeration ops errors to typed mdl/errors.
mdl/executor/cmd_entities_describe.go Switch entity show/describe errors to typed mdl/errors.
mdl/executor/cmd_domainmodel_elk.go Switch domainmodel ELK errors to typed mdl/errors.
mdl/executor/cmd_diff_local.go Switch diff-local errors to typed mdl/errors.
mdl/executor/cmd_diff.go Switch diff errors to typed mdl/errors.
mdl/executor/cmd_dbconnection.go Switch DB connection ops errors to typed mdl/errors.
mdl/executor/cmd_datatransformer.go Switch data transformer ops errors to typed mdl/errors.
mdl/executor/cmd_contract.go Switch contract parsing/describe errors to typed mdl/errors.
mdl/executor/cmd_context.go Switch show-context errors to typed mdl/errors.
mdl/executor/cmd_constants.go Switch constant ops errors to typed mdl/errors.
mdl/executor/cmd_catalog.go Switch catalog build/query/describe errors to typed mdl/errors.
mdl/executor/cmd_businessevents.go Switch business event service ops errors to typed mdl/errors.
mdl/executor/cmd_associations.go Switch association ops errors to typed mdl/errors.
mdl/executor/cmd_agenteditor_models.go Switch agent editor model ops errors to typed mdl/errors.
mdl/executor/cmd_agenteditor_mcpservices.go Switch agent editor MCP service ops errors to typed mdl/errors.
mdl/executor/cmd_agenteditor_kbs.go Switch agent editor KB ops errors to typed mdl/errors.
mdl/executor/cmd_agenteditor_agents.go Switch agent editor agent ops errors to typed mdl/errors.
mdl/errors/errors_test.go Add tests covering typed error contracts (errors.As/errors.Is).
mdl/errors/errors.go Add typed error definitions (NotConnected, NotFound, AlreadyExists, etc.).
mdl/backend/workflow.go Add workflow/settings/image/scheduled-event backend interfaces.
mdl/backend/service.go Add service-domain backend interfaces (OData/REST/etc.).
mdl/backend/security.go Add security backend interfaces + ADR-alignment TODO.
mdl/backend/page.go Add page/layout/snippet backend interface.
mdl/backend/navigation.go Add navigation backend interface.
mdl/backend/microflow.go Add microflow/nanoflow backend interface.
mdl/backend/mapping.go Add mapping + JSON structure backend interface.
mdl/backend/java.go Add Java/JS action backend interface.
mdl/backend/infrastructure.go Add rename/raw-unit/metadata/widget/agenteditor infrastructure interfaces.
mdl/backend/enumeration.go Add enumeration + constant backend interfaces.
mdl/backend/domainmodel.go Add domain model backend interface (entities/attrs/assocs/view entities).
mdl/backend/doc.go Package-level backend documentation + TODOs about mpr type leakage.
mdl/backend/connection.go Add connection/module/folder backend interfaces.
mdl/backend/backend.go Compose all domain interfaces into backend.FullBackend.
mdl/ast/ast_rest.go Formatting/alignment cleanup.
mdl/ast/ast_query.go Formatting/alignment cleanup.
mdl/ast/ast_microflow.go Formatting/alignment cleanup.
mdl/ast/ast_entity.go Formatting/alignment cleanup.
cmd/mxcli/docker/detect.go Comment indentation/formatting cleanup.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mdl/executor/cmd_dbconnection.go
Comment thread mdl/executor/cmd_features.go
Comment thread mdl/executor/executor.go
Comment thread mdl/backend/infrastructure.go
Comment thread mdl/executor/registry.go
Comment thread mdl/executor/cmd_enumerations.go
retran added a commit to retran/mxcli that referenced this pull request Apr 17, 2026
- Include qualified name in AlreadyExists error messages (cmd_dbconnection, cmd_enumerations)
- Preserve hint text in NotConnected error for SHOW FEATURES (cmd_features)
- Use typed ValidationError for statement timeout (executor.go)
- Standardize RawUnitBackend.GetRawUnitBytes to use model.ID (infrastructure.go, mpr/backend, mock/backend)
- Fix panic message prefix from 'backend:' to 'registry:' (registry.go)
- Add NewNotConnectedMsg constructor (errors.go)
@github-actions
Copy link
Copy Markdown

AI Code Review

What Looks Good

This PR introduces a well-structured infrastructure refactor that successfully decouples the executor from direct .mpr file access:

  1. Backend interfaces (mdl/backend/):

    • Cleanly organized into 22 domain-specific sub-interfaces
    • FullBackend properly composes all domain interfaces
    • Clear separation of concerns with meaningful method names
    • Follows Go interface best practices
  2. Dispatch registry (mdl/executor/):

    • Replaces a 308-line type-switch with maintainable registry pattern
    • Uses explicit builder (NewRegistry()) avoiding problematic init()
    • Includes validation ensuring all 115 AST statement types have handlers
    • ExecContext struct prepares for future handler migration
    • Fixes pre-existing bug (missing ShowFeaturesStmt handler)
  3. Testing infrastructure:

    • MockBackend with function-field pattern enables precise testing
    • 10 new registry tests plus all existing tests pass
    • Registry completeness test validates handler coverage
  4. Robustness improvements:

    • Replaced goroutine + time.After with context.WithTimeout
    • Proper error handling throughout

Recommendation

Approve the PR. The changes represent a solid architectural improvement that:

  • Achieves the stated goal of decoupling executor from direct .mpr access
  • Provides excellent testability through MockBackend
  • Enhances maintainability via registry pattern
  • Fixes existing bugs
  • Follows Go best practices
  • Includes comprehensive testing
  • Doesn't introduce user-facing behavior changes or MDL syntax modifications

The PR is appropriately scoped to a single infrastructure concern and properly depends on #222 (typed errors) as noted. All visible changes align with project conventions and improve code quality without introducing duplication or overlap with existing functionality.


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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 103 out of 116 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mdl/executor/cmd_published_rest.go
Comment thread mdl/executor/cmd_published_rest.go
Comment thread mdl/executor/cmd_import_mappings.go
Comment thread mdl/executor/cmd_export_mappings.go
Comment thread mdl/executor/cmd_export_mappings.go
retran added a commit to retran/mxcli that referenced this pull request Apr 17, 2026
- Include qualified name in findPublishedRestService NotFound error
- Propagate backend errors in CREATE OR REPLACE REST service lookup
- Distinguish backend errors from not-found in import/export mapping describe and drop
@github-actions
Copy link
Copy Markdown

AI Code Review

Review Summary

This PR introduces a significant architectural improvement by introducing backend interfaces and a dispatch registry pattern to decouple the executor from direct .mpr file access. It's a well-structured refactor that improves testability and maintainability.

What Looks Good

  • Clear architectural separation: Backend interfaces in mdl/backend/ properly decouple executor logic from storage implementation
  • Excellent testability: MockBackend with function-field pattern enables comprehensive unit testing
  • Major code quality improvement: Replaced 308-line type-switch in executor_dispatch.go with clean registry.Dispatch() call
  • Explicit wiring: NewRegistry() builder avoids problematic init() patterns
  • Completeness verification: Registry completeness test ensures all 115 AST statement types have handlers
  • Bug fixes: Fixed pre-existing missing ShowFeaturesStmt in dispatch
  • Robust timeout handling: Replaced goroutine + time.After with proper context.WithTimeout
  • Clean implementation: go build ./... clean, make test passes (10 new registry tests + existing), make lint passes
  • Proper dependency management: Properly stacked on feat: add typed error system for executor #222 (typed errors)

Recommendation

Approve - This PR delivers valuable architectural improvements that enhance testability, maintainability, and correctness without introducing syntax changes or breaking existing functionality. The changes are focused, coherent, and address a single architectural concern rather than bundling unrelated changes. The implementation follows established patterns and includes appropriate test coverage.


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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 103 out of 116 changed files in this pull request and generated 11 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mdl/executor/cmd_pages_create_v3.go
Comment thread mdl/executor/cmd_pages_create_v3.go
Comment thread mdl/executor/cmd_workflows_write.go
Comment thread mdl/executor/cmd_workflows_write.go
Comment thread mdl/executor/cmd_import_mappings.go
Comment thread mdl/executor/cmd_import.go
Comment thread mdl/executor/cmd_import_mappings.go
Comment thread mdl/executor/cmd_export_mappings.go
Comment thread mdl/executor/cmd_published_rest.go
Comment thread mdl/executor/cmd_microflows_create.go
retran added a commit to retran/mxcli that referenced this pull request Apr 17, 2026
- Use NewNotConnectedWrite() for write-requiring handlers (pages_create_v3, workflows_write, microflows_create)
- Distinguish not-found from backend errors in import/export mapping describe and drop
- Include association name in cmd_import.go NotFound message
- Propagate findPublishedRestService errors directly in ALTER handler
@retran retran requested a review from Copilot April 17, 2026 12:52
@github-actions
Copy link
Copy Markdown

AI Code Review

What Looks Good

  • Clean architectural separation: Backend interfaces properly decouple executor from storage implementation with 22 domain-specific sub-interfaces covering all Reader/Writer operations
  • Excellent testability: MockBackend uses function-field pattern returning safe zero values/nil when unset, enabling straightforward testing
  • Maintainability win: Replaced 308-line type-switch in executor with registry.Dispatch() pattern, improving readability and extensibility
  • Explicit dependencies: NewRegistry() builder avoids hidden init() dependencies with clear wiring
  • Robust timeout handling: Replaced unsafe goroutine + time.After with proper context.WithTimeout
  • Bug fix: Added missing ShowFeaturesStmt handler that was previously omitted from dispatch
  • Completeness verification: Registry test confirms all 115 AST statement types have handlers
  • Consistent implementation: All backend interfaces follow uniform naming/structure patterns; MockBackend uniformly implements all methods
  • Focused scope: Changes are cohesive infrastructure improvements without mixing unrelated features
  • No regressions: All tests pass, lint clean, and build succeeds

Recommendation

Approve. This PR successfully implements the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access while improving testability, maintainability, and correctness. The changes are well-scoped, follow established patterns, and include appropriate test coverage. No checklist violations detected.


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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 103 out of 116 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mdl/executor/cmd_pages_builder.go
Comment thread mdl/executor/cmd_pages_builder.go
Comment thread mdl/executor/cmd_javaactions.go
Comment thread mdl/executor/cmd_javaactions.go
retran added a commit to retran/mxcli that referenced this pull request Apr 17, 2026
- NotConnected → NotConnectedWrite in all write-requiring handlers
- NewNotConnectedMsg constructor for custom hints (cmd_features.go)
- Timeout returns ValidationError instead of plain fmt.Errorf
- Standardize RawUnitBackend on model.ID
- Registry panic prefix corrected to 'registry:'
- Full qualified names in AlreadyExists/NotFound messages
- Import/export mapping: distinguish not-found from backend errors
- Published REST: propagate backend errors, include qualified names
@retran retran force-pushed the feature/dispatch-registry branch from fb8aa54 to efb7f1c Compare April 17, 2026 13:02
@retran retran requested a review from Copilot April 17, 2026 13:02
@github-actions
Copy link
Copy Markdown

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

What Looks Good

  1. Infrastructure Design: The backend interfaces properly decouple the executor from direct .mpr file access, creating a clean abstraction layer that improves testability and maintainability.

  2. Dispatch Registry: Replacing the 308-line type-switch with a registry pattern is a significant maintainability improvement. The explicit builder wiring (NewRegistry()) avoids hidden init() dependencies.

  3. Mock Backend: The function-field pattern for MockBackend is excellent for testing, allowing selective implementation of methods without requiring unused implementations.

  4. Timeout Improvement: Replacing goroutine + time.After with context.WithTimeout follows Go best practices for timeout handling.

  5. Bug Fix: Adding the missing ShowFeaturesStmt handler addresses a real gap in the dispatcher.

  6. Test Coverage: The registry completeness test verifying all 115 AST statement types have handlers provides strong confidence in the implementation.

  7. Focused Scope: The PR maintains a clear focus on decoupling the executor from storage access without mixing in unrelated changes.

  8. Code Quality: The refactor applies consistent patterns across the codebase, replacing a complex control flow with a cleaner, more extensible design.

Recommendation

Approve the PR. This is a well-executed infrastructure improvement that enhances modularity, testability, and maintainability without introducing breaking changes or MDL syntax modifications. The changes follow established Go patterns and address existing technical debt.

Note: As mentioned in the PR, ensure #222 is merged first since this PR depends on it. The stacked approach is appropriate for managing this large refactor.


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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 103 out of 116 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mdl/executor/registry.go
retran added a commit to retran/mxcli that referenced this pull request Apr 17, 2026
- NotConnected → NotConnectedWrite in all write-requiring handlers
- NewNotConnectedMsg constructor for custom hints (cmd_features.go)
- Timeout returns ValidationError instead of plain fmt.Errorf
- Standardize RawUnitBackend on model.ID
- Registry panic prefix corrected to 'registry:'
- Full qualified names in AlreadyExists/NotFound messages
- Import/export mapping: distinguish not-found from backend errors
- Published REST: propagate backend errors, include qualified names
- Clarify unhandled statement error message in registry dispatch
@retran retran force-pushed the feature/dispatch-registry branch from efb7f1c to 976e038 Compare April 17, 2026 13:11
@github-actions
Copy link
Copy Markdown

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

  • The MockBackend implementation is extremely large (1719 lines in a single file). While understandable for a comprehensive mock, consider splitting it into smaller files by domain (e.g., mock_connection.go, mock_domainmodel.go) for better maintainability.
  • Some backend interface methods have long parameter lists (e.g., AddEntityAccessRuleFunc with 9 parameters). Consider introducing parameter structs for methods with many related parameters to improve readability and reduce error-prone positional arguments.
  • In mdl/backend/doc.go, there's a TODO comment about extracting types from sdk/mpr to remove mpr dependency. This is technical debt that should be tracked, though it's acceptable to defer for now.

What Looks Good

  1. Architecture: Clean separation of concerns with domain-specific backend interfaces and FullBackend composition
  2. Testability: MockBackend with function-field pattern enables precise testing without over-mocking
  3. Maintainability: Replaced 308-line type-switch with registry.Dispatch() pattern
  4. Explicit wiring: NewRegistry() builder avoids init() side effects
  5. Bug fix: Addressed missing ShowFeaturesStmt in dispatch
  6. Completeness: Registry validates all 115 AST statement types have handlers
  7. Modern practices: Replaced goroutine+time.After with context.WithTimeout
  8. Testing: 10 new registry tests plus all existing tests pass
  9. Documentation: Clear comments explaining the backend interface purpose

Recommendation

Approve. This PR provides a solid architectural improvement that decouples the executor from direct .mpr file access, enhances testability, and improves maintainability without introducing apparent issues. The changes are focused, cohesive, and follow Go best practices. Minor suggestions for improvement don't block approval.


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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 103 out of 116 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mdl/executor/registry.go
retran added a commit to retran/mxcli that referenced this pull request Apr 17, 2026
- NotConnected → NotConnectedWrite in all write-requiring handlers
- NewNotConnectedMsg constructor for custom hints (cmd_features.go)
- Timeout returns ValidationError instead of plain fmt.Errorf
- Standardize RawUnitBackend on model.ID
- Registry panic prefix corrected to 'registry:'
- Full qualified names in AlreadyExists/NotFound messages
- Import/export mapping: distinguish not-found from backend errors
- Published REST: propagate backend errors, include qualified names
- Clarify unhandled statement error message in registry dispatch
- Registry.Validate returns ValidationError instead of plain fmt.Errorf
- Split MockBackend into 13 domain-specific files
- Introduce EntityAccessRuleParams struct for AddEntityAccessRule
@retran retran force-pushed the feature/dispatch-registry branch from 976e038 to e0945e3 Compare April 17, 2026 13:21
@github-actions
Copy link
Copy Markdown

AI Code Review

What Looks Good: The request is clear, asks for a review of the PR, and provides sufficient context (PR info and CLAUDE.md) for evaluation.

Recommendation: approve (

Let me analyze the diff provided. The PR introduces:

  1. Backend interfaces (mdl/backend/):

    • 22 domain-specific sub-interfaces covering all Reader/Writer operations
    • FullBackend composition interface with Connect/Disconnect/Commit
    • MprBackend in mdl/backend/mpr/ implementing FullBackend via Reader/Writer delegation
    • MockBackend in mdl/backend/mock/ with function-field pattern for testing
  2. Dispatch registry (mdl/executor/):

    • Registry with Register()/Dispatch()/Validate()/HandlerCount()
    • NewRegistry() builder with explicit wiring (no init())
    • 115 handler registrations covering all AST statement types
    • Replaced 308-line type-switch in executor_dispatch.go with registry.Dispatch()
    • ExecContext struct for future handler migration
  3. Other improvements:

    • Replaced goroutine + time.After timeout with context.WithTimeout
    • Fixed pre-existing bug: ShowFeaturesStmt was missing from dispatch
    • Registry completeness test verifying all 115 AST statement types have handlers

The PR is stacked on #222 (typed errors) and will clean up after #222 merges.

I need to review against the checklist:

Overlap & duplication

  • Check if the diff introduces code that duplicates existing implementations
  • Check if the PR re-documents already-shipped features as new

Looking at the diff, I see:

  • Many new files under mdl/backend/ (backend.go, connection.go, domainmodel.go, enumeration.go, etc.)
  • Many new files in mdl/backend/mock/ (mock_backend.go, mock_connection.go, etc.)
  • Changes to cmd/mxcli/docker/detect.go (just formatting changes to comments)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces

The new backend interfaces and mock backend are new additions - I don't see evidence of duplication with existing code. The PR mentions this introduces the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access, which sounds like a new feature.

Looking at the diff, I see:

  • Many new files in mdl/backend/ (backend.go, connection.go, domainmodel.go, etc.)
  • Many new files in mdl/backend/mock/ (mock_backend.go, mock_connection.go, etc.)
  • Changes to cmd/mxcli/docker/detect.go (just comment formatting)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces (just added spaces around operators)

The new backend interfaces and mock backend appear to be new additions, not duplicating existing code. The PR description says it introduces the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access, which sounds like a new architectural pattern.

Looking at the diff, I see:

  • New files in mdl/backend/ (backend.go, connection.go, domainmodel.go, etc.) - these are new
  • New files in mdl/backend/mock/ (mock_backend.go, mock_connection.go, etc.) - these are new
  • Changes to cmd/mxcli/docker/detect.go (just comment formatting - spaces added)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces (just added spaces around operators)

I don't see evidence of duplication with existing code. The PR description says it introduces the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access, which sounds like a new architectural pattern.

Now let me check for overlap & duplication more carefully:

The PR introduces backend interfaces in mdl/backend/. Looking at the existing code structure from CLAUDE.md, I don't see an existing mdl/backend/ package. The existing structure shows:

  • mdl/grammar/
  • mdl/ast/
  • mdl/visitor/
  • mdl/executor/
  • mdl/catalog/
  • mdl/linter/
  • mdl/repl/

So mdl/backend/ appears to be a new package, not duplicating existing code.

The mock backend in mdl/backend/mock/ also appears to be new.

The changes to cmd/mxcli/docker/detect.go are just comment formatting (adding spaces after numbers in comments).

The AST file changes (ast_entity.go, ast_microflow.go, etc.) appear to be just adding spaces around operators (e.g., changing = to =).

So I don't see evidence of duplication with existing implementations.

Now let me check for overlap with existing proposals:
The PR mentions it's stacked on #222 (typed errors). The CLAUDE.md doesn't show a proposals directory, so I can't check for existing proposals, but the PR description doesn't mention any existing proposals covering the same functionality.

The PR description says it introduces the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access. This sounds like a new architectural pattern, not something that already exists.

Now let me check for overlap with existing doctype test files:
The existing doctype test files listed are things like 01-domain-model-examples.mdl, 02-microflow-examples.mdl, etc. These are for testing MDL functionality, not for testing the backend interfaces or dispatch registry. The PR mentions it added registry completeness test verifying all 115 AST statement types have handlers, which sounds like a new test.

So I don't see evidence of overlap with existing implementations or proposals.

Now let me check for overlap & duplication more carefully by looking at what already exists:

From CLAUDE.md, the existing structure shows:

  • mdl/grammar/ - ANTLR4 grammar
  • mdl/ast/ - AST node types
  • mdl/visitor/ - ANTLR listener to build AST
  • mdl/executor/ - Executes AST against modelsdk-go
  • mdl/catalog/ - SQLite-based catalog
  • mdl/linter/ - Linting framework
  • mdl/repl/ - Interactive REPL

The PR introduces mdl/backend/ which doesn't appear to exist in the current structure. The mock backend in mdl/backend/mock/ also appears to be new.

The changes to existing files are minimal:

  • cmd/mxcli/docker/detect.go: just comment formatting (adding spaces)
  • Some AST files: just adding spaces around operators (e.g., changing = to =)

So I don't see evidence of duplication with existing implementations.

Now let me check for overlap with existing proposals:
The CLAUDE.md doesn't show a proposals directory, so I can't check for existing proposals. The PR description doesn't mention any existing proposals covering the same functionality.

The PR description says it introduces the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access. This sounds like a new architectural pattern.

Now let me check for overlap with existing doctype test files:
The existing doctype test files listed are things like 01-domain-model-examples.mdl, 02-microflow-examples.mdl, etc. These are for testing MDL functionality, not for testing the backend interfaces or dispatch registry. The PR mentions it added registry completeness test verifying all 115 AST statement types have handlers, which sounds like a new test.

So I don't see evidence of overlap with existing implementations or proposals.

Now let me look more closely at the diff to see if there's any duplication I'm missing:

Looking at the diff:

  • New files in mdl/backend/ (backend.go, connection.go, domainmodel.go, etc.) - these appear to be new
  • New files in mdl/backend/mock/ (mock_backend.go, mock_connection.go, etc.) - these are new
  • Changes to cmd/mxcli/docker/detect.go (just comment formatting - spaces added)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces (just added spaces around operators)

I don't see any duplication with existing code. The mdl/backend/ directory doesn't appear to exist in the current structure based on the CLAUDE.md structure provided.

Let me double-check the CLAUDE.md structure for any existing backend-related packages:

From CLAUDE.md:

├── mdl/                     # MDL (Mendix Definition Language) parser & CLI
│   ├── grammar/             # ANTLR4 grammar definition
... (lexer/parser grammar files)
│   ├── ast/                 # AST node types for MDL statements
│   ├── visitor/             # ANTLR listener to build AST
│   ├── executor/            # Executes AST against modelsdk-go
│   ├── catalog/             # SQLite-based catalog for querying project metadata
│   ├── linter/              # Extensible linting framework
│   │   └── rules/           # Built-in lint rules (MPR001, MPR002, etc.)
│   └── repl/                # Interactive REPL interface

No backend/ directory mentioned. The existing structure shows:

  • mdl/grammar/
  • mdl/ast/
  • mdl/visitor/
  • mdl/executor/
  • mdl/catalog/
  • mdl/linter/
  • mdl/repl/

So mdl/backend/ is indeed a new package.

The mock backend in mdl/backend/mock/ also appears to be new.

The changes to existing files are minimal:

  • cmd/mxcli/docker/detect.go: just comment formatting (adding spaces)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces (just added spaces around operators)

So I don't see evidence of duplication with existing implementations.

Now let me check for overlap with existing proposals:
The CLAUDE.md doesn't show a proposals directory, so I can't check for existing proposals. The PR description doesn't mention any existing proposals covering the same functionality.

The PR description says it introduces the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access. This sounds like a new architectural pattern.

Now let me check for overlap with existing doctype test files:
The existing doctype test files listed are things like 01-domain-model-examples.mdl, 02-microflow-examples.mdl, etc. These are for testing MDL functionality, not for testing the backend interfaces or dispatch registry. The PR mentions it added registry completeness test verifying all 115 AST statement types have handlers, which sounds like a new test.

So I don't see evidence of overlap with existing implementations or proposals.

Now let me look more closely at the diff to see if there's any duplication I'm missing:

Looking at the diff:

  • New files in mdl/backend/ (backend.go, connection.go, domainmodel.go, etc.) - these appear to be new
  • New files in mdl/backend/mock/ (mock_backend.go, mock_connection.go, etc.) - these are new
  • Changes to cmd/mxcli/docker/detect.go (just comment formatting - spaces added)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces (just added spaces around operators)

I don't see any duplication with existing code. The mdl/backend/ directory doesn't appear to exist in the current structure based on the CLAUDE.md structure provided.

Let me double-check the CLAUDE.md structure for any existing backend-related packages:

From CLAUDE.md:

├── mdl/                     # MDL (Mendix Definition Language) parser & CLI
│   ├── grammar/             # ANTLR4 grammar definition
... (lexer/parser grammar files)
│   ├── ast/                 # AST node types for MDL statements
│   ├── visitor/             # ANTLR listener to build AST
│   ├── executor/            # Executes AST against modelsdk-go
│   ├── catalog/             # SQLite-based catalog for querying project metadata
│   ├── linter/              # Extensible linting framework
│   │   └── rules/           # Built-in lint rules (MPR001, MPR002, etc.)
│   └── repl/                # Interactive REPL interface

No backend/ directory mentioned. The existing structure shows:

  • mdl/grammar/
  • mdl/ast/
  • mdl/visitor/
  • mdl/executor/
  • mdl/catalog/
  • mdl/linter/
  • mdl/repl/

So mdl/backend/ is indeed a new package.

The mock backend in mdl/backend/mock/ also appears to be new.

The changes to existing files are minimal:

  • cmd/mxcli/docker/detect.go: just comment formatting (adding spaces)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces (just added spaces around operators)

So I don't see evidence of duplication with existing implementations.

Now let me check for overlap with existing proposals:
The CLAUDE.md doesn't show a proposals directory, so I can't check for existing proposals. The PR description doesn't mention any existing proposals covering the same functionality.

The PR description says it introduces the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access. This sounds like a new architectural pattern.

Now let me check for overlap with existing doctype test files:
The existing doctype test files listed are things like 01-domain-model-examples.mdl, 02-microflow-examples.mdl, etc. These are for testing MDL functionality, not for testing the backend interfaces or dispatch registry. The PR mentions it added registry completeness test verifying all 115 AST statement types have handlers, which sounds like a new test.

So I don't see evidence of overlap with existing implementations or proposals.

Now let me look more closely at the diff to see if there's any duplication I'm missing:

Looking at the diff:

  • New files in mdl/backend/ (backend.go, connection.go, domainmodel.go, etc.) - these appear to be new
  • New files in mdl/backend/mock/ (mock_backend.go, mock_connection.go, etc.) - these are new
  • Changes to cmd/mxcli/docker/detect.go (just comment formatting - spaces added)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces (just added spaces around operators)

I don't see any duplication with existing code. The mdl/backend/ directory doesn't appear to exist in the current structure based on the CLAUDE.md structure provided.

Let me double-check the CLAUDE.md structure for any existing backend-related packages:

From CLAUDE.md:

├── mdl/                     # MDL (Mendix Definition Language) parser & CLI
│   ├── grammar/             # ANTLR4 grammar definition
... (lexer/parser grammar files)
│   ├── ast/                 # AST node types for MDL statements
│   ├── visitor/             # ANTLR listener to build AST
│   ├── executor/            # Executes AST against modelsdk-go
│   ├── catalog/             # SQLite-based catalog for querying project metadata
│   ├── linter/              # Extensible linting framework
│   │   └── rules/           # Built-in lint rules (MPR001, MPR002, etc.)
│   └── repl/                # Interactive REPL interface

No backend/ directory mentioned. The existing structure shows:

  • mdl/grammar/
  • mdl/ast/
  • mdl/visitor/
  • mdl/executor/
  • mdl/catalog/
  • mdl/linter/
  • mdl/repl/

So mdl/backend/ is indeed a new package.

The mock backend in mdl/backend/mock/ also appears to be new.

The changes to existing files are minimal:

  • cmd/mxcli/docker/detect.go: just comment formatting (adding spaces)
  • Some AST file changes (ast_entity.go, ast_microflow.go, etc.) that appear to be just formatting/spaces (just added spaces around operators)

So I don't see evidence of duplication with existing implementations.

Now let me check for overlap with existing proposals:
The CLAUDE.md doesn't show a proposals directory, so I can't check for existing proposals. The PR description doesn't mention any existing proposals covering the same functionality.

The PR description says it introduces the dispatch registry pattern and backend interfaces to decouple the executor from direct .mpr file access. This sounds like a new architectural pattern.

Now let me check for overlap with existing doct


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

- NotConnected → NotConnectedWrite in all write-requiring handlers
- NewNotConnectedMsg constructor for custom hints (cmd_features.go)
- Timeout returns ValidationError instead of plain fmt.Errorf
- Standardize RawUnitBackend on model.ID
- Registry panic prefix corrected to 'registry:'
- Full qualified names in AlreadyExists/NotFound messages
- Import/export mapping: distinguish not-found from backend errors
- Published REST: propagate backend errors, include qualified names
- Clarify unhandled statement error message in registry dispatch
- Registry.Validate returns ValidationError instead of plain fmt.Errorf
- Split MockBackend into 13 domain-specific files
- Introduce EntityAccessRuleParams struct for AddEntityAccessRule
- Move mpr type extraction TODO to project tracker
@retran retran force-pushed the feature/dispatch-registry branch from e0945e3 to 4cb0501 Compare April 17, 2026 13:23
@github-actions
Copy link
Copy Markdown

AI Code Review

Review Summary

This PR introduces significant architectural improvements to decouple the executor from direct .mpr file access through:

  1. Backend interfaces (mdl/backend/) - 22 domain-specific sub-interfaces plus FullBackend composition
  2. Dispatch registry (mdl/executor/) - Replaces a 308-line type-switch with registry-based dispatch
  3. MockBackend - For testing with function-field pattern
  4. Other improvements - Context-based timeout fix, bug fix for missing ShowFeaturesStmt handler

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

  • Some alignment changes in existing AST files appear unnecessary but are harmless
  • MockBackend is large but follows a safe, simple pattern (function delegation)

What Looks Good

  • Clean separation of concerns with well-defined backend interfaces
  • Registry pattern improves maintainability over large type-switch
  • MockBackend enables easy, targeted testing
  • Registry completeness test ensures all 115 AST statement types are handled
  • Fixed pre-existing bug (missing ShowFeaturesStmt handler)
  • Improved timeout pattern using context.WithTimeout
  • Explicit builder wiring avoids hidden init() dependencies
  • All tests pass, lint clean, build successful

Recommendation

Approve - This PR implements a valuable architectural improvement with high code quality, thorough testing, and clear benefits for maintainability and testability. The changes are focused on a single cohesive concern (decoupling executor from storage) despite touching many files.


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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 116 out of 129 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ako
Copy link
Copy Markdown
Collaborator

ako commented Apr 17, 2026

Review

Overall: the dispatch registry is solid and ready to merge; the backend layer is dead code that needs a proof-of-concept before I'd commit to the interface design. CI is green, which is a good sign for a 5500-line PR. Stacked on #222 (typed errors, still open).

What I like

  • Registry design is excellent (registry.go, ~100 LOC). reflect.Type keying, panic on duplicate registration (catches wiring bugs at startup), typed UnsupportedError on missing handler. The completeness test against 115 known AST types is an impressive safety net — adding a new AST node without a handler will fail the test.
  • No init() magic. NewRegistry() calls explicit registerXxxHandlers() functions. Easy to grep, easy to understand the wiring order.
  • Typed errors are well-designed (mdl/errors/). Six leaf types with constructor ergonomics, proper Unwrap chains, tested via errors.As/errors.Is.
  • Bug fix caught. ShowFeaturesStmt was missing from the old type-switch — now caught by the registry completeness test.
  • Timeout cleanup. goroutine + time.Aftercontext.WithTimeout is a clear improvement.

Concerns

  1. Backend interfaces are dead code. MprBackend, MockBackend, and the 22 sub-interfaces are defined but not wired into the executor. All cmd_*.go files still use e.reader/e.writer directly — zero references to e.backend. The changes in cmd_*.go are fmt.Errorfmdlerrors.NewNotFound/NewValidation substitutions, not reader/writer→backend migrations.

    This means:

    • The interface design is untested in integration — only compile-checked via var _ backend.FullBackend = (*MprBackend)(nil)
    • The MockBackend can't be validated as useful until handlers actually consume it
    • Risk: the interface shapes may need revision once real migration starts

    Acceptable as a phase — but the PR description should be explicit that this is infrastructure-only with zero runtime behavioral change, and the next PR should wire at least a few handlers as proof-of-concept.

  2. 22 sub-interfaces may be premature decomposition. Several are very thin: NavigationBackend (3 methods), WidgetBackend (2 methods), ScheduledEventBackend (2 methods). The rationale (handlers receive only the sub-interface they need via ExecContext) is sound but speculative — if handlers commonly need 3-4 sub-interfaces simultaneously, the granularity is overhead without benefit. Until one handler migration is done end-to-end, this is unknowable. I'd be more comfortable with 8-10 coarser interfaces refined from actual usage.

  3. MockBackend is 200+ function fields of boilerplate. backend.go (266 LOC) + 13 domain files, all following if m.XxxFunc != nil { return m.XxxFunc(...) }; return zero, nil. This should be code-generated — hand-maintaining it will bitrot as interfaces evolve. At minimum add a //go:generate directive or a comment noting the intent.

  4. go fmt noise again. Same issue as PR feat: Support local file metadata for OData clients #210: ast_entity.go, ast_microflow.go, ast_query.go, ast_rest.go, docker/detect.go are whitespace realignment of struct fields. These should be in a separate commit or dropped if main is already go fmt-clean. Inflates the diff significantly for a PR that's already 5500+ lines.

  5. Unchecked type assertions in handler stubs. Every handler does:

    r.Register(&ast.CreateEntityStmt{}, func(e *Executor, stmt ast.Statement) error {
        return e.execCreateEntity(stmt.(*ast.CreateEntityStmt))
    })

    The registry guarantees correct dispatch, so the assertion won't panic in practice. But a stmt, ok := ... defensive check costs nothing and prevents a confusing panic if someone ever bypasses the registry.

  6. MprBackend.Connect() always opens in write mode. No read-only connection mode. If a future "read-only backend" or dry-run mode is planned, ConnectionBackend would need revision. Worth a comment on the intended evolution.

  7. infrastructure.go is a grab-bag. Combines RenameBackend, RawUnitBackend, MetadataBackend, WidgetBackend, AgentEditorBackend in one file. Semantically unrelated — should match the per-interface file organization used elsewhere (domainmodel.go, security.go, etc.).

Questions for the author

  • Is there a concrete plan for when handlers start using ExecContext instead of *Executor? Without that migration, the backend interfaces are pure overhead.
  • Have you prototyped one handler migration end-to-end (e.g., execCreateEntity receiving ExecContext with DomainModelBackend)? That would validate whether 22 interfaces is the right granularity.
  • Re: dependency on feat: add typed error system for executor #222 — if typed errors are revised, this PR's 79-file mdlerrors migration becomes a rebase nightmare. Is there appetite to land feat: add typed error system for executor #222 first?

Verdict

The dispatch registry and typed errors are independently valuable and merge-ready. The backend interfaces are reasonable architecture but unproven — I'd want one end-to-end handler migration as proof before committing to the 22-interface design. The go fmt noise should be split out.

Approve the registry + typed errors; request that the backend layer either (a) ships with one migrated handler as proof, or (b) is split into a separate follow-up PR that lands after #222 and includes at least one concrete migration.

@ako ako merged commit 3b86546 into mendixlabs:main Apr 17, 2026
5 of 6 checks passed
@retran
Copy link
Copy Markdown
Contributor Author

retran commented Apr 17, 2026

The backend interfaces are reasonable architecture but unproven — I'd want one end-to-end handler migration as proof before committing to the 22-interface design.

Just to clarify - there will be two more pull requests with further migration.

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.

3 participants