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

Make pointers and/or OmittableNullable nullable in the generated OpenAPI spec #238

Closed
gregoryjjb opened this issue Feb 11, 2024 · 15 comments · Fixed by #351
Closed

Make pointers and/or OmittableNullable nullable in the generated OpenAPI spec #238

gregoryjjb opened this issue Feb 11, 2024 · 15 comments · Fixed by #351
Labels
enhancement New feature or request

Comments

@gregoryjjb
Copy link

How can I set fields to be nullable in the generated spec? I can't find any way to set nullability on a Schema.

Ideally I'd like to make pointers always nullable (to match my Go code as closely as possible), and/or have custom types like the OmittableNullable example generate as nullable.

Thanks in advance ❤️

@danielgtaylor
Copy link
Owner

@gregoryjjb thanks for the question! Couple of things to clarify:

  1. Can you help me understand why you need this? In Go, the nullable types applied to marshalled formats like JSON/CBOR typically mean they can be omitted, not necessarily that they can or should be supporting an explicit null value. As for OmittableNullable, it generates its own schema so you could modify it to support null there as needed.
  2. How would you model a nullable field in OpenAPI 3.1? It seems there might be multiple ways to do this. What tools are you using and how do they expect it to be modelled?

Thanks!

@gregoryjjb
Copy link
Author

  1. In responses I figured null is more explicit for values that are null in the DB for example, as opposed to being "not required". If this is not what people usually do with OpenAPI I could accomplish the same thing with not required. In that case, is it possible to make pointers optional by default?

  2. I meant to use nullable: true, is that not supported anymore in 3.1? Orval generates types for such fields that look like string | null

My underlying goal: values that are optional/nullable in Go should take as little effort as possible to be marked as such in the spec and typescript types, to reduce chances of spec-to-Go mismatch. ("As little effort as possible" means ideally I don't need to tag each pointer field as optional manually)

@gregoryjjb
Copy link
Author

gregoryjjb commented Feb 12, 2024

@danielgtaylor danielgtaylor added the enhancement New feature or request label Feb 12, 2024
@gregoryjjb
Copy link
Author

It seems that to support the 3.1 way of doing nulllable:

type:
- "string"
- "null" 

then Schema.Type would need to be a slice instead of a string, no? Then OmittableNullable could do something like:

func (n *OmittableNullable[T]) Schema(r huma.Registry) *huma.Schema {
	s := r.Schema(reflect.TypeOf(n.Val), true, "")
	
	// This would be the thing to do I think?
	s.Type = append(s.Type, "null")

	return s
}

@danielgtaylor
Copy link
Owner

@gregoryjjb thanks, yeah this looks doable but isn't a small fix. I need to dig into what updating Schema.Type to a slice would entail. It will definitely have an impact on the validation code and likely make marshaling/unmarshaling schemas a little more complicated.

I kind of hate this, but as a temporary workaround you can actually use the Schema.Extensions to override the Type field to be an array. The advantage is that this works right now to update the rendered OpenAPI schema and won't break the existing Go validation code. For example:

func (o OmittableNullable[T]) Schema(r huma.Registry) *huma.Schema {
	s := r.Schema(reflect.TypeOf(o.Value), true, "")
	s.Extensions = map[string]any{
		"type": []string{s.Type, "null"},
	}
	return s
}

https://go.dev/play/p/u90K6z6WDkV

@gregoryjjb
Copy link
Author

That does do the trick, thanks!

Last question: is it possible to make it do this by default for native pointer types? Right now, a Go *string is represented as a JSON string in the spec, but will easily return an invalid result (null) when the value of the pointer is null. IMO the generated spec for all pointers should default to nullable.

Here's how I think of the natural defaults:

Field type Schema Supported?
Regular non-pointer required, not nullable ✔️
Pointer required, nullable
Pointer with omitempty set optional, not nullable (since if it's null it will be omitted) ✔️
Custom nullable generic type required, nullable ✔️ thanks to your workaround

@gregoryjjb
Copy link
Author

I had a go at implementing the above rules and it's actually tricky, because the fact that omitempty can change the nullability of the field means that the registry potentially needs to keep track of two versions of the type.

@danielgtaylor
Copy link
Owner

@gregoryjjb yes this seems kind of tricky overall. I'll have to take some time soon and dig deeper into it. For now I'll leave this open as a feature request and anyone is welcome to pitch in!

@danielgtaylor
Copy link
Owner

I wonder if it would be simpler to support it using something like this, but I'm not sure how SDK generators would handle it:

MyType:
  oneOf:
    - type: "null"
    - type: string

The advantage is we already support basic oneOf functionality in the validator and you never need to modify the original struct type to support the two different cases you ran into as it's rolled up into the oneOf instead. Maybe we can do some testing and see if that results in acceptable generated Typescript and other language SDKs.

@gregoryjjb
Copy link
Author

Tested this with Orval and it does produce usable results:

"200": {
	Content: map[string]*huma.MediaType{
		"application/json": {
			Schema: &huma.Schema{
				Type: huma.TypeObject,
				Properties: map[string]*huma.Schema{
					"field": {
						OneOf: []*huma.Schema{
							{
								Type: "string",
							},
							{
								Type: "null",
							},
						},
					},
				},
				Required: []string{"field"},

			},
		},
	},
},

produces the following Typescript:

export type TestNullable200Field = string | null;

export type TestNullable200 = {
  field: TestNullable200Field;
};

Another thing I'm experimenting with

Since Go doesn't support union types anyway, what if the nullability is a special case in Huma that marshals to an OpenAPI slice?

// SchemaType represents a JSON Schema type. While JSON Schema supports union
// types by way of an array of types, Go doesn't support union types, so this
// SchemaType will only marshal to an array if it is nullable. e.g.:
//   { Name: "string", Nullable: false } -> { "type": "string" }
//   { Name: "string", Nullable: true }  -> { "type": ["string", "null"] }
type SchemaType struct {
	Name     string
	Nullable bool
}

type Schema struct { //nolint: musttag
	Type                 *SchemaType        `yaml:"type,omitempty"`
	...
}

Validation could be updated to allow nil values when Nullable == true, which is much simpler than having to support an arbitrary slice of types.

This explicitly avoids supporting the union type arrays in a general way but maybe that's better for Go?

I'm still not sure what to do about what is the most difficult part IMO, which is how to change the nullability of a Schema based on the omitempty tag on its struct field.

@lazharichir
Copy link

One thing I am not a huge fan of is the need for omitempty to get a non-required field in a request body.

type CreateDocumentRequest struct {
	actor.Actor
	Body struct {
		MatterID    *string `json:"MatterID,omitempty"`
		Title       *string `json:"Title,omitempty"`
		Description *string `json:"Description,omitempty"`
		WorkspaceID string
		UploadID    string
	}
}

As you can see, it forces me to repeat the field name in the json struct tag. If I use json:",omitempty", it renders a namless on-required field. Would be nice to have a pointer mean nullable, and required:"false" mean non required.

@amsal
Copy link

amsal commented Mar 27, 2024

Hi @danielgtaylor

Would you be open or is it possible to add a custom tag like oapiNullable: "true" to pointer fields in struct manually, so that custom generic nullable types are not needed.

@danielgtaylor
Copy link
Owner

@gregoryjjb @lazharichir @amsal please take a look at #351 and let me know what you think.

@lazharichir
Copy link

@gregoryjjb @lazharichir @amsal please take a look at #351 and let me know what you think.

Will check this tonight against my three repos that use Huma and would definitely benefit from this!

@highvelcty
Copy link

I came across this thread looking for a way to set the default value for an nullable field to "null". I'm not sure there is a way to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants