Skip to content

feat: get plan bulk with cache#95

Open
tomerqodo wants to merge 4 commits intogreptile_combined_20260121_qodo_grep_cursor_copilot_1_base_feat_get_plan_bulk_with_cache_pr445from
greptile_combined_20260121_qodo_grep_cursor_copilot_1_head_feat_get_plan_bulk_with_cache_pr445
Open

feat: get plan bulk with cache#95
tomerqodo wants to merge 4 commits intogreptile_combined_20260121_qodo_grep_cursor_copilot_1_base_feat_get_plan_bulk_with_cache_pr445from
greptile_combined_20260121_qodo_grep_cursor_copilot_1_head_feat_get_plan_bulk_with_cache_pr445

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from qodo-benchmark#445

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Jan 21, 2026

Greptile Summary

This PR adds a new get_plan_bulk_with_cache method to BillingService that reduces billing API load by caching tenant subscription plans in Redis with a 10-minute TTL. The implementation uses mget for batch cache reads and pipelines for batch writes.

  • Added Redis caching layer for bulk plan retrieval with cache-aside pattern
  • Added error handling for individual tenant plan validation failures in get_plan_bulk
  • Comprehensive integration and unit tests for cache hit/miss scenarios

Issues requiring attention:

  • Dead code: __str__ method in SubscriptionPlan TypedDict will never be called (TypedDict instances are plain dicts at runtime)
  • Type safety violation: get_plan_bulk assigns None to results but return type is dict[str, SubscriptionPlan]
  • Test-code mismatch: Unit test assertions for invalid tenant handling contradict actual code behavior

Confidence Score: 2/5

  • This PR has type safety violations and test-code mismatches that should be resolved before merging.
  • Score reflects the TypedDict method bug (dead code), type annotation mismatch that violates the return type contract, and unit test assertions that will fail against the actual code behavior.
  • api/services/billing_service.py needs fixes for the TypedDict __str__ method and the type annotation mismatch. api/tests/unit_tests/services/test_billing_service.py needs assertions updated to match code behavior.

Important Files Changed

Filename Overview
api/services/billing_service.py Adds new get_plan_bulk_with_cache method with Redis caching. Contains critical bugs: dead __str__ method in TypedDict and type annotation mismatch where None values violate return type.
api/tests/test_containers_integration_tests/services/test_billing_service.py New integration tests for get_plan_bulk_with_cache covering cache hit/miss, Redis failures, and TTL expiration scenarios.
api/tests/unit_tests/services/test_billing_service.py New unit test for invalid plan handling has assertion that contradicts actual code behavior - test expects key not in result, but code adds key with None value.

Sequence Diagram

sequenceDiagram
    participant Client
    participant BillingService
    participant Redis
    participant BillingAPI

    Client->>BillingService: get_plan_bulk_with_cache(tenant_ids)
    BillingService->>Redis: mget(cache_keys)
    
    alt All Cache Hits
        Redis-->>BillingService: cached plans
        BillingService-->>Client: tenant_plans
    else Partial/Full Cache Miss
        Redis-->>BillingService: partial/empty results
        BillingService->>BillingAPI: get_plan_bulk(missing_ids)
        BillingAPI-->>BillingService: subscription plans
        BillingService->>Redis: pipeline.setex(plans, TTL=600)
        BillingService-->>Client: merged tenant_plans
    else Redis Failure
        Redis-->>BillingService: Exception
        BillingService->>BillingAPI: get_plan_bulk(all_tenant_ids)
        BillingAPI-->>BillingService: subscription plans
        BillingService->>Redis: pipeline.setex(plans, TTL=600)
        BillingService-->>Client: tenant_plans
    end
Loading

Copy link
Copy Markdown

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +28 to +30
def __str__(self) -> str:
"""Return a human-readable string representation for debugging."""
return f"Plan: {self['plan']}, Expiration: {self['expiration_date']}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

logic: TypedDict is just a dict at runtime - methods defined in the class body are ignored. This __str__ method will never be called on SubscriptionPlan instances.

Suggested change
def __str__(self) -> str:
"""Return a human-readable string representation for debugging."""
return f"Plan: {self['plan']}, Expiration: {self['expiration_date']}"
Prompt To Fix With AI
This is a comment left during a code review.
Path: api/services/billing_service.py
Line: 28:30

Comment:
**logic:** `TypedDict` is just a dict at runtime - methods defined in the class body are ignored. This `__str__` method will never be called on `SubscriptionPlan` instances.

```suggestion
```

How can I resolve this? If you propose a fix, please make it concise.

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.

logic: Assigning None violates the return type dict[str, SubscriptionPlan]. Either update the return type to dict[str, SubscriptionPlan | None] or skip adding the key entirely by using continue instead.

Suggested change
results[tenant_id] = None
continue
Prompt To Fix With AI
This is a comment left during a code review.
Path: api/services/billing_service.py
Line: 292:292

Comment:
**logic:** Assigning `None` violates the return type `dict[str, SubscriptionPlan]`. Either update the return type to `dict[str, SubscriptionPlan | None]` or skip adding the key entirely by using `continue` instead.

```suggestion
                        continue
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +1316 to +1319
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.

logic: These assertions contradict the actual code behavior. The code at line 292 sets results[tenant_id] = None for invalid tenants, so len(result) will be 3 (not 2) and "tenant-invalid" will be in the result (with None value). Fix the production code or update these assertions.

Prompt To Fix With AI
This is a comment left during a code review.
Path: api/tests/unit_tests/services/test_billing_service.py
Line: 1316:1319

Comment:
**logic:** These assertions contradict the actual code behavior. The code at line 292 sets `results[tenant_id] = None` for invalid tenants, so `len(result)` will be 3 (not 2) and `"tenant-invalid"` will be in the result (with `None` value). Fix the production code or update these assertions.

How can I resolve this? If you propose a fix, please make it concise.

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