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

Support for nullable fields #314

Closed
jacob-pro opened this issue Oct 7, 2022 · 11 comments · Fixed by #498, #509 or #510
Closed

Support for nullable fields #314

jacob-pro opened this issue Oct 7, 2022 · 11 comments · Fixed by #498, #509 or #510

Comments

@jacob-pro
Copy link
Contributor

Currently it doesn't seem there is support for the nullable attribute on a property.

Not required means the field may be omitted completely from the JSON. Whereas nullable means it can be present but have a null value. This distinction is useful for partial update APIs where there is a distinction between making no changes, versus modifying an optional field.

See double_option for more info on usage.

At a minimum we should be able to manually mark fields as nullable. Even better would be to detect double Options and mark them as non-required and nullable.

@juhaku
Copy link
Owner

juhaku commented Oct 7, 2022

Nullable indeed is not supported since the Rust itself does not have null values. The absence is measured via Option. If a value is an Option type it will be marked as non-required. Alternatively currently if field value is decorated with #[serde(default)] it will be non-required as well.

It might not be a too big of a stretch to support double Option and mark the field nullable. Or at least add derive parameter nullable for a field. However if double Option is considered a nullable it should not be done so automatically but instead behind a feature flag. Not all might want to enable such behavior.

In Rust world I reckon the partial updates works really well without double Option as well. Values that are None can be easily handled in application so that these values are not updated to whatever that update target might be e.g. database. I know that there are lot of languages e.g. Java and TypeScript which are heavily in the world of null which is a curse word and a cancer within languages. And in these languages the null is used to represent absence. Exception in TypeScript where absence is measured with undefined and null value is a null value which is another annoying issue where to me both undefined and null are used to "solve" same problem.

Thinking this further the serde itself will actually serialize all Option fields with None value as null in JSON by default. Only when field is decorated with #[serde(skip_serializing_if = "Option::is_none")] the field will not be present in the JSON. Perhaps current logic could also be enhanced so that if the field is decorated with serde's skip_serializing_if attribute the field will be just non-required field and otherwise the field would actually also be nullable by default.

@jacob-pro
Copy link
Contributor Author

jacob-pro commented Oct 9, 2022

Let's say you have an API that allows updating a user account, it is a partial update API, so only properties that are included will be changed, e.g. PUT /users/123

The input { "phone_number": null } would remove the phone number from the user account, whereas an input of { } would simply make no changes to the phone number.

So as far as Serde is concerned:

pub struct OptionalVsNull {
    
    // When serialized if empty it will be omitted from the JSON
    #[serde(rename = "optional_nonnull", skip_serializing_if = "Option::is_none")]
    pub optional_nonnull: Option<String>,

    // If the field is missing from the JSON then it will error - however `null` is allowed.
    #[serde(rename = "required_nullable", deserialize_with = "Option::deserialize")]
    pub required_nullable: Option<String>,

    // None will result in being omitted from the JSON, whereas Some(None) will result in `{ "optional_nullable": null }`
    #[serde(rename = "optional_nullable", default, with = "::serde_with::rust::double_option", skip_serializing_if = "Option::is_none")]
    pub optional_nullable: Option<Option<String>>,
}

@juhaku
Copy link
Owner

juhaku commented Oct 10, 2022

Oh yes, if you make that distinction that null should be allowed to reset some value. Sure we can have support for this.

@nkovacs
Copy link
Contributor

nkovacs commented Nov 17, 2022

A single Option is serialized as null by serde, so unless you're using skip_serializing_if = "Option::is_none", shouldn't it also be nullable?

@jacob-pro
Copy link
Contributor Author

@nkovacs

A single Option is serialized as null by serde, so unless you're using skip_serializing_if = "Option::is_none", shouldn't it also be nullable?

By a strict interpretation of the specification that is correct. If you don't have nullable set in the spec, then you should never be returning nulls!

I'm not sure how friendly or popular having that behavior would be though...

@nkovacs
Copy link
Contributor

nkovacs commented Nov 17, 2022

That's my problem, I have a bunch of fields that are Option<String> etc., and they're not shown as nullable in swagger ui, but I am returning nulls.

Edit: even worse: I have Option<SomeStruct> that can be null, and since that's a Ref (and I don't want to inline it, it's used in multiple places), nullable doesn't work, and I'm not sure the workarounds will work with utoipa (it doesn't support 3.1's null in SchemaType, so I'd have to try the 3.0 workaround).

Edit2: yeah, for the 3.0 workaround, utoipa::openapi::schema::AllOf would have to have a nullable property, so it can't be done.

@juhaku
Copy link
Owner

juhaku commented Nov 18, 2022

Yeah, thats true people abuse the nullability in all ways in OpenAPI usually. But strictier the interpretation is better it would be for code generation though.

Edit2: yeah, for the 3.0 workaround, utoipa::openapi::schema::AllOf would have to have a nullable property, so it can't be done.

This is a sad fact. utoipa does not support 3.1 types at the moment because swagger-ui does not support them. Adding OpenAPI 3.1 support is something that should be done in future, e.g. if support for https://github.com/Redocly/redoc will be added then it would make sence. But having 3.1 support wihout being able to use them is excess.

@juhaku
Copy link
Owner

juhaku commented Nov 30, 2022

@nkovacs

Edit: even worse: I have Option that can be null, and since that's a Ref (and I don't want to inline it, it's used in multiple places), nullable doesn't work, and I'm not sure the workarounds will work with utoipa (it doesn't support 3.1's null in SchemaType, so I'd have to try the 3.0 workaround).

I am adding the missing attributes to the OpenAPI types which indicates that soonish those "workarounds" can be implemented but there is one thing that need to be kept in mind. That in short coming future I start add support for OpenAPI 3.1 types (at least I hope so, when I get the now all missing pieces together)

With that I need to introduce conditionaly to the OpenAPI schema version that is being used by the users. E.g users should / could choose whether they want to use 3.0 or 3.1 types. Yet Swagger UI does not support 3.1 spec but perhaps when redocly is added the support will come handly.

@rossng
Copy link

rossng commented Jan 20, 2023

Thinking this further the serde itself will actually serialize all Option fields with None value as null in JSON by default. Only when field is decorated with #[serde(skip_serializing_if = "Option::is_none")] the field will not be present in the JSON. Perhaps current logic could also be enhanced so that if the field is decorated with serde's skip_serializing_if attribute the field will be just non-required field and otherwise the field would actually also be nullable by default.

I just came across this issue myself. I use openapi-zod-client to autogenerate a TypeScript client from the OpenAPI spec created by utoipa.

I assumed that using an Option<String> in my schema type would cause the OpenAPI spec to reflect that the value could be null, but instead utoipa removes the optional property from the required properties list.

When my TypeScript client received the { "foo": null }, it threw an exception because it expected to receive either {} or { "foo": "some string" } as per the strict interpretation of utoipa's OpenAPI output.

I was able to get things to work for my use case using skip_serializing_if, but I would support the idea that an Optional should emit nullable: true (or equivalent for OpenAPI 3.1) instead of changing the required properties.

@juhaku
Copy link
Owner

juhaku commented Feb 15, 2023

I would support the idea that an Optional should emit nullable: true (or equivalent for OpenAPI 3.1) instead of changing the required properties

I agree, I think it should do both. If user has defined skip_serializing_if then it should be non required, but without it it should also set the value to nullable: true.

@juhaku
Copy link
Owner

juhaku commented Feb 22, 2023

There is a new breaking change coming up which changes this naive nullable and required field behavior. In short changes are

  • Field is considered nullable when field type is Option
  • Field is considered required if it has not:
    • serde default attribute
    • or serde_with double_option
    • or serde skip_serializing_if attribute

The support is so far only implemented to ToSchema macro. More details can be found from the PR #498

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released
4 participants