fix: autopatch fixes, add tests

danielgtaylor · Oct 28, 2023 · 6c3ab8d · 6c3ab8d
1 parent f0fa371
commit 6c3ab8d
Show file tree

Hide file tree

Showing 2 changed files with 304 additions and 5 deletions.
diff --git a/autopatch/autopatch.go b/autopatch/autopatch.go
@@ -34,11 +34,25 @@ var jsonPatchType = reflect.TypeOf([]jsonPatchOp{})
 // resource. This method may be safely called multiple times.
 func AutoPatch(api huma.API) {
 	oapi := api.OpenAPI()
+	registry := oapi.Components.Schemas
 	for _, path := range oapi.Paths {
 		if path.Get != nil && path.Put != nil && path.Patch == nil {
-			// TODO: ensure that the GET & PUT operations are for the same resource
-			// and it is a struct.
-			generatePatch(api, path)
+			body := path.Put.RequestBody
+			if body != nil && body.Content != nil && body.Content["application/json"] != nil {
+				ct := body.Content["application/json"]
+				if ct.Schema != nil {
+					s := ct.Schema
+					if s.Ref != "" {
+						// Dereference if needed so we can find the underlying type.
+						s = registry.SchemaFromRef(s.Ref)
+					}
+					// Only objects can be patched automatically. No arrays or
+					// primitives so skip those.
+					if s.Type == "object" {
+						generatePatch(api, path)
+					}
+				}
+			}
 		}
 	}
 }
@@ -235,7 +249,7 @@ func generatePatch(api huma.API, path *huma.PathItem) {
 			}
 		default:
 			// A content type we explicitly do not support was passed.
-			huma.WriteErr(api, ctx, http.StatusUnsupportedMediaType, "Content type should be one of application/merge-patch+json or application/json-patch+json", nil)
+			huma.WriteErr(api, ctx, http.StatusUnsupportedMediaType, "Content type should be one of application/merge-patch+json or application/json-patch+json")
 			return
 		}
 
@@ -245,7 +259,7 @@ func generatePatch(api huma.API, path *huma.PathItem) {
 		}
 
 		// Write the updated data back to the server!
-		putReq, err := http.NewRequest(http.MethodPut, op.Path, bytes.NewReader(patched))
+		putReq, err := http.NewRequest(http.MethodPut, ctx.URL().Path, bytes.NewReader(patched))
 		if err != nil {
 			huma.WriteErr(api, ctx, http.StatusInternalServerError, "Unable to put modified resource", err)
 			return

diff --git a/autopatch/autopatch_test.go b/autopatch/autopatch_test.go
@@ -0,0 +1,285 @@
+package autopatch
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"testing/iotest"
+
+	"github.com/danielgtaylor/huma/v2"
+	"github.com/danielgtaylor/huma/v2/humatest"
+	"github.com/stretchr/testify/assert"
+)
+
+type SaleModel struct {
+	Location string `json:"location"`
+	Count    int    `json:"count"`
+}
+
+func (m SaleModel) String() string {
+	return fmt.Sprintf("%s%d", m.Location, m.Count)
+}
+
+type ThingModel struct {
+	ID    string      `json:"id"`
+	Price float32     `json:"price,omitempty"`
+	Sales []SaleModel `json:"sales,omitempty"`
+	Tags  []string    `json:"tags,omitempty"`
+}
+
+func (m ThingModel) ETag() string {
+	return fmt.Sprintf("%s%v%v%v", m.ID, m.Price, m.Sales, m.Tags)
+}
+
+type ThingIDParam struct {
+	ThingID string `path:"thing-id"`
+}
+
+func TestPatch(t *testing.T) {
+	db := map[string]*ThingModel{
+		"test": {
+			ID:    "test",
+			Price: 1.00,
+			Sales: []SaleModel{
+				{Location: "US", Count: 123},
+				{Location: "EU", Count: 456},
+			},
+		},
+	}
+
+	_, api := humatest.New(t)
+
+	type GetThingResponse struct {
+		ETag string `header:"ETag"`
+		Body *ThingModel
+	}
+
+	huma.Register(api, huma.Operation{
+		OperationID: "get-thing",
+		Method:      http.MethodGet,
+		Path:        "/things/{thing-id}",
+		Errors:      []int{404},
+	}, func(ctx context.Context, input *struct {
+		ThingIDParam
+	}) (*GetThingResponse, error) {
+		thing := db[input.ThingID]
+		if thing == nil {
+			return nil, huma.Error404NotFound("Not found")
+		}
+		resp := &GetThingResponse{
+			ETag: thing.ETag(),
+			Body: thing,
+		}
+		return resp, nil
+	})
+
+	huma.Register(api, huma.Operation{
+		OperationID: "put-thing",
+		Method:      http.MethodPut,
+		Path:        "/things/{thing-id}",
+		Errors:      []int{404, 412},
+	}, func(ctx context.Context, input *struct {
+		ThingIDParam
+		Body    ThingModel
+		IfMatch []string `header:"If-Match" doc:"Succeeds if the server's resource matches one of the passed values."`
+	}) (*GetThingResponse, error) {
+		if len(input.IfMatch) > 0 {
+			found := false
+			if existing := db[input.ThingID]; existing != nil {
+				for _, possible := range input.IfMatch {
+					if possible == existing.ETag() {
+						found = true
+						break
+					}
+				}
+			}
+			if !found {
+				return nil, huma.Error412PreconditionFailed("ETag '" + strings.Join(input.IfMatch, ", ") + "' does not match")
+			}
+		} else {
+			// Since the GET returns an ETag, and the auto-patch feature should always
+			// use it when available, we can fail the test if we ever get here.
+			t.Fatal("No If-Match header set during PUT")
+		}
+		db[input.ThingID] = &input.Body
+		resp := &GetThingResponse{
+			ETag: db[input.ThingID].ETag(),
+			Body: db[input.ThingID],
+		}
+		return resp, nil
+	})
+
+	AutoPatch(api)
+
+	w := api.Patch("/things/test",
+		"Content-Type: application/merge-patch+json",
+		strings.NewReader(`{"price": 1.23}`),
+	)
+	assert.Equal(t, http.StatusOK, w.Code, w.Body.String())
+	assert.Equal(t, "test1.23[US123 EU456][]", w.Result().Header.Get("ETag"))
+
+	// Same change results in a 304 (patches are idempotent)
+	w = api.Patch("/things/test",
+		"Content-Type: application/merge-patch+json",
+		strings.NewReader(`{"price": 1.23}`),
+	)
+	assert.Equal(t, http.StatusNotModified, w.Code, w.Body.String())
+
+	app := api.Adapter()
+	// New change but with wrong manual ETag, should fail!
+	w = httptest.NewRecorder()
+	req, _ := http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`{"price": 4.56}`))
+	req.Header.Set("Content-Type", "application/merge-patch+json")
+	req.Header.Set("If-Match", "abc123")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusPreconditionFailed, w.Code, w.Body.String())
+
+	// Correct manual ETag should pass!
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`{"price": 4.56}`))
+	req.Header.Set("Content-Type", "application/merge-patch+json")
+	req.Header.Set("If-Match", "test1.23[US123 EU456][]")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusOK, w.Code, w.Body.String())
+	assert.Equal(t, "test4.56[US123 EU456][]", w.Result().Header.Get("ETag"))
+
+	// Merge Patch: invalid
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`{`))
+	req.Header.Set("Content-Type", "application/merge-patch+json")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusUnprocessableEntity, w.Code, w.Body.String())
+
+	// JSON Patch Test
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`[
+		{"op": "add", "path": "/tags", "value": ["b"]},
+		{"op": "add", "path": "/tags/0", "value": "a"}
+	]`))
+	req.Header.Set("Content-Type", "application/json-patch+json")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusOK, w.Code, w.Body.String())
+	assert.Equal(t, "test4.56[US123 EU456][a b]", w.Result().Header.Get("ETag"))
+
+	// JSON Patch: bad JSON
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`[`))
+	req.Header.Set("Content-Type", "application/json-patch+json")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusUnprocessableEntity, w.Code, w.Body.String())
+
+	// JSON Patch: invalid patch
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`[{"op": "unsupported"}]`))
+	req.Header.Set("Content-Type", "application/json-patch+json")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusUnprocessableEntity, w.Code, w.Body.String())
+
+	// Shorthand Patch Test
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`{tags[]: c}`))
+	req.Header.Set("Content-Type", "application/merge-patch+shorthand")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusOK, w.Code, w.Body.String())
+	assert.Equal(t, "test4.56[US123 EU456][a b c]", w.Result().Header.Get("ETag"))
+
+	// Shorthand Patch: bad input
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`[`))
+	req.Header.Set("Content-Type", "application/merge-patch+shorthand")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusUnprocessableEntity, w.Code, w.Body.String())
+
+	// Bad content type
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/test", strings.NewReader(`{}`))
+	req.Header.Set("Content-Type", "application/unsupported-content-type")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusUnsupportedMediaType, w.Code, w.Body.String())
+
+	// PATCH body read error
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/notfound", iotest.ErrReader(fmt.Errorf("test error")))
+	req.Header.Set("Content-Type", "application/merge-patch+json")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusBadRequest, w.Code, w.Body.String())
+
+	// GET error
+	w = httptest.NewRecorder()
+	req, _ = http.NewRequest(http.MethodPatch, "/things/notfound", strings.NewReader(`{}`))
+	req.Header.Set("Content-Type", "application/merge-patch+json")
+	app.ServeHTTP(w, req)
+	assert.Equal(t, http.StatusNotFound, w.Code, w.Body.String())
+}
+
+func TestPatchPutNoBody(t *testing.T) {
+	_, api := humatest.New(t)
+
+	huma.Register(api, huma.Operation{
+		OperationID: "get-thing",
+		Method:      http.MethodGet,
+		Path:        "/things/{thing-id}",
+	}, func(ctx context.Context, input *struct {
+		ThingIDParam
+		// Note: no body!
+	}) (*struct{}, error) {
+		return nil, nil
+	})
+
+	huma.Register(api, huma.Operation{
+		OperationID: "put-thing",
+		Method:      http.MethodPut,
+		Path:        "/things/{thing-id}",
+	}, func(ctx context.Context, input *struct {
+		ThingIDParam
+		// Note: no body!
+	}) (*struct{}, error) {
+		return nil, nil
+	})
+
+	AutoPatch(api)
+
+	// There should be no generated PATCH since there is nothing to
+	// write in the PUT!
+	w := httptest.NewRecorder()
+	req, _ := http.NewRequest(http.MethodPatch, "/things/test", nil)
+	api.Adapter().ServeHTTP(w, req)
+	assert.Equal(t, http.StatusMethodNotAllowed, w.Code, w.Body.String())
+}
+
+func TestDeprecatedPatch(t *testing.T) {
+	_, api := humatest.New(t)
+
+	huma.Register(api, huma.Operation{
+		OperationID: "get-thing",
+		Method:      http.MethodGet,
+		Path:        "/things/{thing-id}",
+	}, func(ctx context.Context, input *struct {
+		ThingIDParam
+	}) (*struct {
+		Body *ThingModel
+	}, error) {
+		return &struct{ Body *ThingModel }{&ThingModel{}}, nil
+	})
+
+	huma.Register(api, huma.Operation{
+		OperationID: "put-thing",
+		Method:      http.MethodPut,
+		Path:        "/things/{thing-id}",
+		Deprecated:  true,
+	}, func(ctx context.Context, input *struct {
+		ThingIDParam
+		Body ThingModel
+	}) (*struct {
+		Body *ThingModel
+	}, error) {
+		return &struct{ Body *ThingModel }{&ThingModel{}}, nil
+	})
+
+	AutoPatch(api)
+
+	assert.Equal(t, true, api.OpenAPI().Paths["/things/{thing-id}"].Patch.Deprecated)
+}