Skip to content

Conversation

@heyOnuoha
Copy link
Contributor

@heyOnuoha heyOnuoha commented Sep 1, 2025

Description

This PR adds a Rate Limiting Middleware to pharaoh middlewares.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

- Implement token bucket and sliding window rate limiting algorithms
- Add configurable middleware with Express.js-like API
- Include comprehensive test coverage (18 tests passing)
- Add examples and documentation
- Support custom key generation, skip functions, and headers
- Compatible with existing Pharaoh middleware system
- Fix import path and API usage for rate limiting example
- Add proper dependency configuration in pubspec.yaml
- Create comprehensive test script demonstrating all features
- Verify rate limiting works with token bucket and sliding window
- Test custom key generation, skip functionality, and headers
This commit applies consistent formatting, improves code readability, and fixes minor whitespace issues across rate limiting middleware, algorithms, and example/test files. No functional changes were made; the update is purely stylistic to enhance maintainability.
- Fix dart analyze warning for unused 'body' variable
- Consume response body without storing in unused variable
- All analysis checks now pass
@codekeyz
Copy link
Owner

codekeyz commented Sep 2, 2025

Looks good to me. @heyOnuoha

Worth adding actual http tests that verify the rate limiting in action.

@codekeyz codekeyz added the enhancement New feature or request label Sep 2, 2025
@codekeyz codekeyz changed the title add feature limiting middleware feat: Add rate limiting middleware Sep 2, 2025
@codekeyz codekeyz self-requested a review September 2, 2025 00:30
@heyOnuoha
Copy link
Contributor Author

Looks good to me. @heyOnuoha

Worth adding actual http tests that verify the rate limiting in action.

Got it @codekeyz, Will add that in

- Create comprehensive HTTP tests that verify rate limiting in action
- Test 429 responses, rate limit headers, and middleware behavior
- Address maintainer feedback for actual HTTP verification
- Include tests for custom key generation and skip functionality
- All existing unit tests continue to pass (18 tests)
- Create focused HTTP test that verifies rate limiting in action
- Test 429 responses, rate limit headers, and middleware behavior
- Use Spookie framework following Pharaoh testing patterns
- Remove complex HTTP client test attempts
- All 22 tests now pass including HTTP integration tests
- Addresses maintainer feedback for actual HTTP verification
Added new code coverage JSON reports for multiple test files across pharaoh, pharaoh_basic_auth, pharaoh_jwt_auth, pharaoh_rate_limit, spanner, and spookie packages. Removed the unused test_rate_limiting.dart from pharaoh_examples and updated rate_limit_http_test.dart in pharaoh_rate_limit.
Copy link
Owner

@codekeyz codekeyz 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 you added some new files by mistake **.dart.vm.json

@heyOnuoha
Copy link
Contributor Author

I think you added some new files by mistake **.dart.vm.json

Yes, that file was generated by the coverate script melos exec -c 1 -- "dart test --coverage=coverage" --fail-fast

@codekeyz
Copy link
Owner

codekeyz commented Sep 4, 2025

I think you added some new files by mistake **.dart.vm.json

Yes, that file was generated by the coverate script melos exec -c 1 -- "dart test --coverage=coverage" --fail-fast

You'll need to remove them. We don't need them aded to the repo.

@heyOnuoha
Copy link
Contributor Author

I think you added some new files by mistake **.dart.vm.json

Yes, that file was generated by the coverate script melos exec -c 1 -- "dart test --coverage=coverage" --fail-fast

You'll need to remove them. We don't need them aded to the repo.

Alright got it, I'll take them out

Deleted multiple .vm.json files containing test coverage data from various package directories. This cleanup likely prepares for regeneration of coverage reports or reduces repository size by removing generated artifacts.
@codekeyz
Copy link
Owner

Looks good to me, I'll merge now ✅

@codekeyz codekeyz merged commit 061ca82 into codekeyz:main Sep 11, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants