LCORE-1061: Table namespace used in quota handlers #913
Conversation
WalkthroughAdds configurable PostgreSQL namespace injection into connections (default "public") and splits quota table SQL into PostgreSQL and SQLite variants, updating initialization code and tests to choose and apply the dialect-specific CREATE TABLE statement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/quota/sql.py (1)
16-26: UseTEXTexplicitly for the SQLite schema instead oftimestamp with time zone.SQLite has no built-in
timestamp with time zonedatatype; it will store this as TEXT. While SQLite's flexible type system allows the statement to execute, explicitly declaringTEXTin theCREATE_QUOTA_TABLE_SQLITEconstant clarifies intent and aligns with SQLite best practices. This improves code clarity and makes the dialect-specific design explicit without affecting functionality.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/quota/connect_pg.py(2 hunks)src/quota/revokable_quota_limiter.py(2 hunks)src/quota/sql.py(1 hunks)src/runners/quota_scheduler.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/quota/sql.pysrc/quota/connect_pg.pysrc/quota/revokable_quota_limiter.pysrc/runners/quota_scheduler.py
🪛 GitHub Actions: Black
src/quota/connect_pg.py
[error] 1-1: Black formatting check failed; 1 file would be reformatted. Run 'black --write' to fix.
🪛 GitHub Actions: Pyright
src/runners/quota_scheduler.py
[error] 63-63: Pyright: Type 'None' is not assignable to declared type 'str' (reportAssignmentType)
🪛 GitHub Actions: Type checks
src/runners/quota_scheduler.py
[error] 63-63: mypy: Incompatible types in assignment (expression has type "None", variable has type "str") [assignment] in src/runners/quota_scheduler.py:63. Command failed: uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (5)
src/quota/connect_pg.py (1)
42-42: Clarify the hardcodeddboin the search_path.The search_path includes
dboas the first schema before the configured namespace. If this is intentional (e.g., for shared objects), consider documenting whydbois included. If not, this may cause unexpected schema resolution behavior.Is including
dboin the search_path intentional? If so, consider adding a comment explaining its purpose. If only the configured namespace should be used:- options=f"-c search_path=dbo,{namespace}" + options=f"-c search_path={namespace}"src/quota/revokable_quota_limiter.py (2)
11-12: LGTM!The imports are correctly updated to include the dialect-specific CREATE TABLE constants.
185-194: LGTM with a minor observation.The dialect selection logic correctly mirrors the pattern used elsewhere in the class. However, if neither
sqlite_connection_confignorpostgres_connection_configis set, the cursor is created and closed without executing anything, which is a harmless no-op but could be worth logging for debugging.src/runners/quota_scheduler.py (2)
19-21: LGTM!The imports are correctly updated to include the dialect-specific CREATE TABLE constants.
306-319: LGTM!The updated
init_tablesfunction correctly accepts the dialect-specific CREATE TABLE statement and executes it. The docstring is properly updated to document the new parameter.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/quota/connect_pg.py (1)
28-31: Namespace defaulting logic is correct.The defensive handling of
config.namespaceproperly falls back to"public"when None, even though the Pydantic model now defaults to"public". This provides robustness if the field is explicitly set to None.Note: The formatting issue flagged in the previous review (missing space around
=on line 30) has been resolved.src/runners/quota_scheduler.py (1)
63-70: Fix type annotation to resolve pipeline failures.The type annotation
create_quota_table: Optional[str] = Nonecauses type errors when passed toinit_tables(line 70), which expectsstr(line 308). Since the guard at line 50 ensures at least one backend is configured, the variable will always be assigned before use.Apply this diff to fix the type error:
- create_quota_table: Optional[str] = None + create_quota_table: str if config.postgres is not None: create_quota_table = CREATE_QUOTA_TABLE_PG - elif config.sqlite is not None: + else: + # SQLite must be configured if we reach here (guard at line 50) create_quota_table = CREATE_QUOTA_TABLE_SQLITE - - if create_quota_table is not None: - init_tables(connection, create_quota_table) + + init_tables(connection, create_quota_table)This approach ensures
create_quota_tableis always astrand eliminates the redundant None check.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/config.json(1 hunks)src/models/config.py(1 hunks)src/quota/connect_pg.py(2 hunks)src/runners/quota_scheduler.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/models/config.pysrc/runners/quota_scheduler.pysrc/quota/connect_pg.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.py
🧬 Code graph analysis (2)
src/runners/quota_scheduler.py (1)
src/models/config.py (1)
config(324-341)
src/quota/connect_pg.py (1)
src/models/config.py (1)
config(324-341)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (5)
docs/config.json (1)
729-729: LGTM! Documentation aligns with code change.The default namespace change to
"public"matches the updated default insrc/models/config.pyand aligns with PostgreSQL's standard default schema.src/models/config.py (1)
220-224: LGTM! Default namespace updated to PostgreSQL standard.Changing the default from
"lightspeed-stack"to"public"aligns with PostgreSQL's standard default schema and improves interoperability.src/runners/quota_scheduler.py (3)
3-3: LGTM! Import addition supports optional type annotation.The
Optionalimport fromtypingis correctly added to support the type annotation on line 63.
19-26: LGTM! Backend-specific SQL constants imported.The imports correctly bring in separate
CREATE_QUOTA_TABLE_PGandCREATE_QUOTA_TABLE_SQLITEconstants, enabling backend-aware table creation.
308-321: LGTM! Function signature updated to accept backend-specific SQL.The
init_tablesfunction now acceptscreate_quota_table: strparameter, enabling dynamic backend-specific table creation. The parameter is properly documented and used.
tisnik
force-pushed
the
lcore-1061-table-namespace-used-in-quota-handler
branch
from
bb7da0c to
35f9f15
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/quota/connect_pg.py(2 hunks)tests/unit/models/config/test_dump_configuration.py(5 hunks)tests/unit/models/config/test_postgresql_database_configuration.py(1 hunks)tests/unit/quota/test_connect_pg.py(2 hunks)tests/unit/quota/test_quota_limiter_factory.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/quota/connect_pg.py
🧰 Additional context used
📓 Path-based instructions (2)
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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/quota/test_connect_pg.pytests/unit/quota/test_quota_limiter_factory.pytests/unit/models/config/test_postgresql_database_configuration.pytests/unit/models/config/test_dump_configuration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/quota/test_connect_pg.pytests/unit/quota/test_quota_limiter_factory.pytests/unit/models/config/test_postgresql_database_configuration.pytests/unit/models/config/test_dump_configuration.py
🧬 Code graph analysis (2)
tests/unit/models/config/test_postgresql_database_configuration.py (1)
src/models/config.py (1)
PostgreSQLDatabaseConfiguration(176-260)
tests/unit/models/config/test_dump_configuration.py (1)
src/models/config.py (4)
Configuration(1367-1481)DatabaseConfiguration(263-341)PostgreSQLDatabaseConfiguration(176-260)dump(1469-1481)
🪛 GitHub Actions: Black
tests/unit/models/config/test_dump_configuration.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted by Black. Run 'black' to fix. Command: 'uv tool run black --check .'
🪛 GitHub Actions: Python linter
tests/unit/models/config/test_dump_configuration.py
[warning] 853-853: Pylint: Trailing newlines detected (C0305).
[warning] 823-823: Pylint: Duplicate key 'namespace' in dictionary (W0109).
🪛 GitHub Actions: Ruff
tests/unit/models/config/test_dump_configuration.py
[error] 833-833: ruff (F601): Dictionary key literal "namespace" repeated. Remove repeated key literal "namespace".
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (6)
tests/unit/quota/test_quota_limiter_factory.py (1)
30-35: LGTM! Namespace parameter consistently added across all PostgreSQL test configurations.The addition of
namespace="foo"to all PostgreSQLDatabaseConfiguration instances ensures that the new namespace functionality is properly exercised in quota limiter tests.Also applies to: 55-60, 82-87, 129-134, 174-179, 207-212
tests/unit/models/config/test_postgresql_database_configuration.py (2)
30-31: LGTM! Default namespace assertion updated correctly.The test now correctly expects the default namespace to be "public" and includes an explicit assertion for
ca_cert_path, improving test completeness.
34-49: LGTM! Good test coverage for custom namespace specification.The new test function comprehensively validates that a custom namespace value (e.g., "foo") is properly stored and retrieved, ensuring the namespace field works as expected.
tests/unit/quota/test_connect_pg.py (1)
15-20: LGTM! Namespace parameter added to connection tests.Both test cases now include
namespace="foo"in the PostgreSQL configuration, ensuring the connection logic properly handles the namespace parameter.Also applies to: 33-39
tests/unit/models/config/test_dump_configuration.py (2)
170-170: LGTM! Default namespace correctly updated to "public".The expected namespace value in serialized configurations has been correctly updated to reflect the new default value of "public".
Also applies to: 470-470
545-545: LGTM! Custom namespace correctly set and verified.The test properly configures a custom namespace ("foo") and validates it in the serialized output.
Also applies to: 656-656
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/quota/connect_pg.py (1)
28-31: Namespace-basedsearch_pathconfiguration looks solid; consider a small cleanupUsing a default
"public"namespace and overriding fromconfig.namespace, then wiring it intooptions=f"-c search_path={namespace}", is consistent with the configuration model and gives the desired per-namespace behavior.If you want to tighten this up a bit, you could optionally:
- Collapse the default/override into a single expression and normalize whitespace, e.g.:
- namespace = "public" - if config.namespace is not None: - namespace = config.namespace + namespace = (config.namespace or "public").strip()
- (Optionally) treat an empty string as “no override” and skip the
optionsargument entirely ifnamespaceends up empty.These are purely cosmetic/robustness tweaks; the current logic is correct.
Also applies to: 42-42
tests/unit/models/config/test_postgresql_database_configuration.py (1)
30-32: Tests correctly cover default and custom namespace; consider an extra edge-caseThe updates to assert the default namespace
"public"and the newtest_postgresql_database_configuration_namespace_specificationexercising a custom"foo"namespace look consistent with the configuration model and give good coverage of the new field.If you care about the explicit
namespace=Nonebehavior (given theOptional[str]type), you might optionally add a small test to lock in how that should behave, for example:+def test_postgresql_database_configuration_namespace_none() -> None: + """Explicit None namespace should fall back to public (or be None, per spec).""" + # pylint: disable=no-member + c = PostgreSQLDatabaseConfiguration( + db="db", + user="user", + password="password", + namespace=None, + ) + # Decide and assert the intended behavior here: + # e.g., assert c.namespace is None # or "public"But as-is, the new and updated tests already validate the primary namespace use cases.
Also applies to: 34-49
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/quota/connect_pg.py(2 hunks)tests/unit/models/config/test_dump_configuration.py(5 hunks)tests/unit/models/config/test_postgresql_database_configuration.py(1 hunks)tests/unit/quota/test_connect_pg.py(2 hunks)tests/unit/quota/test_quota_limiter_factory.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/quota/test_quota_limiter_factory.py
- tests/unit/quota/test_connect_pg.py
- tests/unit/models/config/test_dump_configuration.py
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/quota/connect_pg.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 framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/models/config/test_postgresql_database_configuration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/models/config/test_postgresql_database_configuration.py
🧬 Code graph analysis (2)
src/quota/connect_pg.py (1)
src/models/config.py (1)
config(324-341)
tests/unit/models/config/test_postgresql_database_configuration.py (1)
src/models/config.py (1)
PostgreSQLDatabaseConfiguration(176-260)
⏰ 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). (1)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
1 participant
Description
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.