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

Adding UseFinal Middleware to Ensure Consistent HTTP Response Management #101

Merged
merged 3 commits into from
May 6, 2023

Conversation

shakedoron
Copy link
Contributor

We have a situation where the Before middleware is returning an HTTP response.
Additionally, in the same service, we have an After middleware that we use to manage a metric of HTTP response status codes.
The current design does not support this case because the middleware chain is interrupted since the Before middleware returned an HTTP response, and we could not call ctx.next in this case.

In our implementation, we initially assumed that the after middleware would always be called.
For this case, and many others, we believe it is important to add middleware that will be called just before sending the HTTP response, without relying on what happened in the before, view, or after stages.

We decided not to use the existing middleware interface because it does not make sense for logic that runs at this stage to return an error since we did not want to change the ctx.Response based on these functions.
These functions should have logic that is separate from the existing logic for handling the request.

To solve the issue of HTTP responses being returned by Before middleware, we propose defining a new middleware method called UseFinal.
Any logic added to this middleware will be executed regardless of what happened in the before, view, or after stages, and will be called just before sending the HTTP response.

This approach will allow us to properly manage HTTP response status codes in all scenarios, without relying on the behavior of other middleware in the chain.
We believe this is a clean and effective solution that will improve the maintainability and scalability of our codebase.

@savsgio
Copy link
Owner

savsgio commented May 5, 2023

LGTM! I think it's a great solution, without break backward-compatibility and keeping the middleware semantics!

Thank you so much!

@savsgio savsgio added enhancement New feature or request middleware labels May 5, 2023
@coveralls
Copy link

coveralls commented May 5, 2023

Pull Request Test Coverage Report for Build 4895435820

  • 25 of 25 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 4446681524: 0.0%
Covered Lines: 771
Relevant Lines: 771

💛 - Coveralls

@savsgio
Copy link
Owner

savsgio commented May 5, 2023

Can you fix the linter issues, please?

@shakedoron
Copy link
Contributor Author

@savsgio Thanks for your fast response!
Happy to hear that you love our solution.

I fixed the linter issues :)
Let me know if it is ok now

@savsgio savsgio merged commit 008d944 into savsgio:master May 6, 2023
@savsgio
Copy link
Owner

savsgio commented May 6, 2023

Thank you so much! @shakedoron

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request middleware
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants