fix: address review feedback from PR #210

Fixes four issues identified by @ako in code review:

1. Windows file:// URL RFC 8089 compliance
   - Add leading slash for Windows paths: file:///C:/path
   - Previously: file://C:/path (incorrect, treats C: as host)
   - Now: file:///C:/path (correct per RFC 8089)

2. Deduplicate path resolution in fetchODataMetadata
   - Remove redundant file:// extraction and relative path resolution
   - Now relies on NormalizeURL() already called in createODataClient
   - Simplify to: PathFromURL() + os.ReadFile()
   - Remove unused mprDir parameter

3. Improve ServiceUrl validation error message
   - Add migration hint explaining the breaking change
   - Show how to create constant: CREATE CONSTANT Module.ApiLocation
   - Explain this enforces Mendix best practice

4. Fix unsafe os.Chdir in tests
   - Replace TestFetchODataMetadata_RelativePathWithoutProject
   - New test uses NormalizeURL() to generate file:// URL
   - No process-global state mutation
   - Tests now expect normalized URLs (matching production flow)

All tests pass. Addresses review comments without changing functionality.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Author: dionesiusap

internal/pathutil/uri.go

@@ -78,7 +78,13 @@ func NormalizeURL(rawURL string, baseDir string) (string, error) {
 	}
 
 	// Return as file:// URL with forward slashes (cross-platform)
-	return "file://" + filepath.ToSlash(absPath), nil
+	// RFC 8089 requires three slashes: file:///path or file:///C:/path
+	slashed := filepath.ToSlash(absPath)
+	if !strings.HasPrefix(slashed, "/") {
+		// Windows path like C:/Users/x needs leading slash: file:///C:/Users/x
+		slashed = "/" + slashed
+	}
+	return "file://" + slashed, nil
 }
 
 // PathFromURL extracts a filesystem path from a URL, handling both file:// URLs and HTTP(S) URLs.

mdl/executor/cmd_odata.go

@@ -993,7 +993,13 @@ func (e *Executor) createODataClient(stmt *ast.CreateODataClientStmt) error {
 		if stmt.ServiceUrl != "" {
 			// ServiceUrl must be a constant reference (e.g., @Module.ConstantName)
 			if !strings.HasPrefix(stmt.ServiceUrl, "@") {
-				return fmt.Errorf("ServiceUrl must be a constant reference starting with '@' (e.g., '@Module.LocationConstant'), got: %s", stmt.ServiceUrl)
+				return fmt.Errorf(`ServiceUrl must now be a constant reference (e.g., '@Module.ApiLocation').
+Previously literal URLs were allowed; this enforces the Mendix best practice of externalizing configuration.
+Create a constant first:
+  CREATE CONSTANT Module.ApiLocation TYPE String DEFAULT 'https://api.example.com/';
+Then reference it:
+  ServiceUrl: '@Module.ApiLocation'
+Got: %s`, stmt.ServiceUrl)
 			}
 			cfg.OverrideLocation = true
 			cfg.CustomLocation = stmt.ServiceUrl
@@ -1022,7 +1028,7 @@ func (e *Executor) createODataClient(stmt *ast.CreateODataClientStmt) error {
 		}
 		newSvc.MetadataUrl = normalizedUrl
 
-		metadata, hash, err := fetchODataMetadata(normalizedUrl, mprDir)
+		metadata, hash, err := fetchODataMetadata(normalizedUrl)
 		if err != nil {
 			fmt.Fprintf(e.output, "Warning: could not fetch $metadata: %v\n", err)
 		} else if metadata != "" {
@@ -1427,46 +1433,26 @@ func astEntityDefToModel(def *ast.PublishedEntityDef) (*model.PublishedEntityTyp
 // fetchODataMetadata downloads or reads the $metadata document.
 // Supports:
 //   - https://... or http://... (HTTP fetch)
-//   - file:///abs/path (local absolute path)
-//   - ./path or path/file.xml (local relative path, resolved against mprDir)
+//   - file:///abs/path (local absolute path from normalized URL)
 //
 // Returns the metadata XML and its SHA-256 hash, or empty strings if the fetch fails.
-// If mprDir is empty and a relative path is given, resolves against cwd.
-func fetchODataMetadata(metadataUrl string, mprDir string) (metadata string, hash string, err error) {
+// Note: metadataUrl is expected to be already normalized by NormalizeURL() in createODataClient,
+// so all relative paths have been converted to absolute file:// URLs.
+func fetchODataMetadata(metadataUrl string) (metadata string, hash string, err error) {
 	if metadataUrl == "" {
 		return "", "", nil
 	}
 
 	var body []byte
 
-	// Detect if this is a local file path (file:// or not http/https)
-	isHTTP := strings.HasPrefix(metadataUrl, "http://") || strings.HasPrefix(metadataUrl, "https://")
+	// At this point, metadataUrl is already normalized by NormalizeURL() in createODataClient:
+	// - Relative paths have been converted to absolute file:// URLs
+	// - HTTP(S) URLs are unchanged
+	// So we only need to distinguish file:// vs HTTP(S)
 
-	if !isHTTP {
-		// Local file path - extract and resolve
-		filePath := metadataUrl
-		if strings.HasPrefix(metadataUrl, "file://") {
-			filePath = pathutil.URIToPath(metadataUrl)
-			if filePath == "" {
-				return "", "", fmt.Errorf("invalid file:// URI: %s", metadataUrl)
-			}
-		}
-
-		// Resolve relative paths
-		if !filepath.IsAbs(filePath) {
-			if mprDir != "" {
-				filePath = filepath.Join(mprDir, filePath)
-			} else {
-				// No project loaded - use cwd
-				cwd, err := os.Getwd()
-				if err != nil {
-					return "", "", fmt.Errorf("failed to resolve relative path: %w", err)
-				}
-				filePath = filepath.Join(cwd, filePath)
-			}
-		}
-
-		// Read local file
+	filePath := pathutil.PathFromURL(metadataUrl)
+	if filePath != "" {
+		// Local file - read directly (path is already absolute)
 		body, err = os.ReadFile(filePath)
 		if err != nil {
 			return "", "", fmt.Errorf("failed to read local metadata file %s: %w", filePath, err)

mdl/executor/cmd_odata_test.go

@@ -20,49 +20,34 @@ func TestFetchODataMetadata_LocalFile(t *testing.T) {
 		t.Fatalf("Failed to create test metadata file: %v", err)
 	}
 
+	// Convert to proper file:// URL (RFC 8089 compliant)
+	fileURL, err := pathutil.NormalizeURL(metadataPath, tmpDir)
+	if err != nil {
+		t.Fatalf("Failed to normalize path: %v", err)
+	}
+
 	tests := []struct {
 		name        string
 		url         string
-		mprDir      string
 		wantErr     bool
 		errContains string
 	}{
 		{
-			name:    "absolute file:// URL",
-			url:     "file://" + metadataPath,
-			mprDir:  "",
-			wantErr: false,
-		},
-		{
-			name:    "absolute path without file://",
-			url:     metadataPath,
-			mprDir:  "",
-			wantErr: false,
-		},
-		{
-			name:    "relative path with mprDir",
-			url:     "metadata.xml",
-			mprDir:  tmpDir,
-			wantErr: false,
-		},
-		{
-			name:    "relative path with ./ prefix",
-			url:     "./metadata.xml",
-			mprDir:  tmpDir,
+			name:    "RFC 8089 file:// URL",
+			url:     fileURL,
 			wantErr: false,
 		},
 		{
 			name:        "nonexistent file",
 			url:         "file:///nonexistent/metadata.xml",
-			mprDir:      "",
 			wantErr:     true,
 			errContains: "failed to read local metadata file",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			metadata, hash, err := fetchODataMetadata(tt.url, tt.mprDir)
+			metadata, hash, err := fetchODataMetadata(tt.url)
 
 			if tt.wantErr {
 				if err == nil {
@@ -87,7 +72,7 @@ func TestFetchODataMetadata_LocalFile(t *testing.T) {
 			}
 
 			// Hash should be consistent
-			_, hash2, _ := fetchODataMetadata(tt.url, tt.mprDir)
+			_, hash2, _ := fetchODataMetadata(tt.url)
 			if hash != hash2 {
 				t.Errorf("Hash inconsistent between calls: %q vs %q", hash, hash2)
 			}
@@ -180,20 +165,24 @@ func TestNormalizeMetadataUrl(t *testing.T) {
 	}
 }
 
-func TestFetchODataMetadata_RelativePathWithoutProject(t *testing.T) {
-	// Create metadata file in current directory
+func TestFetchODataMetadata_LocalFileAbsolute(t *testing.T) {
+	// Create metadata file with absolute path
 	tmpDir := t.TempDir()
-	oldCwd, _ := os.Getwd()
-	defer os.Chdir(oldCwd)
-	os.Chdir(tmpDir)
 
 	metadataContent := `<?xml version="1.0"?><edmx:Edmx xmlns:edmx="http://docs.oasis-open.org/odata/ns/edmx" Version="4.0"></edmx:Edmx>`
-	if err := os.WriteFile("local.xml", []byte(metadataContent), 0644); err != nil {
+	filePath := filepath.Join(tmpDir, "local.xml")
+	if err := os.WriteFile(filePath, []byte(metadataContent), 0644); err != nil {
 		t.Fatalf("Failed to create test file: %v", err)
 	}
 
-	// Test with empty mprDir (should resolve against cwd)
-	metadata, hash, err := fetchODataMetadata("local.xml", "")
+	// Convert to file:// URL (simulates what NormalizeURL does)
+	fileURL := "file://" + filepath.ToSlash(filePath)
+	if !strings.HasPrefix(filePath, "/") {
+		// Windows: add leading slash for RFC 8089 compliance
+		fileURL = "file:///" + filepath.ToSlash(filePath)
+	}
+
+	metadata, hash, err := fetchODataMetadata(fileURL)
 	if err != nil {
 		t.Errorf("Unexpected error: %v", err)
 	}