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

middleware wrapping tollboth/v8 #34

Closed
wants to merge 4 commits into from
Closed

middleware wrapping tollboth/v8 #34

wants to merge 4 commits into from

Conversation

umputun
Copy link
Member

@umputun umputun commented Jan 1, 2025

This PR adds a middleware wrapper for tollbooth/v8 limiter.

The wrapper makes it possible to use the limiter with any router supporting the stdlib http-related signatures. The code is inspired by tollbooth_chi but has several notable differences:

  • It works with v8 instead of v7 of the tollbooth.
  • The chi dependency has been removed.
  • The actual handler code is "inlined" and doesn't need to create an internal limiterWrapper anymore.
  • It sets IP lookup to RemoteAddr by default, only if not set by the limiter itself.

@umputun umputun requested a review from paskal January 1, 2025 21:20
@coveralls
Copy link

Pull Request Test Coverage Report for Build 12574489619

Details

  • 20 of 20 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 88.324%

Totals Coverage Status
Change from base Build 12447427982: 0.3%
Covered Lines: 764
Relevant Lines: 865

💛 - Coveralls

@umputun
Copy link
Member Author

umputun commented Jan 1, 2025

this PR is on hold now, waiting for progress on didip/tollbooth#113
if that one is accepted and merged in, we won't need the wrapper on our level

@paskal
Copy link
Collaborator

paskal commented Jan 2, 2025

Could change be backported except for logic change to https://github.com/didip/tollbooth_chi once tollbooth change is merged?

@umputun
Copy link
Member Author

umputun commented Jan 2, 2025

Could change be backported except for logic change to https://github.com/didip/tollbooth_chi once tollbooth change is merged?

Not sure what the end goal will be. The change in the tollbooth has already merged and been tagged. This wrapper seems to be fully redundant and not needed anymore, unless I missed the point.

@umputun
Copy link
Member Author

umputun commented Jan 2, 2025

closing without merging, as the upstream accepted direct PR with HTTPMidleware

@umputun umputun closed this Jan 2, 2025
@paskal paskal deleted the tollboth branch January 2, 2025 19:43
@paskal
Copy link
Collaborator

paskal commented Jan 2, 2025

@umputun we might want to submit a PR to https://github.com/didip/tollbooth_chi stating how to use new HTTPMidleware directly without involving that package.

Also I've missed it initially, but didip/tollbooth#113 didn't update the Readme to mention the new functionality.

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