Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested struct visibility #314

Merged
merged 4 commits into from
Mar 17, 2024

Conversation

CptKirk
Copy link
Contributor

@CptKirk CptKirk commented Mar 14, 2024

This pull request enables readOnly, writeOnly and deprecated struct tags on nested structs.

Resolves issue: #313

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.13%. Comparing base (07a489e) to head (9321465).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #314   +/-   ##
=======================================
  Coverage   95.13%   95.13%           
=======================================
  Files          19       19           
  Lines        2773     2777    +4     
=======================================
+ Hits         2638     2642    +4     
  Misses         98       98           
  Partials       37       37           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielgtaylor
Copy link
Owner

@CptKirk I added a commit with a slightly different approach. JSON Schema allows for $ref to have siblings for validation (see spec example), in this case we generate something like this:

type: object
properties:
  foo:
    $ref: '#/components/schemas/FooSchema'
    readOnly: true

The validator didn't support that as it always resolved refs before checking validations. The commit I added changes that behavior to check at the top level, which supports how we generate schemas and works fine if no $ref is present. This has the added benefit of not requiring a new method in the Registry interface, and it doesn't run into issues when you re-use a struct which is sometimes read-only and sometimes not.

Please let me know what you think!

The only weird thing I ran into is that Stoplight Docs don't mark it as read-only, but that seems to be a bug as both Swagger UI and Scalar Docs do the right thing for this simple example:

type GreetingOutput struct {
	Body struct {
		Message string `json:"message" example:"Hello, world!" doc:"Greeting message"`
		Another struct {
			Foo string `json:"foo"`
		} `json:"another" readOnly:"true"`
	}
}
Screenshot 2024-03-14 at 22 39 32 Screenshot 2024-03-14 at 22 41 13

@CptKirk
Copy link
Contributor Author

CptKirk commented Mar 15, 2024

@danielgtaylor thanks for providing the input! I checked out your solution and indeed I can confirm, that the validation works correctly with your changes but the stoplight docs do not. The problem seems to be, that Stoplight parses the OpenAPI spec differently than Swagger UI, because with my proposed changes Stoplight Docs become correct. I have created a little test case to figure out what the exact problem is. I created the following case:

type NestedStruct struct {
	Value int `json:"value"`
	Foo   struct {
		Bar string `json:"bar"`
	} `json:"foo" readOnly:"true"`
}

func main() {
	router := chi.NewMux()
	api := humachi.New(router, huma.DefaultConfig("My API", "1.0.0"))

	huma.Register(api, huma.Operation{
		OperationID: "post-nested",
		Method:      http.MethodPost,
		Path:        "/nested",
		Summary:     "Create a nested",
		Tags:        []string{"Nested"},
	}, func(
		ctx context.Context,
		input *struct {
			Body *NestedStruct
		}) (
		*struct {
			Body *NestedStruct
		}, error) {
		resp := &NestedStruct{}
		return &struct{ Body *NestedStruct }{Body: resp}, nil
	})

	http.ListenAndServe("127.0.0.1:8888", router)
}

Then I downloaded the OpenAPI spec for mine and your solution, ran a diff and indeed there is a small difference:

--- danielgtaylor.json	2024-03-15 11:51:57.546482363 +0100
+++ cptkirk.json	2024-03-15 11:52:40.267077258 +0100
@@ -100,20 +100,21 @@
           ],
           "type": "object"
         },
         "NestedStructFooStruct": {
           "additionalProperties": false,
           "properties": {
             "bar": {
               "type": "string"
             }
           },
+          "readOnly": true,
           "required": [
             "bar"
           ],
           "type": "object"
         }
       }
     },
     "info": {
       "title": "My API",
       "version": "1.0.0"

I agree that a new method in the Registry interface is not the ideal solution, but not having correct Stoplight docs would be a bummer for DX, at least in our case. Could we solve the problem another way so the Schema for the struct in the Registry has the right flags?

@danielgtaylor
Copy link
Owner

@CptKirk the problem I'm running into is this scenario:

type Foo struct {
	Field string `json:"field"`
}

type ExampleResponse1 struct {
	Body struct {
		Example Foo `json:"example" readOnly:"true"`
	}
}

type ExampleResponse2 struct {
	Body struct {
		Example Foo `json:"example"` // NOT readOnly!
	}
}

In this case it is incorrect to mark the generated Foo schema as readOnly as it is only sometimes read-only!

I wonder if we can get Stoplight to fix their support of $ref with other validation 🤔

@CptKirk
Copy link
Contributor Author

CptKirk commented Mar 15, 2024

@danielgtaylor Ah, now I see the problem. Yeah you are right, this is a case where my proposed solution would break things. Thanks for taking the time explaining it to me. I think you can merge your solution as it fixes the problem with validation. I will look over the weekend into the issue with Stoplight and will try to fix it.

@danielgtaylor danielgtaylor merged commit 428dcfd into danielgtaylor:main Mar 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants