Can't use different result types inside GitHubKey

[artemohanjanyan]

I can't compile following example with servant-github-webhook >= 0.4: https://github.com/onrock-eng/github-webhooks/tree/master/examples/servant

GitHubKey is specified without the result type there. I can't figure out a good way to do something similar (use one GitHubKey for request bodies of different types) with the latest version of servant-github-webhook. I can use Data.Aeson.Object instead of different types, but it's not a pleasant thing to do.

[kvanbere]

What’s the error? This should trivially work, as I just checked the commit history and there hasn’t been any divergence by either library.

Also all the examples pass a CI build, so they shouldn’t be able to silently break.

[artemohanjanyan]

@donkeybonks I'm talking about the change in 0.4.0.0 "Add dynamic key capabilities". Examples in github-webhooks pass on an earlier version of servant-github-webhook. They use GitHubKey of kind *, it has kind * -> * now, so they won't compile.

[kvanbere]

@artemohanjanyan feel free to open a pull request if you're able to find a way to make them build.

We won't have time for a little while to look at it.

[kirelagin]

I’m afraid, there is not easy way to fix this. The problem is that the value added to the context essentially has a polymorphic type forall result. GitHubKey result and this breaks the instances:

   • Overlapping instances for Servant.Server.Internal.Context.HasContextEntry
                                  '[GitHubKey result0]
                                  (Servant.GitHub.Webhook.GitHubKey' () CreateEvent)
        arising from a use of ‘serveWithContext’
      Matching instances:
        two instances involving out-of-scope types
          instance [overlappable] [safe] Servant.Server.Internal.Context.HasContextEntry
                                           xs val =>
                                         Servant.Server.Internal.Context.HasContextEntry
                                           (notIt : xs) val
            -- Defined in ‘Servant.Server.Internal.Context’
          instance [overlapping] [safe] Servant.Server.Internal.Context.HasContextEntry
                                          (val : xs) val
            -- Defined in ‘Servant.Server.Internal.Context’
      (The choice depends on the instantiation of ‘result0’
       To pick the first instance above, use IncoherentInstances
       when compiling the other instance declarations)

I currently worked this around by wrapping the polymorphic type and providing a custom instance:

-- do not import `gitHubKey` and `GitHubKey` unqualified
import qualified Servant.GitHub.Webhook (GitHubKey, gitHubKey)


-- HACK
newtype GitHubKey = GitHubKey (forall result. Servant.GitHub.Webhook.GitHubKey result)

gitHubKey :: IO ByteString -> GitHubKey
gitHubKey k = GitHubKey (Servant.GitHub.Webhook.gitHubKey k)

instance HasContextEntry '[GitHubKey] (Servant.GitHub.Webhook.GitHubKey result) where
    getContextEntry (GitHubKey x :. _) = x

[evanrelf]

This is still a problem. I was pulling my hair out trying to figure out what the problem was until I found this issue.

[kvanbere]

Hi @evanrelf

I have noticed that if you pin the version of this package to the older version, it is working a lot better (more straightforward code). Nethertheless, a month or so ago I updated the examples to work correctly with latest using the hack as above.

[tsani]

@evanrelf sorry to hear that this issue got in the way of getting things done!

I've thought about this issue some more, and I think there are a number of possible solutions.

1. I can ship the newtype hack with the library and explain in the documentation that this is the way to create "static" keys, that are disconnected from any particular result type.
2. I can go back to having just one type parameter on GitHubKey (i.e. drop GitHubKey') but instead have result existentially quantified inside with a Data.Typeable constraint, so it can analyze the response type to decide what key to use.
3. This is perhaps an improved version of (2): instead of using Data.Typeable, the request body given to the githubkey function is fixed to be an Aeson.Object.
4. Use a dependent pair: if we have a singleton for each repo event, then we can make a GADT to connect these tags to the particular, parsed JSON objects. Then instead of result we simply have an unconstrained instance of this GADT. This approach would also make the route handlers more strongly typed, but the githubkey function would have to consider every possible webhook. This is probably not a big problem since I reckon that the main use case is to look at the user or repo that the webhook is associated to, and this information is present in every webhook request.

I'll need to implement each of these to evaluate what the cost looks like for users. @TomMD do you think you could weigh in on these approaches? Your PR introduced the result type parameter so I want to make sure I don't break your use-case.

[evanrelf]

@tsani No worries!

Unfortunately I'm not familiar enough with why the change was made, why it's problematic for cases like this, or why the solution works. But I think a valid solution is just documenting this case so people don't have trouble in the future.

[TomMD]

I don't have much bandwidth with which I may offer input. My needs are merely to keep the package working github apps. A first glance suggested the PR resulted in a much more complex API than needed - I would take zero offense to if someone replaces it all with better, clearer, awesomer code.

If we can use servant to serve someAPI gitHubKey someTextValue and handle (and authenticate!) events from a github application then my needs will be satisfied.