fix: request and response migrations (#24)

Author: ichung08

README.md

@@ -176,14 +176,13 @@ change := cadwyn.NewVersionChange(
 
 Cadwyn automatically detects versions from:
 - **Headers** (default): `X-API-Version: 2024-01-01`
-- **Query params**: `?version=2024-01-01`
-- **URL path**: `/v2024-01-01/users`
+- **URL path**: `/v2024-01-01/users` or `/2024-01-01/users`
 
 Configure the detection method:
 ```go
 cadwyn.NewCadwyn().
-    WithVersionLocation(cadwyn.VersionLocationHeader).  // or Query, Path
-    WithVersionParameter("X-API-Version").              // Custom header/param name
+    WithVersionLocation(cadwyn.VersionLocationHeader).  // or Path
+    WithVersionParameter("X-API-Version").              // Custom header name
     Build()
 ```
 
@@ -241,7 +240,7 @@ Demonstrates:
 ## How It Works
 
 1. **You write handlers for the HEAD (latest) version only**
-2. **Cadwyn middleware detects the requested API version** from headers/query/path
+2. **Cadwyn middleware detects the requested API version** from headers or URL path
 3. **Request migration**: Transforms incoming request from old → new format
 4. **Your handler executes** with the migrated request
 5. **Response migration**: Transforms outgoing response from new → old format
@@ -253,6 +252,67 @@ Client (v1) → [v1 Request] → Migration → [v2 Request] → Handler (v2)
 Client (v1) ← [v1 Response] ← Migration ← [v2 Response] ← Handler (v2)
 ```
 
+## Best Practices
+
+### One VersionChange Per Schema
+
+Create separate `VersionChange` objects for different models/schemas. All migrations in a single `VersionChange` are applied together, so mixing multiple schemas can cause unwanted side effects.
+
+**✅ Good:**
+```go
+// Separate changes for different schemas
+userChange := cadwyn.NewVersionChange(
+    "Add email to User",
+    v1, v2,
+    &cadwyn.AlterResponseInstruction{
+        Schemas: []interface{}{User{}},
+        Transformer: func(resp *cadwyn.ResponseInfo) error {
+            // Only transforms User responses
+            if userMap, ok := resp.Body.(map[string]interface{}); ok {
+                delete(userMap, "email")
+            }
+            return nil
+        },
+    },
+)
+
+productChange := cadwyn.NewVersionChange(
+    "Add SKU to Product",
+    v1, v2,
+    &cadwyn.AlterResponseInstruction{
+        Schemas: []interface{}{Product{}},
+        Transformer: func(resp *cadwyn.ResponseInfo) error {
+            // Only transforms Product responses
+            if productMap, ok := resp.Body.(map[string]interface{}); ok {
+                delete(productMap, "sku")
+            }
+            return nil
+        },
+    },
+)
+
+cadwyn.NewCadwyn().
+    WithChanges(userChange, productChange).
+    Build()
+```
+
+**❌ Avoid:**
+```go
+// Multiple schemas in one VersionChange - transformers run on ALL responses!
+change := cadwyn.NewVersionChange(
+    "Multiple changes",
+    v1, v2,
+    &cadwyn.AlterResponseInstruction{
+        Schemas: []interface{}{User{}},
+        Transformer: func(resp) { /* runs on User AND Product */ },
+    },
+    &cadwyn.AlterResponseInstruction{
+        Schemas: []interface{}{Product{}},
+        Transformer: func(resp) { /* runs on User AND Product */ },
+    },
+)
+```
+
 ## Architecture: Middleware vs Router Generation
 
 Cadwyn-Go uses a **middleware approach** instead of Python Cadwyn's router generation:

cadwyn/cadwyn.go

@@ -104,6 +104,7 @@ type CadwynBuilder struct {
 	changes       []*VersionChange
 	types         []reflect.Type
 	versionConfig VersionConfig
+	errors        []error // Accumulated errors during building
 }
 
 // WithVersions sets the versions for the application
@@ -113,21 +114,29 @@ func (cb *CadwynBuilder) WithVersions(versions ...*Version) *CadwynBuilder {
 }
 
 // WithDateVersions creates and adds date-based versions
+// Invalid date strings are collected and will cause Build() to fail with a detailed error.
 func (cb *CadwynBuilder) WithDateVersions(dates ...string) *CadwynBuilder {
 	for _, dateStr := range dates {
-		if v, err := NewDateVersion(dateStr); err == nil {
-			cb.versions = append(cb.versions, v)
+		v, err := NewDateVersion(dateStr)
+		if err != nil {
+			cb.errors = append(cb.errors, fmt.Errorf("invalid date version '%s': %w", dateStr, err))
+			continue
 		}
+		cb.versions = append(cb.versions, v)
 	}
 	return cb
 }
 
 // WithSemverVersions creates and adds semantic versions (supports both major.minor.patch and major.minor)
+// Invalid semver strings are collected and will cause Build() to fail with a detailed error.
 func (cb *CadwynBuilder) WithSemverVersions(semvers ...string) *CadwynBuilder {
 	for _, semverStr := range semvers {
-		if v, err := NewSemverVersion(semverStr); err == nil {
-			cb.versions = append(cb.versions, v)
+		v, err := NewSemverVersion(semverStr)
+		if err != nil {
+			cb.errors = append(cb.errors, fmt.Errorf("invalid semver version '%s': %w", semverStr, err))
+			continue
 		}
+		cb.versions = append(cb.versions, v)
 	}
 	return cb
 }
@@ -191,6 +200,19 @@ func (cb *CadwynBuilder) WithTypes(types ...interface{}) *CadwynBuilder {
 
 // Build creates the Cadwyn instance
 func (cb *CadwynBuilder) Build() (*Cadwyn, error) {
+	// Check for accumulated errors from builder methods
+	if len(cb.errors) > 0 {
+		// Return first error with context about all errors
+		if len(cb.errors) == 1 {
+			return nil, fmt.Errorf("builder validation failed: %w", cb.errors[0])
+		}
+		errMsg := fmt.Sprintf("builder validation failed with %d errors:", len(cb.errors))
+		for i, err := range cb.errors {
+			errMsg += fmt.Sprintf("\n  %d. %v", i+1, err)
+		}
+		return nil, fmt.Errorf("%s", errMsg)
+	}
+
 	if len(cb.versions) == 0 {
 		return nil, fmt.Errorf("at least one version must be specified")
 	}

cadwyn/middleware.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"reflect"
 	"regexp"
 	"strings"
 
@@ -18,7 +17,6 @@ type VersionLocation string
 
 const (
 	VersionLocationHeader VersionLocation = "header"
-	VersionLocationQuery  VersionLocation = "query"
 	VersionLocationPath   VersionLocation = "path"
 )
 
@@ -56,23 +54,6 @@ func (hvm *HeaderVersionManager) GetVersion(c *gin.Context) (string, error) {
 	return version, nil
 }
 
-// QueryVersionManager extracts version from query parameters
-type QueryVersionManager struct {
-	paramName string
-}
-
-// NewQueryVersionManager creates a new query-based version manager
-func NewQueryVersionManager(paramName string) *QueryVersionManager {
-	return &QueryVersionManager{
-		paramName: paramName,
-	}
-}
-
-func (qvm *QueryVersionManager) GetVersion(c *gin.Context) (string, error) {
-	version := c.Query(qvm.paramName)
-	return version, nil
-}
-
 // PathVersionManager extracts version from URL path
 type PathVersionManager struct {
 	versionRegex     *regexp.Regexp
@@ -135,18 +116,17 @@ func NewVersionMiddleware(config MiddlewareConfig) *VersionMiddleware {
 	var versionManager VersionManager
 
 	switch config.Location {
-	case VersionLocationHeader:
-		versionManager = NewHeaderVersionManager(config.ParameterName)
-	case VersionLocationQuery:
-		versionManager = NewQueryVersionManager(config.ParameterName)
 	case VersionLocationPath:
 		versions := make([]string, len(config.VersionBundle.GetVersions()))
 		for i, v := range config.VersionBundle.GetVersions() {
 			versions[i] = v.String()
 		}
 		versionManager = NewPathVersionManager(versions)
+	case VersionLocationHeader:
+		fallthrough
 	default:
-		versionManager = NewHeaderVersionManager("X-API-Version")
+		// Default to header-based versioning
+		versionManager = NewHeaderVersionManager(config.ParameterName)
 	}
 
 	return &VersionMiddleware{
@@ -194,10 +174,16 @@ func (vm *VersionMiddleware) Middleware() gin.HandlerFunc {
 					requestedVersion = vm.findClosestOlderVersion(versionStr)
 				}
 				if requestedVersion == nil {
+					var hint string
+					if vm.location == VersionLocationHeader {
+						hint = fmt.Sprintf("Use '%s' header with one of the available versions", vm.parameterName)
+					} else {
+						hint = "Include version in the URL path (e.g., /v1/resource or /2024-01-01/resource)"
+					}
 					c.JSON(http.StatusBadRequest, gin.H{
 						"error":              fmt.Sprintf("Unknown version: %s", versionStr),
 						"available_versions": vm.versionBundle.GetVersionValues(),
-						"hint":               fmt.Sprintf("Use %s header with one of the available versions", vm.parameterName),
+						"hint":               hint,
 					})
 					c.Abort()
 					return
@@ -369,11 +355,24 @@ func (vah *VersionAwareHandler) migrateRequest(c *gin.Context, fromVersion *Vers
 		return nil // No body to migrate
 	}
 
-	// Parse JSON body directly - ShouldBindJSON handles reading and parsing
+	// Read body once and preserve it
+	bodyBytes, err := io.ReadAll(c.Request.Body)
+	if err != nil {
+		return fmt.Errorf("failed to read request body: %w", err)
+	}
+	c.Request.Body.Close()
+
+	// If body is empty, nothing to migrate
+	if len(bodyBytes) == 0 {
+		c.Request.Body = io.NopCloser(bytes.NewReader(bodyBytes))
+		return nil
+	}
+
+	// Parse JSON body
 	var bodyData interface{}
-	if err := c.ShouldBindJSON(&bodyData); err != nil {
-		// If JSON parsing fails, the body is already consumed
-		// We can't restore it, so just return (handler will also fail to parse)
+	if err := json.Unmarshal(bodyBytes, &bodyData); err != nil {
+		// If JSON parsing fails, restore original body and let handler deal with it
+		c.Request.Body = io.NopCloser(bytes.NewReader(bodyBytes))
 		return nil
 	}
 
@@ -385,8 +384,10 @@ func (vah *VersionAwareHandler) migrateRequest(c *gin.Context, fromVersion *Vers
 	migrationChain := vah.migrationChain.GetMigrationPath(fromVersion, headVersion)
 
 	// Apply migrations in forward direction
+	// Note: We pass nil for bodyType since JSON unmarshaling creates map[string]interface{}
+	// which doesn't match the original struct types. Schema-based matching doesn't work with JSON.
 	for _, change := range migrationChain {
-		if err := change.MigrateRequest(c.Request.Context(), requestInfo, reflect.TypeOf(requestInfo.Body), 0); err != nil {
+		if err := change.MigrateRequest(c.Request.Context(), requestInfo, nil, 0); err != nil {
 			return fmt.Errorf("failed to migrate request with change %s: %w", change.Description(), err)
 		}
 	}
@@ -427,19 +428,22 @@ func (vah *VersionAwareHandler) migrateResponse(c *gin.Context, toVersion *Versi
 	migrationChain := vah.migrationChain.GetMigrationPath(headVersion, toVersion)
 
 	// Apply migrations in reverse direction
+	// Note: We pass nil for responseType since JSON unmarshaling creates map[string]interface{}
+	// which doesn't match the original struct types. Schema-based matching doesn't work with JSON.
 	for i := len(migrationChain) - 1; i >= 0; i-- {
 		change := migrationChain[i]
-		if err := change.MigrateResponse(c.Request.Context(), responseInfo, reflect.TypeOf(responseInfo.Body), 0); err != nil {
+		if err := change.MigrateResponse(c.Request.Context(), responseInfo, nil, 0); err != nil {
 			return fmt.Errorf("failed to migrate response with change %s: %w", change.Description(), err)
 		}
 	}
 
 	// Write the migrated response
 	c.Writer = responseCapture.ResponseWriter
-	c.Writer.WriteHeader(responseInfo.StatusCode)
 
 	if responseInfo.Body != nil {
 		c.JSON(responseInfo.StatusCode, responseInfo.Body)
+	} else {
+		c.Writer.WriteHeader(responseInfo.StatusCode)
 	}
 
 	return nil

cadwyn/middleware_test.go

@@ -55,34 +55,6 @@ var _ = Describe("Middleware", func() {
 		})
 	})
 
-	Describe("QueryVersionManager", func() {
-		var manager *QueryVersionManager
-
-		BeforeEach(func() {
-			manager = NewQueryVersionManager("version")
-		})
-
-		It("should extract version from query parameter", func() {
-			req := httptest.NewRequest("GET", "/test?version=1.0.0", nil)
-			c, _ := gin.CreateTestContext(httptest.NewRecorder())
-			c.Request = req
-
-			version, err := manager.GetVersion(c)
-			Expect(err).NotTo(HaveOccurred())
-			Expect(version).To(Equal("1.0.0"))
-		})
-
-		It("should return empty string when query parameter is missing", func() {
-			req := httptest.NewRequest("GET", "/test", nil)
-			c, _ := gin.CreateTestContext(httptest.NewRecorder())
-			c.Request = req
-
-			version, err := manager.GetVersion(c)
-			Expect(err).NotTo(HaveOccurred())
-			Expect(version).To(Equal(""))
-		})
-	})
-
 	Describe("PathVersionManager", func() {
 		var manager *PathVersionManager
 
@@ -138,17 +110,6 @@ var _ = Describe("Middleware", func() {
 				Expect(mw).NotTo(BeNil())
 			})
 
-			It("should create middleware with query location", func() {
-				config := MiddlewareConfig{
-					VersionBundle:  bundle,
-					MigrationChain: chain,
-					ParameterName:  "version",
-					Location:       VersionLocationQuery,
-				}
-				mw := NewVersionMiddleware(config)
-				Expect(mw).NotTo(BeNil())
-			})
-
 			It("should create middleware with path location", func() {
 				config := MiddlewareConfig{
 					VersionBundle:  bundle,