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

Servant MultiVerb #1649

Merged
merged 26 commits into from
Jul 22, 2021
Merged

Servant MultiVerb #1649

merged 26 commits into from
Jul 22, 2021

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Jul 9, 2021

This introduces a new Verb-like type for Servant endpoints: MultiVerb. The MultiVerb type can be used as an alternative to UVerb, and it provides approximately the same functionality as the latter, with a few differences:

  • The handler return type is provided by the user, and does not have to be a union. In fact, MultiVerb can subsume Verb.
  • It is possible to associate arbitrary metadata to responses without affecting the return type.
  • It is designed to be extensible with other kinds of responses.

This PR only adds the MultiVerb type and uses it for the two user HEAD endpoints (qualified and unqualified). The followup PR #1657 applies MultiVerb to a few other endpoints and makes ErrorDescription types work as MultiVerb responses.

Checklist

  • Title of this PR explains the impact of the change.
  • The description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • The CHANGELOG.md file in the Unreleased section has been updated to explain the change which will be included in the release notes.
  • Added documentation.
  • Implemented Swagger generation
  • Implemented header support
  • Implemented Client support

@pcapriotti pcapriotti force-pushed the pcapriotti/multiverb branch from bc78f9e to 2f57302 Compare July 12, 2021 06:58
By the time we find a content type mismatch, the handler has already
run, so we should fail with FailFatal to avoid trying another handler.
@pcapriotti pcapriotti changed the title [RFC] Servant MultiVerb Servant MultiVerb Jul 14, 2021
@pcapriotti pcapriotti marked this pull request as ready for review July 14, 2021 06:50
@pcapriotti pcapriotti marked this pull request as draft July 14, 2021 07:08
@pcapriotti pcapriotti marked this pull request as ready for review July 15, 2021 08:16
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I left a few minor comments.

libs/wire-api/src/Wire/API/Routes/MultiVerb.hs Outdated Show resolved Hide resolved
It should use the underlying monoid instance of `UndenderResult Void`.
Thanks @mdimjasevic for spotting this.
@pcapriotti pcapriotti requested a review from akshaymankar July 19, 2021 12:08
@pcapriotti pcapriotti force-pushed the pcapriotti/multiverb branch from 7f74b8a to d2fa599 Compare July 19, 2021 12:15
Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks very neat ✨
I have posted a few questions in comments.

Comment on lines 85 to 86
data UnrenderResult a = Mismatch | UnrenderError String | UnrenderSuccess a
deriving (Eq, Show, Functor)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does Mismatch mean?

type ResponseStatus (Respond s desc a) = s

responseRender (AcceptHeader acc) x =
M.mapAcceptMedia (map (uncurry mkRenderOutput) (allMimeRender (Proxy @cs) x)) acc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we don't use it, but for my education, would this make the server render output for all media types that the server supports and then choose which one to respond with? If so, we may want to rewrite this using M.matchAccept and then rendering?

fromHeaders = id
toHeaders = id

data DescHeader (name :: Symbol) (desc :: Symbol) (a :: *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to see if we could support having Servant.Description in the mods variable of Servant.Header'. Did you already try that? Might save us some code here and may even be easily upstream-able into servant-swagger.

@pcapriotti pcapriotti merged commit c58fb1a into develop Jul 22, 2021
@pcapriotti pcapriotti deleted the pcapriotti/multiverb branch July 22, 2021 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants