guardrail-dev / guardrail-dev/guardrail

Handing of required/nullable is in schemas not correct

Open
#315 11 comments 1 reaction 0 assignees View on GitHub
bug core java scala
Dominant language
Scala
Stars
541
Forks
138
PR merge metrics
No merged PRs in 30d

Description

After looking at [a SO post](https://stackoverflow.com/a/45577763/1626395), I've realized we're not quite handling `required` correctly. The `nullable` property (which defaults to false) was added in OpenAPI v3 (there is no analog in Swagger v2).

(Note in the below tables, when I say `required: true` I mean "the property is included in the `required` array".)

## Serialization

| `required` | `nullable` | Output generation |
|:------------:|:------------:|:------------------------:|
| false | false | Property may be missing or present, but, if present, must not be `null` |
| false | true | Property may be missing or present, and, if present, may be `null` |
| true | false | Property must be present and must not be `null` |
| true | true | Property must be present but may be `null` |

Currently we do not do anything with `nullable`. We treat `required: true` as `(required: true, nullable: false)`, and `required: false` as `(required: true, nullable: true)`.

It's not entirely clear what we should do for Swagger v2, which does not support `nullable`. I would suggest that, for backward compatibility, we retain the current behavior there. Since OpenAPI v3 support/use in Guardrail is still relatively new, we can correct the behavior without too much collateral damage.

## Deserialization

The spec is (I think) a little underspecified here; I think we should be as permissive as possible:

| `required` | `nullable` | Input parsing |
|:------------:|:------------:|:-----------------:|
| false | false | Allow `null` and treat it as not-present |
| false | true | Anything goes |
| true | false | Property must be present and non-`null` |
| true | true | Allow not-present and treat it as `null` |

I'm not 100% sure, but I _think_ we currently treat `required: true` as `(required: true, nullable: false)` and `required: false` as `(required: false, nullable: true)`.

## Other Stuff

The next question: how do we represent the tri-state of (absent, present-but-null, present) in our generated APIs? (I have some thoughts here but will add as a comment later.)

As a somewhat-off-topic rant, I'm a little annoyed that OpenAPI didn't just follow JSON schema here and allow an array for `type:`, so you'd do `type: [ "string", "null" ]` instead of using `nullable: true`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.