Refactor NoContentVerb into NoContentVerbWithStatus (#1532) #1550
Conversation
…1532) There are several HTTP status codes that correspond to a response body with `NoContent`. This commit introduces `NoContentVerbWithStatus` which generalizes `NoContentVerb` to cases when the return status may be different from 204. The former replaces the latter anywhere possible. `NoContentVerb` is kept as a special case of `NoContentVerbWithStatus` for backwards compatibility.
| -- | @NoContentVerbWithStatus@ is a specific type to represent 'NoContent' responses. | ||
| -- It does not require either a list of content types (because there's no content). | ||
| -- It still requires a status code, because several statuses may have no content. | ||
| -- (e.g. 204, 301, 302, or 303). |
There was a problem hiding this comment.
I do have a philosophical question here: when using 30X status codes, the Location header must be set. We are not enforcing this here — and in fact, your test does not do that either.
Wouldn't it be better to provide redirect combinators that do enforce this constraint ?
There was a problem hiding this comment.
I think we have two different problems here:
- A
NoContentresponse can correspond to other status codes than 204 (as stated in VerbNoContent should allow status codes to be set not hardcode to 204 #1532) - Servant does not enforce anything about headers when returning a 3xx status code
I think those issues should be addressed separately, unless maybe if the only other way to return NoContent is to have a 3xx status code, in which case we could kill two birds with one stone.
I may be mistaken, but I think NoContent could be returned in a number of other cases, for instance on endpoints that would be hardcoded to return 4xx errors.
I suggest we proceed with the PR as it is, then tackle the issue of Location headers separately.
There are several HTTP status codes that correspond to a response body with
NoContent. This PR introducesNoContentVerbWithStatuswhich generalizesNoContentVerbto cases when the return status may bedifferent from 204. The former replaces the latter anywhere possible.
NoContentVerbis kept as a special case ofNoContentVerbWithStatusfor backwards compatibility.This PR also adds a test case for
NoContentVerbWithStatusinServerSpec.hs