feat(taxcode): prevent deletion of subscription-referenced tax codes#4758
feat(taxcode): prevent deletion of subscription-referenced tax codes#4758borbelyr-kong wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds tax-code filtering to subscription listing and introduces a registered pre-delete hook that blocks tax code deletion when active or scheduled subscription rate cards reference the tax code. ChangesTax Code Subscription Protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TaxCodeService
participant TaxCodeSubscriptionHook
participant SubscriptionService
participant SubscriptionRepository
TaxCodeService->>TaxCodeSubscriptionHook: PreDelete tax code
TaxCodeSubscriptionHook->>SubscriptionService: List by tax code
SubscriptionService->>SubscriptionRepository: Query matching phase items
SubscriptionRepository-->>SubscriptionService: Matching subscriptions
TaxCodeSubscriptionHook->>SubscriptionService: Fetch subscription views
SubscriptionService-->>TaxCodeSubscriptionHook: Rate-card references
TaxCodeSubscriptionHook-->>TaxCodeService: Reference error or nil
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| affected, err := e.subscriptionService.List(ctx, subscription.ListSubscriptionsInput{ | ||
| Namespaces: []string{tc.Namespace}, | ||
| Status: []subscription.SubscriptionStatus{ | ||
| subscription.SubscriptionStatusActive, | ||
| subscription.SubscriptionStatusCanceled, | ||
| subscription.SubscriptionStatusInactive, | ||
| subscription.SubscriptionStatusScheduled, | ||
| }, | ||
| TaxCodes: &filter.FilterString{ | ||
| In: &[]string{ | ||
| tc.ID, | ||
| }, | ||
| }, | ||
| Page: pagination.Page{ | ||
| PageSize: 5, | ||
| PageNumber: 1, | ||
| }, | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
The guard only scans subscriptions before the tax-code delete. A concurrent subscription creation or rate-card update can add this tax-code ID after List completes but before deletion, so both operations can commit and leave a subscription referencing a deleted tax code unless the database transaction locks or otherwise serializes those writes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/taxcode/service/hooks/subscriptionhook.go
Line: 51-69
Comment:
**Reference Check Races With Writes**
The guard only scans subscriptions before the tax-code delete. A concurrent subscription creation or rate-card update can add this tax-code ID after `List` completes but before deletion, so both operations can commit and leave a subscription referencing a deleted tax code unless the database transaction locks or otherwise serializes those writes.
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/taxcode/service/hooks/subscriptionhook.go (2)
89-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the reference-scanning loop into a named helper.
The phase → item-by-key → item nested loop that checks each rate card's tax code reference is meaningful domain logic; pulling it into a small helper (e.g.
collectReferencingRateCardErrors(view, taxCodeID)) would makePreDeleteeasier to read at a glance and let the reference-detection logic be unit-tested without going through the DB-backed integration suite.As per path instructions: "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/taxcode/service/hooks/subscriptionhook.go` around lines 89 - 101, The nested phase → item-by-key → item scanning logic in PreDelete should be extracted into a named helper such as collectReferencingRateCardErrors. Move the tax-code matching and taxcode.NewTaxCodeReferencedByRateCardError construction into that helper, have it accept the view and tax code ID, and update PreDelete to append the helper’s results.
24-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
Validate()with the repo's validation-error pattern.Every other
Validate() errorin this PR (e.g.ListSubscriptionsInput.Validate()) collects failures intoerrsand returnsmodels.NewNillableGenericValidationError(errors.Join(errs...)). This one returns a barefmt.Errorfinstead, which is inconsistent and skips the field-context wrapping convention.♻️ Proposed fix
func (e SubscriptionHookConfig) Validate() error { - if e.SubscriptionService == nil { - return fmt.Errorf("subscription service is required") - } - - return nil + var errs []error + + if e.SubscriptionService == nil { + errs = append(errs, errors.New("subscription service is required")) + } + + return models.NewNillableGenericValidationError(errors.Join(errs...)) }As per coding guidelines: "For Validate() error methods, collect all validation failures and return models.NewNillableGenericValidationError(errors.Join(errs...)), preserving field context with wrapped errors."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/taxcode/service/hooks/subscriptionhook.go` around lines 24 - 30, Update SubscriptionHookConfig.Validate to collect validation failures in an errs slice, wrap the missing SubscriptionService error with its field context, and return models.NewNillableGenericValidationError(errors.Join(errs...)) instead of a bare fmt.Errorf; preserve nil for valid configurations.Source: Coding guidelines
openmeter/taxcode/service/hooks/subscriptionhook_test.go (1)
54-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNice, thorough coverage of the reference-detection edge cases. One small nit: the DB-deps setup, hook registration, and org-defaults provisioning block is duplicated verbatim across all three top-level test functions. Pulling it into a shared
setupSubscriptionHookTest(t)helper would trim the repetition.Also applies to: 139-163, 203-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/taxcode/service/hooks/subscriptionhook_test.go` around lines 54 - 75, Extract the repeated DB dependency setup, subscription hook registration, and organization-default tax-code provisioning from the three top-level tests into a shared setupSubscriptionHookTest(t) helper. Have the helper return the initialized dependencies and namespace needed by each test, while preserving existing cleanup and clock-reset behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/taxcode/service/hooks/subscriptionhook_test.go`:
- Around line 54-75: Extract the repeated DB dependency setup, subscription hook
registration, and organization-default tax-code provisioning from the three
top-level tests into a shared setupSubscriptionHookTest(t) helper. Have the
helper return the initialized dependencies and namespace needed by each test,
while preserving existing cleanup and clock-reset behavior.
In `@openmeter/taxcode/service/hooks/subscriptionhook.go`:
- Around line 89-101: The nested phase → item-by-key → item scanning logic in
PreDelete should be extracted into a named helper such as
collectReferencingRateCardErrors. Move the tax-code matching and
taxcode.NewTaxCodeReferencedByRateCardError construction into that helper, have
it accept the view and tax code ID, and update PreDelete to append the helper’s
results.
- Around line 24-30: Update SubscriptionHookConfig.Validate to collect
validation failures in an errs slice, wrap the missing SubscriptionService error
with its field context, and return
models.NewNillableGenericValidationError(errors.Join(errs...)) instead of a bare
fmt.Errorf; preserve nil for valid configurations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec63fcde-78eb-4a15-90aa-16f2c7511aa7
📒 Files selected for processing (7)
app/common/taxcode.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/subscription/list.goopenmeter/subscription/repo/subscriptionrepo.goopenmeter/taxcode/service/hooks/subscriptionhook.goopenmeter/taxcode/service/hooks/subscriptionhook_test.go
1 participant
What
Deleting a tax code that is still referenced by a subscription's rate cards is now blocked with
a typed
TaxCodeReferencedByRateCarderror, mirroring the existing plan and add-on deleteguards.
Why
A tax code referenced by a live, scheduled, canceled, or inactive subscription could previously
be deleted, leaving those subscriptions pointing at a non-existent tax code.
How
SubscriptionHook(openmeter/taxcode/service/hooks/subscriptionhook.go) registeredon the tax code service as a
PreDeletehook. It lists subscriptions filtered by tax-codereference (all statuses), expands each to a view, and returns a typed referenced-by error per
matching rate card.
ListSubscriptionsInput.TaxCodesfilter (list.go+subscriptionrepo.go) filterssubscriptions by the tax code IDs on their rate-card items. The repo filter mirrors the
subscription view's soft-delete gate (
deleted_at IS NULL OR deleted_at > now), so itemssoft-deleted in the past don't produce spurious matches.
app/common/taxcode.go,cmd/server/wire*.go) constructs the hook after both thesubscription and tax code services exist, avoiding a construction cycle.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR prevents deleting tax codes referenced by subscription rate cards. The main changes are:
Confidence Score: 4/5
Concurrent subscription writes can bypass the new deletion guard.
openmeter/taxcode/service/hooks/subscriptionhook.go
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller participant TaxCode as TaxCodeService participant Hook as SubscriptionHook participant Subs as SubscriptionService Caller->>TaxCode: DeleteTaxCode TaxCode->>Hook: PreDelete Hook->>Subs: List by namespace, status, and tax code Subs-->>Hook: Matching subscriptions loop Each match Hook->>Subs: GetView Subs-->>Hook: Phases and rate cards end alt Reference found Hook-->>TaxCode: TaxCodeReferencedByRateCard TaxCode-->>Caller: Deletion blocked else No reference found Hook-->>TaxCode: nil TaxCode->>TaxCode: Delete tax code TaxCode-->>Caller: Success end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Caller participant TaxCode as TaxCodeService participant Hook as SubscriptionHook participant Subs as SubscriptionService Caller->>TaxCode: DeleteTaxCode TaxCode->>Hook: PreDelete Hook->>Subs: List by namespace, status, and tax code Subs-->>Hook: Matching subscriptions loop Each match Hook->>Subs: GetView Subs-->>Hook: Phases and rate cards end alt Reference found Hook-->>TaxCode: TaxCodeReferencedByRateCard TaxCode-->>Caller: Deletion blocked else No reference found Hook-->>TaxCode: nil TaxCode->>TaxCode: Delete tax code TaxCode-->>Caller: Success endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(taxcode): prevent deletion of subsc..." | Re-trigger Greptile
Context used: