feat: get plan bulk with cache #36
Conversation
WalkthroughRedis-backed caching system added to BillingService for bulk plan fetching with fallback to API calls on cache misses. Per-item error handling enhanced to validate and skip invalid plans. New integration and unit tests cover caching scenarios, TTL behavior, cache failures, and error handling in plan validation. Changes
Sequence DiagramsequenceDiagram
participant Client
participant BillingService
participant Redis
participant API as Subscription API
Client->>BillingService: get_plan_bulk_with_cache(tenant_ids)
BillingService->>Redis: mget(cache_keys)
Redis-->>BillingService: cached_plans (partial/empty)
alt All Cached
BillingService-->>Client: return cached plans
else Cache Miss or Partial
BillingService->>API: get_plan_bulk(missing_ids)
API-->>BillingService: api_plans (with validation)
BillingService->>Redis: pipeline(setex for each plan)
Redis-->>BillingService: ack
BillingService-->>Client: return merged(cached + api_plans)
else Redis Failure
BillingService->>API: get_plan_bulk(all_ids)
API-->>BillingService: api_plans
BillingService-->>Client: return api_plans
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 1
🤖 Fix all issues with AI agents
In `@api/services/billing_service.py`:
- Around line 285-295: The code currently inserts results[tenant_id] = None when
subscription_adapter.validate_python raises; instead, in get_plan_bulk only add
tenant entries to results when validation succeeds (and subscription_plan is not
None). Locate the subscription_adapter.validate_python call inside get_plan_bulk
and remove the fallback assignment results[tenant_id] = None in the inner
except; simply log the exception (logger.exception(...)) and do not modify
results for that tenant so invalid tenants are omitted. Ensure the outer except
still continues on batch fetch failure.
🧹 Nitpick comments (1)
api/services/billing_service.py (1)
28-30: Remove the__str__method fromSubscriptionPlanTypedDict; it will never execute.
TypedDictis a static typing construct only. At runtime,TypeAdapter(SubscriptionPlan).validate_python(...)returns a plaindict, not aSubscriptionPlaninstance, so the custom__str__method is dead code. If human-readable logging is needed, use a PydanticBaseModel, dataclass, or a standalone formatter function instead.
📜 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/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/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/services/billing_service.py (3)
api/controllers/console/billing/billing.py (2)
get(37-41)get(50-53)api/libs/helper.py (1)
RateLimiter(376-402)api/extensions/ext_redis.py (1)
setex(73-73)
🔇 Additional comments (20)
api/services/billing_service.py (4)
1-1: Import supports cache serialization.Nice to see JSON is explicitly available for cache payloads.
39-42: Cache key prefix and TTL are explicit.Clear constants make cache behavior easy to tune.
299-301: Cache key helper looks good.
303-387: Cache-miss fallback and pipeline write-back are solid.api/tests/unit_tests/services/test_billing_service.py (1)
1297-1331: Good coverage for invalid tenant plan handling.api/tests/test_containers_integration_tests/services/test_billing_service.py (15)
1-8: Imports match the test needs.
10-21: Class-level scenario coverage is clearly documented.
22-34: Redis cleanup fixture is clear.
35-37: Helper keeps plan data setup concise.
39-43: Cache set helper is straightforward.
45-53: Cache get helper is straightforward.
55-85: Cache-hit scenario is well covered.
86-125: Cache-miss + write-back path is well covered.
126-156: Partial cache-hit behavior is covered.
157-187: Redis mget failure fallback is covered.
188-239: Invalid JSON cache fallback is covered.
240-272: Schema mismatch fallback is covered.
273-301: Pipeline failure resiliency is covered.
302-319: Empty input case is covered.
320-365: TTL expiry behavior is covered.
✏️ 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 | ||
| except Exception: | ||
| logger.exception("Failed to fetch billing info batch for tenants: %s", chunk) | ||
| logger.exception("get_plan_bulk: failed to fetch billing info batch for tenants: %s", chunk) | ||
| continue |
There was a problem hiding this comment.
Skip invalid tenants instead of inserting None.
Inserting None violates the declared return type and conflicts with the new unit test that expects invalid tenants to be omitted.
💡 Suggested fix
for tenant_id, plan in data.items():
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
+ # Skip invalid tenant entry
+ continue🤖 Prompt for AI Agents
In `@api/services/billing_service.py` around lines 285 - 295, The code currently
inserts results[tenant_id] = None when subscription_adapter.validate_python
raises; instead, in get_plan_bulk only add tenant entries to results when
validation succeeds (and subscription_plan is not None). Locate the
subscription_adapter.validate_python call inside get_plan_bulk and remove the
fallback assignment results[tenant_id] = None in the inner except; simply log
the exception (logger.exception(...)) and do not modify results for that tenant
so invalid tenants are omitted. Ensure the outer except still continues on batch
fetch failure.
Benchmark PR from agentic-review-benchmarks#10
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.