refactor: code quality — deterministic output, doc comments, naming#239
Draft
retran wants to merge 19 commits intomendixlabs:mainfrom
Draft
refactor: code quality — deterministic output, doc comments, naming#239retran wants to merge 19 commits intomendixlabs:mainfrom
retran wants to merge 19 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.
This PR completes the shared-types refactoring by moving executor/catalog code off sdk/mpr-specific types and raw BSON manipulation, improving determinism and testability while tightening correctness around IDs and JSON handling.
Changes:
- Refactors many executor/catalog paths to use
mdl/typesand backend abstractions (incl. page mutation) instead of directsdk/mpr/BSON operations. - Introduces
mdl/bsonutilto perform UUID↔BSON Binary conversions without pulling in CGO-heavy dependencies. - Adds a large set of MockBackend-based tests for SHOW/DESCRIBE commands and connection failure paths.
Reviewed changes
Copilot reviewed 112 out of 155 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| mdl/executor/cmd_widgets.go | Switch widget updates from raw BSON edits to backend PageMutator flow |
| mdl/executor/cmd_structure.go | Rename “reader” terminology to “backend” and update helper naming |
| mdl/executor/cmd_settings_mock_test.go | Add MockBackend coverage for settings output |
| mdl/executor/cmd_security_write.go | Move security write DTOs to mdl/types and normalize error wrapping |
| mdl/executor/cmd_security_mock_test.go | Add MockBackend coverage for security show/describe flows |
| mdl/executor/cmd_rest_clients_mock_test.go | Add MockBackend coverage for REST clients show/describe |
| mdl/executor/cmd_rename.go | Switch rename hit types from sdk/mpr to mdl/types |
| mdl/executor/cmd_published_rest_mock_test.go | Add MockBackend coverage for published REST services |
| mdl/executor/cmd_pages_mock_test.go | Add MockBackend coverage for pages/snippets/layouts show |
| mdl/executor/cmd_pages_create_v3.go | Page/snippet creation uses backend references; wires widget backend |
| mdl/executor/cmd_pages_builder_v3_pluggable.go | Remove BSON-based pluggable widget builder implementation |
| mdl/executor/cmd_pages_builder_v3_layout.go | Replace ID generation from mpr.GenerateID to types.GenerateID |
| mdl/executor/cmd_pages_builder_input_filters.go | Remove BSON-based filter widget construction helpers |
| mdl/executor/cmd_pages_builder_input_cloning.go | Remove BSON deep-clone helpers (moved/centralized elsewhere) |
| mdl/executor/cmd_pages_builder_input.go | Shift snippet/microflow resolution to backend APIs |
| mdl/executor/cmd_pages_builder.go | Make pageBuilder backend-driven; introduce widgetBackend + types-based folders |
| mdl/executor/cmd_odata_mock_test.go | Add MockBackend coverage for OData client/service show/describe |
| mdl/executor/cmd_odata.go | Switch ID generation and EDMX parsing to mdl/types |
| mdl/executor/cmd_notconnected_mock_test.go | Add negative-path tests for “not connected” handling across commands |
| mdl/executor/cmd_navigation_mock_test.go | Add MockBackend coverage for navigation show/describe |
| mdl/executor/cmd_navigation.go | Move navigation DTOs/specs to mdl/types |
| mdl/executor/cmd_modules_mock_test.go | Add MockBackend coverage for module listing and counting |
| mdl/executor/cmd_misc_mock_test.go | Add MockBackend coverage for version display |
| mdl/executor/cmd_microflows_mock_test.go | Add MockBackend coverage for microflows show/describe |
| mdl/executor/cmd_microflows_helpers.go | Update enum lookup helpers to use backend instead of reader |
| mdl/executor/cmd_microflows_create.go | Switch ID generation + flowBuilder wiring to backend |
| mdl/executor/cmd_microflows_builder_workflow.go | Use types.GenerateID for workflow/microflow object creation |
| mdl/executor/cmd_microflows_builder_graph.go | Use types.GenerateID for microflow graph objects |
| mdl/executor/cmd_microflows_builder_flows.go | Use types.GenerateID and pass backend through builders |
| mdl/executor/cmd_microflows_builder_control.go | Use types.GenerateID and pass backend through loop/while builders |
| mdl/executor/cmd_microflows_builder_annotations.go | Use types.GenerateID for annotations and flows |
| mdl/executor/cmd_microflows_builder.go | Rename reader field to backend for reference resolution |
| mdl/executor/cmd_mermaid_mock_test.go | Add MockBackend coverage for mermaid domain model description |
| mdl/executor/cmd_lint.go | Update lint context to use executor Reader accessor |
| mdl/executor/cmd_jsonstructures_mock_test.go | Add MockBackend coverage for JSON structures show |
| mdl/executor/cmd_jsonstructures.go | Move JSON structure model/types and JSON utilities to mdl/types |
| mdl/executor/cmd_javascript_actions_mock_test.go | Add MockBackend coverage for JavaScript actions show/describe |
| mdl/executor/cmd_javaactions_mock_test.go | Add MockBackend coverage for Java actions show/describe |
| mdl/executor/cmd_javaactions.go | Switch Java action ID generation to types.GenerateID |
| mdl/executor/cmd_import_mappings_mock_test.go | Add MockBackend coverage for import mappings show |
| mdl/executor/cmd_import_mappings.go | Switch JSON element types + attribute typing resolution to backend + mdl/types |
| mdl/executor/cmd_imagecollections_mock_test.go | Add MockBackend coverage for image collections show/describe |
| mdl/executor/cmd_imagecollections.go | Move image collection types to mdl/types |
| mdl/executor/cmd_fragments_mock_test.go | Add Mock-context tests for fragment listing |
| mdl/executor/cmd_folders.go | Switch folder info type from sdk/mpr to mdl/types |
| mdl/executor/cmd_features.go | Update documentation wording from reader to connection state |
| mdl/executor/cmd_export_mappings_mock_test.go | Add MockBackend coverage for export mappings show |
| mdl/executor/cmd_export_mappings.go | Switch JSON element types + attribute typing resolution to backend + mdl/types |
| mdl/executor/cmd_enumerations_mock_test.go | Simplify and expand enumeration MockBackend tests (incl. describe) |
| mdl/executor/cmd_entities_mock_test.go | Add MockBackend coverage for entities show/filter |
| mdl/executor/cmd_entities.go | Switch ID generation + error wrapping to types/mdlerrors |
| mdl/executor/cmd_diff_local.go | Delegate microflow parsing from raw maps to backend |
| mdl/executor/cmd_dbconnection_mock_test.go | Add MockBackend coverage for DB connections show/describe |
| mdl/executor/cmd_datatransformer_mock_test.go | Add MockBackend coverage for data transformers list/describe |
| mdl/executor/cmd_contract.go | Move EDMX/AsyncAPI parsing and DTOs to mdl/types |
| mdl/executor/cmd_constants_mock_test.go | Add MockBackend coverage for constants show/describe |
| mdl/executor/cmd_catalog.go | Update docs and local executor wiring from reader→backend |
| mdl/executor/cmd_businessevents_mock_test.go | Add MockBackend coverage for business event services show/describe |
| mdl/executor/cmd_businessevents.go | Switch ID generation to types.GenerateID |
| mdl/executor/cmd_associations_mock_test.go | Add MockBackend coverage for associations show/filter |
| mdl/executor/cmd_agenteditor_mock_test.go | Add MockBackend coverage for agent editor show/describe commands |
| mdl/executor/bugfix_test.go | Rename reader reference to backend in regression test |
| mdl/executor/bugfix_regression_test.go | Update test comment to backend terminology |
| mdl/catalog/builder_references.go | Switch navigation menu item DTO type to mdl/types |
| mdl/catalog/builder_navigation.go | Switch navigation menu item DTO type to mdl/types |
| mdl/catalog/builder_contract.go | Switch EDMX/AsyncAPI parsing to mdl/types |
| mdl/catalog/builder.go | Switch catalog reader shared DTO types to mdl/types |
| mdl/bsonutil/bsonutil.go | Add CGO-free BSON Binary ↔ UUID conversion utilities |
| mdl/backend/workflow.go | Remove unrelated backend interface definitions from workflow backend file |
| mdl/backend/security.go | Move member access DTO types to mdl/types |
| mdl/backend/page.go | Clarify snippet API expectations in comments |
| mdl/backend/navigation.go | Move navigation DTO/spec types to mdl/types |
| mdl/backend/mpr/datagrid_builder_test.go | Update tests to mpr backend pkg; use bsonutil + bytes.Equal |
| mdl/backend/mpr/convert.go | Add conversion glue between sdk/mpr and mdl/types |
| mdl/backend/mock/mock_workflow.go | Switch image collection DTOs to mdl/types |
| mdl/backend/mock/mock_security.go | Switch entity access revocation DTO to mdl/types |
| mdl/backend/mock/mock_navigation.go | Switch navigation DTO/spec types to mdl/types |
| mdl/backend/mock/mock_mutation.go | Add mock implementations for mutation/serialization/widget builder backends |
| mdl/backend/mock/mock_module.go | Switch folder DTOs to mdl/types |
| mdl/backend/mock/mock_microflow.go | Add ParseMicroflowFromRaw hook to MockBackend |
| mdl/backend/mock/mock_mapping.go | Switch JSON structure DTOs to mdl/types |
| mdl/backend/mock/mock_java.go | Switch Java/JS action DTOs to mdl/types |
| mdl/backend/mock/mock_infrastructure.go | Switch core DTOs to mdl/types and expand AgentEditor mocks |
| mdl/backend/mock/mock_connection.go | Switch version DTOs to mdl/types |
| mdl/backend/mock/backend.go | Update MockBackend fields and interfaces to mdl/types; add new backend hooks |
| mdl/backend/microflow.go | Add ParseMicroflowFromRaw to MicroflowBackend interface |
| mdl/backend/mapping.go | Switch JSON structure DTOs to mdl/types and document DeleteJsonStructure contract |
| mdl/backend/java.go | Switch Java/JS action DTOs to mdl/types |
| mdl/backend/infrastructure.go | Switch raw-unit and metadata DTOs to mdl/types; add Settings/Image/ScheduledEvent backends |
| mdl/backend/doc.go | Update backend package docs for mdl/types + conversion layer |
| mdl/backend/connection.go | Switch version/folder DTOs to mdl/types and improve interface docs |
| mdl/backend/backend.go | Expand FullBackend to include mutation/serialization/widget builder backends |
| examples/create_datagrid2_page/main.go | Wire executor to mpr backend via backend factory |
| cmd/mxcli/project_tree.go | Switch menu item DTO type to mdl/types |
| cmd/mxcli/main.go | Wire CLI executor to mpr backend via backend factory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+16
to
+21
| 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.
MockBackend's default OpenPageForMutation implementation returns (nil, nil). Callers (e.g., widget updates) will treat this as success and then dereference the mutator, causing a panic. Return a non-nil error by default (e.g., "not implemented"), or provide a safe no-op mutator implementation that satisfies the interface.
| if m.ParseMicroflowFromRawFunc != nil { | ||
| return m.ParseMicroflowFromRawFunc(raw, unitID, containerID) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
ParseMicroflowFromRaw now sits on the backend interface and is used by cmd_diff_local.go. Returning nil by default in the mock backend can cause downstream nil dereferences in code that assumes a parsed microflow is always returned. Consider returning a minimal non-nil *microflows.Microflow (or adjusting the consuming code to handle nil safely) so mock-based executions don't crash.
Suggested change
| return nil | |
| return µflows.Microflow{} |
Comment on lines
+77
to
+83
| // 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 pb.backend. Update the comment to match the new backend-based architecture (e.g., "underlying backend").
Comment on lines
+13
to
+30
| // IDToBsonBinary converts a hex UUID string to a BSON binary value. | ||
| // If the input is not a valid UUID, a new random ID is generated as a fallback. | ||
| // This matches the legacy sdk/mpr behavior where callers expect a valid binary | ||
| // result without error handling. Consider using ValidateID first if strict | ||
| // validation is needed. | ||
| 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, | ||
| } | ||
| } | ||
|
|
||
| // BsonBinaryToID converts a BSON binary value to a hex UUID string. | ||
| func BsonBinaryToID(bin primitive.Binary) string { |
There was a problem hiding this comment.
This new utility is security-/correctness-adjacent (ID conversion that affects persisted BSON). Add focused unit tests covering: (1) valid UUID round-trip (ID→Binary→ID), (2) invalid UUID input (ensuring it generates a valid 16-byte result and does not panic), and (3) edge-case binary lengths for BsonBinaryToID.
Suggested change
| // IDToBsonBinary converts a hex UUID string to a BSON binary value. | |
| // If the input is not a valid UUID, a new random ID is generated as a fallback. | |
| // This matches the legacy sdk/mpr behavior where callers expect a valid binary | |
| // result without error handling. Consider using ValidateID first if strict | |
| // validation is needed. | |
| 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, | |
| } | |
| } | |
| // BsonBinaryToID converts a BSON binary value to a hex UUID string. | |
| func BsonBinaryToID(bin primitive.Binary) string { | |
| func validUUIDBlobOrGenerated(id string) []byte { | |
| blob := types.UUIDToBlob(id) | |
| if len(blob) == 16 { | |
| return blob | |
| } | |
| return types.UUIDToBlob(types.GenerateID()) | |
| } | |
| // IDToBsonBinary converts a hex UUID string to a BSON binary value. | |
| // If the input is not a valid UUID, a new random ID is generated as a fallback. | |
| // This matches the legacy sdk/mpr behavior where callers expect a valid binary | |
| // result without error handling. Consider using ValidateID first if strict | |
| // validation is needed. | |
| func IDToBsonBinary(id string) primitive.Binary { | |
| return primitive.Binary{ | |
| Subtype: 0x00, | |
| Data: validUUIDBlobOrGenerated(id), | |
| } | |
| } | |
| // BsonBinaryToID converts a BSON binary value to a hex UUID string. | |
| // If the input does not contain exactly 16 bytes, a new random ID is generated | |
| // as a fallback to preserve the legacy expectation of a valid ID result. | |
| func BsonBinaryToID(bin primitive.Binary) string { | |
| if len(bin.Data) != 16 { | |
| return types.GenerateID() | |
| } |
This was referenced Apr 19, 2026
- 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
retran
force-pushed
the
feature/code-quality-polish
branch
from
bebb7e6 to
7b93cef
Compare
…ert.go signatures
…lePropertyOp; add TODO comments in panic stubs
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.
retran
force-pushed
the
feature/code-quality-polish
branch
from
7b93cef to
ccdacbd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
PRs #235–#238 performed the structural refactoring. This final PR addresses code quality issues found during 10 iterations of self-review across the entire branch (129 files). No behavioral changes — only correctness fixes, documentation, and cleanup.
What changed (incremental from PR #238)
62 files changed, +345/-346 (net -1 lines)
Correctness:
asyncapi.go,widget_builder.go,datagrid_builder.go,cmd_alter_page.gofor deterministic serialization outputGenerateDeterministicID±2^53) injson_utils.golen(s) < 19panic guard in datetime normalizationTestHash_EmptyInputto expect correct SHA-256 of empty inputDocumentation:
ModuleBackend,FolderBackend,WorkflowMutatormethods,WidgetObjectBuilder,PageMutator)WidgetObjectBuilderdeferred error handling designNaming consistency:
readerfield/references tobackendacross executor (flowBuilder.reader,countByModuleFromReader, stale comments)xmlDataServices→xmlDataServicereader_types.goCleanup:
convert.go— remove redundant deep-copy conversions where types are already identical (passthrough)go fmtformatting across 17 filessdk/mpr/writer_core.gobackward-compatible fallback for invalid/empty UUIDs (tests use placeholder IDs)Stack
PR 5/5 (final) in the shared-types refactoring stack.
Merge order: #232 → #235 → #236 → #237 → #238 → this