Skip to content

feat: get plan bulk with cache#45

Open
tomerqodo wants to merge 4 commits intoaugment_combined_20260121_augment_sentry_coderabbit_1_base_feat_get_plan_bulk_with_cache_pr445from
augment_combined_20260121_augment_sentry_coderabbit_1_head_feat_get_plan_bulk_with_cache_pr445
Open

feat: get plan bulk with cache#45
tomerqodo wants to merge 4 commits intoaugment_combined_20260121_augment_sentry_coderabbit_1_base_feat_get_plan_bulk_with_cache_pr445from
augment_combined_20260121_augment_sentry_coderabbit_1_head_feat_get_plan_bulk_with_cache_pr445

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from qodo-benchmark#445

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Jan 22, 2026

🤖 Augment PR Summary

Summary: Adds a cached bulk subscription-plan fetch path to reduce Billing API load in batch scenarios.

Changes:

  • Adds Redis-backed caching for tenant subscription plans with a 10-minute TTL (`get_plan_bulk_with_cache`).
  • Introduces a Redis key convention (`tenant_plan:{tenant_id}`) and helper to build cache keys.
  • Adds per-tenant validation error handling when parsing Billing API bulk responses.
  • Implements cache read via `mget` and cache writes via Redis pipeline + `setex`.
  • Adds new testcontainers-based integration tests covering cache hit/miss, Redis failures, invalid cache data, and TTL expiry.
  • Adds a unit test for handling invalid tenant plan payloads from the Billing API.

Technical Notes: Cached values are JSON-serialized plan dicts and re-validated via Pydantic TypeAdapter on read; Redis failures fall back to the Billing API.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

logger.exception(
"get_plan_bulk: failed to validate subscription plan for tenant(%s)", tenant_id
)
results[tenant_id] = None
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_plan_bulk() is annotated/documented to return SubscriptionPlan values, but on validation failure it stores None in results which can break callers that assume a dict with plan/expiration_date (and it also diverges from the new unit test expectation of “skip invalid tenant”). Consider making the invalid-tenant behavior consistent (either skip, or change typing/consumers to explicitly handle None).

Other Locations
  • api/services/billing_service.py:366
  • api/services/billing_service.py:376

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

assert len(result) == 2
assert "tenant-valid-1" in result
assert "tenant-valid-2" in result
assert "tenant-invalid" not in result
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test expects the invalid tenant to be omitted ("tenant-invalid" not in result), but the current get_plan_bulk() implementation adds the key with a None value on validation failure, so this will fail. Consider aligning the test with the intended behavior (skip vs None).

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants