refactor: decouple executor from storage layer#238
refactor: decouple executor from storage layer#238retran wants to merge 12 commits intomendixlabs:mainfrom
Conversation
Phase 1-8: 85 read/write handler tests with mock backend infrastructure Error paths: 49 tests covering backend error propagation for all handler groups Not-connected: 29 tests verifying Connected/ConnectedForWrite guards JSON format: 26 tests validating JSON output for show/list handlers Production code changes: - Added Func fields to MockBackend for 8 agent-editor write methods - Fixed ContainerID parameter semantics in withContainer and all call sites
Create mdl/types/ package with WASM-safe shared types extracted from sdk/mpr: domain model types, ID utilities, EDMX/AsyncAPI parsing, JSON formatting helpers. Migrate all executor handlers to use mdl/types directly, removing type aliases from sdk/mpr/reader_types.go. - Extract 16+ domain types to mdl/types/ (infrastructure, java, navigation, mapping) - Extract GenerateID, BlobToUUID, ValidateID to mdl/types/id.go - Extract ParseEdmx, ParseAsyncAPI to mdl/types/edmx.go, asyncapi.go - Extract PrettyPrintJSON, BuildJsonElementsFromSnippet to json_utils.go - Migrate 30+ executor handler files off sdk/mpr type references - sdk/mpr retains thin delegation wrappers for backward compatibility
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the MDL executor to stop manipulating sdk/mpr/BSON directly and instead operate through backend abstractions (including new mutation interfaces), while migrating various shared value types into mdl/types.
Changes:
- Replaced many
sdk/mprdependencies withmdl/typesand backend interface calls across executor and catalog. - Introduced backend mutation interfaces (
PageMutator,WorkflowMutator, widget builder/serialization backends) and updated widget update flow to use them. - Added extensive MockBackend-based executor tests and adjusted CLI/example wiring to set a backend factory.
Reviewed changes
Copilot reviewed 109 out of 148 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mdl/executor/cmd_workflows_write.go | Switch workflow UUID/ID generation to mdl/types. |
| mdl/executor/cmd_workflows_mock_test.go | Add mock-backend tests for workflow show/describe. |
| mdl/executor/cmd_widgets.go | Replace raw BSON widget updates with PageMutator. |
| mdl/executor/cmd_settings_mock_test.go | Add mock-backend tests for settings show/describe. |
| mdl/executor/cmd_security_write.go | Migrate security write types from mpr to types. |
| mdl/executor/cmd_security_mock_test.go | Add mock-backend tests for security show/describe paths. |
| mdl/executor/cmd_rest_clients_mock_test.go | Add mock-backend tests for REST clients show/describe. |
| mdl/executor/cmd_rename.go | Convert rename hit types from mpr to types. |
| mdl/executor/cmd_published_rest_mock_test.go | Add mock-backend tests for published REST services. |
| mdl/executor/cmd_pages_mock_test.go | Add mock-backend tests for pages/snippets/layouts list. |
| mdl/executor/cmd_pages_create_v3.go | Build pages/snippets via backend-backed pageBuilder. |
| mdl/executor/cmd_pages_builder_v3_pluggable.go | Remove BSON-based pluggable widget builder helpers. |
| mdl/executor/cmd_pages_builder_v3_layout.go | Swap mpr.GenerateID usages to types.GenerateID. |
| mdl/executor/cmd_pages_builder_input_filters.go | Remove BSON filter widget construction helpers. |
| mdl/executor/cmd_pages_builder_input_cloning.go | Remove BSON deep-clone helpers from executor. |
| mdl/executor/cmd_pages_builder_input.go | Move snippet resolution to backend list APIs. |
| mdl/executor/cmd_pages_builder.go | Refactor pageBuilder to use backend.FullBackend + mdl/types. |
| mdl/executor/cmd_odata_mock_test.go | Add mock-backend tests for OData show/describe. |
| mdl/executor/cmd_odata.go | Replace mpr helpers with types (IDs, EDMX parsing). |
| mdl/executor/cmd_notconnected_mock_test.go | Add tests for “not connected” behavior across commands. |
| mdl/executor/cmd_navigation_mock_test.go | Add mock-backend tests for navigation show/describe. |
| mdl/executor/cmd_navigation.go | Switch navigation document/spec types to mdl/types. |
| mdl/executor/cmd_modules_mock_test.go | Add mock-backend tests for module listing aggregation. |
| mdl/executor/cmd_misc_mock_test.go | Add mock-backend test for version output. |
| mdl/executor/cmd_microflows_mock_test.go | Add mock-backend tests for microflow show/describe. |
| mdl/executor/cmd_microflows_create.go | Switch IDs to types.GenerateID; use backend for lookups. |
| mdl/executor/cmd_microflows_builder_workflow.go | Switch microflow IDs to types.GenerateID. |
| mdl/executor/cmd_microflows_builder_graph.go | Switch graph element IDs to types.GenerateID. |
| mdl/executor/cmd_microflows_builder_flows.go | Switch sequence flow IDs to types.GenerateID. |
| mdl/executor/cmd_microflows_builder_control.go | Switch control structure IDs to types.GenerateID. |
| mdl/executor/cmd_microflows_builder_annotations.go | Switch annotation IDs to types.GenerateID. |
| mdl/executor/cmd_microflows_builder.go | Generalize microflow builder “reader” to backend interface. |
| mdl/executor/cmd_mermaid_mock_test.go | Add mock-backend test for mermaid domain model describe. |
| mdl/executor/cmd_lint.go | Route lint reader via executor accessor. |
| mdl/executor/cmd_jsonstructures_mock_test.go | Add mock-backend tests for JSON structures show. |
| mdl/executor/cmd_jsonstructures.go | Migrate JSON structure helpers/types to mdl/types. |
| mdl/executor/cmd_javascript_actions_mock_test.go | Add mock-backend tests for JavaScript actions. |
| mdl/executor/cmd_javaactions_mock_test.go | Add mock-backend tests for Java actions. |
| mdl/executor/cmd_javaactions.go | Migrate Java action creation IDs to types.GenerateID. |
| mdl/executor/cmd_import_mappings_mock_test.go | Add mock-backend tests for import mappings show. |
| mdl/executor/cmd_import_mappings.go | Switch JSON element typing to types and use backend for DM lookup. |
| mdl/executor/cmd_imagecollections_mock_test.go | Add mock-backend tests for image collections. |
| mdl/executor/cmd_imagecollections.go | Switch image collection typings to mdl/types. |
| mdl/executor/cmd_fragments_mock_test.go | Add tests for fragment listing behavior. |
| mdl/executor/cmd_folders.go | Switch folder info typing to mdl/types. |
| mdl/executor/cmd_features.go | Update comment to reflect “not connected” behavior. |
| mdl/executor/cmd_export_mappings_mock_test.go | Add mock-backend tests for export mappings show. |
| mdl/executor/cmd_export_mappings.go | Switch JSON element typing to types and use backend for DM lookup. |
| mdl/executor/cmd_enumerations_mock_test.go | Refactor + expand mock tests for enumerations show/describe. |
| mdl/executor/cmd_entities_mock_test.go | Add mock-backend tests for entity listing. |
| mdl/executor/cmd_entities.go | Switch entity creation IDs to types.GenerateID. |
| mdl/executor/cmd_diff_local.go | Delegate microflow parsing from raw BSON map to backend. |
| mdl/executor/cmd_dbconnection_mock_test.go | Add mock-backend tests for database connections show/describe. |
| mdl/executor/cmd_datatransformer_mock_test.go | Add mock-backend tests for data transformers show/describe. |
| mdl/executor/cmd_contract.go | Switch EDMX/AsyncAPI parsing and types to mdl/types. |
| mdl/executor/cmd_constants_mock_test.go | Add mock-backend tests for constants show/describe. |
| mdl/executor/cmd_catalog.go | Update local executor creation to use backend field. |
| mdl/executor/cmd_businessevents_mock_test.go | Add mock-backend tests for business event services. |
| mdl/executor/cmd_businessevents.go | Switch channel ID generation to types.GenerateID. |
| mdl/executor/cmd_associations_mock_test.go | Add mock-backend tests for associations show. |
| mdl/executor/cmd_agenteditor_mock_test.go | Add mock-backend tests for agent editor documents. |
| mdl/catalog/builder_references.go | Use types.NavMenuItem in reference extraction. |
| mdl/catalog/builder_navigation.go | Use types.NavMenuItem in navigation indexing. |
| mdl/catalog/builder_contract.go | Switch contract parsing to types.ParseEdmx / types.ParseAsyncAPI. |
| mdl/catalog/builder.go | Update CatalogReader interface to use mdl/types shared structs. |
| mdl/bsonutil/bsonutil.go | Add BSON ID conversion utilities without sdk/mpr dependency. |
| mdl/backend/workflow.go | Replace image backend signatures to use mdl/types. |
| mdl/backend/security.go | Replace security member access/revocation types to mdl/types. |
| mdl/backend/navigation.go | Replace navigation backend types/specs to mdl/types. |
| mdl/backend/mutation.go | Introduce mutation + widget builder/serialization backend interfaces. |
| mdl/backend/mpr/datagrid_builder_test.go | Move deep-clone BSON tests into mpr backend package, using bsonutil. |
| mdl/backend/mock/mock_workflow.go | Update mock image backend signatures to mdl/types. |
| mdl/backend/mock/mock_security.go | Update mock signatures to use mdl/types. |
| mdl/backend/mock/mock_navigation.go | Update mock signatures to use mdl/types. |
| mdl/backend/mock/mock_mutation.go | Add mock implementations for new mutation/builder interfaces. |
| mdl/backend/mock/mock_module.go | Update mock folder list signatures to mdl/types. |
| mdl/backend/mock/mock_microflow.go | Add mock hook for ParseMicroflowFromRaw. |
| mdl/backend/mock/mock_mapping.go | Update JSON structure signatures to mdl/types. |
| mdl/backend/mock/mock_java.go | Update Java/JS action signatures to mdl/types. |
| mdl/backend/mock/mock_infrastructure.go | Port raw unit/rename/widget type signatures to mdl/types; expand agent editor mocks. |
| mdl/backend/mock/mock_connection.go | Switch version/project version types to mdl/types. |
| mdl/backend/mock/backend.go | Extend MockBackend struct for new types + mutation/builder hooks. |
| mdl/backend/microflow.go | Add backend-level ParseMicroflowFromRaw API. |
| mdl/backend/mapping.go | Port mapping backend JSON structures API to mdl/types. |
| mdl/backend/java.go | Port Java backend typed list/read APIs to mdl/types. |
| mdl/backend/infrastructure.go | Port rename/raw unit/widget type APIs to mdl/types. |
| mdl/backend/doc.go | Update package docs to reflect mdl/types shared types. |
| mdl/backend/connection.go | Port connection + folder backend types to mdl/types. |
| mdl/backend/backend.go | Extend FullBackend with mutation/builder/serialization interfaces. |
| examples/create_datagrid2_page/main.go | Wire executor to mpr backend via backend factory. |
| cmd/mxcli/project_tree.go | Switch menu tree node input type to types.NavMenuItem. |
| cmd/mxcli/main.go | Wire CLI executor to mpr backend via backend factory. |
Comments suppressed due to low confidence (1)
mdl/executor/cmd_diff_local.go:503
microflowBsonToMDLclaims it “falls back to a header-only stub if parsing fails”, but it does not handleParseMicroflowFromRawreturningnil. If the backend cannot parse the raw map (or a mock returns nil),renderMicroflowMDLis likely to panic. Add an explicitnilcheck and return the intended stub output (or an empty/diagnostic MDL representation) when parsing returns nil.
func microflowBsonToMDL(ctx *ExecContext, raw map[string]any, qualifiedName string) string {
qn := splitQualifiedName(qualifiedName)
mf := ctx.Backend.ParseMicroflowFromRaw(raw, model.ID(qn.Name), "")
entityNames, microflowNames := buildNameLookups(ctx)
return renderMicroflowMDL(ctx, mf, qn, entityNames, microflowNames, nil)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := bson.Unmarshal(rawBytes, &rawData); err != nil { | ||
| return 0, mdlerrors.NewBackend(fmt.Sprintf("unmarshal snippet %s", containerName), err) | ||
| return 0, mdlerrors.NewBackend(fmt.Sprintf("open %s for mutation", containerName), err) | ||
| } |
There was a problem hiding this comment.
updateWidgetsInContainer assumes OpenPageForMutation returns a non-nil mutator when err == nil. If an implementation (notably the default MockBackend in this PR) returns (nil, nil), the subsequent mutator.FindWidget(...) will panic. Treat a nil mutator as an error here (e.g., return a backend error indicating the mutator wasn’t provided).
| } | |
| } | |
| if mutator == nil { | |
| return 0, mdlerrors.NewBackend( | |
| fmt.Sprintf("open %s for mutation", containerName), | |
| fmt.Errorf("backend returned nil page mutator"), | |
| ) | |
| } |
| func (m *MockBackend) OpenPageForMutation(unitID model.ID) (backend.PageMutator, error) { | ||
| if m.OpenPageForMutationFunc != nil { | ||
| return m.OpenPageForMutationFunc(unitID) | ||
| } | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
The default mock implementation returns (nil, nil) when the function hook isn’t configured. This makes missing mock setup fail later with nil dereferences instead of producing a clear, localized test failure. Prefer returning a non-nil error indicating the method is not configured/implemented in the mock (apply similarly to other newly added mock mutation/builder/serialization methods that currently return (nil, nil) by default).
| func IDToBsonBinary(id string) primitive.Binary { | ||
| blob := types.UUIDToBlob(id) | ||
| if blob == nil || len(blob) != 16 { | ||
| blob = types.UUIDToBlob(types.GenerateID()) | ||
| } | ||
| return primitive.Binary{ | ||
| Subtype: 0x00, | ||
| Data: blob, | ||
| } | ||
| } |
There was a problem hiding this comment.
IDToBsonBinary silently replaces invalid input with a newly generated random ID. That can mask upstream bugs and can also break referential integrity if an invalid ID ever reaches this function. Consider returning an error (e.g., func IDToBsonBinary(id string) (primitive.Binary, error)) or providing two APIs (a strict one returning an error and a convenience Must... variant) so callers can choose explicit failure vs. fallback behavior.
| // getProjectPath returns the project directory path from the underlying reader. | ||
| func (pb *pageBuilder) getProjectPath() string { | ||
| if pb.backend != nil { | ||
| return pb.backend.Path() | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
The comment mentions “underlying reader”, but the implementation reads from pb.backend. Update the comment to match the new abstraction to avoid confusion when debugging/refactoring.
- id_test.go: UUID generation, roundtrip, validation, hash - json_utils_test.go: pretty-print, datetime normalization, snippet builder - edmx_test.go: OData4 parsing, enums, capabilities, FindEntityType - asyncapi_test.go: parsing, sorted channels/messages, FindMessage - convert_test.go: prove sdk/mpr type aliases are identical to mdl/types - fix normalizeDateTimeValue matching '-' in date portion (search from idx 19+)
- normalizeDateTimeValue: pad fractional seconds even without timezone suffix - float64→int64: add safe integer range guard (±2^53) - fix reservedExposedNames comment (remove 'Name' not in map) - clarify singularize is intentionally naive (matches Studio Pro)
Add PageMutator, WorkflowMutator, WidgetSerializationBackend interfaces to mdl/backend/mutation.go for BSON-free handler decoupling. Extract BSON ID helpers (IDToBsonBinary, BsonBinaryToID, NewIDBsonBinary) to mdl/bsonutil/ package. Add panic stubs to MprBackend and mock function fields to MockBackend for all new interface methods. - Create mdl/bsonutil/bsonutil.go with BSON ID conversion utilities - Migrate 10 handler files from mpr.IDToBsonBinary to bsonutil.* - Define PageMutationBackend, WorkflowMutationBackend interfaces - Define WidgetSerializationBackend with opaque return types - Add PluggablePropertyContext for domain-typed widget property input
Implement PageMutator, WorkflowMutator, and WidgetBuilderBackend in mdl/backend/mpr/. Rewrite ALTER PAGE (1721→256 lines) and ALTER WORKFLOW (887→178 lines) as thin orchestrators using mutator sessions. Implement PluggableWidgetEngine with WidgetObjectBuilder interface, eliminating all BSON from widget_engine.go. - Create mdl/backend/mpr/page_mutator.go (1554 lines) - Create mdl/backend/mpr/workflow_mutator.go (771 lines) - Create mdl/backend/mpr/widget_builder.go (1007 lines) - Migrate SerializeWidget/ClientAction/DataSource to backend interface - Add ParseMicroflowFromRaw to MicroflowBackend interface - Delete widget_operations.go, widget_templates.go, widget_defaults.go - Move ALTER PAGE/WORKFLOW tests to backend/mpr/ package
Replace *mpr.Reader/*mpr.Writer with backend.FullBackend throughout executor. Inject BackendFactory to remove mprbackend import from executor_connect.go. Move all remaining write-path BSON construction (DataGrid2, filters, cloning, widget property updates) behind backend interface. - Remove writer/reader fields from Executor struct - Add BackendFactory injection pattern for connect/disconnect - Create mdl/backend/mpr/datagrid_builder.go (1260 lines) - Add BuildDataGrid2Widget, BuildFilterWidget to WidgetBuilderBackend - Delete bson_helpers.go, cmd_pages_builder_input_cloning.go, cmd_pages_builder_input_datagrid.go, cmd_pages_builder_v3_pluggable.go - Remaining BSON: 3 read-only files (describe, diff) — WASM-safe
017f206 to
07c9ad9
Compare
Why
After PRs #235–#237, the executor still holds direct references to
*mpr.Readerand*mpr.Writerin its struct, and several command files still construct BSON for datagrids, filters, and widget cloning. This PR completes the decoupling: the executor no longer importssdk/mprat all and works entirely through thebackend.FullBackendabstraction.This is the architectural pivot — dependency inversion via a
BackendFactorypattern replaces the direct MPR reader/writer construction.What changed (incremental from PR #237)
32 files changed, +1,523/-3,121 (net -1,598 lines)
Executor struct decoupled:
writerandreaderfields from theExecutorstructBackendFactoryinjection —executor_connect.gorewritten to create backends through the factory instead of constructingmpr.Reader/mpr.Writerdirectlycmd/mxcli/main.goupdated to inject the factoryRemaining BSON extracted:
mdl/backend/mpr/datagrid_builder.go(1,260 lines) — DataGrid2 construction, filter widgets, column building moved from executor to backendmdl/backend/mpr/datagrid_builder_test.go— test coverage for datagrid buildingDeleted from executor (5 files, ~1,800 lines):
bson_helpers.go— BSON utility functionscmd_pages_builder_input_cloning.go— widget cloning logiccmd_pages_builder_input_datagrid.go— datagrid constructioncmd_pages_builder_input_filters.go— filter widget constructioncmd_pages_builder_v3_pluggable.go— pluggable widget BSONResult: The only BSON remaining in the executor is in 3 read-only files (describe, diff) that deserialize for display — no write-path BSON.
Stack
PR 4/5 in the shared-types refactoring stack.
Merge order: #232 → #235 → #236 → #237 → this → #239