feat: get plan bulk with cache#161
Open
tomerqodo wants to merge 4 commits intoqodo_claude_vs_qodo_base_feat_get_plan_bulk_with_cache_pr10from
Open
Conversation
Review Summary by QodoAdd Redis caching for bulk plan retrieval with fallback
WalkthroughsDescription• Add caching layer to get_plan_bulk with Redis to reduce billing API load • Implement get_plan_bulk_with_cache method with fallback to API on cache miss • Add comprehensive integration and unit tests for cache scenarios • Improve error handling in get_plan_bulk with per-tenant validation Diagramflowchart LR
A["Bulk Plan Request"] --> B["Check Redis Cache"]
B --> C{Cache Hit?}
C -->|Yes| D["Return Cached Plans"]
C -->|No| E["Fetch from Billing API"]
E --> F["Validate Plans"]
F --> G["Store in Redis"]
G --> H["Return Plans"]
D --> H
File Changes1. api/services/billing_service.py
|
Code Review by Qodo
1. Integration tests missing annotations
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+28
to
+31
| def __str__(self) -> str: | ||
| """Return a human-readable string representation for debugging.""" | ||
| return f"Plan: {self['plan']}, Expiration: {self['expiration_date']}" | ||
|
|
There was a problem hiding this comment.
1. subscriptionplan.str unnecessary override 📘 Rule violation ✓ Correctness
SubscriptionPlan is a TypedDict (dict-typed structure) but the PR adds a __str__ method that will not apply to the actual dict instances used at runtime. This is an unnecessary dunder override and can mislead debugging/maintenance.
Agent Prompt
## Issue description
A `__str__` dunder override was added to a `TypedDict` (`SubscriptionPlan`). `TypedDict` is a typing construct and the actual objects used are plain `dict`s, so this override is misleading and effectively unused.
## Issue Context
The compliance rule restricts unnecessary dunder overrides.
## Fix Focus Areas
- api/services/billing_service.py[28-31]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+325
to
+328
| if len(cached_values) != len(tenant_ids): | ||
| raise Exception( | ||
| "get_plan_bulk_with_cache: unexpected error: redis mget failed: cached values length mismatch" | ||
| ) |
There was a problem hiding this comment.
2. get_plan_bulk_with_cache raises exception 📘 Rule violation ⛯ Reliability
The new caching flow raises a generic Exception for a Redis mget length mismatch, which conflicts with the requirement to use domain-specific exceptions in services. This reduces error clarity and makes consistent controller-level translation harder.
Agent Prompt
## Issue description
`get_plan_bulk_with_cache` raises a generic `Exception` in service code, which violates the requirement to use domain-specific exceptions (or avoid raising for expected fallback paths).
## Issue Context
This error is used to trigger fallback behavior; using a generic exception reduces clarity and standardization.
## Fix Focus Areas
- api/services/billing_service.py[325-328]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+40
| def setup_redis_cleanup(self, flask_app_with_containers): | ||
| """Clean up Redis cache before and after each test.""" | ||
| with flask_app_with_containers.app_context(): | ||
| # Clean up before test | ||
| yield | ||
| # Clean up after test | ||
| # Delete all test cache keys | ||
| pattern = f"{BillingService._PLAN_CACHE_KEY_PREFIX}*" | ||
| keys = redis_client.keys(pattern) | ||
| if keys: | ||
| redis_client.delete(*keys) | ||
|
|
||
| def _create_test_plan_data(self, plan: str = "sandbox", expiration_date: int = 1735689600): | ||
| """Helper to create test SubscriptionPlan data.""" | ||
| return {"plan": plan, "expiration_date": expiration_date} | ||
|
|
||
| def _set_cache(self, tenant_id: str, plan_data: dict, ttl: int = 600): | ||
| """Helper to set cache data in Redis.""" |
There was a problem hiding this comment.
3. Integration tests missing annotations 📘 Rule violation ✓ Correctness
New test helper and test functions are missing required type annotations (parameters and/or return types). This violates the rule requiring explicit type hints for all Python function definitions.
Agent Prompt
## Issue description
Several newly-added test functions/helpers lack required type annotations (parameters and return types).
## Issue Context
The repository requires explicit type annotations for all Python function definitions.
## Fix Focus Areas
- api/tests/test_containers_integration_tests/services/test_billing_service.py[23-24]
- api/tests/test_containers_integration_tests/services/test_billing_service.py[35-37]
- api/tests/test_containers_integration_tests/services/test_billing_service.py[39-43]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+285
to
+292
| try: | ||
| subscription_plan = subscription_adapter.validate_python(plan) | ||
| results[tenant_id] = subscription_plan | ||
| except Exception: | ||
| logger.exception( | ||
| "get_plan_bulk: failed to validate subscription plan for tenant(%s)", tenant_id | ||
| ) | ||
| results[tenant_id] = None |
There was a problem hiding this comment.
4. None plan returned 🐞 Bug ✓ Correctness
BillingService.get_plan_bulk stores None for tenants whose plan payload fails validation even though it declares returning dict[str, SubscriptionPlan]. This will either break callers that index into the plan (e.g., plan['plan']) or fail the new unit test that expects invalid tenants to be omitted.
Agent Prompt
### Issue description
`BillingService.get_plan_bulk()` is annotated/documented as returning `dict[str, SubscriptionPlan]` but currently inserts `None` for tenants whose payload fails validation. This violates the function contract and contradicts the newly added unit test that expects invalid tenants to be omitted.
### Issue Context
Keeping `None` values will also cascade into `get_plan_bulk_with_cache()` (which consumes `get_plan_bulk()`), potentially returning `None` to callers that expect a dict-like `SubscriptionPlan`.
### Fix Focus Areas
- api/services/billing_service.py[266-297]
- api/tests/unit_tests/services/test_billing_service.py[1297-1320]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+365
to
+377
| for tenant_id, subscription_plan in bulk_plans.items(): | ||
| tenant_plans[tenant_id] = subscription_plan | ||
| plans_to_cache[tenant_id] = subscription_plan | ||
|
|
||
| # Step 3: Batch update Redis cache using pipeline | ||
| if plans_to_cache: | ||
| try: | ||
| pipe = redis_client.pipeline() | ||
| for tenant_id, subscription_plan in bulk_plans.items(): | ||
| redis_key = cls._make_plan_cache_key(tenant_id) | ||
| # Serialize dict to JSON string | ||
| json_str = json.dumps(subscription_plan) | ||
| pipe.setex(redis_key, cls._PLAN_CACHE_TTL, json_str) |
There was a problem hiding this comment.
5. Caches none plans 🐞 Bug ⛯ Reliability
get_plan_bulk_with_cache caches and returns all values from get_plan_bulk without filtering, including None entries. This can write "null" into Redis and propagate None to callers, causing repeated cache misses or runtime errors when accessing plan fields.
Agent Prompt
### Issue description
`get_plan_bulk_with_cache()` currently assumes `get_plan_bulk()` returns valid `SubscriptionPlan` values, but the current implementation can include `None`. The cache path then serializes and stores these invalid values and returns them to callers.
### Issue Context
`json.dumps(None)` becomes `"null"`, which will be stored in Redis for the TTL window and will never validate back into `SubscriptionPlan`.
### Fix Focus Areas
- api/services/billing_service.py[358-387]
- api/services/billing_service.py[266-297]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+22
to
+33
| @pytest.fixture(autouse=True) | ||
| def setup_redis_cleanup(self, flask_app_with_containers): | ||
| """Clean up Redis cache before and after each test.""" | ||
| with flask_app_with_containers.app_context(): | ||
| # Clean up before test | ||
| yield | ||
| # Clean up after test | ||
| # Delete all test cache keys | ||
| pattern = f"{BillingService._PLAN_CACHE_KEY_PREFIX}*" | ||
| keys = redis_client.keys(pattern) | ||
| if keys: | ||
| redis_client.delete(*keys) |
There was a problem hiding this comment.
6. No pre-test redis clean 🐞 Bug ⛯ Reliability
The new integration test fixture states it cleans Redis before and after each test, but it only deletes keys after the test yields. If stale keys with the same prefix exist before the suite starts, the first cache-hit/miss test can be affected.
Agent Prompt
### Issue description
The integration test fixture claims to clean Redis before/after, but currently only cleans after the test.
### Issue Context
This mainly impacts the first test executed if Redis contains leftover keys from earlier runs.
### Fix Focus Areas
- api/tests/test_containers_integration_tests/services/test_billing_service.py[22-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#10