feat: get plan bulk with cache

[tomerqodo]

Benchmark PR from qodo-benchmark#445

[Copilot]

Pull request overview

This PR adds a caching layer for bulk billing plan retrieval to reduce load on the billing API during batch operations. It introduces get_plan_bulk_with_cache() that uses Redis to cache subscription plans with a 10-minute TTL, falling back to the API for cache misses.

Changes:

• Added Redis-backed caching functionality for bulk plan retrieval with graceful fallback behavior
• Enhanced error handling in get_plan_bulk to skip invalid tenant plans instead of failing entirely
• Added comprehensive integration tests using testcontainers to verify cache behavior, TTL handling, and error recovery

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
api/services/billing_service.py	Implements get_plan_bulk_with_cache() method with Redis caching, adds cache key generation helper, improves error handling in get_plan_bulk, and adds __str__ method to SubscriptionPlan
api/tests/unit_tests/services/test_billing_service.py	Adds unit test verifying that invalid tenant plans are skipped during bulk retrieval
api/tests/test_containers_integration_tests/services/test_billing_service.py	Adds comprehensive integration tests covering cache hit/miss scenarios, Redis failures, invalid data handling, and TTL expiration

[Copilot]

The iteration should use plans_to_cache.items() instead of bulk_plans.items() to ensure consistency with the dictionary being checked for caching. Although both dictionaries contain the same data in this context, using the same variable that was checked improves code clarity and maintainability.

Suggested change

	for tenant_id, subscription_plan in bulk_plans.items():
	for tenant_id, subscription_plan in plans_to_cache.items():

[Copilot]

The test expects invalid tenants to be excluded from the result (asserting 'tenant-invalid' not in result), but the implementation at line 292 sets results[tenant_id] = None for invalid plans. This means the test will fail because 'tenant-invalid' will be present in the result dictionary with a None value. The implementation should either skip adding invalid entries entirely (remove line 292) or the test expectations need to be updated.

[Copilot]

Using time.sleep(2) in tests makes them slow and fragile. Consider using a shorter TTL or mocking the time/Redis behavior instead. This 2-second sleep will make the test suite noticeably slower, especially if run frequently in CI/CD pipelines.

Suggested change

	# Wait for TTL to expire (key will be deleted by Redis)
	import time
	time.sleep(2)
	# Verify cache is expired (key doesn't exist)
	cache_key_1 = BillingService._make_plan_cache_key("tenant-1")
	# Simulate TTL expiration by explicitly deleting the key from Redis
	cache_key_1 = BillingService._make_plan_cache_key("tenant-1")
	redis_client.delete(cache_key_1)
	# Verify cache is expired (key doesn't exist)

[Copilot]

The caching logic assumes all values in bulk_plans are valid SubscriptionPlan dictionaries, but get_plan_bulk can return None values for invalid tenants (line 292). When iterating over bulk_plans.items(), if a subscription_plan is None, the json.dumps(subscription_plan) call will serialize it as "null", and subsequent cache reads will fail validation. Consider filtering out None values from bulk_plans before caching, or ensuring get_plan_bulk never returns None values.

[Copilot]

The documentation comment contains a grammatical error: "if you want to high data consistency" should be "if you want high data consistency" or "if you want to ensure high data consistency".

Suggested change

	NOTE: if you want to high data consistency, use get_plan_bulk instead.
	NOTE: if you want high data consistency, use get_plan_bulk instead.

[Copilot]

Setting results[tenant_id] = None for invalid tenant plans can cause issues downstream. The return type annotation is dict[str, SubscriptionPlan], but this stores None values instead of excluding invalid entries. Consider removing the invalid tenant from results entirely or filtering out None values before returning, consistent with the new caching behavior where invalid entries are skipped.

Suggested change

	results[tenant_id] = None
	# Skip invalid subscription plans instead of storing None
	continue

[Copilot]

The __str__ method is defined in a TypedDict class, which is not the correct place for instance methods. TypedDict classes are purely type annotations and do not support custom methods at runtime. This method will not be callable and should be removed or moved to a separate utility function.

Suggested change

	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(subscription: SubscriptionPlan) -> str:
	"""Return a human-readable string representation for a SubscriptionPlan for debugging."""
	return f"Plan: {subscription['plan']}, Expiration: {subscription['expiration_date']}"