feat: get plan bulk with cache#23
Conversation
WalkthroughRedis-backed caching is introduced for BillingService tenant plan data. A new public method Changes
Sequence DiagramsequenceDiagram
participant Client
participant BillingService
participant Redis
participant ExternalAPI as External API
Client->>BillingService: get_plan_bulk_with_cache(tenant_ids)
BillingService->>Redis: mget(cache_keys)
Redis-->>BillingService: cached_plans (partial/empty)
alt Cache Miss or Invalid Data
BillingService->>ExternalAPI: get_plan_bulk(missing_tenant_ids)
ExternalAPI-->>BillingService: api_plans
BillingService->>BillingService: validate_plans (per-item)
end
BillingService->>Redis: pipeline write (valid_plans with TTL)
Redis-->>BillingService: ack
BillingService-->>Client: merged_result {plan_data}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/services/billing_service.py (1)
22-31: Remove__str__method fromSubscriptionPlanTypedDict.Per PEP 589, TypedDict instances are plain dict objects at runtime. Methods cannot be defined on TypedDict, and any
__str__method will never be invoked—the dict's default string representation will be used instead. This method is dead code and should be removed.If you need custom string formatting for debugging, use one of these alternatives:
- Create a Pydantic
BaseModelinstead ofTypedDict- Use a
dataclass- Create a helper function for formatting
🤖 Fix all issues with AI agents
In `@api/services/billing_service.py`:
- Around line 362-377: The code currently caches None results from
get_plan_bulk, causing "null" to be stored and later Pydantic validation
failures; update the loop that builds tenant_plans and plans_to_cache to skip
entries where subscription_plan is None (i.e., only set tenant_plans[tenant_id]
and plans_to_cache[tenant_id] when subscription_plan is not None), and ensure
the subsequent pipeline loop over bulk_plans uses the filtered plans_to_cache
(or iterates plans_to_cache.items()) when calling cls._make_plan_cache_key,
json.dumps(subscription_plan) and pipe.setex(..., cls._PLAN_CACHE_TTL, ...).
- Around line 285-292: The declared return type for get_plan_bulk (dict[str,
SubscriptionPlan]) mismatches runtime behavior where results[tenant_id] is set
to None on validation failure; fix this by either updating the function's return
annotation to dict[str, SubscriptionPlan | None] (and adjust callers as needed)
or change the loop that uses subscription_adapter.validate_python to skip
invalid tenants and only add successful subscription_plan entries to results;
locate symbols get_plan_bulk, subscription_adapter.validate_python, and results
to implement the chosen fix.
In `@api/tests/unit_tests/services/test_billing_service.py`:
- Around line 1297-1331: The test currently expects invalid tenants to be
omitted but BillingService.get_plan_bulk sets results[tenant_id] = None for
invalid tenants; update the test assertions to reflect that behavior by
asserting result["tenant-invalid"] is None (instead of asserting the tenant is
not in result) and keep the logger.exception assertions (adjust call arg
indexing if needed) so the test verifies the invalid tenant was logged while
still present with a None value; reference BillingService.get_plan_bulk and the
test function test_get_plan_bulk_with_invalid_tenant_plan_skipped when making
the change.
🧹 Nitpick comments (3)
api/services/billing_service.py (2)
325-328: Overly defensive check for Redismgetresponse length.Redis
mgetalways returns a list with the same length as the input keys list (returningNonefor missing keys). This length mismatch check will never trigger under normal circumstances and adds unnecessary code complexity.Consider removing this check or converting it to a debug assertion.
♻️ Proposed simplification
- if len(cached_values) != len(tenant_ids): - raise Exception( - "get_plan_bulk_with_cache: unexpected error: redis mget failed: cached values length mismatch" - ) + assert len(cached_values) == len(tenant_ids), "Redis mget returned unexpected length"
360-360: Minor: Useclsinstead of class name for internal method call.For consistency with the rest of the class methods, use
cls.get_plan_bulk(cache_misses)instead ofBillingService.get_plan_bulk(cache_misses).♻️ Proposed fix
- bulk_plans = BillingService.get_plan_bulk(cache_misses) + bulk_plans = cls.get_plan_bulk(cache_misses)api/tests/test_containers_integration_tests/services/test_billing_service.py (1)
320-365: Consider test performance:time.sleep(2)slows the test suite.While using actual sleep to test Redis TTL expiration is valid for integration tests, it adds 2 seconds to the test suite runtime. For CI pipelines running many tests, this can accumulate.
If test speed becomes a concern, consider:
- Mocking Redis
get/mgetto returnNonefor "expired" keys in a separate unit test- Using a shorter TTL (e.g., 100ms) if Redis allows sub-second precision
However, if the goal is to validate actual Redis TTL behavior, the current approach is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/services/billing_service.pyapi/tests/test_containers_integration_tests/services/test_billing_service.pyapi/tests/unit_tests/services/test_billing_service.py
🧰 Additional context used
🧬 Code graph analysis (3)
api/tests/test_containers_integration_tests/services/test_billing_service.py (1)
api/services/billing_service.py (3)
BillingService(33-396)_make_plan_cache_key(300-301)get_plan_bulk_with_cache(304-387)
api/tests/unit_tests/services/test_billing_service.py (2)
api/tests/unit_tests/extensions/test_ext_request_logging.py (1)
mock_logger(60-63)api/services/billing_service.py (1)
get_plan_bulk(267-297)
api/services/billing_service.py (2)
api/libs/helper.py (1)
RateLimiter(376-402)api/extensions/ext_redis.py (1)
setex(73-73)
🔇 Additional comments (11)
api/services/billing_service.py (2)
1-1: LGTM!The
jsonimport is appropriately added to support the new caching functionality.
39-42: LGTM!Cache constants are well-defined with appropriate naming conventions and reasonable TTL value for billing plan data.
api/tests/test_containers_integration_tests/services/test_billing_service.py (9)
22-34: LGTM!The cleanup fixture with
autouse=Trueensures proper test isolation by deleting cached keys after each test. Using pattern-based deletion is appropriate for integration tests.
55-84: LGTM!Thorough test for cache hit scenario with proper verification that the API is not called when all data is cached.
86-124: LGTM!Comprehensive test for cache miss scenario with proper verification of cache writes and TTL settings.
126-155: LGTM!Effective test for partial cache hit scenario, verifying that only cache-missed tenants trigger API calls.
157-186: LGTM!Good coverage for Redis failure fallback scenario with verification that cache is populated after successful API call.
188-238: LGTM!Excellent test for invalid JSON handling with verification that corrupted cache entries are replaced with valid data from the API.
240-271: LGTM!Good test for schema validation failure handling in cached data.
273-300: LGTM!Good test verifying that Redis pipeline failures don't affect the method's return value.
302-318: LGTM!Appropriate edge case test for empty input handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| 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.
Return type annotation inconsistent with actual behavior.
The method signature declares dict[str, SubscriptionPlan] as the return type (line 267), but on validation failure, None is assigned to results[tenant_id]. This type mismatch could cause runtime errors in callers expecting SubscriptionPlan objects.
Consider updating the return type to dict[str, SubscriptionPlan | None] or filtering out None values before returning.
♻️ Option 1: Update return type annotation
`@classmethod`
- def get_plan_bulk(cls, tenant_ids: Sequence[str]) -> dict[str, SubscriptionPlan]:
+ def get_plan_bulk(cls, tenant_ids: Sequence[str]) -> dict[str, SubscriptionPlan | None]:♻️ Option 2: Exclude invalid tenants from results
except Exception:
logger.exception(
"get_plan_bulk: failed to validate subscription plan for tenant(%s)", tenant_id
)
- results[tenant_id] = None
+ # Skip invalid tenant, don't add to results
+ continue📝 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.
| 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 | |
| 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 | |
| ) | |
| # Skip invalid tenant, don't add to results | |
| continue | |
| results[tenant_id] = None |
🤖 Prompt for AI Agents
In `@api/services/billing_service.py` around lines 285 - 292, The declared return
type for get_plan_bulk (dict[str, SubscriptionPlan]) mismatches runtime behavior
where results[tenant_id] is set to None on validation failure; fix this by
either updating the function's return annotation to dict[str, SubscriptionPlan |
None] (and adjust callers as needed) or change the loop that uses
subscription_adapter.validate_python to skip invalid tenants and only add
successful subscription_plan entries to results; locate symbols get_plan_bulk,
subscription_adapter.validate_python, and results to implement the chosen fix.
| if bulk_plans: | ||
| plans_to_cache: dict[str, SubscriptionPlan] = {} | ||
|
|
||
| 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.
None values from get_plan_bulk will be cached, causing downstream issues.
If get_plan_bulk returns None for invalid tenants (as per lines 285-292), those None values will be serialized as "null" JSON and cached. On subsequent cache hits, json.loads("null") returns None, which will fail Pydantic validation at line 339, triggering unnecessary API fallbacks.
Consider filtering out None values before caching:
♻️ Proposed fix to filter None values before caching
if bulk_plans:
plans_to_cache: dict[str, SubscriptionPlan] = {}
for tenant_id, subscription_plan in bulk_plans.items():
- tenant_plans[tenant_id] = subscription_plan
- plans_to_cache[tenant_id] = subscription_plan
+ if subscription_plan is not None:
+ 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():
+ for tenant_id, subscription_plan in plans_to_cache.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)
pipe.execute()🤖 Prompt for AI Agents
In `@api/services/billing_service.py` around lines 362 - 377, The code currently
caches None results from get_plan_bulk, causing "null" to be stored and later
Pydantic validation failures; update the loop that builds tenant_plans and
plans_to_cache to skip entries where subscription_plan is None (i.e., only set
tenant_plans[tenant_id] and plans_to_cache[tenant_id] when subscription_plan is
not None), and ensure the subsequent pipeline loop over bulk_plans uses the
filtered plans_to_cache (or iterates plans_to_cache.items()) when calling
cls._make_plan_cache_key, json.dumps(subscription_plan) and pipe.setex(...,
cls._PLAN_CACHE_TTL, ...).
| def test_get_plan_bulk_with_invalid_tenant_plan_skipped(self, mock_send_request): | ||
| """Test bulk plan retrieval when one tenant has invalid plan data (should skip that tenant).""" | ||
| # Arrange | ||
| tenant_ids = ["tenant-valid-1", "tenant-invalid", "tenant-valid-2"] | ||
|
|
||
| # Response with one invalid tenant plan (missing expiration_date) and two valid ones | ||
| mock_send_request.return_value = { | ||
| "data": { | ||
| "tenant-valid-1": {"plan": "sandbox", "expiration_date": 1735689600}, | ||
| "tenant-invalid": {"plan": "professional"}, # Missing expiration_date field | ||
| "tenant-valid-2": {"plan": "team", "expiration_date": 1767225600}, | ||
| } | ||
| } | ||
|
|
||
| # Act | ||
| with patch("services.billing_service.logger") as mock_logger: | ||
| result = BillingService.get_plan_bulk(tenant_ids) | ||
|
|
||
| # Assert - should only contain valid tenants | ||
| assert len(result) == 2 | ||
| assert "tenant-valid-1" in result | ||
| assert "tenant-valid-2" in result | ||
| assert "tenant-invalid" not in result | ||
|
|
||
| # Verify valid tenants have correct data | ||
| assert result["tenant-valid-1"]["plan"] == "sandbox" | ||
| assert result["tenant-valid-1"]["expiration_date"] == 1735689600 | ||
| assert result["tenant-valid-2"]["plan"] == "team" | ||
| assert result["tenant-valid-2"]["expiration_date"] == 1767225600 | ||
|
|
||
| # Verify exception was logged for the invalid tenant | ||
| mock_logger.exception.assert_called_once() | ||
| log_call_args = mock_logger.exception.call_args[0] | ||
| assert "get_plan_bulk: failed to validate subscription plan for tenant" in log_call_args[0] | ||
| assert "tenant-invalid" in log_call_args[1] |
There was a problem hiding this comment.
Test assertion doesn't match implementation behavior.
The test asserts that "tenant-invalid" not in result (line 1319), but according to the get_plan_bulk implementation at lines 285-292 in billing_service.py, invalid tenants are assigned results[tenant_id] = None rather than being excluded from the results dictionary.
Either:
- The test assertion is incorrect and should check
assert result["tenant-invalid"] is None, or - The implementation should be updated to exclude invalid tenants instead of mapping them to
None.
🐛 Proposed fix if behavior should map invalid tenants to None
- # Assert - should only contain valid tenants
- assert len(result) == 2
- assert "tenant-valid-1" in result
- assert "tenant-valid-2" in result
- assert "tenant-invalid" not in result
+ # Assert - should contain all tenants, with invalid ones mapped to None
+ assert len(result) == 3
+ assert "tenant-valid-1" in result
+ assert "tenant-valid-2" in result
+ assert "tenant-invalid" in result
+ assert result["tenant-invalid"] is None🤖 Prompt for AI Agents
In `@api/tests/unit_tests/services/test_billing_service.py` around lines 1297 -
1331, The test currently expects invalid tenants to be omitted but
BillingService.get_plan_bulk sets results[tenant_id] = None for invalid tenants;
update the test assertions to reflect that behavior by asserting
result["tenant-invalid"] is None (instead of asserting the tenant is not in
result) and keep the logger.exception assertions (adjust call arg indexing if
needed) so the test verifies the invalid tenant was logged while still present
with a None value; reference BillingService.get_plan_bulk and the test function
test_get_plan_bulk_with_invalid_tenant_plan_skipped when making the change.
3 participants
Benchmark PR from qodo-benchmark#445
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.