danielgtaylor / huma

Huma REST/HTTP API Framework for Golang with OpenAPI 3.1
https://huma.rocks/
MIT License
2.12k stars 151 forks source link

[panicking] `Schema.Properties` gets inconsistent with the private `Schema.propertyNames`. #607

Closed superstas closed 3 weeks ago

superstas commented 1 month ago

Hi there,

I have the following types

main.go

type (
    UpsertRequest[D any] struct {
        Tag       string `json:"tag"`
        InputData D      `json:"input_data"`
    }

    User struct {
        Name    string `json:"name" minLength:"1" maxLength:"32"`
        Surname string `json:"surname" minLength:"1" maxLength:"32"`
    }

    noInputData = map[string]interface{}

    CreateUserInput struct {
        Body UpsertRequest[User]
    }

    CreateUserOutput struct {
        Body struct {
            Message string `json:"message"`
        }
    }

    updateUserRequest struct {
        UpsertRequest[noInputData]
    }

    UpdateUserInput struct {
        Body updateUserRequest
    }

    UpdateUserOutput struct {
        Body struct {
            Message string `json:"message"`
        }
    }
)

var _ huma.SchemaTransformer = updateUserRequest{}

func (i updateUserRequest) TransformSchema(r huma.Registry, s *huma.Schema) *huma.Schema {
    s.Description = "UpdateUserInput"
    delete(s.Properties, "input_data")
    return s
}

func addRoutes(api huma.API) {
    huma.Register(api, huma.Operation{
        OperationID: "CreateUser",
        Method:      http.MethodPost,
        Path:        "/user",
    }, func(ctx context.Context, input *CreateUserInput) (*CreateUserOutput, error) {
        resp := &CreateUserOutput{}
        resp.Body.Message = "CreateUser works!"
        return resp, nil
    })

    huma.Register(api, huma.Operation{
        OperationID: "UpdateUser",
        Method:      http.MethodPut,
        Path:        "/user",
    }, func(ctx context.Context, input *UpdateUserInput) (*UpdateUserOutput, error) {
        resp := &UpdateUserOutput{}
        resp.Body.Message = "UpdateUser works!"
        return resp, nil
    })
}

Here's the full generated spec.

spec ```yaml components: schemas: CreateUserOutputBody: additionalProperties: false properties: $schema: description: A URL to the JSON Schema for this object. examples: - https://example.com/schemas/CreateUserOutputBody.json format: uri readOnly: true type: string message: type: string required: - message type: object ErrorDetail: additionalProperties: false properties: location: description: Where the error occurred, e.g. 'body.items[3].tags' or 'path.thing-id' type: string message: description: Error message text type: string value: description: The value at the given location type: object ErrorModel: additionalProperties: false properties: $schema: description: A URL to the JSON Schema for this object. examples: - https://example.com/schemas/ErrorModel.json format: uri readOnly: true type: string detail: description: A human-readable explanation specific to this occurrence of the problem. examples: - Property foo is required but is missing. type: string errors: description: Optional list of individual error details items: $ref: "#/components/schemas/ErrorDetail" type: - array - "null" instance: description: A URI reference that identifies the specific occurrence of the problem. examples: - https://example.com/error-log/abc123 format: uri type: string status: description: HTTP status code examples: - 400 format: int64 type: integer title: description: A short, human-readable summary of the problem type. This value should not change between occurrences of the error. examples: - Bad Request type: string type: default: about:blank description: A URI reference to human-readable documentation for the error. examples: - https://example.com/errors/example format: uri type: string type: object UpdateUserOutputBody: additionalProperties: false properties: $schema: description: A URL to the JSON Schema for this object. examples: - https://example.com/schemas/UpdateUserOutputBody.json format: uri readOnly: true type: string message: type: string required: - message type: object UpdateUserRequest: additionalProperties: false description: UpdateUserInput properties: $schema: description: A URL to the JSON Schema for this object. examples: - https://example.com/schemas/UpdateUserRequest.json format: uri readOnly: true type: string tag: type: string required: - tag - input_data type: object UpsertRequestUser: additionalProperties: false properties: $schema: description: A URL to the JSON Schema for this object. examples: - https://example.com/schemas/UpsertRequestUser.json format: uri readOnly: true type: string input_data: $ref: "#/components/schemas/User" tag: type: string required: - tag - input_data type: object User: additionalProperties: false properties: name: maxLength: 32 minLength: 1 type: string surname: maxLength: 32 minLength: 1 type: string required: - name - surname type: object info: title: My API version: 1.0.0 openapi: 3.1.0 paths: /user: post: operationId: CreateUser requestBody: content: application/json: schema: $ref: "#/components/schemas/UpsertRequestUser" required: true responses: "200": content: application/json: schema: $ref: "#/components/schemas/CreateUserOutputBody" description: OK default: content: application/problem+json: schema: $ref: "#/components/schemas/ErrorModel" description: Error put: operationId: UpdateUser requestBody: content: application/json: schema: $ref: "#/components/schemas/UpdateUserRequest" required: true responses: "200": content: application/json: schema: $ref: "#/components/schemas/UpdateUserOutputBody" description: OK default: content: application/problem+json: schema: $ref: "#/components/schemas/ErrorModel" description: Error ```

In this case, I want to have two operations Create and Update that:

To avoid duplication of the types ( IRL, these types are quite huge and complex ), I reuse them under CreateUserInput and updateUserRequest ( a wrapper for a human-readable schema name ) + UpdateUserInput.

updateUserRequest has TransformSchema that removes input_data from s.Properties

Everything works perfectly for the OAS generation.

The issue

main_test.go

func TestUserCreate(t *testing.T) {
    _, api := humatest.New(t, huma.DefaultConfig("Test API", "1.0.0"))
    addRoutes(api)

    // Create a new user
    _ = api.Post("/user", map[string]any{"tag": "tag1", "input_data": map[string]any{"name": "John", "surname": "Doe"}})

    // Update the user
    _ = api.Put("/user", map[string]any{"tag": "tag1", "input_data": map[string]any{"name": "John", "surname": "Doe"}})
}

I get a panic on the PUT /user request with input_data ( that I expect shouldn't pass the validation )

$> go test ./...

--- FAIL: TestUserCreate (0.00s)
    main_test.go:15: Making request:
        POST /user HTTP/1.1
        Content-Type: application/json

        {
          "input_data": {
            "name": "John",
            "surname": "Doe"
          },
          "tag": "tag1"
        }
    main_test.go:15: Got response:
        HTTP/1.1 200 OK
        Connection: close
        Content-Type: application/json
        Link: </schemas/UpdateUserOutputBody.json>; rel="describedBy"

        {
          "$schema": "https:///schemas/UpdateUserOutputBody.json",
          "message": "CreateUser works!"
        }
    main_test.go:18: Making request:
        PUT /user HTTP/1.1
        Content-Type: application/json

        {
          "input_data": {
            "name": "John",
            "surname": "Doe"
          },
          "tag": "tag1"
        }
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x161 pc=0x5f20ca]

goroutine 6 [running]:
testing.tRunner.func1.2({0x64ae60, 0x882c00})
    /home/superstas/.gvm/pkgsets/go1.21.13/global/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.8.linux-amd64/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
    /home/superstas/.gvm/pkgsets/go1.21.13/global/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.8.linux-amd64/src/testing/testing.go:1634 +0x377
panic({0x64ae60?, 0x882c00?})
    /home/superstas/.gvm/pkgsets/go1.21.13/global/pkg/mod/golang.org/toolchain@v0.0.1-go1.22.8.linux-amd64/src/runtime/panic.go:770 +0x132
github.com/danielgtaylor/huma/v2.handleMapString({0x7001c0, 0xc000016e00}, 0xc00018ef08, 0xc000078c60, 0x1, 0xc0001d4cc0, 0xc000012558)
    /home/superstas/.gvm/pkgsets/go1.21.13/global/pkg/mod/github.com/danielgtaylor/huma/v2@v2.23.0/validate.go:595 +0x4ca
github.com/danielgtaylor/huma/v2.Validate({0x7001c0, 0xc000016e00}, 0x6fe080?, 0xc000078c60, 0x1, {0x6489a0, 0xc0001d4cc0}, 0xc000012558)
    /home/superstas/.gvm/pkgsets/go1.21.13/global/pkg/mod/github.com/danielgtaylor/huma/v2@v2.23.0/validate.go:524 +0xbf4
github.com/danielgtaylor/huma/v2.Register[...].func1()
    /home/superstas/.gvm/pkgsets/go1.21.13/global/pkg/mod/github.com/danielgtaylor/huma/v2@v2.23.0/huma.go:1268 +0x1b45

My internal research showed that modifying s.Properties directly makes it inconsistent with the private propertyNames ( ref ).

I fixed this internally by this patch

diff --git a/validate.go b/validate.go
index 365a237..193f295 100644
--- a/validate.go
+++ b/validate.go
@@ -586,7 +586,11 @@ func handleMapString(r Registry, s *Schema, path *PathBuffer, mode ValidateMode,
    }

    for _, k := range s.propertyNames {
-       v := s.Properties[k]
+       v, ok := s.Properties[k]
+       if !ok {
+           continue
+       }

        // Schemas are generated such that the read/write-only properties are set
        // alongside the `$ref`, if it is present (i.e. for objects). If not,
@@ -680,7 +684,11 @@ func handleMapAny(r Registry, s *Schema, path *PathBuffer, mode ValidateMode, m
    }

    for _, k := range s.propertyNames {
-       v := s.Properties[k]
+       v, ok := s.Properties[k]
+       if !ok {
+           continue
+       }

        // Schemas are generated such that the read/write-only properties are set
        // alongside the `$ref`, if it is present (i.e. for objects). If not,

But there are a few questions:

Thank you.

danielgtaylor commented 4 weeks ago

@superstas nice find, I think this is a bug. I've opened a PR - let me know what you think!