guardrail-dev / guardrail-dev/guardrail
Handing of required/nullable is in schemas not correct
- 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
Assessment
This issue has not been assessed yet.