feat: get plan bulk with cache

[tomerqodo]

Benchmark PR from agentic-review-benchmarks#10

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (3) 📎 Requirement gaps (0)

1. get_plan_bulk returns None values 📘 Rule violation ⛯ Reliability

Description

• get_plan_bulk() claims to return dict[str, SubscriptionPlan], but on validation failure it
  stores None as a value, creating an inconsistent contract for callers.
• This can cause downstream runtime errors (e.g., when indexing plan fields) and also propagates
  into get_plan_bulk_with_cache() where None may be cached as JSON null.
• The edge case of invalid plan payloads should be handled explicitly by either skipping the tenant,
  returning a typed union (SubscriptionPlan | None), or raising a domain-specific error with
  context.

Code

api/services/billing_service.py[R285-292]

+                    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

Evidence

PR Compliance ID 3 requires explicit handling of edge cases and meaningful error management. The new
code assigns None into a dict typed/documented as SubscriptionPlan, which is an unhandled edge
case for consumers and can lead to follow-on failures and caching of invalid data.

Rule 3: Generic: Robust Error Handling and Edge Case Management
api/services/billing_service.py[267-297]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BillingService.get_plan_bulk()` is annotated/documented as returning `dict[str, SubscriptionPlan]` but stores `None` for invalid tenant plans. This breaks the function contract and can propagate invalid values into `get_plan_bulk_with_cache()` and Redis.

## Issue Context
The code currently catches validation exceptions, logs them, and assigns `None`. Callers (and caching code) may assume the returned value is always a valid `SubscriptionPlan` mapping.

## Fix Focus Areas
- api/services/billing_service.py[267-387]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. get_plan_bulk_with_cache raises Exception 📘 Rule violation ⛯ Reliability

Description

• get_plan_bulk_with_cache() raises a generic Exception when Redis mget returns an unexpected
  length, instead of using a domain-specific service error.
• This violates the requirement that services raise exceptions from services/errors or
  core/errors, improving clarity and enabling consistent controller-to-HTTP mapping.
• Replace the generic exception with a billing/cache-specific service error (e.g.,
  BillingCacheReadError(BaseServiceError)) including relevant context.

Code

api/services/billing_service.py[R325-328]

+            if len(cached_values) != len(tenant_ids):
+                raise Exception(
+                    "get_plan_bulk_with_cache: unexpected error: redis mget failed: cached values length mismatch"
+                )

Evidence

PR Compliance ID 21 requires domain-specific exceptions in services. The new code explicitly raises
a generic Exception inside a service method, which is not a domain-specific error type and
complicates consistent error translation/handling.

AGENTS.md
api/services/billing_service.py[325-328]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BillingService.get_plan_bulk_with_cache()` raises a generic `Exception` for a Redis `mget` length mismatch.

## Issue Context
Service-layer errors should be domain-specific (from `services/errors` or `core/errors`) to enable consistent handling and clearer operational debugging.

## Fix Focus Areas
- api/services/billing_service.py[320-357]
- api/services/errors/base.py[1-2]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Integration tests missing type hints 📘 Rule violation ✓ Correctness

Description

• New test helper and test functions are missing parameter and return type annotations (e.g.,
  setup_redis_cleanup, _create_test_plan_data, _set_cache, _get_cache, and multiple test_*
  methods).
• This violates the requirement that all Python function definitions include type annotations for
  parameters and return values (as enforced by basedpyright).
• Add explicit return types (often -> None) and annotate parameters (use Any where fixture types
  are not easily expressible).

Code

api/tests/test_containers_integration_tests/services/test_billing_service.py[R22-53]

+    @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)
+
+    def _create_test_plan_data(self, plan: str = "sandbox", expiration_date: int = 1735689600):
+        """Helper to create test SubscriptionPlan data."""
+        return {"plan": plan, "expiration_date": expiration_date}
+
+    def _set_cache(self, tenant_id: str, plan_data: dict, ttl: int = 600):
+        """Helper to set cache data in Redis."""
+        cache_key = BillingService._make_plan_cache_key(tenant_id)
+        json_str = json.dumps(plan_data)
+        redis_client.setex(cache_key, ttl, json_str)
+
+    def _get_cache(self, tenant_id: str):
+        """Helper to get cache data from Redis."""
+        cache_key = BillingService._make_plan_cache_key(tenant_id)
+        value = redis_client.get(cache_key)
+        if value:
+            if isinstance(value, bytes):
+                return value.decode("utf-8")
+            return value
+        return None

Evidence

PR Compliance ID 7 requires type annotations for all Python function parameters and return values.
The new integration test file introduces multiple function definitions without return type
annotations and with untyped parameters (beyond a few basic str/int types).

AGENTS.md
api/tests/test_containers_integration_tests/services/test_billing_service.py[22-55]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The newly added integration test module defines multiple functions without full type annotations.

## Issue Context
The repository enforces type annotations for all Python function parameters and return values via basedpyright.

## Fix Focus Areas
- api/tests/test_containers_integration_tests/services/test_billing_service.py[22-365]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-code-review]

1. Get_plan_bulk returns none values 📘 Rule violation ⛯ Reliability

• get_plan_bulk() claims to return dict[str, SubscriptionPlan], but on validation failure it
  stores None as a value, creating an inconsistent contract for callers.
• This can cause downstream runtime errors (e.g., when indexing plan fields) and also propagates
  into get_plan_bulk_with_cache() where None may be cached as JSON null.
• The edge case of invalid plan payloads should be handled explicitly by either skipping the tenant,
  returning a typed union (SubscriptionPlan | None), or raising a domain-specific error with
  context.

Agent prompt

## Issue description
`BillingService.get_plan_bulk()` is annotated/documented as returning `dict[str, SubscriptionPlan]` but stores `None` for invalid tenant plans. This breaks the function contract and can propagate invalid values into `get_plan_bulk_with_cache()` and Redis.

## Issue Context
The code currently catches validation exceptions, logs them, and assigns `None`. Callers (and caching code) may assume the returned value is always a valid `SubscriptionPlan` mapping.

## Fix Focus Areas
- api/services/billing_service.py[267-387]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

2. Get_plan_bulk_with_cache raises exception 📘 Rule violation ⛯ Reliability

• get_plan_bulk_with_cache() raises a generic Exception when Redis mget returns an unexpected
  length, instead of using a domain-specific service error.
• This violates the requirement that services raise exceptions from services/errors or
  core/errors, improving clarity and enabling consistent controller-to-HTTP mapping.
• Replace the generic exception with a billing/cache-specific service error (e.g.,
  BillingCacheReadError(BaseServiceError)) including relevant context.

Agent prompt

## Issue description
`BillingService.get_plan_bulk_with_cache()` raises a generic `Exception` for a Redis `mget` length mismatch.

## Issue Context
Service-layer errors should be domain-specific (from `services/errors` or `core/errors`) to enable consistent handling and clearer operational debugging.

## Fix Focus Areas
- api/services/billing_service.py[320-357]
- api/services/errors/base.py[1-2]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

3. Integration tests missing type hints 📘 Rule violation ✓ Correctness

• New test helper and test functions are missing parameter and return type annotations (e.g.,
  setup_redis_cleanup, _create_test_plan_data, _set_cache, _get_cache, and multiple test_*
  methods).
• This violates the requirement that all Python function definitions include type annotations for
  parameters and return values (as enforced by basedpyright).
• Add explicit return types (often -> None) and annotate parameters (use Any where fixture types
  are not easily expressible).

Agent prompt

## Issue description
The newly added integration test module defines multiple functions without full type annotations.

## Issue Context
The repository enforces type annotations for all Python function parameters and return values via basedpyright.

## Fix Focus Areas
- api/tests/test_containers_integration_tests/services/test_billing_service.py[22-365]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools