-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
feat: better support for optional/nullable types #351
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #351 +/- ##
==========================================
+ Coverage 95.25% 95.43% +0.17%
==========================================
Files 19 19
Lines 2848 2937 +89
==========================================
+ Hits 2713 2803 +90
+ Misses 98 97 -1
Partials 37 37 ☔ View full report in Codecov by Sentry. |
Looks good to me in my generated SDKs (which are then used on the frontend via https://openapi-ts.pages.dev/openapi-fetch/ and deepmap for other go services). My nullable/omittable types are simple (scalars) so leaving complex ones to others who have experience with such use cases. HUGE quality of life improvement with this PR and the latest release (unnamed omitempty) ;) Thanks Daniel! |
Great work and looks good. One thing I would prefer is pointer fields required by default and use
Eg. If I have a output like this and all 3 fields have a nullable column value from a database table. type GreetingOutput struct {
Body struct {
Field1 *string `json:"field1"`
Field2 *string `json:"field2" nullable:"true" required:"true"`
Field3 *string `json:"field3,omitempty"`
}
}
func main() {
router := chi.NewMux()
api := humachi.New(router, huma.DefaultConfig("My API", "1.0.0"))
huma.Get(api, "/greeting/{name}", func(ctx context.Context, input *struct {
Name string `path:"name"`
}) (*GreetingOutput, error) {
resp := &GreetingOutput{}
resp.Body.Field1 = nil
resp.Body.Field2 = nil
resp.Body.Field3 = nil
return resp, nil
})
http.ListenAndServe("localhost:8080", router)
} The generated schema for GreetingOutput would be something like this GreetingOutputBody:
additionalProperties: false
properties:
field1:
type: string
field2:
type:
- string
- "null"
field3:
type: string
required:
- field2
type: object The default behaviour suggests |
@amsal thanks for the feedback! I will think about this a bit. This is a bit complex/interesting because these types can be used for either or both of the inputs/outputs and for unmarshaling the Several people have now suggested pointers should result in JSON Schema which is optional by default, but I do see the point that optional is different from Go serializing an explicit |
I totally get where you are coming from but from my and most developers' point of view, the instinctive expectation would be |
Ideally this is how I would suggest it work as well, but given the complexity, using a nullable tag is a good compromise. The required part about my comment was the inclusion of the field in the response only and not about the value. |
@amsal would it be helpful for pointers to simple scalar types to be generated as nullable in addition to being optional? If we slightly modify your example: Body struct {
Field1 *string `json:"field1"`
Field2 *string `json:"field2" required:"true"`
Field3 *string `json:"field3,omitempty"`
} Would generate as: GreetingOutputBody:
additionalProperties: false
properties:
field1:
type: [string, "null"]
field2:
type: [string, "null"]
field3:
type: [string, "null"]
required:
- field2
type: object Go will still always serialize I guess the biggest remaining question for this PR is also what a pointer in Go means. Since Go doesn't differentiate between IMO, whatever we decide for handling inputs should also apply to outputs as the models can be shared / round-tripped. |
I do believe pointers (even more so with scalars) should always mean Body struct {
Field1 *string `json:"field1"`
Field2 *string `json:"field2" required:"true"`
Field3 *string `json:"field3,omitempty"`
Field4 *string `json:"field4,omitempty" nullable:"false"`
} To give: GreetingOutputBody:
additionalProperties: false
properties:
field1:
type: [string, "null"]
field2:
type: [string, "null"]
field3:
type: [string, "null"]
field4:
type: string
required:
- field2
type: object
Agreed! |
@bclements @bekabaz @victoraugustolls any opinions on this? |
Yes that would be helpful. Regarding the what pointer means for this, I don't think it matters whether it is null or undefined in inputs as both cases the value would be nil. With the suggested change, I think response fields are covered too for scalar values. |
Would you be open to change this to use OneOf instead of panic-ing here. This would be only for documentation purposes (not code generation since thats poorly supported) and unlike scalar values, pointer structs would need explicit nullable tag to have this behaviour. // schema.go L505
if fs.Nullable && fs.Ref != "" {
fs.OneOf = []*Schema{
{
Ref: fs.Ref,
},
{
Type: "null",
},
}
fs.Ref = ""
fs.Nullable = false
} My very simple usage previewed correctly in Spotlight and the built-in OpenAPI tool in GoLand (Redocly/Swagger), and doesn't seem to have any 3.1 spec violations. What do you think? I may have missed some where this change doesn't play right for more complex responses or even inputs. type Body2 struct {
Field3 string `json:"field3"`
}
type GreetingOutput struct {
Body struct {
Field1 *Body2 `json:"field1" nullable:"true"`
Field2 []*Body2 `json:"field2" nullable:"true"`
}
} "GreetingOutputBody": {
"properties": {
"field1": {
"oneOf": [
{"$ref": "#/components/schemas/Body2"},
{"type": "null"}
]
},
"field2": {
"items": {"$ref": "#/components/schemas/Body2"},
"type": ["array", "null"]
}
},
} |
@amsal @danielgtaylor this is the closest that I found: OAI/OpenAPI-Specification#3148 It says that both are valid. I for one prefer a more succinct format, but I have no strong opinions on this since both seems to be valid. Other than that, I think this feature is indeed important for Huma and agree with the approach! Will take a look at the changes later today! |
I agree with @amsal . IMO as a user I would think that a pointer by default says "This value can be null, but it must explicitly be set to null", while being a pointer with Pointers semantically dictate the value, giving it the option of being null. It should be explicit whether the user is required to send it. TL;DR |
Follow my previous comments, I think the outcome in @lazharichir example should be:
|
@amsal @victoraugustolls I understand that using a GreetingOutputBody:
additionalProperties: false
properties:
message:
oneOf:
- type: "null"
- description: Greeting message
type: string You might expect something simple and easy to use like this in Typescript, and that's indeed what you get: type GreetingOutputBody = {
message?: string | null;
}; Buuuuuuut... let's look at a language like Go without union types. The output from oapi-codegen after downgrading to OpenAPI 3.0.3 is this fun and almost unusable code: // GreetingOutputBody defines model for GreetingOutputBody.
type GreetingOutputBody struct {
Message *GreetingOutputBody_Message `json:"message,omitempty"`
}
// GreetingOutputBodyMessage0 defines model for .
type GreetingOutputBodyMessage0 = interface{}
// GreetingOutputBodyMessage1 Greeting message
type GreetingOutputBodyMessage1 = string
// GreetingOutputBody_Message defines model for GreetingOutputBody.Message.
type GreetingOutputBody_Message struct {
union json.RawMessage
}
// AsGreetingOutputBodyMessage0 returns the union data inside the GreetingOutputBody_Message as a GreetingOutputBodyMessage0
func (t GreetingOutputBody_Message) AsGreetingOutputBodyMessage0() (GreetingOutputBodyMessage0, error) {
var body GreetingOutputBodyMessage0
err := json.Unmarshal(t.union, &body)
return body, err
}
// AsGreetingOutputBodyMessage1 returns the union data inside the GreetingOutputBody_Message as a GreetingOutputBodyMessage1
func (t GreetingOutputBody_Message) AsGreetingOutputBodyMessage1() (GreetingOutputBodyMessage1, error) {
var body GreetingOutputBodyMessage1
err := json.Unmarshal(t.union, &body)
return body, err
} Okay, let's try openapi-generator, that's super popular: // GreetingOutputBodyMessage - struct for GreetingOutputBodyMessage
type GreetingOutputBodyMessage struct {
Any *interface{}
}
// interface{}AsGreetingOutputBodyMessage is a convenience function that returns interface{} wrapped in GreetingOutputBodyMessage
func AnyAsGreetingOutputBodyMessage(v *interface{}) GreetingOutputBodyMessage {
return GreetingOutputBodyMessage{
Any: v,
}
}
// Unmarshal JSON data into one of the pointers in the struct
func (dst *GreetingOutputBodyMessage) UnmarshalJSON(data []byte) error {
var err error
match := 0
// try to unmarshal data into Any
err = newStrictDecoder(data).Decode(&dst.Any)
if err == nil {
jsonAny, _ := json.Marshal(dst.Any)
if string(jsonAny) == "{}" { // empty struct
dst.Any = nil
} else {
match++
}
} else {
dst.Any = nil
}
if match > 1 { // more than 1 match
// reset to nil
dst.Any = nil
return fmt.Errorf("data matches more than one schema in oneOf(GreetingOutputBodyMessage)")
} else if match == 1 {
return nil // exactly one match
} else { // no match
return fmt.Errorf("data failed to match schemas in oneOf(GreetingOutputBodyMessage)")
}
}
// Marshal data from the first non-nil pointers in the struct to JSON
func (src GreetingOutputBodyMessage) MarshalJSON() ([]byte, error) {
if src.Any != nil {
return json.Marshal(&src.Any)
}
return nil, nil // no data in oneOf schemas
}
// Get the actual instance
func (obj *GreetingOutputBodyMessage) GetActualInstance() (interface{}) {
if obj == nil {
return nil
}
if obj.Any != nil {
return obj.Any
}
// all schemas are nil
return nil
} That gives us not just Even languages like Python which do support union types get some crazy output for Let's compare a simple schema without GreetingOutputBody:
additionalProperties: false
properties:
message:
description: Greeting message
type: string
type: object oapi-codegen output: // GreetingOutputBody defines model for GreetingOutputBody.
type GreetingOutputBody struct {
// Message Greeting message
Message *string `json:"message,omitempty"`
} openapi-generator output is still not good, but simpler at least ( // GreetingOutputBody struct for GreetingOutputBody
type GreetingOutputBody struct {
// Greeting message
Message *string `json:"message,omitempty"`
AdditionalProperties map[string]interface{}
} Given someone using Huma is likely a Go programmer, and may want to consume their API from Go as well, I think we need to take into account the developer experience of using a |
OMG I understand that Go does not have native union types, but wouldn't this help? A generic |
@danielgtaylor what is your opinion on pointer being nullable but required? The code LGTM, I just need to take a closer look at the "downgrade" part. |
@victoraugustolls I'm kind of on the fence, leaning toward leaving nullability as an opt-in for users who need/want it for compatibility with other languages due to the complexity. Just to recap, Also, from the code generators perspective it seems to use "optional" as "should be pointer". For example, given: schemas:
Foo:
type: object
properties:
bar:
type: string
GreetingOutputBody:
additionalProperties: false
properties:
message:
$ref: "#/components/schemas/Foo"
type: object Notice there is nothing about nullability in that schema. Neither // Foo defines model for Foo.
type Foo struct {
Bar *string `json:"bar,omitempty"`
}
// GreetingOutputBody defines model for GreetingOutputBody.
type GreetingOutputBody struct {
Message *Foo `json:"message,omitempty"`
} And this from openapi-generator: // Foo struct for Foo
type Foo struct {
Bar *string `json:"bar,omitempty"`
}
// GreetingOutputBody struct for GreetingOutputBody
type GreetingOutputBody struct {
Message *Foo `json:"message,omitempty"`
AdditionalProperties map[string]interface{}
} I'm fine with reversing that logic to "if pointer, then optional" but I do see the argument for nullability, it just isn't necessary in many languages. In the end I want Huma to generate usable, developer-friendly output more than the "most correct" possible schemas that may result in bad developer experience down the line. Regardless of what we choose as the defaults, I also want to enable users to override the behavior as needed, hence the |
Since Huma is loosely based on FastAPI, there is also this discussion on pydantic (used by FastAPI to generate schemas / do validation) related to this problem: pydantic/pydantic#7161 |
@danielgtaylor implementation looks great, making That oneOf/anyOf Go generated to support nullability does look pretty ugly. To back up, I don't think it's very useful to support nullability separately from optionality; the API probably just wants some way to say that a field is not required. However, the benefit of nullability is that you can match exactly what Go generates. Go will marshal an nil pointer field to Perhaps a cleaner solution would be a custom marshaler. If we could treat all pointers as |
If I understood this correctly, I kind of disagree. A client explicitly sending null VS not sending a field have totally different meanings and are useful on their own. It forces the user to be explicit in one's intention, and at least for me I always choose the explicit route when possible. |
That is what I believe in! And we can always talk with maintainers of the tools you mentioned to extend support for any of / one of. |
Okay we've had some good discussions above and I understand this is a complex issue. Let me see if we can agree on something to get an incremental step over the line. Does anyone object to this behavior proposal? Optional / Required
Pointers have no effect on optional/required. The same rules apply regardless of whether the struct is being used for request input or response output. Nullability
I think this means stuff would generally Just Work ™️ outside of structs, and for now (while a bit annoying) people can work around the structs limitation explicitly via: type MyType struct {
Field string `json:"field"`
}
type NullableMyType struct {
MyType
_ struct{} `nullable:"true"`
} You can also still create any schema you like manually via |
@lazharichir @amsal @victoraugustolls @bekabaz @bclements @gregoryjjb @ssoroka @x-user I've updated the code to my latest proposal, what do you think? |
// `anyOf` or `not` which is not supported by all code generators, or is | ||
// supported poorly & generates hard-to-use code. This is less than ideal | ||
// but a compromise for now to support some nullability built-in. | ||
panic(fmt.Errorf("nullable is not supported for field '%s' which is type '%s'", f.Name, fs.Ref)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use %q
instead of '%s'
if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly like the panic (maybe having nullable tag = true on objects/arrays does nothing?) but otherwise LGTM. Thanks for implementing this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the dislike of panics but this is a guard against surprising behavior (i.e. no error, or returning an error which isn't checked). I've had teams think things work for a long time not realizing they are broken, and would prefer the service not even start up in that case so it is painfully obvious some things cannot work right now. We can always remove this in the future when the anyOf
/ oneOf
stuff is settled, or remove it without a breaking change if it becomes too much of a burden anytime in the future.
One more thought: should pointer fields that have |
Fair, but Go's unmarshaling behavior will treat them both the same (not that that's good, just how it is) |
@danielgtaylor tested the branch in my project, generated schema looks good |
Looks good to me. Only question i have is why tags checked in inconsistent way (some with |
Same as @amsal, not a massive fan of the panic but not a dealbreaker either. I tried it out with my repos, and the generated schemas look good to me, so great all around! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not a fan of panic and would vouch for Huma returning errors instead of panic. I also think the codebase could opt for using only tag.Lookup
instead of tag.Get
since there are parts of it that only matter if any value is set.
But those two points have no correlation with the feature in the discussion here, so LGTM!!!
@danielgtaylor, would you prefer a discussion or an issue to further discuss object nullability with |
@victoraugustolls I'm going to merge this and push a release soon. I've enabled the Github discussions feature for this repo so we can have further discussions there: https://github.com/danielgtaylor/huma/discussions Thanks for the feedback everyone! |
This is a proposal for how to support optional/nullable types properly in Huma. It doesn't handle every possible case but should be a step in the right direction, which we can incrementally build on in the future as needed.
Optional / Required
omitempty
, it is optionalrequired:"false"
, it is optionalrequired:"true"
, it is requiredPointers have no effect on optional/required. The same rules apply regardless of whether the struct is being used for request input or response output.
Nullability
boolean
,integer
,number
,string
: it is nullable unless it hasomitempty
.array
,object
: we defer this decision to a later date due to complexity and bad support foranyOf
/oneOf
, understanding it may break some clients in the future.nullable:"false"
, it is not nullablenullable:"true"
:boolean
,integer
,number
,string
: it is nullablearray
,object
: panic saying this is not currently supported_
withnullable: true
, the struct is nullable enabling users to opt-in forobject
without theanyOf
/oneOf
complication.I think this means stuff would generally Just Work ™️ outside of structs, and for now (while a bit annoying) people can work around the structs limitation explicitly via:
You can also still create any schema you like manually via
huma.Schema
, including setting theNullable
field and/orAnyOf
,OneOf
,Not
, etc.Additional Changes
/openapi-3.0.json
and/openapi-3.0.yaml
. This isn't perfect, but is best effort and should handle all auto-generated schemas. Notably this will help with OpenAPI 3.1 support? oapi-codegen/oapi-codegen#373nil
in the validator.I tried out various SDK generators and many have issues with
oneOf
and/ortype
-with-$ref
, so this proposal is dead simple in just providing optional types by default and letting users choose how and when to apply the"null"
type.Confirmed that VSCode can still provide linting and intellisense as you type given schemas with an array of types.
It would be really helpful to get some feedback on this PR and have people test it out for generating SDKs in various languages to see if you run into any issues or gotchas.
Fixes #238.