refactor: extract shared types and utility functions to mdl/types#235
refactor: extract shared types and utility functions to mdl/types#235ako merged 9 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
AI Code ReviewWhat Looks GoodThis PR successfully executes a shared types refactoring to decouple the executor layer from
The conversion layer in RecommendationApprove - This PR is a clean, well-executed refactor that achieves its stated goal of reducing coupling between the executor layer and Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
This PR starts the “shared-types” refactor by moving a broad set of shared MDL/MPR-facing value types and utility helpers out of sdk/mpr into a new mdl/types package, then updating mdl/ (executor, catalog, backend interfaces, linter, CLI) to depend on mdl/types rather than sdk/mpr for those definitions—reducing coupling and enabling alternative backends.
Changes:
- Introduces
mdl/typeswith extracted shared document/value types (navigation, mapping/json structures, images, infra metadata, IDs, AsyncAPI, EDMX, JSON helpers). - Updates
mdl/backendinterfaces andmdl/backend/mpr+mdl/backend/mockimplementations to usemdl/typestypes. - Migrates many executor/catalog call sites and adds/updates mock-based tests around the updated interfaces.
Reviewed changes
Copilot reviewed 108 out of 111 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/mpr/writer_imagecollection_test.go | Updates tests to use types.ImageCollection. |
| sdk/mpr/writer_imagecollection.go | Switches writer APIs/serialization to types.ImageCollection. |
| sdk/mpr/writer_core.go | Delegates UUID/blob conversion and ID generation to types. |
| sdk/mpr/utils.go | Delegates shared ID/UUID/hash utilities to types and reworks BSON ID conversion. |
| sdk/mpr/reader.go | Delegates blob→UUID conversion to types. |
| sdk/mpr/parser_misc.go | Updates parsed unit return types to mdl/types (Java/JS actions, navigation, images, JSON structure). |
| sdk/mpr/asyncapi.go | Replaces AsyncAPI structs with type aliases to mdl/types and delegates parsing. |
| mdl/visitor/visitor_agenteditor.go | Removes stray blank line. |
| mdl/types/navigation.go | Adds navigation document and related types/specs. |
| mdl/types/mapping.go | Adds JsonStructure/JsonElement/ImageCollection/Image types. |
| mdl/types/java.go | Adds lightweight JavaAction/JavaScriptAction descriptor types. |
| mdl/types/infrastructure.go | Adds shared infra metadata types (versions, units, folders, raw units, rename hits, access types). |
| mdl/types/id.go | Adds shared ID generation, UUID/blob conversions, validation, and hashing. |
| mdl/types/doc.go | Package documentation for mdl/types. |
| mdl/types/asyncapi.go | Adds AsyncAPI parsing/types in mdl/types. |
| mdl/linter/rules/page_navigation_security.go | Switches nav menu item traversal to types.NavMenuItem. |
| mdl/executor/widget_templates.go | Uses types.GenerateID for widget template ID generation. |
| mdl/executor/widget_operations.go | Uses types.GenerateID when creating translation IDs. |
| mdl/executor/widget_engine.go | Uses types.GenerateID for widget template loading and widget IDs. |
| mdl/executor/mock_test_helpers_test.go | Adds shared mock ExecContext/model factories/assertion helpers for executor tests. |
| mdl/executor/helpers.go | Switches folder cache element type + uses types.GenerateID for new folders. |
| mdl/executor/executor.go | Updates executor cache folder/unit info types to mdl/types. |
| mdl/executor/cmd_write_handlers_mock_test.go | Adds mock tests for write handlers using updated backend interfaces/types. |
| mdl/executor/cmd_workflows_write.go | Uses types.GenerateID for workflow element IDs. |
| mdl/executor/cmd_workflows_mock_test.go | Adds workflow show/describe mock tests. |
| mdl/executor/cmd_settings_mock_test.go | Adds settings show/describe mock tests. |
| mdl/executor/cmd_security_write.go | Switches member access/revocation structs to mdl/types. |
| mdl/executor/cmd_security_mock_test.go | Adds mock tests for security show/describe flows. |
| mdl/executor/cmd_rest_clients_mock_test.go | Adds mock tests for REST client show/describe. |
| mdl/executor/cmd_rename.go | Switches rename reporting types from mpr.RenameHit to types.RenameHit. |
| mdl/executor/cmd_published_rest_mock_test.go | Adds mock tests for published REST show/describe. |
| mdl/executor/cmd_pages_mock_test.go | Adds mock tests for pages/snippets/layouts listing behaviors. |
| mdl/executor/cmd_pages_builder_v3_layout.go | Uses types.GenerateID throughout page widget building. |
| mdl/executor/cmd_pages_builder.go | Switches folder cache types to mdl/types. |
| mdl/executor/cmd_odata_mock_test.go | Adds mock tests for OData client/service show/describe. |
| mdl/executor/cmd_odata.go | Uses types.GenerateID for created objects and types.ParseEdmx for metadata parsing. |
| mdl/executor/cmd_notconnected_mock_test.go | Adds not-connected guard tests across handlers. |
| mdl/executor/cmd_navigation_mock_test.go | Adds navigation show/describe mock tests using mdl/types. |
| mdl/executor/cmd_navigation.go | Switches navigation spec/profile/menu types to mdl/types. |
| mdl/executor/cmd_modules_mock_test.go | Adds modules listing mock test using types.UnitInfo. |
| mdl/executor/cmd_misc_mock_test.go | Adds version show mock test using types.ProjectVersion. |
| mdl/executor/cmd_microflows_mock_test.go | Adds microflows/nanoflows show/describe mock tests. |
| mdl/executor/cmd_microflows_create.go | Uses types.GenerateID for created microflow IDs/parameters. |
| mdl/executor/cmd_microflows_builder_workflow.go | Uses types.GenerateID when constructing workflow-related microflow actions. |
| mdl/executor/cmd_microflows_builder_graph.go | Uses types.GenerateID for graph object IDs. |
| mdl/executor/cmd_microflows_builder_flows.go | Uses types.GenerateID for flow/merge/event IDs. |
| mdl/executor/cmd_microflows_builder_control.go | Uses types.GenerateID for control-flow node IDs. |
| mdl/executor/cmd_microflows_builder_annotations.go | Uses types.GenerateID for annotation/event IDs. |
| mdl/executor/cmd_mermaid_mock_test.go | Adds mermaid describe mock test for domain model graphs. |
| mdl/executor/cmd_jsonstructures_mock_test.go | Adds JSON structure show mock test using types.JsonStructure. |
| mdl/executor/cmd_jsonstructures.go | Switches JSON structure operations/helpers to mdl/types utilities/types. |
| mdl/executor/cmd_javascript_actions_mock_test.go | Adds JS action show/describe mock tests using types.JavaScriptAction. |
| mdl/executor/cmd_javaactions_mock_test.go | Adds Java action show/describe mock tests. |
| mdl/executor/cmd_javaactions.go | Uses types.GenerateID for Java action construction. |
| mdl/executor/cmd_import_mappings_mock_test.go | Adds import mappings show mock test. |
| mdl/executor/cmd_import_mappings.go | Switches JSON-structure element map types to types.JsonElement. |
| mdl/executor/cmd_imagecollections_mock_test.go | Adds image collection show/describe mock tests using types.ImageCollection. |
| mdl/executor/cmd_imagecollections.go | Switches image collection construction/list lookup to mdl/types. |
| mdl/executor/cmd_fragments_mock_test.go | Adds fragment show mock tests. |
| mdl/executor/cmd_folders.go | Switches folder info parameter types to types.FolderInfo. |
| mdl/executor/cmd_export_mappings_mock_test.go | Adds export mappings show mock test. |
| mdl/executor/cmd_export_mappings.go | Switches JSON-structure element map types to types.JsonElement. |
| mdl/executor/cmd_enumerations_mock_test.go | Refactors enumeration tests to use shared mock helpers + adds describe/not-found coverage. |
| mdl/executor/cmd_entities_mock_test.go | Adds entity show mock tests. |
| mdl/executor/cmd_entities.go | Uses types.GenerateID for entity creation/alter IDs (attrs, rules, indexes). |
| mdl/executor/cmd_dbconnection_mock_test.go | Adds database connection show/describe mock tests. |
| mdl/executor/cmd_datatransformer_mock_test.go | Adds data transformer list/describe mock tests. |
| mdl/executor/cmd_constants_mock_test.go | Adds constants show/describe/not-found mock tests. |
| mdl/executor/cmd_businessevents_mock_test.go | Adds business event service show/describe mock tests. |
| mdl/executor/cmd_businessevents.go | Uses types.GenerateID to generate channel names. |
| mdl/executor/cmd_associations_mock_test.go | Adds association show mock tests. |
| mdl/executor/cmd_agenteditor_write.go | Formatting alignment changes in agent creation struct literal. |
| mdl/executor/cmd_agenteditor_models.go | Formatting alignment changes in agent editor model creation struct literal. |
| mdl/executor/cmd_agenteditor_mock_test.go | Adds agent editor show/describe mock tests. |
| mdl/catalog/builder_references.go | Switches nav menu item reference extraction to types.NavMenuItem. |
| mdl/catalog/builder_navigation.go | Switches navigation menu item traversal to types.NavMenuItem. |
| mdl/catalog/builder_contract.go | Switches EDMX/AsyncAPI parsing to types.ParseEdmx / types.ParseAsyncAPI. |
| mdl/catalog/builder.go | Updates CatalogReader interface to use mdl/types for infra/navigation/json structure types. |
| mdl/backend/workflow.go | Updates ImageBackend types to types.ImageCollection. |
| mdl/backend/security.go | Updates security backend params to types.EntityMemberAccess / types.EntityAccessRevocation. |
| mdl/backend/navigation.go | Updates navigation backend types to mdl/types. |
| mdl/backend/mpr/backend.go | Adapts MPR backend to mdl/types via conversion helpers. |
| mdl/backend/mock/mock_workflow.go | Updates mock backend image methods to types.ImageCollection. |
| mdl/backend/mock/mock_security.go | Updates mock backend revoke signature to types.EntityAccessRevocation. |
| mdl/backend/mock/mock_navigation.go | Updates mock backend navigation types/specs to mdl/types. |
| mdl/backend/mock/mock_module.go | Updates mock backend folder list to types.FolderInfo. |
| mdl/backend/mock/mock_mapping.go | Updates mock backend JSON structure methods to types.JsonStructure. |
| mdl/backend/mock/mock_java.go | Updates mock backend Java/JS action list/read types to mdl/types. |
| mdl/backend/mock/mock_infrastructure.go | Updates mock backend rename/rawunit/unit/widget signatures to mdl/types. |
| mdl/backend/mock/mock_connection.go | Updates mock backend version/project version types to mdl/types. |
| mdl/backend/mock/backend.go | Updates mock backend function fields to mdl/types across infra/navigation/mapping/java/image/widget/version. |
| mdl/backend/mapping.go | Updates MappingBackend JSON structure methods to mdl/types. |
| mdl/backend/java.go | Updates JavaBackend lightweight Java/JS action methods to mdl/types. |
| mdl/backend/infrastructure.go | Updates infra backend rename/rawunit/unit/widget types to mdl/types. |
| mdl/backend/doc.go | Updates backend package docs to reflect mdl/types extraction + conversion in mdl/backend/mpr. |
| mdl/backend/connection.go | Updates connection/version + folder list types to mdl/types. |
| internal/marketplace/types.go | Minor comment formatting cleanup. |
| cmd/mxcli/project_tree.go | Uses types.NavMenuItem when building navigation tree nodes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 108 out of 111 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Code ReviewLet me review this PR against the checklist provided. The PR is titled "refactor: extract shared types and utility functions to mdl/types" and its purpose is to extract shared types from sdk/mpr into a new mdl/types package to decouple the executor layer from the MPR reader/writer stack. Let me go through the checklist items: Overlap & duplication
MDL syntax design (for new or modified MDL syntax)The PR doesn't appear to add or modify any MDL syntax - it's purely a refactor of internal types. There are no changes to grammar rules, AST nodes, or executor commands that would affect MDL syntax. So this section doesn't apply. Full-stack consistency (for MDL features)Since this is a refactor that doesn't add or modify MDL syntax/features, this section doesn't apply. Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
Looking at the diff, I can see:
The refactor appears to be correctly implemented:
I don't see any obvious issues with the refactor. The changes appear to be mechanical and consistent. The PR description mentions that it's "purely mechanical import swaps with no logic changes" for the migration of 65 files, which aligns with what I see in the diff. One thing I noticed: in the diff for mdl/backend/mpr/convert.go, there are some inconsistencies in the conversion functions. For example: func convertUnitInfoSlice(in []*types.UnitInfo, err error) ([]*types.UnitInfo, error) {This function takes func convertUnitInfoSlice(in []*types.UnitInfo, err error) ([]*types.UnitInfo, error) {
if err != nil || in == nil {
return nil, err
}
out := make([]*types.UnitInfo, len(in))
for i, u := range in {
out[i] = &types.UnitInfo{
ID: u.ID, ContainerID: u.ContainerID,
ContainmentName: u.ContainmentName, Type: u.Type,
}
}
return out, nil
}This is just copying the values, not converting from mpr types. This seems wrong. Let me check other similar functions. Looking at func convertFolderInfoSlice(in []*types.FolderInfo, err error) ([]*types.FolderInfo, error) {This also takes types.FolderInfo as input, but should be taking mpr.FolderInfo. I think there's a mix-up in the parameter types. The functions named Similarly, functions named Let me verify with one function that looks correct:
But then: I see the issue. The parameter types in many of the conversion functions are incorrect. They're using types.* as input when they should be using mpr.* as input for the convert functions. Let me check a few more:
So there's inconsistency in the conversion functions. Some have the correct parameter types, others have them swapped. This is a critical issue because if the conversion functions are wrong, the backend won't work correctly. Let me also check the unconvert functions:
Actually, looking at the implementation: func unconvertJsonStructure(in *types.JsonStructure) *types.JsonStructure {
js := &types.JsonStructure{
// ... copying fields from in
}
// ...
return js
}This is definitely wrong. It should be returning *mpr.JsonStructure. Similarly, This is a significant bug in the conversion layer. Let me also check if there are any missing conversions. For example, I see references to Actually, looking more carefully at the backend.go diff: func (b *MprBackend) Version() types.MPRVersion { return convertMPRVersion(b.reader.Version()) }
func (b *MprBackend) ProjectVersion() *types.ProjectVersion { return convertProjectVersion(b.reader.ProjectVersion()) }These look correct based on the conversion functions I saw earlier. But for the FolderInfo: func (b *MprBackend) ListFolders() ([]*types.FolderInfo, error) { return convertFolderInfoSlice(b.reader.ListFolders()) }If So there must be a mismatch between what I'm seeing in the diff and what the actual code is. Let me look again at the convertFolderInfoSlice function in the diff: func convertFolderInfoSlice(in []*types.FolderInfo, err error) ([]*types.FolderInfo, error) {But it should be: func convertFolderInfoSlice(in []*mpr.FolderInfo, err error) ([]*types.FolderInfo, error) {Similarly for the implementation. I believe there's an error in the diff as presented, or perhaps I'm misreading it. Let me check the actual text again. Actually, looking at the diff more carefully: +func convertFolderInfoSlice(in []*types.FolderInfo, err error) ([]*types.FolderInfo, error) {
+ if err != nil || in == nil {
+ return nil, err
+ }
+ out := make([]*types.FolderInfo, len(in))
+ for i, f := range in {
+ out[i] = &types.FolderInfo{ID: f.ID, ContainerID: f.ContainerID, Name: f.Name}
+ }
+ return out, nil
+}This is definitely wrong. The parameter However, the implementation copies the fields directly: out[i] = &types.FolderInfo{ID: f.ID, ContainerID: f.ContainerID, Name: f.Name}If But if So I suspect that in the actual code, the types.FolderInfo and mpr.FolderInfo have compatible structures, and the parameter type in the diff is incorrect. Let me check another one: +func convertUnitInfoSlice(in []*types.UnitInfo, err error) ([]*types.UnitInfo, error) {
+ if err != nil || in == nil {
+ return nil, err
+ }
+ out := make([]*types.UnitInfo, len(in))
+ for i, u := range in {
+ out[i] = &types.UnitInfo{
+ ID: u.ID, ContainerID: u.ContainerID,
+ ContainmentName: u.ContainmentName, Type: u.Type,
+ }
+ }
+ return out, nil
+}Same issue - parameter should be But then I see: +func convertRenameHitSlice(in []mpr.RenameHit, err error) ([]types.RenameHit, error) {
+ if err != nil || in == nil {
+ return nil, err
+ }
+ out := make([]types.RenameHit, len(in))
+ for i, h := range in {
+ out[i] = types.RenameHit{UnitID: h.UnitID, UnitType: h.UnitType, Name: h.Name, Count: h.Count}
+ }
+ return out, nil
+}This one correctly has And: +func convertRawUnitInfoSlice(in []*mpr.RawUnitInfo, err error) ([]*types.RawUnitInfo, error) {
+ if err != nil || in == nil {
+ return nil, err
+ }
+ out := make([]*types.RawUnitInfo, len(in))
+ for i, r := range in {
+ out[i] = &types.RawUnitInfo{
+ ID: r.ID, QualifiedName: r.QualifiedName, Type: r.Type,
+ ModuleName: r.ModuleName, Contents: r.Contents,
+ }
+ }
+ return out, nil
+}This one is correct. So it seems like about half of the conversion functions have the correct parameter types, and half have them wrong (showing types.* instead of mpr.*). This is likely an error in how the diff was generated or presented, but based on what I can see in the diff, there are inconsistencies. However, given that:
I think the diff might have errors in how it's showing the parameter types, but the actual implementation is correct. Let me check if there are any other issues. I notice in the mdl/backend/mpr/backend.go file, there are calls like: return convertFolderInfoSlice(b.reader.ListFolders())And b.reader.ListFolders() returns ([]*mpr.FolderInfo, error), so convertFolderInfoSlice must accept []*mpr.FolderInfo. Similarly: return convertUnitInfoSlice(b.reader.ListUnits())So the conversion functions must be correctly typed to accept mpr types and return types. Therefore, I believe the diff as presented has errors in showing the parameter types of the conversion functions (it shows types.* when it should show mpr.* for the input parameters of convert* functions), but the actual code is correct. Another thing to check: the PR mentions that sdk/mpr retains thin delegation wrappers. I don't see those in the diff, but the PR description says they're there for backward compatibility, so they might be in sdk/mpr itself which isn't shown in this diff. Let me check if there are any obvious logic errors. In the convert.go file, I see: func convertJavaScriptActionSlice(in []*types.JavaAction, err error) ([]*types.JavaAction, error) {This should be But then: func convertJavaScriptActionSlice(in []*types.JavaAction, err error) ([]*types.JavaAction, error) {And the implementation: for i, jsa := range in {
out[i] = convertJavaScriptAction(jsa)
}If in is []*types.JavaAction, then jsa is *types.JavaAction, and convertJavaScriptAction expects *types.JavaAction (based on its signature later in the file), so this would work but wouldn't actually convert from mpr to types. Then later: func convertJavaScriptActionPtr(in *types.JavaScriptAction, err error) (*types.JavaScriptAction, error) {This should be *mpr.JavaScriptAction. And: func convertJavaScriptAction(in *types.JavaScriptAction) *types.JavaScriptAction {This should take *mpr.JavaScriptAction. But then I see: func convertJavaScriptAction(in *types.JavaScriptAction) *types.JavaScriptAction {
return &types.JavaScriptAction{
BaseElement: in.BaseElement,
// ...
}
}If in is *types.JavaAction, this is just copying types.JavaAction to types.JavaAction. But if in is *mpr.JavaScriptAction, and the fields are compatible, then this makes sense. So again, I think the diff has the parameter types wrong in the function signatures, but the implementation suggests they're correct. Let me look at one that's definitely shown correctly in the diff: +func convertRawUnitInfoPtr(in *mpr.RawUnitInfo, err error) (*types.RawUnitInfo, error) {
+ if err != nil || in == nil {
+ return nil, err
+ }
+ return &types.RawUnitInfo{
+ ID: in.ID, QualifiedName: in.QualifiedName, Type: in.Type,
+ ModuleName: in.ModuleName, Contents: in.Contents,
+ }, nil
+}This one correctly shows So the pattern is:
And looking at the ones that are shown correctly in the diff, they follow this pattern. Therefore, I believe the diff as presented has errors in how it's displaying some of the function signatures (showing types.* for input when it should show mpr.*), but the actual code is correct based on:
Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 108 out of 111 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor Issues
What Looks Good
RecommendationRequest changes due to the type parameter errors in the conversion functions that would prevent compilation. Once these are fixed, the PR is ready for approval. The errors are:
Fix these conversion function signatures and the PR will be correct. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 108 out of 111 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the reviews. RE the conversion function parameter types flagged in the third review: these are not bugs. The |
AI Code ReviewCritical IssuesIncorrect Conversion Function SignaturesMultiple conversion functions in "convert" functions (sdk/mpr → mdl/types) with wrong input type:
"unconvert" functions (mdl/types → sdk/mpr) with wrong output type:
These errors would prevent the code from compiling or cause incorrect type conversions at runtime. Missing Tests for New PackageThe PR creates a new
RecommendationRequest changes to fix the conversion function type signatures and add tests for the new Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 106 out of 109 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Code ReviewCritical Issues(None found) Moderate Issues(None found) Minor Issues
What Looks Good
RecommendationApprove this PR. The refactoring successfully addresses the coupling issue described in the PR body, maintains backward compatibility, makes only mechanical changes with no logic alterations, and includes appropriate test coverage. The changes are cleanly scoped to a single concern (extracting shared types) and follow the project's architectural principles. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
- 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+)
|
RE the "Critical Issues" about conversion function signatures (comment from 18:10:13) and the missing tests note (comment from 18:15:46): Conversion signatures are correct — not bugs. The // sdk/mpr/reader_types.go
type FolderInfo = types.FolderInfo
type JavaAction = types.JavaAction
type JsonStructure = types.JsonStructure
// ... (16 aliases total)Because these are This is now proven by var _ *types.JavaAction = new(mpr.JavaAction)
var typesSlice []*types.FolderInfo
var mprSlice []*mpr.FolderInfo = typesSlice // compiles because same typeTests added in f2c8e12 (+1094 lines across 5 test files):
Also fixed a real bug caught by the new tests: |
AI Code ReviewOkay, let me review this pull request. The user is asking me to review a PR that extracts shared types from First, I need to understand what the PR is doing based on the description and the diff provided. The PR body says:
Looking at the diff, I can see:
Now I need to review this against the checklist provided in CLAUDE.md. Let me go through the checklist items: Overlap & duplication
MDL syntax designThe PR doesn't appear to add or modify any MDL syntax (grammar rules, keywords, statement types). It's purely a refactoring of internal types. So this section doesn't apply. Full-stack consistency for MDL featuresSince this isn't adding new MDL syntax or features, this section doesn't apply. Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
Now, let me look for specific issues in the diff:
Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 110 out of 113 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- 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)
AI Code ReviewReview SummaryThis PR refactors the codebase to extract shared types from What Looks Good
Minor Observations
Checklist Compliance
The PR successfully achieves its goal of reducing coupling between the MDL executor and MPR implementation while maintaining full backward compatibility. The changes are mechanical, consistent, and well-tested for the conversion layer. Recommendation: Approve Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Code ReviewOverviewThis is a significant architectural refactoring (113 files, +7647/-2090 lines) that decouples the Positive Aspects
Issues & Concerns1. Spec types are duplicated, not aliased — medium maintenance risk
This means:
Suggestion: Either alias these too ( 2. Conversion functions are untested — medium risk
If a field is added to Suggestion: Add at least one round-trip test per conversion family — construct a fully-populated 3. No tests for reverse (unconvert) paths The Suggestion: Add unit tests for each 4. Manual field copying is a long-term maintenance burden The conversion functions do explicit field-by-field copies. Any struct evolution (new field added, field renamed) requires updating the copy in This is somewhat unavoidable given the approach, but it's worth flagging. A Checklist Against Project Standards
VerdictApprove with suggestions. The architectural goal is correct, the extraction is complete, and the alias strategy is the right implementation choice. The two concrete risks are (a) the duplicated Spec types diverging silently and (b) conversion functions having no field-preservation tests. Neither blocks merging, but both should be tracked as follow-up work — the longer they go without tests, the more expensive they become to retrofit. |
Exercise all convert (mpr→types) and unconvert (types→mpr) functions with fully-populated structs. Covers nil handling, error passthrough, and recursive structures (NavMenuItem, JsonElement). Addresses review feedback from PR mendixlabs#235 (conversion round-trip tests and unconvert path coverage).
Why
The executor layer (
mdl/) importssdk/mprfor shared type definitions —EdmxDocument,JavaAction,NavigationProfile,JsonStructure, and dozens more. This coupling means any package that needs these types must pull in the entire MPR reader/writer stack, preventing alternative backend implementations and making themdl/subtree unsuitable for lightweight compilation targets.This PR extracts all shared types into a new
mdl/typespackage that has no dependency onsdk/mpr.What changed
New packages:
mdl/types/— 9 files containing all shared domain types previously defined insdk/mpr/reader_types.go,sdk/mpr/edmx.go,sdk/mpr/asyncapi.go,sdk/mpr/utils.go, andsdk/mpr/writer_jsonstructure.gomdl/backend/mpr/convert.go— bidirectional conversion layer betweensdk/mprreader/writer types andmdl/typesExtracted types and functions:
GenerateID,GenerateDeterministicID,BlobToUUID,UUIDToBlob,ValidateID,HashMPRVersion,ProjectVersion,FolderInfo,UnitInfo,RenameHit,RawUnit,RawUnitInfo,RawCustomWidgetType,EntityMemberAccess,EntityAccessRevocationJavaAction,JavaScriptActionJsonStructure,JsonElement,ImageCollection,ImageNavigationDocument,NavigationProfile,NavHomePage,NavRoleBasedHome,NavMenuItem,NavOfflineEntity, plus spec typesEdm*types andParseEdmxAsyncAPI*types andParseAsyncAPIPrettyPrintJSON,BuildJsonElementsFromSnippet, datetime normalizationMigration (65 files): All executor handlers, catalog builders, backend interfaces, and mock implementations updated from
mpr.*totypes.*— purely mechanical import swaps with no logic changes.Backward compatibility:
sdk/mprretains thin delegation wrappers and returnstypes.*directly, so external callers are unaffected.Stack
PR 1/5 in the shared-types refactoring stack.
Merge order: #232 → this → #236 → #237 → #238 → #239