Skip to content

feat: get plan bulk with cache#36

Open
tomerqodo wants to merge 4 commits intosentry_combined_20260121_augment_sentry_coderabbit_1_base_feat_get_plan_bulk_with_cache_pr445from
sentry_combined_20260121_augment_sentry_coderabbit_1_head_feat_get_plan_bulk_with_cache_pr445
Open

feat: get plan bulk with cache#36
tomerqodo wants to merge 4 commits intosentry_combined_20260121_augment_sentry_coderabbit_1_base_feat_get_plan_bulk_with_cache_pr445from
sentry_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

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.

Bug: The get_plan_bulk method returns None for invalid tenants, which is then cached as a JSON "null". This causes validation errors on subsequent cache reads.
Severity: HIGH

Suggested Fix

Modify the get_plan_bulk method to not add entries for tenants with invalid plans to the results dictionary. Instead of results[tenant_id] = None, the code should simply continue or skip adding the key to the dictionary. This ensures the return value adheres to the dict[str, SubscriptionPlan] type hint and prevents None values from being cached.

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#L292

Potential issue: In the `get_plan_bulk` method, when subscription plan data validation
fails for a given tenant, the code assigns `None` to the results for that tenant ID.
This `None` value is then passed to `get_plan_bulk_with_cache`, which attempts to cache
it. The caching logic serializes `None` into the JSON string `"null"` and stores it in
Redis. On subsequent cache reads for that tenant, the `"null"` string is deserialized
back to a `None` object, which then causes a validation failure when
`subscription_adapter.validate_python` is called, as `None` is not a valid
`SubscriptionPlan` object. This leads to repeated cache misses and unnecessary API calls
for invalid tenants.

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