Improvement Autopatch (adding a body) #496
Conversation
WalkthroughThe updates enhance the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
autopatch/autopatch_test.go (3)
333-333: Consider renaming the test function for clarity.The function name
TestAutoPatchOptionalSchemais clear but could be more descriptive to indicate that it tests the PATCH operation generation.- func TestAutoPatchOptionalSchema(t *testing.T) { + func TestGenerateOptionalPatchSchema(t *testing.T) {
336-341: Consider adding comments to the struct fields.Adding comments to the
TestModelstruct fields can improve readability and understanding of the test data.type TestModel struct { ID string `json:"id"` // Unique identifier Name string `json:"name"` // Name of the entity Age int `json:"age"` // Age of the entity Optional *string `json:"optional,omitempty"` // Optional field }
343-366: Consider adding error handling for the GET and PUT handlers.The GET and PUT handlers in the test function do not handle errors, which might lead to confusion if the handlers fail. Adding error handling can improve the robustness of the test.
huma.Register(api, huma.Operation{ OperationID: "get-test", Method: http.MethodGet, Path: "/test/{id}", }, func(ctx context.Context, input *struct { ID string `path:"id"` }) (*struct { Body *TestModel }, error) { if input.ID == "" { return nil, fmt.Errorf("ID is required") } return &struct{ Body *TestModel }{&TestModel{}}, nil }) huma.Register(api, huma.Operation{ OperationID: "put-test", Method: http.MethodPut, Path: "/test/{id}", }, func(ctx context.Context, input *struct { ID string `path:"id"` Body TestModel `json:"body"` }) (*struct { Body *TestModel }, error) { if input.ID == "" { return nil, fmt.Errorf("ID is required") } return &struct{ Body *TestModel }{&TestModel{}}, nil })autopatch/autopatch.go (2)
Line range hint
1-1:
Consider adding more comments to explain the logic.Adding comments to explain the logic in the
AutoPatchfunction can improve readability and maintainability.// Iterate over paths to find resources with GET and PUT but no PATCH operations for _, path := range oapi.Paths { // Check if both GET and PUT operations exist and PATCH operation is missing if path.Get != nil && path.Put != nil && path.Patch == nil { // Iterate over GET and PUT operations to check for autopatch metadata for _, op := range []*huma.Operation{path.Get, path.Put} { if op.Metadata != nil && op.Metadata["autopatch"] != nil { if b, ok := op.Metadata["autopatch"].(bool); ok && !b { // Skip if autopatch is explicitly disabled continue Outer } } } // Check if the PUT request body has a JSON schema 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 s = registry.SchemaFromRef(s.Ref) } // Only generate PATCH operations for objects if s.Type == "object" { PatchResource(api, path) } } } } }
175-175: Consider adding comments to explain the handler logic.Adding comments to explain the logic in the handler function can improve readability and maintainability.
// Manually register the handler with the router. adapter.Handle(op, func(ctx huma.Context) { // Read the request body patchData, err := io.ReadAll(ctx.BodyReader()) if err != nil { huma.WriteErr(api, ctx, http.StatusBadRequest, "Unable to read request body", err) return } // Perform the GET request to fetch the original resource origReq, err := http.NewRequest(http.MethodGet, ctx.URL().Path, nil) if err != nil { huma.WriteErr(api, ctx, http.StatusInternalServerError, "Unable to get resource", err) return } // Copy incoming headers ctx.EachHeader(func(k, v string) { if k == "Accept" || k == "Accept-Encoding" { // Force these headers to be JSON for easier handling return } if k == "If-Match" || k == "If-None-Match" || k == "If-Modified-Since" || k == "If-Unmodified-Since" { // Ignore conditional request headers here return } if k == "Content-Type" || k == "Content-Length" { // GET request will be empty return } origReq.Header.Add(k, v) }) // Accept JSON for the patches origReq.Header.Set("Accept", "application/json") origReq.Header.Set("Accept-Encoding", "") // Perform the GET request to fetch the original resource origWriter := httptest.NewRecorder() adapter.ServeHTTP(origWriter, origReq) if origWriter.Code >= 300 { // Handle error on the GET side for key, values := range origWriter.Header() { for _, value := range values { ctx.SetHeader(key, value) } } ctx.SetStatus(origWriter.Code) io.Copy(ctx.BodyWriter(), origWriter.Body) return } // Apply the patch var patched []byte switch strings.Split(ctx.Header("Content-Type"), ";")[0] { case "application/json-patch+json": patch, err := jsonpatch.DecodePatch(patchData) if err != nil { huma.WriteErr(api, ctx, http.StatusUnprocessableEntity, "Unable to decode JSON Patch", err) return } patched, err = patch.Apply(origWriter.Body.Bytes()) if err != nil { huma.WriteErr(api, ctx, http.StatusUnprocessableEntity, "Unable to apply patch", err) return } case "application/merge-patch+json", "application/json", "": // Assume most cases are merge-patch patched, err = jsonpatch.MergePatch(origWriter.Body.Bytes(), patchData) if err != nil { huma.WriteErr(api, ctx, http.StatusUnprocessableEntity, "Unable to apply patch", err) return } case "application/merge-patch+shorthand": // Load the original data as a base var tmp any if err := json.Unmarshal(origWriter.Body.Bytes(), &tmp); err != nil { huma.WriteErr(api, ctx, http.StatusUnprocessableEntity, "Unable to apply patch", err) return } // Unmarshal the shorthand over the existing data tmp, err = shorthand.Unmarshal(string(patchData), shorthand.ParseOptions{ ForceStringKeys: true, }, tmp) if err != nil { huma.WriteErr(api, ctx, http.StatusUnprocessableEntity, "Unable to apply patch", err) return } // Marshal the updated data back for the PUT request patched, err = json.Marshal(tmp) if err != nil { huma.WriteErr(api, ctx, http.StatusUnprocessableEntity, "Unable to apply patch", err) return } default: // Unsupported content type huma.WriteErr(api, ctx, http.StatusUnsupportedMediaType, "Content type should be one of application/merge-patch+json or application/json-patch+json") return } if jsonpatch.Equal(patched, origWriter.Body.Bytes()) { ctx.SetStatus(http.StatusNotModified) return } // Write the updated data back to the server 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 } ctx.EachHeader(func(k, v string) { if k == "Content-Type" || k == "Content-Length" { return } putReq.Header.Add(k, v) }) putReq.Header.Set("Content-Type", "application/json") // Set conditional headers if available h := putReq.Header if h.Get("If-Match") == "" && h.Get("If-None-Match") == "" && h.Get("If-Unmodified-Since") == "" && h.Get("If-Modified-Since") == "" { oh := origWriter.Header() if etag := oh.Get("ETag"); etag != "" { h.Set("If-Match", etag) } else if modified := oh.Get("Last-Modified"); modified != "" { h.Set("If-Unmodified-Since", modified) } } // Perform the PUT request to update the resource putWriter := httptest.NewRecorder() adapter.ServeHTTP(putWriter, putReq) for key, values := range putWriter.Header() { for _, value := range values { ctx.SetHeader(key, value) } } ctx.SetStatus(putWriter.Code) io.Copy(ctx.BodyWriter(), putWriter.Body) })
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- autopatch/autopatch.go (5 hunks)
- autopatch/autopatch_test.go (1 hunks)
Additional comments not posted (10)
autopatch/autopatch_test.go (2)
368-391: Ensure that all fields are nullable in the PATCH schema.The loop verifies that all fields in the PATCH schema are nullable. This is a crucial part of the test, and it looks good.
393-399: The PATCH operation test looks good.The test for the generated PATCH operation is well-structured and ensures the PATCH operation works as expected.
autopatch/autopatch.go (8)
133-137: Ensure the schema is correctly dereferenced.The code correctly dereferences the schema if needed.
139-141: The optional schema creation looks good.The code correctly creates an optional version of the PUT schema.
157-160: Ensure consistency in content types.The code ensures that the optional schema is used for both
application/merge-patch+jsonandapplication/merge-patch+shorthandcontent types.
325-328: Handle nil schema case properly.The function correctly handles the case where the input schema is nil.
330-361: Ensure all schema attributes are copied.The function correctly copies all relevant attributes from the original schema to the optional schema.
363-371: Ensure recursive handling of items and properties.The function correctly handles nested schemas by recursively calling
makeOptionalSchemafor items and properties.
374-397: Ensure recursive handling of OneOf, AnyOf, AllOf, and Not.The function correctly handles
OneOf,AnyOf,AllOf, andNotby recursively callingmakeOptionalSchema.
399-403: Ensure all properties are made optional.The function correctly sets the
Requiredfield to nil to make all properties optional.
removed nullable as it didnt play nicely in the api generation
generating it like this
type: string,null
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- autopatch/autopatch.go (5 hunks)
- autopatch/autopatch_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- autopatch/autopatch.go
- autopatch/autopatch_test.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #496 +/- ##
=======================================
Coverage 92.83% 92.84%
=======================================
Files 22 22
Lines 3784 3842 +58
=======================================
+ Hits 3513 3567 +54
- Misses 227 230 +3
- Partials 44 45 +1 ☔ View full report in Codecov by Sentry. |
|
@ScriptType this looks great, but can we get the test coverage up a little before merging? |
|
Will do it over the weekend :). Glad that you like it |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- autopatch/autopatch_test.go (1 hunks)
Additional comments not posted (7)
autopatch/autopatch_test.go (7)
333-349: Test for basic properties inmakeOptionalSchemalooks good.The test comprehensively checks that the properties are correctly handled and made optional.
351-364: Test forAnyOfinmakeOptionalSchemalooks good.The test correctly verifies that the
AnyOfschemas are handled as expected.
366-381: Test forAllOfinmakeOptionalSchemalooks good.The test correctly verifies that the
AllOfschemas are handled as expected.
383-394: Test forNotinmakeOptionalSchemalooks good.The test correctly verifies that the
Notschemas are handled as expected.
396-398: Test for nil input inmakeOptionalSchemalooks good.The test correctly verifies that the function handles nil input as expected.
400-419: Test for nested schemas inmakeOptionalSchemalooks good.The test correctly verifies that nested schemas are handled as expected.
Line range hint
1-332:
Test forAutoPatchwith optional schema looks good.The test comprehensively covers various scenarios to ensure the PATCH operation works as expected with the optional schema.
Improve AutoPatch to generate optional schema for PATCH operations
Problem
The current AutoPatch functionality generates PATCH operations for resources with GET and PUT operations, but it doesn't create a specific type for the PATCH request that matches the PUT operation with all fields optional.
Solution
This PR modifies the AutoPatch function to create an optional schema for PATCH operations based on the PUT operation's schema.
makeOptionalSchemafunction that creates an optional version of a given schema.PatchResourcefunction to use this new optional schema for PATCH operations.Changes
makeOptionalSchemafunction inautopatch.goPatchResourcefunction to use the new optional schemaTesting
A new test function
TestAutoPatchOptionalSchemahas been added to verify that:Its my First contribution in golang would be happy to hear from your review :).
Summary by CodeRabbit
New Features
Tests