LCORE-741: quota handlers config #676
Conversation
WalkthroughAdds a new QuotaHandlersConfig model (fields: sqlite, postgres, enable_token_history) and a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-09-18T16:46:33.353ZApplied to files:
📚 Learning: 2025-09-18T16:46:33.353ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/models/config.py (1)
567-573: Consider adding validation for database configuration consistency.Similar configuration classes like
DatabaseConfiguration(lines 113-127) andConversationCacheConfiguration(lines 521-551) include@model_validatormethods to ensure configuration consistency. Consider whetherQuotaHandlersConfigshould validate:
- Whether both
sqliteandpostgrescan be configured simultaneously- Whether at least one backend must be configured when quota handlers are enabled
- Whether
enable_token_historyrequires a specific backendExample validation pattern:
@model_validator(mode="after") def check_quota_handlers_configuration(self) -> Self: """Check quota handlers configuration.""" # Add validation logic based on your requirements # For example, if only one backend should be configured: backends_configured = sum([ self.sqlite is not None, self.postgres is not None ]) if backends_configured > 1: raise ValueError("Only one database backend can be configured for quota handlers") return self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/models/config.py(2 hunks)tests/unit/models/config/test_dump_configuration.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/models/config.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/models/config.pytests/unit/models/config/test_dump_configuration.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/models/config.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/config.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/models/config/test_dump_configuration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/models/config/test_dump_configuration.py
🪛 GitHub Actions: Black
tests/unit/models/config/test_dump_configuration.py
[error] 1-1: Black formatting check failed. Run 'black' to reformat the file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-pr
- GitHub Check: e2e_tests (azure)
- GitHub Check: e2e_tests (ci)
🔇 Additional comments (4)
src/models/config.py (1)
594-594: LGTM!The
quota_handlersfield is properly added to theConfigurationclass with an appropriate default factory, following the established pattern used by other configuration fields likeconversation_cache(line 590-592).tests/unit/models/config/test_dump_configuration.py (3)
92-92: LGTM!The assertion correctly verifies that the new
quota_handlersfield is present in the serialized configuration.
175-179: LGTM!The expected JSON structure correctly reflects the default values for the
QuotaHandlersConfigmodel. The test properly validates serialization of the new configuration field.Note: If the field name is changed from
storagetopostgresas suggested in the review ofsrc/models/config.py, this test expectation will need to be updated accordingly (see the note in that review comment).
1-1: Install and run Black to fix formatting. CI is failing due to Black formatting errors. Ensure Black is installed in your environment (pip install black), then run:black tests/unit/models/config/test_dump_configuration.pyCommit the reformatted file and re-run the pipeline to confirm the issue is resolved.
| class QuotaHandlersConfig(ConfigurationBase): | ||
| """Quota limiter configuration.""" | ||
|
|
||
| sqlite: Optional[SQLiteDatabaseConfiguration] = None | ||
| storage: Optional[PostgreSQLDatabaseConfiguration] = None | ||
| enable_token_history: bool = False | ||
|
|
There was a problem hiding this comment.
Inconsistent field naming: use postgres instead of storage.
The field storage holds a PostgreSQLDatabaseConfiguration but doesn't follow the naming pattern established in similar configuration classes. Both DatabaseConfiguration (line 111) and ConversationCacheConfiguration (line 519) use postgres for their PostgreSQL fields.
Apply this diff to align with the established naming pattern:
class QuotaHandlersConfig(ConfigurationBase):
"""Quota limiter configuration."""
sqlite: Optional[SQLiteDatabaseConfiguration] = None
- storage: Optional[PostgreSQLDatabaseConfiguration] = None
+ postgres: Optional[PostgreSQLDatabaseConfiguration] = None
enable_token_history: bool = FalseNote: This change will also require updating the test expectations in tests/unit/models/config/test_dump_configuration.py at line 177:
"quota_handlers": {
"sqlite": None,
- "storage": None,
+ "postgres": None,
"enable_token_history": False
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class QuotaHandlersConfig(ConfigurationBase): | |
| """Quota limiter configuration.""" | |
| sqlite: Optional[SQLiteDatabaseConfiguration] = None | |
| storage: Optional[PostgreSQLDatabaseConfiguration] = None | |
| enable_token_history: bool = False | |
| class QuotaHandlersConfig(ConfigurationBase): | |
| """Quota limiter configuration.""" | |
| sqlite: Optional[SQLiteDatabaseConfiguration] = None | |
| postgres: Optional[PostgreSQLDatabaseConfiguration] = None | |
| enable_token_history: bool = False |
🤖 Prompt for AI Agents
In src/models/config.py around lines 567 to 573, the field named `storage`
should be renamed to `postgres` to match existing naming patterns; change the
attribute to `postgres: Optional[PostgreSQLDatabaseConfiguration] = None`
(keeping the same type and default), update any imports/typing if necessary, and
then update all references across the codebase and tests (notably
tests/unit/models/config/test_dump_configuration.py at line 177) to use
`postgres` instead of `storage`.
1 participant
Description
LCORE-741: quota handlers config
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Tests