-
Notifications
You must be signed in to change notification settings - Fork 707
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
WIP: Add user defined OnBeforeRequest middlewares on all response paths, improve documentation #372
base: main
Are you sure you want to change the base?
Conversation
…mprove documentation
e65daf1
to
1e6bd6b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
- Coverage 96.19% 95.90% -0.29%
==========================================
Files 10 10
Lines 1235 1246 +11
==========================================
+ Hits 1188 1195 +7
- Misses 26 29 +3
- Partials 21 22 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lggomez Thanks for creating PR to describing an enhancement. Now, I have an understanding of the proposal.
- I have shared few comments, please take care.
- Any reason, why these two (
responseLogger
,parseResponseBody
) response middleware is not called in the proposal? At least I think logger one needs to be executed.
Thanks again!
I made the suggested changes
I would like to be able to use a custom logger (at least in my use case), and I can see the default For the general case of intercepting all return paths (including errors where responses can be in an invalid state) I'd leave that responsibility up to the user and their middleware implementation (unless otherwise specified in the documentation like the body drain mentions) |
To clarify a bit more about the use case: my application is designed to support debug flows on a per-request basis based on a flag so the current per-client configuration doesn't really fit and a user defined middleware in this setup could allow for per-request behaviors without coupling to the internal middlewares |
@lggomez I see your viewpoint and use case. Let's do the following -
Sounds like a plan? |
All of them sound fine; for the scope of the PR I'd keep with implementing just the first item for now Regarding that, one of the issues of moving the internal middleware to all paths is that suddenly the applyResponseMiddlewares has to return an error:
I pushed the current change, so I'll await your opinion on this |
@lggomez Do you mean returning an error of response middleware from L852, L866, and L875 it will break the existing behavior? If it is not returned; then the previous error is getting eaten/suppressed by resty, isn't it?. Typically any libraries shouldn't eat any errors, only the end-user can make a decision on ignoring the error. |
Yeah, changing c.applyResponseMiddlewares(response)
return response, err to return response, c.applyResponseMiddlewares(response) breaks the following (only on the first early return on the execute() method):
This issue aside, the applyResponseMiddlewares() error would be shadowing the original execute() error, so doing this right is a bit hairier than I thought initially. We want to be able to cancel the middleware stack but we also want to be able to propagate accordingly the errors (maybe wrap the mw error?) |
@lggomez I'm sorry for the delay in response. I think wrapping the error will be a good choice. Please go ahead. Thanks. Also, streamlining this part in the next major version as breaking change will help everyone. |
Work for #356