chore: reduce customer subscription fetching #3540
Conversation
📝 WalkthroughWalkthroughThis PR introduces an expansion-driven architecture for loading customer subscriptions. Instead of eagerly fetching subscriptions in all cases, subscription loading is now gated by an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/customer/httpdriver/customer.go (1)
86-104: Subscriptions are fetched even when not requested; gate by expand.This defeats the PR’s goal. Fetch only if subscriptions are expanded.
Apply:
- // Get the customer's subscriptions - var customerSubscriptions map[string][]subscription.Subscription - - if len(resp.Items) > 0 { + // Get the customer's subscriptions only if requested + var customerSubscriptions map[string][]subscription.Subscription + if len(resp.Items) > 0 && lo.Contains([]customer.Expand(request.Expands), customer.ExpandSubscriptions) { customerIDs := lo.Map(resp.Items, func(item customer.Customer, _ int) string { return item.ID }) - subscriptions, err := h.subscriptionService.List(ctx, subscription.ListSubscriptionsInput{ - Namespaces: []string{request.Namespace}, - CustomerIDs: customerIDs, - ActiveAt: lo.ToPtr(time.Now()), - }) + subscriptions, err := h.subscriptionService.List(ctx, subscription.ListSubscriptionsInput{ + Namespaces: []string{request.Namespace}, + CustomerIDs: customerIDs, + ActiveAt: lo.ToPtr(clock.Now()), + }) if err != nil { return ListCustomersResponse{}, err } customerSubscriptions = lo.GroupBy(subscriptions.Items, func(item subscription.Subscription) string { return item.CustomerId }) }Note: if you adopt clock.Now, remove unused time import.
🧹 Nitpick comments (8)
openmeter/customer/httpdriver/apimapping.go (1)
150-161: Prefer ActiveSubscriptionIDs for currentSubscriptionId; avoid relying on slice orderWhen subscriptions aren’t expanded, the slice is often empty; when expanded, we already carry ActiveSubscriptionIDs on customer.Customer. Prefer IDs from c.ActiveSubscriptionIDs (if present) and fall back to the slice only if needed.
Apply this focused diff:
- // Map the subscriptions to the API Subscriptions - if len(subscriptions) > 0 { - // Let's find the active one - // FIXME: this will only work with single subscription per customer - apiCustomer.CurrentSubscriptionId = lo.ToPtr(subscriptions[0].ID) - - // Map the subscriptions to the API Subscriptions if the expand is set - if lo.Contains(expand, customer.ExpandSubscriptions) { - apiCustomer.Subscriptions = lo.ToPtr(lo.Map(subscriptions, func(s subscription.Subscription, _ int) api.Subscription { - return subscriptionhttp.MapSubscriptionToAPI(s) - })) - } - } + // Current subscription: prefer expanded IDs to avoid relying on slice order + if lo.Contains(expand, customer.ExpandSubscriptions) { + ids := c.ActiveSubscriptionIDs.OrEmpty() + if len(ids) > 0 { + apiCustomer.CurrentSubscriptionId = lo.ToPtr(ids[0]) + } + } + // Fallback to first subscription if provided explicitly + if apiCustomer.CurrentSubscriptionId == nil && len(subscriptions) > 0 { + apiCustomer.CurrentSubscriptionId = lo.ToPtr(subscriptions[0].ID) + } + // Map full subscriptions if expanded + if lo.Contains(expand, customer.ExpandSubscriptions) && len(subscriptions) > 0 { + apiCustomer.Subscriptions = lo.ToPtr(lo.Map(subscriptions, func(s subscription.Subscription, _ int) api.Subscription { + return subscriptionhttp.MapSubscriptionToAPI(s) + })) + }openmeter/customer/service/customer.go (1)
80-83: Return a typed validation error for non-expanded subscriptionsThis is a client/usage error; use the validation wrapper for consistent 4xx mapping.
- if cus.ActiveSubscriptionIDs.IsAbsent() { - return fmt.Errorf("customer subscriptions are not expanded") - } + if cus.ActiveSubscriptionIDs.IsAbsent() { + return models.NewGenericValidationError(fmt.Errorf("customer subscriptions are not expanded")) + }openmeter/customer/adapter/customer.go (1)
40-42: Good gating: only load subscriptions when expandedConditionally calling WithActiveSubscriptions reduces load as intended. Nice.
If you want to reduce duplication, consider a local
withSubs := slices.Contains(input.Expands, customer.ExpandSubscriptions)in each method to reuse.Also applies to: 394-396, 472-474
openmeter/customer/adapter/entitymapping.go (1)
80-99: Add context to “subscriptions must be loaded” errorIncluding namespace/id eases debugging when edges aren’t preloaded.
-import "errors" +import ( + "errors" + "fmt" +) ... - if db.IsNotLoaded(err) { - return nil, errors.New("subscriptions must be loaded for customer") - } + if db.IsNotLoaded(err) { + return nil, fmt.Errorf("subscriptions must be loaded for customer [namespace=%s customer.id=%s]", e.Namespace, e.ID) + }Optionally mirror the same pattern in subjectKeysFromDBEntity for consistency.
openmeter/customer/customer.go (2)
27-35: Avoid double-wrapping validation errors from Expands.Validate.Expands.Validate already returns a models.GenericValidationError, but its callers wrap it again. Return a plain error here and let callers wrap once.
Apply:
- if expand != ExpandSubscriptions { - return models.NewGenericValidationError(fmt.Errorf("invalid expand: %s", expand)) - } + if expand != ExpandSubscriptions { + return fmt.Errorf("invalid expand: %q", expand) + }Callers that do
models.NewGenericValidationError(i.Expands.Validate())can stay as-is. Based on learnings.
18-21: Make Expands interop-friendly (lo.Contains) or provide a Has helper.Named slice type Expands may not match []T for generic helpers like lo.Contains. Prefer either:
- Alias:
type Expands = []Expand, or- Add
func (e Expands) Has(x Expand) booland useif !e.Has(ExpandSubscriptions) { ... }.This avoids scattered conversions and type frictions across packages.
openmeter/customer/httpdriver/customer.go (2)
91-95: Use clock.Now for time sourcing.Prefer clock.Now over time.Now for testability and consistency.
Apply:
- ActiveAt: lo.ToPtr(time.Now()), + ActiveAt: lo.ToPtr(clock.Now()),and
- ActiveAt: lo.ToPtr(time.Now()), + ActiveAt: lo.ToPtr(clock.Now()),Remove the unused time import if no longer referenced.
Also applies to: 479-482
115-117: Minor: clarify error message.“failed to cast customer customer” → “failed to map customer”.
Apply:
- return item, fmt.Errorf("failed to cast customer customer: %w", err) + return item, fmt.Errorf("failed to map customer: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
openmeter/customer/adapter/customer.go(7 hunks)openmeter/customer/adapter/entitymapping.go(3 hunks)openmeter/customer/customer.go(8 hunks)openmeter/customer/httpdriver/apimapping.go(2 hunks)openmeter/customer/httpdriver/customer.go(7 hunks)openmeter/customer/service/customer.go(2 hunks)openmeter/entitlement/adapter/entitlement.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
openmeter/customer/httpdriver/apimapping.go (1)
openmeter/customer/customer.go (3)
Customer(40-52)Expands(20-20)ExpandSubscriptions(24-24)
openmeter/customer/adapter/customer.go (2)
openmeter/customer/customer.go (2)
Expands(20-20)ExpandSubscriptions(24-24)openmeter/customer/adapter/entitymapping.go (1)
CustomerFromDBEntity(15-78)
openmeter/entitlement/adapter/entitlement.go (2)
openmeter/customer/adapter/entitymapping.go (1)
CustomerFromDBEntity(15-78)openmeter/customer/customer.go (2)
Expands(20-20)Customer(40-52)
openmeter/customer/customer.go (2)
pkg/models/validator.go (1)
Validate(16-26)pkg/models/errors.go (1)
NewGenericValidationError(138-140)
openmeter/customer/service/customer.go (3)
openmeter/customer/customer.go (2)
Expands(20-20)ExpandSubscriptions(24-24)pkg/models/errors.go (1)
NewGenericPreConditionFailedError(233-235)openmeter/customer/errors.go (1)
NewErrDeletingCustomerWithActiveSubscriptions(116-118)
openmeter/customer/httpdriver/customer.go (3)
openmeter/customer/customer.go (5)
ListCustomersInput(239-259)Expands(20-20)Expand(19-19)Customer(40-52)ExpandSubscriptions(24-24)openmeter/subscription/list.go (2)
OrderBy(14-14)ListSubscriptionsInput(29-39)openmeter/customer/httpdriver/apimapping.go (1)
CustomerToAPI(113-165)
openmeter/customer/adapter/entitymapping.go (3)
openmeter/customer/customer.go (3)
Customer(40-52)Expands(20-20)ExpandSubscriptions(24-24)openmeter/ent/db/customer.go (2)
Customer(20-64)Customer(142-157)openmeter/ent/db/ent.go (1)
IsNotLoaded(341-347)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
openmeter/customer/httpdriver/apimapping.go (1)
113-113: Signature update aligns with expands modelAccepting customer.Expands here is consistent with the new gating approach. No issues.
openmeter/entitlement/adapter/entitlement.go (1)
151-154: LGTM: expands-aware mapping without loading subscriptionsPassing customer.Expands{} is correct here; entitlement paths don’t need subscription IDs. Consistent with the new gating.
Also applies to: 550-552
openmeter/customer/adapter/customer.go (1)
116-117: Propagating expands to CustomerFromDBEntityThis ensures ActiveSubscriptionIDs is set only when requested. Looks good.
Also applies to: 446-447, 491-492
openmeter/customer/adapter/entitymapping.go (1)
15-16: Expands-aware population of ActiveSubscriptionIDsGating via slices.Contains(expands, customer.ExpandSubscriptions) is correct; Option[[]string] differentiates absent vs empty. Good.
Also applies to: 52-59
openmeter/customer/service/customer.go (1)
62-66: ****The code at lines 62–66 is correct. The parameter
inputhas typecustomer.DeleteCustomerInput, which is a type alias forcustomer.CustomerID(itself an alias formodels.NamespacedID). Therefore,&inputalready produces the correct type*customer.CustomerIDexpected byGetCustomerInput.CustomerID.The suggested fix would be redundant, as it unnecessarily reconstructs the same value. Other usages in the codebase that access
&input.CustomerIDdo so because theirinputis a struct with aCustomerIDfield—a different context from theDeleteCustomermethod's function signature.Likely an incorrect or invalid review comment.
openmeter/customer/httpdriver/customer.go (1)
320-323: LGTM: consistent mapping of query expand to internal type.The code correctly:
- Maps
api.CustomerExpandtocustomer.Expandconsistently across both GetCustomer (line 320) and ListCustomer (line 66) endpoints- Defaults to subscriptions expand via
lo.FromPtrOr, preserving backward compatibility- Includes clear
TODO[v2]comment (line 65) noting this is a breaking change to disable by default laterThe
lo.Contains(expand, customer.ExpandSubscriptions)usage pattern in the codebase confirms the expand parameter is properly handled as[]customer.Expandthroughout the adapter and HTTP driver layers.openmeter/customer/customer.go (1)
51-51: Review comment is incorrect and should be disregarded.The concern assumes
customer.Customercould be JSON-encoded directly, but the HTTP handler atopenmeter/customer/httpdriver/customer.go:106explicitly maps it toapi.CustomerviaCustomerToAPI()before returning any response. All HTTP responses use the generatedapi.Customertype with proper JSON tags, not the internalcustomer.Customertype. Themo.Optionfield without a JSON tag poses no risk becausecustomer.Customeris never directly marshaled.Likely an incorrect or invalid review comment.
2 participants
Overview
This patch makes sure that we are only fetching subscriptions as needed. This should reduce DB load by ~20% in realistic scenarios.
Notes for reviewer
Entitlements never return the actual subscription IDs, so we are fine to not fetch that.
The API is not changed as on the httpdriver we require explicit expand.