search

haskell-servant / servant

Main repository for the servant libraries — DSL for describing, serving, querying, mocking, documenting web applications and more!
https://docs.servant.dev/
1.8k stars 407 forks source link

Client middleware #1720

Closed thought2 closed 6 months ago

thought2 commented 6 months ago

At work we had the following requirement: Every endpoint call with servant-client should log it's raw request and response data.

At first, it looked like this may be possible to implement with managerModifyRequest and managerModifyResponse.

However, this turned out to be not a good idea. The docs itself mention for the first function that "This function may be called more than once during request processing". And generally it was important that the logging of the request and response happens at the exact time when they orccur. And those function don't seem to give guarantees about this.

In the end our solution was roughly to create a newtype wrapper around ClientM with a custom instance for RunClient, which calls performRequest but wrapped with some middleware functions.

I wonder if there could be a way to support defining "real" middleware of type (Request -> ClientM Response) -> (Request -> ClientM Response) in servant-client.

This PR is an implementation sketch of how I'd imagine this to work. I contains a simple testcase which should clarify the main idea.

I'd be curious to hear what people think about this approach.

Note: This PR does not cover yet how errors thrown in the middleware should be handled. One way to do this would be to extend the ClientError type with a new constructor MiddlewareError SomeException.

tchoutri commented 6 months ago

Hi, thank you for this contribution! As this is a breaking change (adding a publicly exposed field to a record breaks pattern-matching in consumers) and not trivial addition, would you mind adding a changelog entry? :)

thought2 commented 6 months ago

@tchoutri Sure, I added a changelog entry and another test which shows how errors can be thrown in the middleware.

tchoutri commented 6 months ago

@abailly May I ask for your opinion on this?

abailly commented 6 months ago

@tchoutri Sure, always happy to help but it's been a while since I have used servant in anger :)

thought2 commented 6 months ago

I also added a test for "chaining middleware".

thought2 commented 6 months ago

@abailly I applied all the changes that you suggested, except the one where I left a comment. Thanks for the review!

tchoutri commented 6 months ago

Looks like sqlite-simple-0.4.19.0 can't be compiled with GHC 8.6.5. Since it's an obsolete version it can be safely dropped from CI.

thought2 commented 6 months ago

Great to see this merged, thanks for the support! @abailly @tchoutri