Extract shared types and mutation backends from sdk/mpr#2
Extract shared types and mutation backends from sdk/mpr#2retran wants to merge 5 commits intofeature/mock-handler-testsfrom
Conversation
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
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
…naming consistency Ensure deterministic map iteration order for serialization output. Add doc comments on all exported backend interfaces. Deduplicate IDToBsonBinary into single mdl/bsonutil implementation. Rename reader references to backend across executor. Guard float64-to-int64 cast with safe precision bounds. Apply go fmt formatting.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extracts shared types and ID/BSON utilities from sdk/mpr into mdl/types and mdl/bsonutil, and migrates executor/catalog code to a new backend abstraction for mutations and serialization, removing direct BSON manipulation from the executor layer.
Changes:
- Added
mdl/typesandmdl/bsonutilpackages to decouple shared model types and BSON UUID conversions fromsdk/mpr/ CGO. - Introduced/expanded backend interfaces (mutation + widget serialization/building) and updated executor commands to use them (e.g., page widget updates via
PageMutator). - Updated mock backends and tests to use
mdl/typesequivalents and the new backend-driven flow (including connect via injectedBackendFactory).
Reviewed changes
Copilot reviewed 110 out of 129 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| mdl/types/doc.go | Adds package docs describing the new shared types layer. |
| mdl/types/asyncapi.go | Implements AsyncAPI YAML parsing into shared types structures with deterministic ordering. |
| mdl/bsonutil/bsonutil.go | Adds CGO-free BSON binary UUID conversion helpers for backend implementations/tests. |
| mdl/repl/repl.go | Injects an mpr backend factory into the executor created by the REPL. |
| mdl/linter/rules/page_navigation_security.go | Switches navigation menu traversal to use mdl/types menu item types. |
| mdl/executor/executor.go | Replaces embedded mpr.Reader/Writer with backend + injectible BackendFactory; deprecates Reader() accessor. |
| mdl/executor/executor_connect.go | Moves connect/reconnect/disconnect to backend lifecycle APIs. |
| mdl/executor/hierarchy.go | Generalizes container hierarchy building to accept a backend-like source. |
| mdl/executor/helpers.go | Replaces folder/unit types + ID generation with mdl/types. |
| mdl/executor/cmd_widgets.go | Updates widget patching to PageMutator (removes direct raw BSON edits). |
| mdl/executor/cmd_structure.go | Changes “reader-based” counting helpers to use backend APIs. |
| mdl/executor/cmd_security_write.go | Migrates entity member access / revocation types to mdl/types; improves some error wrapping. |
| mdl/executor/cmd_workflows_write.go | Uses types.GenerateID() for workflow element IDs. |
| mdl/executor/cmd_rename.go | Switches rename hit types to mdl/types. |
| mdl/executor/cmd_pages_create_v3.go | Moves page builder dependency from reader/writer to backend + widget backend. |
| mdl/executor/cmd_pages_builder.go | Refactors page builder to use backend APIs (modules/layouts/pages/microflows/folders) + widget backend. |
| mdl/executor/cmd_pages_builder_v3_layout.go | Replaces mpr.GenerateID() with types.GenerateID() for layout widgets. |
| mdl/executor/cmd_pages_builder_input.go | Removes raw BSON widget property update helpers; uses backend for snippet/microflow resolution. |
| mdl/executor/cmd_odata.go | Uses types.GenerateID() and types.ParseEdmx() for metadata parsing. |
| mdl/executor/cmd_contract.go | Moves EDMX/AsyncAPI parsing + EDM types to mdl/types. |
| mdl/executor/cmd_entities.go | Switches ID generation to types.GenerateID() and improves error wrapping for calculated microflows. |
| mdl/executor/cmd_diff_local.go | Routes microflow parsing from raw maps through backend (ParseMicroflowFromRaw). |
| mdl/executor/cmd_lint.go | Uses deprecated Executor.Reader() to preserve linter integration. |
| mdl/executor/widget_registry.go | Removes OperationRegistry, replacing it with a fixed knownOperations set for validation. |
| mdl/executor/widget_engine_test.go | Updates tests for the removal of OperationRegistry and adds coverage for knownOperations. |
| mdl/executor/widget_registry_test.go | Updates validation calls after removing OperationRegistry; fixes log output reset. |
| mdl/executor/widget_property.go | Removes legacy widget property mutation utilities; keeps widget tree walking utilities. |
| mdl/executor/*_mock_test.go | Updates mock tests to use mdl/types shared structures instead of sdk/mpr types. |
| mdl/catalog/builder.go | Migrates CatalogReader type surface from sdk/mpr types to mdl/types. |
| mdl/catalog/builder_navigation.go | Updates navigation builder to use types.NavMenuItem. |
| mdl/catalog/builder_references.go | Updates reference extraction for navigation menu items to use types.NavMenuItem. |
| mdl/catalog/builder_contract.go | Moves EDMX/AsyncAPI contract parsing to types.ParseEdmx/ParseAsyncAPI. |
| mdl/backend/backend.go | Extends FullBackend with mutation + widget serialization/building interfaces. |
| mdl/backend/connection.go | Moves connection/version return types to mdl/types and expands docs. |
| mdl/backend/infrastructure.go | Migrates raw unit / rename / widget types to mdl/types and relocates Settings/Image/ScheduledEvent interfaces here. |
| mdl/backend/java.go | Migrates Java/JS action list/read types to mdl/types. |
| mdl/backend/mapping.go | Migrates JSON structure types to mdl/types (and clarifies Delete semantics). |
| mdl/backend/microflow.go | Adds backend API for parsing microflows from raw maps. |
| mdl/backend/navigation.go | Migrates navigation document/spec types to mdl/types. |
| mdl/backend/security.go | Migrates entity member access + revocation types to mdl/types. |
| mdl/backend/page.go | Clarifies snippet API comment (no GetSnippet). |
| mdl/backend/workflow.go | Removes unrelated interfaces from this file after reorganizing backend domains. |
| mdl/backend/doc.go | Updates backend package docs to reflect mdl/types extraction. |
| mdl/backend/mpr/convert.go | Adds conversion helpers between sdk/mpr and mdl/types for the mpr backend. |
| mdl/backend/mpr/datagrid_builder_test.go | Moves package + UUID generation to bsonutil; improves binary comparisons. |
| mdl/backend/mock/backend.go | Migrates mock backend function signatures from sdk/mpr types to mdl/types and adds mutation/widget hooks. |
| mdl/backend/mock/mock_connection.go | Migrates version return types to mdl/types. |
| mdl/backend/mock/mock_infrastructure.go | Migrates rename/raw unit/widget type surfaces to mdl/types. |
| mdl/backend/mock/mock_java.go | Migrates Java/JS action types to mdl/types. |
| mdl/backend/mock/mock_mapping.go | Migrates JSON structure types to mdl/types. |
| mdl/backend/mock/mock_microflow.go | Adds ParseMicroflowFromRaw mock hook. |
| mdl/backend/mock/mock_module.go | Migrates folder listing types to mdl/types. |
| mdl/backend/mock/mock_navigation.go | Migrates navigation types/specs to mdl/types. |
| mdl/backend/mock/mock_security.go | Migrates entity access revocation type to mdl/types. |
| mdl/backend/mock/mock_workflow.go | Migrates image collection types to mdl/types. |
| mdl/backend/mock/mock_mutation.go | Adds mock implementations for new mutation + widget builder/serializer interfaces. |
| examples/create_datagrid2_page/main.go | Injects mpr backend factory into the sample executor. |
| cmd/mxcli/main.go | Injects mpr backend factory into CLI-created executors. |
| cmd/mxcli/project_tree.go | Migrates navigation menu tree traversal to use types.NavMenuItem. |
| mdl/executor/widget_templates.go | Deletes legacy BSON template generation helpers (moved behind backend abstraction). |
| mdl/executor/widget_operations.go | Deletes legacy operation registry + BSON mutation operations (moved behind backend abstraction). |
| mdl/executor/widget_defaults.go | Deletes legacy widget default-population BSON utilities (moved behind backend abstraction). |
| mdl/executor/cmd_pages_builder_v3_pluggable.go | Deletes legacy pluggable widget BSON building/cloning helpers (moved behind backend abstraction). |
| mdl/executor/cmd_pages_builder_input_filters.go | Removes legacy filter widget BSON construction helpers (moved behind backend abstraction). |
| mdl/executor/cmd_pages_builder_input_cloning.go | Deletes legacy BSON deep-clone/regenerate-ID helpers (moved behind backend abstraction). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| e.backend.Disconnect() | ||
| } | ||
|
|
||
| // Reopen connection |
There was a problem hiding this comment.
reconnect()callse.backendFactory()without a nil-check, which can panic if an Executor is instantiated without callingSetBackendFactory()(unlikeexecConnect, which guards this). Add the same backendFactory == nilcheck toreconnect()` and return a backend error (consistent with connect) instead of panicking.
| // Reopen connection | |
| // Reopen connection | |
| if e.backendFactory == nil { | |
| return mdlerrors.NewBackend("reconnect", errors.New("backend factory is not configured")) | |
| } |
| // 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 doc comment says “underlying reader”, but the implementation uses backend.Path(). Update the comment to refer to the backend to avoid misleading future maintainers (this code no longer depends on a reader).
| // hierarchySource is the minimal interface needed to build a ContainerHierarchy. | ||
| // Both *mpr.Reader and backend.FullBackend satisfy this. | ||
| type hierarchySource interface { | ||
| ListModules() ([]*model.Module, error) | ||
| ListUnits() ([]*types.UnitInfo, error) | ||
| ListFolders() ([]*types.FolderInfo, error) | ||
| } |
There was a problem hiding this comment.
The comment claims *mpr.Reader satisfies hierarchySource, but this interface uses *types.UnitInfo / *types.FolderInfo. If mpr.Reader doesn’t actually return types.* (or if this changes), this comment becomes incorrect. Consider narrowing the comment to known implementations (e.g., backend.FullBackend) or explicitly noting that *mpr.Reader must expose the types.* versions (or provide an adapter) to satisfy this interface.
| // ParseAsyncAPI parses an AsyncAPI YAML string into an AsyncAPIDocument. | ||
| func ParseAsyncAPI(yamlStr string) (*AsyncAPIDocument, error) { | ||
| if yamlStr == "" { | ||
| return nil, fmt.Errorf("empty AsyncAPI document") | ||
| } | ||
|
|
||
| var raw yamlAsyncAPI | ||
| if err := yaml.Unmarshal([]byte(yamlStr), &raw); err != nil { | ||
| return nil, fmt.Errorf("failed to parse AsyncAPI YAML: %w", err) | ||
| } |
There was a problem hiding this comment.
This introduces new parsing and deterministic ordering behavior (sorted messages/channels/properties) but there are no accompanying unit tests in this PR for ParseAsyncAPI. Add tests that cover: empty input error, invalid YAML error wrapping, ref resolution via $ref, and deterministic ordering (e.g., map input order shouldn’t affect output slice ordering).
| 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 now owns the legacy “invalid UUID → generate a new ID” fallback behavior. Add small unit tests to lock this down (valid UUID round-trip, invalid UUID triggers a 16-byte generated value, and BsonBinaryToID round-trips the generated/valid blobs). This will help prevent subtle regressions in backend serialization.
Summary
The executor currently depends directly on
sdk/mprfor BSON manipulation, type definitions, and storage access. This couples the command layer to a single storage format and makes it impossible to compilemdl/for alternative targets or swap backends without dragging in the entire MPR SDK.This PR decouples the executor from
sdk/mprby:mdl/types/packagemdl/backend/What changed
New
mdl/types/packageShared types previously defined in
sdk/mpr— infrastructure, java, navigation, mapping, JSON utilities, EDMX/AsyncAPI parsing, ID generation — now live inmdl/types/. This package has no CGO or BSON driver dependencies.sdk/mprre-exports these as type aliases for backward compatibility.New
mdl/bsonutil/packageID conversion utilities (
IDToBsonBinary,BsonBinaryToID,BlobToUUID,UUIDToBlob) extracted from scattered implementations acrosssdk/mprinto a single canonical package.Mutation backend interfaces
New interfaces in
mdl/backend/:PageMutator— widget CRUD, property mutations, drag-and-drop operationsWorkflowMutator— activity insertion/removal, branch management, boundary eventsWidgetObjectBuilder— type-safe widget property construction with deferred error handlingDataGrid2Builder— DataGrid2 widget construction with column and filter configurationMPR implementations in
mdl/backend/mpr/encapsulate all BSON document manipulation that was previously inlined in executor handlers.Executor simplification
*mpr.Writer/*mpr.Readerremoved from Executor struct, replaced withBackendFactoryinjectionCode quality
reader→backendnaming across executorTest plan
go test ./...— all existing tests passgo build ./...— cleanmake lint-go— clean