feat: get plan bulk with cache #10
Conversation
📝 WalkthroughWalkthroughThis change introduces Redis-backed caching for subscription plan retrieval in the billing service. A new method Changes
Sequence DiagramsequenceDiagram
actor Caller
participant BillingService
participant Redis
participant BillingAPI
Caller->>BillingService: get_plan_bulk_with_cache(tenant_ids)
BillingService->>Redis: mget(cache_keys)
alt Cache Hit
Redis-->>BillingService: [plan_data...]
BillingService-->>Caller: SubscriptionPlans
else Cache Miss / Redis Failure
BillingService->>BillingAPI: get_plan_bulk(missing_tenant_ids)
BillingAPI-->>BillingService: [plan_data...]
BillingService->>Redis: pipeline.set(cache_keys, json_payloads, ttl)
Redis-->>BillingService: OK
BillingService-->>Caller: SubscriptionPlans
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/services/billing_service.py (2)
22-31:⚠️ Potential issue | 🟠 Major
__str__method on TypedDict has no effect at runtime.
TypedDictis used purely for type annotations. Instances are plain Pythondictobjects, so custom methods like__str__are never called—dict.__str__takes precedence:sp: SubscriptionPlan = {"plan": "sandbox", "expiration_date": 123} print(str(sp)) # Prints: {'plan': 'sandbox', 'expiration_date': 123} # NOT: "Plan: sandbox, Expiration: 123"If you need a string representation for debugging, consider a standalone helper function or convert to a proper
dataclass/Pydanticmodel.Option: Use a helper function instead
class SubscriptionPlan(TypedDict): """Tenant subscriptionplan information.""" plan: str expiration_date: int - def __str__(self) -> str: - """Return a human-readable string representation for debugging.""" - return f"Plan: {self['plan']}, Expiration: {self['expiration_date']}" + +def format_subscription_plan(plan: SubscriptionPlan) -> str: + """Return a human-readable string representation for debugging.""" + return f"Plan: {plan['plan']}, Expiration: {plan['expiration_date']}"
266-297:⚠️ Potential issue | 🟠 MajorReturn type annotation is inconsistent with actual return values.
The return type
dict[str, SubscriptionPlan]promises all values areSubscriptionPlan, but line 292 storesNonefor invalid tenants. Callers trusting the type annotation may encounterAttributeErrororTypeErrorwhen accessing properties onNone.Option 1: Update type annotation to reflect reality
`@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: Skip invalid tenants instead of storing None
except Exception: logger.exception( "get_plan_bulk: failed to validate subscription plan for tenant(%s)", tenant_id ) - results[tenant_id] = None + # Skip invalid tenants - don't add to results + continue
🤖 Fix all issues with AI agents
In `@api/services/billing_service.py`:
- Around line 362-378: bulk_plans may contain None values which are being
serialized to "null" and cached; filter out None before preparing plans_to_cache
and before adding entries to the Redis pipeline so you never call
json.dumps(None) or cache "null". Update the loop that builds plans_to_cache and
the subsequent pipeline loop to skip tenant_id, subscription_plan pairs where
subscription_plan is None (use bulk_plans.items() and continue on None), leaving
tenant_plans population/handling as-is, and keep using cls._make_plan_cache_key
and cls._PLAN_CACHE_TTL when setting non-None entries.
In
`@api/tests/test_containers_integration_tests/services/test_billing_service.py`:
- Around line 22-33: The fixture setup_redis_cleanup currently only deletes
Redis keys after yield (teardown); update it to also perform the same cleanup
before yield so Redis is cleaned both before and after each test. Inside the
with flask_app_with_containers.app_context(): block, run the delete logic using
the pattern f"{BillingService._PLAN_CACHE_KEY_PREFIX}*" and
redis_client.keys(...) / redis_client.delete(...) once before yield and again
after yield (teardown) to ensure both pre-test and post-test cleanup.
In `@api/tests/unit_tests/services/test_billing_service.py`:
- Around line 1315-1319: The test expects invalid tenants to be omitted but the
implementation in billing_service.py currently sets results[tenant_id] = None
for invalid tenants; change the implementation so the code that assigns
results[tenant_id] = None is removed and instead only adds entries to results
when the tenant is valid (i.e., wrap the population of results with a validity
check or continue/skip on invalid tenant_id), ensuring the final results dict
contains only valid tenants and the unit test assertions (expecting two tenants
and absence of "tenant-invalid") will pass.
🧹 Nitpick comments (2)
api/services/billing_service.py (1)
349-353: Consider usingdebuglevel for per-call cache statistics.
logger.infofor cache hit/miss stats will generate a log entry for every call, which could be noisy in batch job scenarios. Reserveinfofor significant events and usedebugfor operational metrics.Use debug level for cache stats
- logger.info( + logger.debug( "get_plan_bulk_with_cache: cache hits=%s, cache misses=%s", len(tenant_plans), len(cache_misses), )api/tests/test_containers_integration_tests/services/test_billing_service.py (1)
328-332: Movetimeimport to the top of the file.The
timemodule import is inside the test method. While functional, imports should be at the module level for consistency and to make dependencies explicit.Move import to top of file
import json +import time from unittest.mock import patchThen remove the local import:
# Wait for TTL to expire (key will be deleted by Redis) - import time - time.sleep(2)
| 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) | ||
| pipe.execute() |
There was a problem hiding this comment.
Caching None values causes repeated API calls.
If get_plan_bulk returns None for some tenants (validation failure), the current code caches "null" in Redis. On subsequent reads, json.loads("null") → None, which fails Pydantic validation, causing another cache miss and API call. This creates a loop of wasted API calls and cache writes.
Filter out 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🤖 Prompt for AI Agents
In `@api/services/billing_service.py` around lines 362 - 378, bulk_plans may
contain None values which are being serialized to "null" and cached; filter out
None before preparing plans_to_cache and before adding entries to the Redis
pipeline so you never call json.dumps(None) or cache "null". Update the loop
that builds plans_to_cache and the subsequent pipeline loop to skip tenant_id,
subscription_plan pairs where subscription_plan is None (use bulk_plans.items()
and continue on None), leaving tenant_plans population/handling as-is, and keep
using cls._make_plan_cache_key and cls._PLAN_CACHE_TTL when setting non-None
entries.
| @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.
Fixture doesn't clean up before tests as the docstring claims.
The comment says "Clean up Redis cache before and after each test" but cleanup only happens after yield (teardown phase). This could cause test flakiness if a previous run crashed before cleanup or if tests are run in isolation.
Add cleanup before test as well
`@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
+ pattern = f"{BillingService._PLAN_CACHE_KEY_PREFIX}*"
+ keys = redis_client.keys(pattern)
+ if keys:
+ redis_client.delete(*keys)
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)📝 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.
| @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) | |
| `@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 | |
| pattern = f"{BillingService._PLAN_CACHE_KEY_PREFIX}*" | |
| keys = redis_client.keys(pattern) | |
| if keys: | |
| redis_client.delete(*keys) | |
| yield | |
| # Clean up after test | |
| pattern = f"{BillingService._PLAN_CACHE_KEY_PREFIX}*" | |
| keys = redis_client.keys(pattern) | |
| if keys: | |
| redis_client.delete(*keys) |
🤖 Prompt for AI Agents
In `@api/tests/test_containers_integration_tests/services/test_billing_service.py`
around lines 22 - 33, The fixture setup_redis_cleanup currently only deletes
Redis keys after yield (teardown); update it to also perform the same cleanup
before yield so Redis is cleaned both before and after each test. Inside the
with flask_app_with_containers.app_context(): block, run the delete logic using
the pattern f"{BillingService._PLAN_CACHE_KEY_PREFIX}*" and
redis_client.keys(...) / redis_client.delete(...) once before yield and again
after yield (teardown) to ensure both pre-test and post-test cleanup.
| # 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 |
There was a problem hiding this comment.
Test assertions don't match implementation behavior.
The implementation at billing_service.py:292 stores results[tenant_id] = None for invalid tenants, but this test expects the invalid tenant to be excluded from the result. This test will fail because:
len(result)will be 3, not 2 (the invalid tenant is included withNonevalue)"tenant-invalid"will be inresult(as a key withNonevalue)
Either fix the test to match current implementation, or fix the implementation to filter out invalid tenants.
Option 1: Fix test to match implementation
- # 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 (invalid one has None value)
+ 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 NoneOption 2: Fix implementation to filter out invalid tenants
In billing_service.py:
except Exception:
logger.exception(
"get_plan_bulk: failed to validate subscription plan for tenant(%s)", tenant_id
)
- results[tenant_id] = None
+ # Skip invalid tenants - 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.
| # 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 (invalid one has None value) | |
| 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 1315 -
1319, The test expects invalid tenants to be omitted but the implementation in
billing_service.py currently sets results[tenant_id] = None for invalid tenants;
change the implementation so the code that assigns results[tenant_id] = None is
removed and instead only adds entries to results when the tenant is valid (i.e.,
wrap the population of results with a validity check or continue/skip on invalid
tenant_id), ensuring the final results dict contains only valid tenants and the
unit test assertions (expecting two tenants and absence of "tenant-invalid")
will pass.
3 participants
Benchmark PR from agentic-review-benchmarks#10
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.