Skip to content

Remove uses of servant-generic in public Galley API #2022

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.

Sign up for GitHub

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

Merged
pcapriotti merged 1 commit into from
Jan 11, 2022
Merged

Remove uses of servant-generic in public Galley API #2022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5-internal/avoid-servant-generics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Replace servant-generic in Galley with a custom `Named` combinator
40 changes: 40 additions & 0 deletions libs/wire-api/src/Wire/API/Routes/Named.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
-- This file is part of the Wire Server implementation.
--
-- Copyright (C) 2022 Wire Swiss GmbH <opensource@wire.com>
--
-- This program is free software: you can redistribute it and/or modify it under
-- the terms of the GNU Affero General Public License as published by the Free
-- Software Foundation, either version 3 of the License, or (at your option) any
-- later version.
--
-- This program is distributed in the hope that it will be useful, but WITHOUT
-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
-- details.
--
-- You should have received a copy of the GNU Affero General Public License along
-- with this program. If not, see <https://www.gnu.org/licenses/>.

module Wire.API.Routes.Named where

import Data.Metrics.Servant
import Data.Proxy
import Imports
import Servant.Server
import Servant.Swagger

newtype Named named x = Named {unnamed :: x}
Copy link
Contributor

@fisx fisx Jan 4, 2022

Choose a reason for hiding this comment

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

wasn't the named record fields thing coincidental, and the true purpose of using records was to have better error messages?

what do the error messages look like with named routes?

Copy link
Contributor Author

@pcapriotti pcapriotti Jan 4, 2022

Choose a reason for hiding this comment

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

The Named stuff compensates for the fact that handlers have to be matched up with their types in order. If you swap two similar unnamed handlers by mistake, you won't get an error if their types are the same. By using Named types and handlers, we get a name mismatch error (which actually looks reasonably nice). I don't mind getting rid of the Named trick altogether, if people think it's not necessary.

Errors messages are in general horrible. GHC unhelpfully spews out the whole API type multiple times in the output, forcing you to use some kind of pager to be able to see the top of the error message. It wasn't much better before, but I have to admit this change did not improve things in this regard.

For example, if you forget a handler, GHC points you to the the last one, instead of the spot where the handler is missing. If you misplace a handler, it just says that some types don't match, with no indication to where the problem is.

There are probably ways to improve this, but I'm in general a bit sceptical about meddling with error messages, as it can result in very confusing messages sometimes (e.g. in servant's Union stuff).

Copy link
Contributor

@fisx fisx Jan 4, 2022

Choose a reason for hiding this comment

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

ok, thanks for the background. now i think i prefer the 'Named' thing, but only slightly.

no further questions! once we've had a few more eyes i'm happy with these changes.

deriving (Functor)

instance HasSwagger api => HasSwagger (Named name api) where
toSwagger _ = toSwagger (Proxy @api)

instance HasServer api ctx => HasServer (Named name api) ctx where
type ServerT (Named name api) m = Named name (ServerT api m)

route _ ctx action = route (Proxy @api) ctx (fmap unnamed action)
hoistServerWithContext _ ctx f =
fmap (hoistServerWithContext (Proxy @api) ctx f)

instance RoutesToPaths api => RoutesToPaths (Named name api) where
getRoutes = getRoutes @api
Loading