refactor: extract shared types and utility functions to mdl/types#234
refactor: extract shared types and utility functions to mdl/types#234retran wants to merge 2 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
|
Closing — renaming branches and recreating stacked PRs with descriptive names. |
AI Code ReviewWhat Looks GoodThis PR successfully extracts shared types from
The refactor touches many files but maintains a single clear purpose: breaking the RecommendationApprove the PR. The refactor is correctly implemented, follows the project's architectural goals, and maintains consistency across the codebase. The approach of extracting shared types with conversion functions is sound and properly scoped. Verification should confirm that:
Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
This PR continues the MDL decoupling effort by moving shared value types and ID/UUID utilities out of sdk/mpr into a new mdl/types package, then updating executor/catalog/backend layers to depend on mdl/types rather than sdk/mpr (reducing coupling and enabling alternate backends).
Changes:
- Introduce
mdl/typeswith shared document/value types (navigation, mappings, Java descriptors, infra types, AsyncAPI, ID utilities). - Refactor
mdl/backend/*,mdl/catalog/*,mdl/executor/*, and related code to usemdl/typesin public interfaces and logic. - Update
sdk/mprimplementations to usemdl/types(including delegations and some type-aliasing/delegation for selected parsers).
Reviewed changes
Copilot reviewed 108 out of 111 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/mpr/writer_imagecollection_test.go | Update tests to use types.ImageCollection. |
| sdk/mpr/writer_imagecollection.go | Writer image-collection APIs now consume types.ImageCollection. |
| sdk/mpr/writer_core.go | Delegate UUID/blob helpers to mdl/types. |
| sdk/mpr/utils.go | Delegate ID/hash/UUID helpers to mdl/types and re-implement BSON ID conversion. |
| sdk/mpr/reader.go | Delegate blob→UUID formatting to mdl/types. |
| sdk/mpr/parser_misc.go | Switch parsed document return types to mdl/types equivalents. |
| sdk/mpr/asyncapi.go | Move AsyncAPI types to mdl/types via aliases + delegate parser. |
| mdl/visitor/visitor_agenteditor.go | Minor whitespace cleanup. |
| mdl/types/navigation.go | New navigation document/profile/menu value types. |
| mdl/types/mapping.go | New JSON structure + image collection value types. |
| mdl/types/java.go | New lightweight Java/JS action descriptor types. |
| mdl/types/infrastructure.go | New shared infra/value types (versions, folder/unit info, raw-unit wrappers, rename hits, security structs). |
| mdl/types/id.go | New shared ID/UUID/hash helpers. |
| mdl/types/doc.go | Package documentation for shared value types. |
| mdl/types/asyncapi.go | AsyncAPI parsing/types moved here from sdk/mpr. |
| mdl/linter/rules/page_navigation_security.go | Linter updated to traverse types.NavMenuItem. |
| mdl/executor/widget_templates.go | Use types.GenerateID for widget template IDs. |
| mdl/executor/widget_operations.go | Use types.GenerateID when creating translation IDs. |
| mdl/executor/widget_engine.go | Use types.GenerateID callback/IDs for widget build. |
| mdl/executor/mock_test_helpers_test.go | New executor mock test helpers (ctx, hierarchy, factories, assertions). |
| mdl/executor/helpers.go | Use types.FolderInfo and types.GenerateID for folder creation. |
| mdl/executor/executor.go | Executor caches now store types.UnitInfo / types.FolderInfo. |
| mdl/executor/cmd_write_handlers_mock_test.go | New mock tests for write handlers. |
| mdl/executor/cmd_workflows_write.go | Use types.GenerateID for workflow element IDs. |
| mdl/executor/cmd_workflows_mock_test.go | New workflow handler mock tests. |
| mdl/executor/cmd_settings_mock_test.go | New settings handler mock tests. |
| mdl/executor/cmd_security_write.go | Use types.EntityMemberAccess / types.EntityAccessRevocation. |
| mdl/executor/cmd_security_mock_test.go | New security handler mock tests. |
| mdl/executor/cmd_rest_clients_mock_test.go | New REST client handler mock tests. |
| mdl/executor/cmd_rename.go | Switch rename report plumbing to types.RenameHit. |
| mdl/executor/cmd_published_rest_mock_test.go | New published REST handler mock tests. |
| mdl/executor/cmd_pages_mock_test.go | New pages/layouts handler mock tests. |
| mdl/executor/cmd_pages_builder_v3_layout.go | Use types.GenerateID for V3 page builder nodes. |
| mdl/executor/cmd_pages_builder.go | Folder cache now uses types.FolderInfo (plus related changes). |
| mdl/executor/cmd_odata_mock_test.go | New OData handler mock tests. |
| mdl/executor/cmd_odata.go | Use types.GenerateID + types.ParseEdmx. |
| mdl/executor/cmd_notconnected_mock_test.go | New not-connected guard mock tests. |
| mdl/executor/cmd_navigation_mock_test.go | New navigation handler mock tests using mdl/types. |
| mdl/executor/cmd_navigation.go | Navigation writer spec/types switched to mdl/types. |
| mdl/executor/cmd_modules_mock_test.go | New modules handler mock tests using types.UnitInfo. |
| mdl/executor/cmd_misc_mock_test.go | New version handler mock tests using types.ProjectVersion. |
| mdl/executor/cmd_microflows_mock_test.go | New microflows handler mock tests. |
| mdl/executor/cmd_microflows_create.go | Use types.GenerateID for microflow IDs/params. |
| mdl/executor/cmd_microflows_builder_workflow.go | Use types.GenerateID for workflow-related microflow objects. |
| mdl/executor/cmd_microflows_builder_graph.go | Use types.GenerateID for microflow graph objects. |
| mdl/executor/cmd_microflows_builder_flows.go | Use types.GenerateID for sequence flows/cases/events. |
| mdl/executor/cmd_microflows_builder_control.go | Use types.GenerateID for control-flow microflow objects. |
| mdl/executor/cmd_microflows_builder_annotations.go | Use types.GenerateID for annotations and flows. |
| mdl/executor/cmd_mermaid_mock_test.go | New mermaid describe mock test. |
| mdl/executor/cmd_jsonstructures_mock_test.go | New JSON structures handler mock test using mdl/types. |
| mdl/executor/cmd_jsonstructures.go | Switch JSON helpers/types to mdl/types. |
| mdl/executor/cmd_javascript_actions_mock_test.go | New JS action handler mock tests using mdl/types. |
| mdl/executor/cmd_javaactions_mock_test.go | New Java action handler mock tests. |
| mdl/executor/cmd_javaactions.go | Use types.GenerateID for Java action model construction. |
| mdl/executor/cmd_import_mappings_mock_test.go | New import mappings handler mock test. |
| mdl/executor/cmd_import_mappings.go | Switch JSON structure element map types to mdl/types. |
| mdl/executor/cmd_imagecollections_mock_test.go | New image collections handler mock tests using mdl/types. |
| mdl/executor/cmd_imagecollections.go | Switch image collection types to mdl/types. |
| mdl/executor/cmd_fragments_mock_test.go | New fragments handler mock tests. |
| mdl/executor/cmd_folders.go | Use types.FolderInfo in folder path resolution. |
| mdl/executor/cmd_export_mappings_mock_test.go | New export mappings handler mock test. |
| mdl/executor/cmd_export_mappings.go | Switch JSON structure element map types to mdl/types. |
| mdl/executor/cmd_enumerations_mock_test.go | Refactor enumeration tests to new mock helpers + add describe/notfound coverage. |
| mdl/executor/cmd_entities_mock_test.go | New entities handler mock tests. |
| mdl/executor/cmd_entities.go | Use types.GenerateID for entity construction (attrs/vr/index IDs). |
| mdl/executor/cmd_dbconnection_mock_test.go | New DB connection handler mock tests. |
| mdl/executor/cmd_datatransformer_mock_test.go | New data transformer handler mock tests. |
| mdl/executor/cmd_constants_mock_test.go | New constants handler mock tests. |
| mdl/executor/cmd_businessevents_mock_test.go | New business events handler mock tests. |
| mdl/executor/cmd_businessevents.go | Use types.GenerateID for channel name generation. |
| mdl/executor/cmd_associations_mock_test.go | New associations handler mock tests. |
| mdl/executor/cmd_agenteditor_write.go | Formatting-only alignment in agent creation struct literal. |
| mdl/executor/cmd_agenteditor_models.go | Formatting-only alignment in model creation struct literal. |
| mdl/executor/cmd_agenteditor_mock_test.go | New agent editor handlers mock tests. |
| mdl/catalog/builder_references.go | Catalog navigation reference extraction now uses types.NavMenuItem. |
| mdl/catalog/builder_navigation.go | Catalog navigation builder now uses mdl/types menu items. |
| mdl/catalog/builder_contract.go | Catalog contract parsing uses types.ParseEdmx / types.ParseAsyncAPI. |
| mdl/catalog/builder.go | CatalogReader interface updated to mdl/types for shared structs. |
| mdl/backend/workflow.go | Backend interfaces updated to types.ImageCollection. |
| mdl/backend/security.go | Backend interfaces updated to types.EntityMemberAccess/revocation. |
| mdl/backend/navigation.go | Backend interfaces updated to types.NavigationDocument/spec. |
| mdl/backend/mpr/backend.go | MPR backend converts between sdk/mpr and mdl/types across interfaces. |
| mdl/backend/mock/mock_workflow.go | Mock backend updated for types.ImageCollection. |
| mdl/backend/mock/mock_security.go | Mock backend updated for types.EntityAccessRevocation. |
| mdl/backend/mock/mock_navigation.go | Mock backend updated for navigation mdl/types. |
| mdl/backend/mock/mock_module.go | Mock backend updated for types.FolderInfo. |
| mdl/backend/mock/mock_mapping.go | Mock backend updated for types.JsonStructure. |
| mdl/backend/mock/mock_java.go | Mock backend updated for types.JavaAction/types.JavaScriptAction. |
| mdl/backend/mock/mock_infrastructure.go | Mock backend updated for mdl/types infra + adds functional AgentEditor write hooks. |
| mdl/backend/mock/mock_connection.go | Mock connection updated to return types.MPRVersion/types.ProjectVersion. |
| mdl/backend/mock/backend.go | MockBackend struct fields migrated from sdk/mpr types to mdl/types. |
| mdl/backend/mapping.go | MappingBackend interface migrated to mdl/types. |
| mdl/backend/java.go | JavaBackend interface migrated to mdl/types. |
| mdl/backend/infrastructure.go | Infra interfaces migrated to mdl/types (rename/raw-unit/widget metadata). |
| mdl/backend/doc.go | Backend package docs updated to reflect mdl/types extraction. |
| mdl/backend/connection.go | Connection/Folder backend interfaces migrated to mdl/types. |
| internal/marketplace/types.go | Minor comment formatting adjustment. |
| cmd/mxcli/project_tree.go | Project tree builder uses types.NavMenuItem for menu rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| currentContainerID = newFolderID | ||
|
|
||
| // Add to cache | ||
| pb.foldersCache = append(pb.foldersCache, &mpr.FolderInfo{ | ||
| pb.foldersCache = append(pb.foldersCache, &types.FolderInfo{ | ||
| ID: newFolderID, | ||
| ContainerID: currentContainerID, | ||
| Name: part, |
There was a problem hiding this comment.
When creating a missing folder, the newly appended FolderInfo sets ContainerID to currentContainerID after it has been updated to newFolderID. This makes the cached folder appear to be its own parent, which will break subsequent lookups/resolution of nested folder paths. Capture the parent container ID before updating currentContainerID, and store that as ContainerID in the cached FolderInfo.
| // CreateImageCollection creates a new empty image collection unit in the MPR. | ||
| func (w *Writer) CreateImageCollection(ic *ImageCollection) error { | ||
| func (w *Writer) CreateImageCollection(ic *types.ImageCollection) error { | ||
| if ic.ID == "" { | ||
| ic.ID = model.ID(generateUUID()) |
There was a problem hiding this comment.
This changes the exported Writer.CreateImageCollection API to accept *types.ImageCollection instead of the previously public *mpr.ImageCollection. Unless sdk/mpr defines type ImageCollection = types.ImageCollection (and callers can keep using mpr.ImageCollection), this is a breaking change for downstream users. Consider adding the missing type alias in sdk/mpr (and optionally switching the signature back to *mpr.ImageCollection via the alias) to preserve backward compatibility as stated in the PR description.
| // IDToBsonBinary converts a UUID string to a BSON binary value. | ||
| func IDToBsonBinary(id string) primitive.Binary { | ||
| return idToBsonBinary(id) | ||
| blob := types.UUIDToBlob(id) | ||
| if blob == nil || len(blob) != 16 { | ||
| blob = types.UUIDToBlob(types.GenerateID()) | ||
| } | ||
| return primitive.Binary{ | ||
| Subtype: 0x00, | ||
| Data: blob, | ||
| } | ||
| } | ||
|
|
||
| // BsonBinaryToID converts a BSON binary value to a UUID string. | ||
| func BsonBinaryToID(bin primitive.Binary) string { | ||
| return BlobToUUID(bin.Data) | ||
| return types.BlobToUUID(bin.Data) | ||
| } |
There was a problem hiding this comment.
PR description says IDToBsonBinary/BsonBinaryToID were extracted into a new mdl/bsonutil package, but this diff keeps these functions in sdk/mpr/utils.go (re-implemented here) and there is no corresponding extraction in the changes shown. Either update the PR description to match the implemented scope, or complete the extraction and have sdk/mpr delegate to mdl/bsonutil for consistency with the stated refactor plan.
Summary
EdmxDocument,AsyncAPIDocument,JsonStructure,ImageCollection, navigation types, Java types, mapping types) fromsdk/mprinto newmdl/typespackageIDToBsonBinary/BsonBinaryToIDinto newmdl/bsonutilpackagemdl/catalogto usemdl/typesinstead ofsdk/mprsdk/mprfor backward compatibilityWhy
The executor layer (
mdl/) currently importssdk/mprfor type definitions, creating a circular dependency that prevents alternative backend implementations. Extracting shared types intomdl/typesbreaks this coupling.Stack
This is PR 1/5 in the shared-types stack:
Depends on PR #232 (mock handler tests).