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

Client middleware #1720

Merged
merged 18 commits into from
Jan 24, 2024
Merged

Client middleware #1720

merged 18 commits into from
Jan 24, 2024

Conversation

m-bock
Copy link
Contributor

@m-bock m-bock commented Jan 19, 2024

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
Copy link
Contributor

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? :)

@m-bock
Copy link
Contributor Author

m-bock commented Jan 22, 2024

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

@tchoutri
Copy link
Contributor

@abailly May I ask for your opinion on this?

@abailly
Copy link
Contributor

abailly commented Jan 23, 2024

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

@m-bock
Copy link
Contributor Author

m-bock commented Jan 23, 2024

I also added a test for "chaining middleware".

Copy link
Contributor

@abailly abailly left a comment

Choose a reason for hiding this comment

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

I think this change completely makes sense and I have often wanted to have that functionality available and, as the proposer, resorted to custom ClientM or RunClient implementations for purposes that could have been well served by a ClientMiddleware like authentication, logging, etc.

I have made general suggestions which seem to make sense to me but might be inappropriate because of the coding conventions in force in the codebase, so take those with a grain of salt.

servant-client/src/Servant/Client/Internal/HttpClient.hs Outdated Show resolved Hide resolved
servant-client/src/Servant/Client/Internal/HttpClient.hs Outdated Show resolved Hide resolved
servant-client/test/Servant/MiddlewareSpec.hs Outdated Show resolved Hide resolved
servant-client/test/Servant/MiddlewareSpec.hs Outdated Show resolved Hide resolved
@m-bock
Copy link
Contributor Author

m-bock commented Jan 23, 2024

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

@tchoutri
Copy link
Contributor

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.

@tchoutri tchoutri merged commit 72b7abb into haskell-servant:master Jan 24, 2024
8 checks passed
@m-bock
Copy link
Contributor Author

m-bock commented Jan 26, 2024

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

@ysangkok ysangkok mentioned this pull request Sep 14, 2024
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