Repository navigation
Add lint for nullable request and response bodies - #32
sunshowers merged 2 commits into
Conversation
| Alternatively, if `None` means that the resource doesn't exist, return a 404 Not | ||
| Found error. |
There was a problem hiding this comment.
I might put this case first?
If None is meant to indicate that the resource doesn't exist, consider returning 404 Not Found instead. ...
There was a problem hiding this comment.
Agreed, reversed the order of recommendations.
| Alternatively, if `None` means that the resource doesn't exist, return a 404 Not | ||
| Found error. | ||
|
|
||
| The same applies to request bodies: rather than `TypedBody<Option<MyParams>>`, |
There was a problem hiding this comment.
This is a little interesting. Option is obviously overloaded in serde/serde_json to mean "maybe present" or "could be null", but it may not be unreasonable to have an optional POST body (even though it's not something dropshot supports well today). That said, I think this is the right restriction given the current capabilities.
Also, this specific phrasing ("the same applies") may make more sense after the "make your Option a struct field" rather than after "None = 404"
There was a problem hiding this comment.
Option is obviously overloaded in serde/serde_json to mean "maybe present" or "could be null", but it may not be unreasonable to have an optional POST body (even though it's not something dropshot supports well today).
Hmm, from the openapi-lint perspective (which only works on OpenAPI documents, not Rust code) this would be represented as "required": false and not "nullable": true, right? So I don't think this change makes that worse in any way. Unless I'm missing something?
|
Thanks. Going to land this, then patch it in to all the repos in the omicron crativerse. |
This is not idiomatic -- see oxidecomputer/dropshot#669 and oxidecomputer/omicron#11623 for more discussion.