fix: deletion of customer with active subscriptions #3368
Conversation
📝 WalkthroughWalkthroughExpose a public WithActiveSubscriptions query helper; enforce preloaded Subjects and Subscriptions in entity mapping and add ActiveSubscriptionIDs to Customer; prevent deleting customers with active subscriptions; and unify entitlement queries to use a single captured timestamp and preload active subscriptions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (7)
✨ Finishing touches
🧪 Generate unit tests
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: 3
🧹 Nitpick comments (4)
openmeter/customer/adapter/customer.go (1)
628-634: Tweak comment and avoid unnecessary plan eager-loading.
- Comment says “subscription”; use “active subscriptions”.
- Loading Plan for every call adds extra JOINs/rows; it’s only needed when filtering by plan key. Consider dropping WithPlan here to reduce query cost.
Apply:
-// withActiveSubscriptions returns a query with the subscription +// withActiveSubscriptions returns a query with active subscriptions preloaded func withActiveSubscriptions(query *entdb.CustomerQuery, at time.Time) *entdb.CustomerQuery { return query.WithSubscription(func(query *entdb.SubscriptionQuery) { applyActiveSubscriptionFilter(query, at) - query.WithPlan() + // Note: do not eager-load plans here; plan JOIN is applied only when filtering by plan key. }) }openmeter/customer/adapter/entitymapping.go (2)
23-29: De-duplicate subject keys to avoid accidental duplicates.While Ent should de-dup, defensive uniqueness here is cheap and prevents surprises from joins.
Apply:
subjectKeys := lo.FilterMap(subjects, func(item *db.CustomerSubjects, _ int) (string, bool) { if item == nil { return "", false } return item.SubjectKey, true }) + subjectKeys = lo.Uniq(subjectKeys)
40-47: De-duplicate subscription IDs as well.Prevents duplicate IDs if multiple joins surface the same row.
Apply:
subscriptionIDs := lo.FilterMap(subscriptions, func(item *db.Subscription, _ int) (string, bool) { if item == nil { return "", false } return item.ID, true }) + subscriptionIDs = lo.Uniq(subscriptionIDs)openmeter/customer/errors.go (1)
116-118: Attribute name clarity.Consider using "subscription_ids" to mirror "subject_keys" naming and be explicit.
Apply:
-func NewErrDeletingCustomerWithActiveSubscriptions(subscriptionIDs []string) error { - return ErrDeletingCustomerWithActiveSubscriptions.WithAttr("subscriptions", subscriptionIDs) -} +func NewErrDeletingCustomerWithActiveSubscriptions(subscriptionIDs []string) error { + return ErrDeletingCustomerWithActiveSubscriptions.WithAttr("subscription_ids", subscriptionIDs) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openmeter/customer/adapter/customer.go(4 hunks)openmeter/customer/adapter/entitymapping.go(3 hunks)openmeter/customer/customer.go(1 hunks)openmeter/customer/errors.go(1 hunks)openmeter/customer/service/customer.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-20T11:15:07.499Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2692
File: openmeter/productcatalog/plan/adapter/mapping.go:64-74
Timestamp: 2025-04-20T11:15:07.499Z
Learning: In the OpenMeter codebase, Ent's edge methods ending in "OrErr" (like AddonsOrErr()) only return NotLoadedError when the edge wasn't loaded, and cannot return DB errors. Simple err != nil checks are sufficient for these methods.
Applied to files:
openmeter/customer/adapter/entitymapping.go
🧬 Code graph analysis (4)
openmeter/customer/errors.go (2)
pkg/models/errors.go (1)
ErrorCode(294-294)pkg/models/validationissue.go (1)
NewValidationIssue(219-230)
openmeter/customer/service/customer.go (2)
pkg/models/errors.go (1)
NewGenericPreConditionFailedError(233-235)openmeter/customer/errors.go (1)
NewErrDeletingCustomerWithActiveSubscriptions(116-118)
openmeter/customer/adapter/entitymapping.go (3)
openmeter/ent/db/ent.go (1)
IsNotLoaded(340-346)openmeter/ent/db/predicate/predicate.go (2)
SubscriptionOrErr(267-275)Subscription(264-264)openmeter/ent/db/subscription.go (2)
Subscription(22-60)Subscription(140-157)
openmeter/customer/adapter/customer.go (1)
openmeter/ent/db/customer_query.go (1)
CustomerQuery(27-43)
⏰ 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). (8)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Code Generators
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
openmeter/customer/adapter/customer.go (3)
38-38: Ensure eager-loading for mapping.Good call to use withActiveSubscriptions; this satisfies CustomerFromDBEntity’s new edge requirement.
316-316: Eager-load subscriptions here as well.LGTM; prevents “subscriptions must be loaded” errors on GetCustomer.
392-392: Keep active-subscription preload for attribution lookup.LGTM; consistent with entity mapping expectations.
openmeter/customer/adapter/entitymapping.go (3)
14-22: Switch to OrErr for edge presence — good.This enforces preloaded edges; error handling with db.IsNotLoaded is appropriate per prior learnings.
31-38: Subscription edge presence check — good.Same pattern as subjects; keeps mapping strict and explicit.
78-79: Expose ActiveSubscriptionIDs — aligned with service precondition.LGTM; mapping surfaces the IDs needed by the delete guard.
chrisgacsal
force-pushed
the
fix/customer-delete
branch
from
e0dd7d1 to
deb9676
Compare
chrisgacsal
force-pushed
the
fix/customer-delete
branch
from
deb9676 to
8f9ca28
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
openmeter/customer/errors.go (1)
109-118: Error code addition looks good; align attribute name and add a guard test.
- Suggest renaming the attribute key to “subscription_ids” to match existing snake_case keys like “subject_keys”.
- Please add a unit test to lock the error code string and prevent regressions.
Apply within this file:
-func NewErrDeletingCustomerWithActiveSubscriptions(subscriptionIDs []string) error { - return ErrDeletingCustomerWithActiveSubscriptions.WithAttr("subscriptions", subscriptionIDs) -} +func NewErrDeletingCustomerWithActiveSubscriptions(subscriptionIDs []string) error { + return ErrDeletingCustomerWithActiveSubscriptions.WithAttr("subscription_ids", subscriptionIDs) +}Minimal test (same package):
package customer import "testing" func TestErrCodeDeletingCustomerWithActiveSubscriptions(t *testing.T) { const want = "deleting_customer_with_active_subscriptions" if string(ErrCodeDeletingCustomerWithActiveSubscriptions) != want { t.Fatalf("unexpected code: %q != %q", ErrCodeDeletingCustomerWithActiveSubscriptions, want) } }openmeter/entitlement/adapter/entitlement.go (3)
62-72: Use a single captured time in GetEntitlement to avoid skew.Capture clock.Now() once and reuse it for preloads and predicates to ensure a consistent snapshot.
func (a *entitlementDBAdapter) GetEntitlement(ctx context.Context, entitlementID models.NamespacedID) (*entitlement.Entitlement, error) { return entutils.TransactingRepo( ctx, a, func(ctx context.Context, repo *entitlementDBAdapter) (*entitlement.Entitlement, error) { + snap := clock.Now().UTC() res, err := withAllUsageResets(repo.db.Entitlement.Query(), []string{entitlementID.Namespace}). WithCustomer(func(q *db.CustomerQuery) { - customeradapter.WithSubjects(q, clock.Now()) - customeradapter.WithActiveSubscriptions(q, clock.Now()) + customeradapter.WithSubjects(q, snap) + customeradapter.WithActiveSubscriptions(q, snap) }). WithSubject(). Where( db_entitlement.ID(entitlementID.ID), db_entitlement.Namespace(entitlementID.Namespace), - db_entitlement.Or(db_entitlement.DeletedAtGT(clock.Now()), db_entitlement.DeletedAtIsNil()), + db_entitlement.Or(db_entitlement.DeletedAtGT(snap), db_entitlement.DeletedAtIsNil()), ). First(ctx)
321-326: Capture ‘now’ once per loop iteration in ListEntitlementsAffectedByIngestEvents.Multiple clock.Now() calls within the same iteration can drift; capture once and reuse for predicates and preloads.
for _, pair := range eventFilters { - entities, err := repo.db.Entitlement.Query(). + snap := clock.Now().UTC() + entities, err := repo.db.Entitlement.Query(). Where( db_entitlement.Namespace(pair.Namespace), db_entitlement.HasCustomerWith( customerdb.Namespace(pair.Namespace), - customerNotDeletedAt(clock.Now()), + customerNotDeletedAt(snap), customerdb.HasSubjectsWith( customersubjectsdb.SubjectKey(pair.EventSubject), customersubjectsdb.DeletedAtIsNil(), ), ), db_entitlement.HasFeatureWith(db_feature.MeterSlugIn(pair.MeterSlugs...)), ). WithFeature(). WithCustomer( func(q *db.CustomerQuery) { - customeradapter.WithSubjects(q, clock.Now()) - customeradapter.WithActiveSubscriptions(q, clock.Now()) + customeradapter.WithSubjects(q, snap) + customeradapter.WithActiveSubscriptions(q, snap) }, ).
227-229: Consider avoiding Plan edge preloading when only IDs are needed.entitymapping maps only subscription IDs; preloading Plan in WithActiveSubscriptions adds I/O and memory. If Plan isn’t used on these paths, consider a lighter helper (e.g., WithActiveSubscriptionIDs) or make Plan preload optional.
Also applies to: 360-362, 808-809, 954-955
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openmeter/customer/adapter/customer.go(4 hunks)openmeter/customer/adapter/entitymapping.go(3 hunks)openmeter/customer/customer.go(1 hunks)openmeter/customer/errors.go(1 hunks)openmeter/customer/service/customer.go(1 hunks)openmeter/entitlement/adapter/entitlement.go(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/customer/customer.go
- openmeter/customer/adapter/entitymapping.go
- openmeter/customer/adapter/customer.go
- openmeter/customer/service/customer.go
🧰 Additional context used
🧬 Code graph analysis (2)
openmeter/entitlement/adapter/entitlement.go (4)
openmeter/customer/adapter/customer.go (2)
WithActiveSubscriptions(629-634)WithSubjects(602-626)pkg/clock/clock.go (1)
Now(14-21)openmeter/ent/db/customer_query.go (1)
CustomerQuery(27-43)openmeter/customer/adapter/entitymapping.go (1)
CustomerFromDBEntity(13-98)
openmeter/customer/errors.go (2)
pkg/models/errors.go (1)
ErrorCode(294-294)pkg/models/validationissue.go (1)
NewValidationIssue(219-230)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
openmeter/entitlement/adapter/entitlement.go (4)
133-144: Consistent time snapshot and edge preloading — nice.Capturing a single UTC time and reusing it across customer resolution and the post-create reload ensures deterministic results. Looks good.
Also applies to: 224-229
398-406: LGTM: unified ‘now’ for listing path.Time is captured once and reused across filters and preloads. Good.
716-730: LGTM: snapshot time reused across edge preloads in upsert path.Consistent and deterministic.
803-816: LGTM: consistent ‘now’ usage in scanners and scheduled queries.Filters and preloads share the same timestamp.
Also applies to: 952-971
chrisgacsal
force-pushed
the
fix/customer-delete
branch
from
8f9ca28 to
aef545d
Compare
3 participants
Overview
Summary by CodeRabbit
New Features
Improvements
Bug Fixes