fix: address PR #224 review feedback

retran · retran · commit efb7f1c3f7e5 · 2026-04-17T15:02:02.000+02:00
- NotConnected → NotConnectedWrite in all write-requiring handlers
- NewNotConnectedMsg constructor for custom hints (cmd_features.go)
- Timeout returns ValidationError instead of plain fmt.Errorf
- Standardize RawUnitBackend on model.ID
- Registry panic prefix corrected to 'registry:'
- Full qualified names in AlreadyExists/NotFound messages
- Import/export mapping: distinguish not-found from backend errors
- Published REST: propagate backend errors, include qualified names
diff --git a/mdl/backend/infrastructure.go b/mdl/backend/infrastructure.go
@@ -19,7 +19,7 @@ type RenameBackend interface {
 // manipulate raw BSON (e.g. widget patching, alter page/workflow).
 type RawUnitBackend interface {
 	GetRawUnit(id model.ID) (map[string]any, error)
-	GetRawUnitBytes(id string) ([]byte, error)
+	GetRawUnitBytes(id model.ID) ([]byte, error)
 	ListRawUnitsByType(typePrefix string) ([]*mpr.RawUnit, error)
 	ListRawUnits(objectType string) ([]*mpr.RawUnitInfo, error)
 	GetRawUnitByName(objectType, qualifiedName string) (*mpr.RawUnitInfo, error)
diff --git a/mdl/backend/mock/backend.go b/mdl/backend/mock/backend.go
@@ -238,7 +238,7 @@ type MockBackend struct {
 
 	// RawUnitBackend
 	GetRawUnitFunc            func(id model.ID) (map[string]any, error)
-	GetRawUnitBytesFunc       func(id string) ([]byte, error)
+	GetRawUnitBytesFunc       func(id model.ID) ([]byte, error)
 	ListRawUnitsByTypeFunc    func(typePrefix string) ([]*mpr.RawUnit, error)
 	ListRawUnitsFunc          func(objectType string) ([]*mpr.RawUnitInfo, error)
 	GetRawUnitByNameFunc      func(objectType, qualifiedName string) (*mpr.RawUnitInfo, error)
@@ -1574,7 +1574,7 @@ func (m *MockBackend) GetRawUnit(id model.ID) (map[string]any, error) {
 	return nil, nil
 }
 
-func (m *MockBackend) GetRawUnitBytes(id string) ([]byte, error) {
+func (m *MockBackend) GetRawUnitBytes(id model.ID) ([]byte, error) {
 	if m.GetRawUnitBytesFunc != nil {
 		return m.GetRawUnitBytesFunc(id)
 	}
diff --git a/mdl/backend/mpr/backend.go b/mdl/backend/mpr/backend.go
@@ -627,8 +627,8 @@ func (b *MprBackend) RenameDocumentByName(moduleName, oldName, newName string) e
 func (b *MprBackend) GetRawUnit(id model.ID) (map[string]any, error) {
 	return b.reader.GetRawUnit(id)
 }
-func (b *MprBackend) GetRawUnitBytes(id string) ([]byte, error) {
-	return b.reader.GetRawUnitBytes(model.ID(id))
+func (b *MprBackend) GetRawUnitBytes(id model.ID) ([]byte, error) {
+	return b.reader.GetRawUnitBytes(id)
 }
 func (b *MprBackend) ListRawUnitsByType(typePrefix string) ([]*mpr.RawUnit, error) {
 	return b.reader.ListRawUnitsByType(typePrefix)
diff --git a/mdl/errors/errors.go b/mdl/errors/errors.go
@@ -33,6 +33,11 @@ func NewNotConnected() *NotConnectedError {
 	return &NotConnectedError{msg: "not connected to a project"}
 }
 
+// NewNotConnectedMsg creates a NotConnectedError with a custom message.
+func NewNotConnectedMsg(msg string) *NotConnectedError {
+	return &NotConnectedError{msg: msg}
+}
+
 // NewNotConnectedWrite creates a NotConnectedError for write access.
 func NewNotConnectedWrite() *NotConnectedError {
 	return &NotConnectedError{WriteMode: true, msg: "not connected to a project in write mode"}
diff --git a/mdl/executor/cmd_dbconnection.go b/mdl/executor/cmd_dbconnection.go
@@ -40,7 +40,7 @@ func (e *Executor) createDatabaseConnection(stmt *ast.CreateDatabaseConnectionSt
 					return mdlerrors.NewBackend("delete existing connection", err)
 				}
 			} else {
-				return mdlerrors.NewAlreadyExistsMsg("database connection", modName+"."+ex.Name, "use CREATE OR MODIFY to update")
+				return mdlerrors.NewAlreadyExistsMsg("database connection", modName+"."+ex.Name, fmt.Sprintf("database connection already exists: %s.%s (use CREATE OR MODIFY to update)", modName, ex.Name))
 			}
 		}
 	}
diff --git a/mdl/executor/cmd_enumerations.go b/mdl/executor/cmd_enumerations.go
@@ -39,7 +39,7 @@ func (e *Executor) execCreateEnumeration(s *ast.CreateEnumerationStmt) error {
 	// Check if enumeration already exists
 	existingEnum := e.findEnumeration(s.Name.Module, s.Name.Name)
 	if existingEnum != nil && !s.CreateOrModify {
-		return mdlerrors.NewAlreadyExistsMsg("enumeration", s.Name.Module+"."+s.Name.Name, "use CREATE OR MODIFY to update")
+		return mdlerrors.NewAlreadyExistsMsg("enumeration", s.Name.Module+"."+s.Name.Name, fmt.Sprintf("enumeration already exists: %s.%s (use CREATE OR MODIFY to update)", s.Name.Module, s.Name.Name))
 	}
 
 	// Create enumeration values
diff --git a/mdl/executor/cmd_export_mappings.go b/mdl/executor/cmd_export_mappings.go
@@ -84,7 +84,10 @@ func (e *Executor) describeExportMapping(name ast.QualifiedName) error {
 
 	em, err := e.reader.GetExportMappingByQualifiedName(name.Module, name.Name)
 	if err != nil {
-		return mdlerrors.NewNotFoundMsg("export mapping", name.String(), err.Error())
+		if strings.Contains(err.Error(), "not found") {
+			return mdlerrors.NewNotFound("export mapping", name.String())
+		}
+		return mdlerrors.NewBackend("get export mapping", err)
 	}
 
 	if em.Documentation != "" {
@@ -363,7 +366,10 @@ func (e *Executor) execDropExportMapping(s *ast.DropExportMappingStmt) error {
 
 	em, err := e.reader.GetExportMappingByQualifiedName(s.Name.Module, s.Name.Name)
 	if err != nil {
-		return mdlerrors.NewNotFoundMsg("export mapping", s.Name.String(), err.Error())
+		if strings.Contains(err.Error(), "not found") {
+			return mdlerrors.NewNotFound("export mapping", s.Name.String())
+		}
+		return mdlerrors.NewBackend("get export mapping", err)
 	}
 
 	if err := e.writer.DeleteExportMapping(em.ID); err != nil {
diff --git a/mdl/executor/cmd_features.go b/mdl/executor/cmd_features.go
@@ -75,7 +75,7 @@ func (e *Executor) execShowFeatures(s *ast.ShowFeaturesStmt) error {
 	default:
 		// SHOW FEATURES [IN area] — requires project connection
 		if e.reader == nil {
-			return mdlerrors.NewNotConnected()
+			return mdlerrors.NewNotConnectedMsg("not connected to a project\n  hint: use SHOW FEATURES FOR VERSION x.y without a project connection")
 		}
 		rpv := e.reader.ProjectVersion()
 		pv = versions.SemVer{Major: rpv.MajorVersion, Minor: rpv.MinorVersion, Patch: rpv.PatchVersion}
diff --git a/mdl/executor/cmd_import.go b/mdl/executor/cmd_import.go
@@ -197,7 +197,7 @@ func (e *Executor) resolveOneLink(
 	}
 
 	if foundAssoc == nil && foundCross == nil {
-		return nil, mdlerrors.NewNotFoundMsg("association", link.AssociationName, fmt.Sprintf("not found in module %q", targetModule))
+		return nil, mdlerrors.NewNotFoundMsg("association", link.AssociationName, fmt.Sprintf("association %q not found in module %q", link.AssociationName, targetModule))
 	}
 
 	// Extract association info
diff --git a/mdl/executor/cmd_import_mappings.go b/mdl/executor/cmd_import_mappings.go
@@ -84,7 +84,10 @@ func (e *Executor) describeImportMapping(name ast.QualifiedName) error {
 
 	im, err := e.reader.GetImportMappingByQualifiedName(name.Module, name.Name)
 	if err != nil {
-		return mdlerrors.NewNotFoundMsg("import mapping", name.String(), err.Error())
+		if strings.Contains(err.Error(), "not found") {
+			return mdlerrors.NewNotFound("import mapping", name.String())
+		}
+		return mdlerrors.NewBackend("get import mapping", err)
 	}
 
 	if im.Documentation != "" {
@@ -377,7 +380,10 @@ func (e *Executor) execDropImportMapping(s *ast.DropImportMappingStmt) error {
 
 	im, err := e.reader.GetImportMappingByQualifiedName(s.Name.Module, s.Name.Name)
 	if err != nil {
-		return mdlerrors.NewNotFoundMsg("import mapping", s.Name.String(), err.Error())
+		if strings.Contains(err.Error(), "not found") {
+			return mdlerrors.NewNotFound("import mapping", s.Name.String())
+		}
+		return mdlerrors.NewBackend("get import mapping", err)
 	}
 
 	if err := e.writer.DeleteImportMapping(im.ID); err != nil {
diff --git a/mdl/executor/cmd_javaactions.go b/mdl/executor/cmd_javaactions.go
@@ -248,7 +248,7 @@ func formatJavaActionReturnType(t javaactions.CodeActionReturnType) string {
 // execDropJavaAction handles DROP JAVA ACTION statements.
 func (e *Executor) execDropJavaAction(s *ast.DropJavaActionStmt) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	// Get hierarchy for module/folder resolution
@@ -281,7 +281,7 @@ func (e *Executor) execDropJavaAction(s *ast.DropJavaActionStmt) error {
 // execCreateJavaAction handles CREATE JAVA ACTION statements.
 func (e *Executor) execCreateJavaAction(s *ast.CreateJavaActionStmt) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	// Get hierarchy for module/folder resolution
diff --git a/mdl/executor/cmd_microflows_create.go b/mdl/executor/cmd_microflows_create.go
@@ -34,7 +34,7 @@ func (e *Executor) loadRestServices() ([]*model.ConsumedRestService, error) {
 
 func (e *Executor) execCreateMicroflow(s *ast.CreateMicroflowStmt) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	// Find or auto-create module
diff --git a/mdl/executor/cmd_pages_builder.go b/mdl/executor/cmd_pages_builder.go
@@ -336,7 +336,7 @@ func (pb *pageBuilder) createFolder(name string, containerID model.ID) (model.ID
 // execDropPage handles DROP PAGE statement.
 func (e *Executor) execDropPage(s *ast.DropPageStmt) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	pages, err := e.reader.ListPages()
@@ -362,7 +362,7 @@ func (e *Executor) execDropPage(s *ast.DropPageStmt) error {
 // execDropSnippet handles DROP SNIPPET statement.
 func (e *Executor) execDropSnippet(s *ast.DropSnippetStmt) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	snippets, err := e.reader.ListSnippets()
diff --git a/mdl/executor/cmd_pages_create_v3.go b/mdl/executor/cmd_pages_create_v3.go
@@ -17,7 +17,7 @@ import (
 // execCreatePageV3 handles CREATE PAGE statement with V3 syntax.
 func (e *Executor) execCreatePageV3(s *ast.CreatePageStmtV3) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	// Version pre-check: page parameters require 11.0+
@@ -101,7 +101,7 @@ func (e *Executor) execCreatePageV3(s *ast.CreatePageStmtV3) error {
 // execCreateSnippetV3 handles CREATE SNIPPET statement with V3 syntax.
 func (e *Executor) execCreateSnippetV3(s *ast.CreateSnippetStmtV3) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	// Find or auto-create module
diff --git a/mdl/executor/cmd_published_rest.go b/mdl/executor/cmd_published_rest.go
@@ -3,6 +3,7 @@
 package executor
 
 import (
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -171,7 +172,7 @@ func (e *Executor) findPublishedRestService(moduleName, name string) (*model.Pub
 			return svc, nil
 		}
 	}
-	return nil, mdlerrors.NewNotFoundMsg("published REST service", "", "not found")
+	return nil, mdlerrors.NewNotFound("published REST service", moduleName+"."+name)
 }
 
 // execCreatePublishedRestService creates a new published REST service.
@@ -188,7 +189,12 @@ func (e *Executor) execCreatePublishedRestService(s *ast.CreatePublishedRestServ
 
 	// Handle CREATE OR REPLACE — delete existing if found
 	if s.CreateOrReplace {
-		if existing, _ := e.findPublishedRestService(s.Name.Module, s.Name.Name); existing != nil {
+		existing, findErr := e.findPublishedRestService(s.Name.Module, s.Name.Name)
+		var nfe *mdlerrors.NotFoundError
+		if findErr != nil && !errors.As(findErr, &nfe) {
+			return mdlerrors.NewBackend("find existing service", findErr)
+		}
+		if existing != nil {
 			if err := e.writer.DeletePublishedRestService(existing.ID); err != nil {
 				return mdlerrors.NewBackend("replace existing service", err)
 			}
@@ -307,7 +313,7 @@ func (e *Executor) execAlterPublishedRestService(s *ast.AlterPublishedRestServic
 
 	svc, err := e.findPublishedRestService(s.Name.Module, s.Name.Name)
 	if err != nil {
-		return mdlerrors.NewNotFound("published REST service", s.Name.Module+"."+s.Name.Name)
+		return err
 	}
 
 	for _, action := range s.Actions {
diff --git a/mdl/executor/cmd_workflows_write.go b/mdl/executor/cmd_workflows_write.go
@@ -18,7 +18,7 @@ import (
 // execCreateWorkflow handles CREATE WORKFLOW statements.
 func (e *Executor) execCreateWorkflow(s *ast.CreateWorkflowStmt) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	module, err := e.findOrCreateModule(s.Name.Module)
@@ -130,7 +130,7 @@ func (e *Executor) execCreateWorkflow(s *ast.CreateWorkflowStmt) error {
 // execDropWorkflow handles DROP WORKFLOW statements.
 func (e *Executor) execDropWorkflow(s *ast.DropWorkflowStmt) error {
 	if e.writer == nil {
-		return mdlerrors.NewNotConnected()
+		return mdlerrors.NewNotConnectedWrite()
 	}
 
 	h, err := e.getHierarchy()
diff --git a/mdl/executor/executor.go b/mdl/executor/executor.go
@@ -207,7 +207,7 @@ func (e *Executor) Execute(stmt ast.Statement) error {
 	case r := <-ch:
 		err = r.err
 	case <-ctx.Done():
-		err = fmt.Errorf("statement timed out after %v", executeTimeout)
+		err = mdlerrors.NewValidationf("statement timed out after %v", executeTimeout)
 	}
 
 	if e.logger != nil {
diff --git a/mdl/executor/registry.go b/mdl/executor/registry.go
@@ -63,7 +63,7 @@ func NewRegistry() *Registry {
 func (r *Registry) Register(stmt ast.Statement, handler StmtHandler) {
 	t := reflect.TypeOf(stmt)
 	if _, exists := r.handlers[t]; exists {
-		panic(fmt.Sprintf("backend: duplicate handler registration for %s", t))
+		panic(fmt.Sprintf("registry: duplicate handler registration for %s", t))
 	}
 	r.handlers[t] = handler
 }

Original file line number	Diff line number	Diff line change
`@@ -627,8 +627,8 @@ func (b *MprBackend) RenameDocumentByName(moduleName, oldName, newName string) e`
`627`	`627`	`func (b *MprBackend) GetRawUnit(id model.ID) (map[string]any, error) {`
`628`	`628`	`return b.reader.GetRawUnit(id)`
`629`	`629`	`}`
`630`		`-func (b *MprBackend) GetRawUnitBytes(id string) ([]byte, error) {`
`631`		`- return b.reader.GetRawUnitBytes(model.ID(id))`
	`630`	`+func (b *MprBackend) GetRawUnitBytes(id model.ID) ([]byte, error) {`
	`631`	`+ return b.reader.GetRawUnitBytes(id)`
`632`	`632`	`}`
`633`	`633`	`func (b MprBackend) ListRawUnitsByType(typePrefix string) ([]mpr.RawUnit, error) {`
`634`	`634`	`return b.reader.ListRawUnitsByType(typePrefix)`
Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,7 @@ func (e Executor) createDatabaseConnection(stmt ast.CreateDatabaseConnectionSt`
`40`	`40`	`return mdlerrors.NewBackend("delete existing connection", err)`
`41`	`41`	`}`
`42`	`42`	`} else {`
`43`		`- return mdlerrors.NewAlreadyExistsMsg("database connection", modName+"."+ex.Name, "use CREATE OR MODIFY to update")`
	`43`	`+ return mdlerrors.NewAlreadyExistsMsg("database connection", modName+"."+ex.Name, fmt.Sprintf("database connection already exists: %s.%s (use CREATE OR MODIFY to update)", modName, ex.Name))`
`44`	`44`	`}`
`45`	`45`	`}`
`46`	`46`	`}`
Original file line number	Diff line number	Diff line change
`@@ -39,7 +39,7 @@ func (e Executor) execCreateEnumeration(s ast.CreateEnumerationStmt) error {`
`39`	`39`	`// Check if enumeration already exists`
`40`	`40`	`existingEnum := e.findEnumeration(s.Name.Module, s.Name.Name)`
`41`	`41`	`if existingEnum != nil && !s.CreateOrModify {`
`42`		`- return mdlerrors.NewAlreadyExistsMsg("enumeration", s.Name.Module+"."+s.Name.Name, "use CREATE OR MODIFY to update")`
	`42`	`+ return mdlerrors.NewAlreadyExistsMsg("enumeration", s.Name.Module+"."+s.Name.Name, fmt.Sprintf("enumeration already exists: %s.%s (use CREATE OR MODIFY to update)", s.Name.Module, s.Name.Name))`
`43`	`43`	`}`
`44`	`44`
`45`	`45`	`// Create enumeration values`
Original file line number	Diff line number	Diff line change
`@@ -75,7 +75,7 @@ func (e Executor) execShowFeatures(s ast.ShowFeaturesStmt) error {`
`75`	`75`	`default:`
`76`	`76`	`// SHOW FEATURES [IN area] — requires project connection`
`77`	`77`	`if e.reader == nil {`
`78`		`- return mdlerrors.NewNotConnected()`
	`78`	`+ return mdlerrors.NewNotConnectedMsg("not connected to a project\n hint: use SHOW FEATURES FOR VERSION x.y without a project connection")`
`79`	`79`	`}`
`80`	`80`	`rpv := e.reader.ProjectVersion()`
`81`	`81`	`pv = versions.SemVer{Major: rpv.MajorVersion, Minor: rpv.MinorVersion, Patch: rpv.PatchVersion}`
Original file line number	Diff line number	Diff line change
`@@ -197,7 +197,7 @@ func (e *Executor) resolveOneLink(`
`197`	`197`	`}`
`198`	`198`
`199`	`199`	`if foundAssoc == nil && foundCross == nil {`
`200`		`- return nil, mdlerrors.NewNotFoundMsg("association", link.AssociationName, fmt.Sprintf("not found in module %q", targetModule))`
	`200`	`+ return nil, mdlerrors.NewNotFoundMsg("association", link.AssociationName, fmt.Sprintf("association %q not found in module %q", link.AssociationName, targetModule))`
`201`	`201`	`}`
`202`	`202`
`203`	`203`	`// Extract association info`