Skip to content

feat: get plan bulk with cache#10

Open
tomerqodo wants to merge 4 commits intosentry_full_base_feat_get_plan_bulk_with_cache_pr10from
sentry_full_head_feat_get_plan_bulk_with_cache_pr10
Open

feat: get plan bulk with cache#10
tomerqodo wants to merge 4 commits intosentry_full_base_feat_get_plan_bulk_with_cache_pr10from
sentry_full_head_feat_get_plan_bulk_with_cache_pr10

Conversation

@tomerqodo
Copy link
Copy Markdown

Benchmark PR from agentic-review-benchmarks#10

Comment on lines +365 to +367
for tenant_id, subscription_plan in bulk_plans.items():
tenant_plans[tenant_id] = subscription_plan
plans_to_cache[tenant_id] = subscription_plan
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The function get_plan_bulk_with_cache caches None values for invalid plans, causing subsequent cache reads to fail validation and trigger unnecessary API calls.
Severity: MEDIUM

Suggested Fix

Filter out entries with None values in get_plan_bulk_with_cache before they are added to the cache. Alternatively, modify get_plan_bulk to not include tenants with invalid plans in its returned dictionary.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: api/services/billing_service.py#L365-L367

Potential issue: When a tenant's subscription plan fails validation, `get_plan_bulk`
returns `None` for that tenant. The `get_plan_bulk_with_cache` function then caches this
`None` value as the JSON string `"null"`. On subsequent requests, this cached `"null"`
is read, parsed back to `None`, and fails validation again. This results in a loop of
cache misses and repeated API calls for tenants with invalid data, defeating the purpose
of the cache and increasing API load.

Did we get this right? 👍 / 👎 to inform future reviews.

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