feat(taxcode): prevent deletion of add-on-referenced tax codes#4561
Open
borbelyr-kong wants to merge 4 commits into
Open
feat(taxcode): prevent deletion of add-on-referenced tax codes#4561borbelyr-kong wants to merge 4 commits into
borbelyr-kong wants to merge 4 commits into
Conversation
borbelyr-kong
added
release-note/misc
Contributor
📝 WalkthroughWalkthroughThis PR threads a TaxCodeResolver through addon service construction, adds addon publish/list validation against tax codes, and introduces tax-code deletion hooks that block removal when referenced by add-ons or plans. Server wiring and test environments are updated to construct and pass the new dependencies. ChangesTax code resolver + add-on protections
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
Author
|
@greptileai review this draft |
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
borbelyr-kong
force-pushed
the
feat/taxcode-plan-publish-validation
branch
from
8a31078 to
7a906df
Compare
borbelyr-kong
force-pushed
the
feat/taxcode-addon-delete-guard
branch
from
e850ada to
5946f52
Compare
borbelyr-kong
force-pushed
the
feat/taxcode-plan-publish-validation
branch
2 times, most recently
from
2f9c5ab to
3a58a03
Compare
borbelyr-kong
force-pushed
the
feat/taxcode-plan-publish-validation
branch
from
f5bc13d to
d50dfbb
Compare
borbelyr-kong
force-pushed
the
feat/taxcode-addon-delete-guard
branch
from
16973cc to
b1f0aff
Compare
Base automatically changed from
feat/taxcode-plan-publish-validation
to
fix/tax-code-delete
July 3, 2026 11:59
chrisgacsal
requested changes
Jul 3, 2026
| return errors.As(err, &vi) && vi.Code() == ErrCodeTaxCodeReferencedByPlan | ||
| } | ||
|
|
||
| const ErrCodeTaxCodeReferencedByAddon models.ErrorCode = "tax_code_referenced_by_addon" |
Collaborator
There was a problem hiding this comment.
Please merge ErrTaxCodeReferencedByPlan and ErrTaxCodeReferencedByAddon in to ErrCodeTaxCodeReferencedByRateCard to use the same error in plans and addons.
borbelyr-kong
force-pushed
the
feat/taxcode-addon-delete-guard
branch
from
b1f0aff to
8e5e07a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/productcatalog/addon/adapter/addon.go (1)
53-60: 🧹 Nitpick | 🔵 TrivialFilter logic looks solid — restricting to non-deleted ratecards before matching on
tax_code_idis exactly the right behavior for the deletion-protection use case.One thing worth a quick check: since this is now used by the tax-code delete pre-hook (via
PreDeleteinaddonhook.go), make sureaddon_rate_card.tax_code_idis indexed so thisHasRatecardsWithsubquery doesn't do a full table scan as the ratecard table grows.🤖 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/productcatalog/addon/adapter/addon.go` around lines 53 - 60, The new tax-code filter in addon adapter is correct, but the `HasRatecardsWith` path used by `PreDelete` in `addonhook.go` may become slow without an index on `addon_rate_card.tax_code_id`. Add or verify a database index for the `tax_code_id` field on the addon rate card model/schema so the subquery in the addon adapter can use it efficiently as the table grows.
🤖 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.
Inline comments:
In `@openmeter/taxcode/service/hooks/addonhook.go`:
- Around line 26-32: Update AddonHookConfig.Validate to follow the shared
validation pattern instead of returning immediately on the first missing field.
Replace the fail-fast check on AddonService with collecting issues in a var errs
[]error, add any validation failures to that slice, and return
models.NewNillableGenericValidationError(errors.Join(errs...)) from Validate.
Keep the method name AddonHookConfig.Validate as the entry point so future
fields can be added without changing the validation pattern.
---
Nitpick comments:
In `@openmeter/productcatalog/addon/adapter/addon.go`:
- Around line 53-60: The new tax-code filter in addon adapter is correct, but
the `HasRatecardsWith` path used by `PreDelete` in `addonhook.go` may become
slow without an index on `addon_rate_card.tax_code_id`. Add or verify a database
index for the `tax_code_id` field on the addon rate card model/schema so the
subquery in the addon adapter can use it efficiently as the table grows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3eb05cb6-6eb5-44f4-80c8-3c54bf0be033
📒 Files selected for processing (20)
app/common/productcatalog.goapp/common/taxcode.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/productcatalog/addon.goopenmeter/productcatalog/addon/adapter/adapter_test.goopenmeter/productcatalog/addon/adapter/addon.goopenmeter/productcatalog/addon/service.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/testutils/service.goopenmeter/taxcode/errors.goopenmeter/taxcode/service/hooks/addonhook.goopenmeter/taxcode/service/hooks/addonhook_test.gotest/billing/subscription_suite.gotest/customer/testenv.go
Comment on lines
+26
to
+32
| func (e AddonHookConfig) Validate() error { | ||
| if e.AddonService == nil { | ||
| return fmt.Errorf("addon service is required") | ||
| } | ||
|
|
||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Validate() fails fast instead of collecting errors.
As per coding guidelines, "For Go Validate() error methods, collect all validation issues into var errs []error and return models.NewNillableGenericValidationError(errors.Join(errs...)) instead of failing fast." This config only has one field today, but that will silently drift once more fields get added.
♻️ Proposed fix
+import "errors"
+
func (e AddonHookConfig) Validate() error {
- if e.AddonService == nil {
- return fmt.Errorf("addon service is required")
- }
-
- return nil
+ var errs []error
+
+ if e.AddonService == nil {
+ errs = append(errs, fmt.Errorf("addon service is required"))
+ }
+
+ return models.NewNillableGenericValidationError(errors.Join(errs...))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (e AddonHookConfig) Validate() error { | |
| if e.AddonService == nil { | |
| return fmt.Errorf("addon service is required") | |
| } | |
| return nil | |
| } | |
| func (e AddonHookConfig) Validate() error { | |
| var errs []error | |
| if e.AddonService == nil { | |
| errs = append(errs, fmt.Errorf("addon service is required")) | |
| } | |
| return models.NewNillableGenericValidationError(errors.Join(errs...)) | |
| } |
🤖 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/addonhook.go` around lines 26 - 32, Update
AddonHookConfig.Validate to follow the shared validation pattern instead of
returning immediately on the first missing field. Replace the fail-fast check on
AddonService with collecting issues in a var errs []error, add any validation
failures to that slice, and return
models.NewNillableGenericValidationError(errors.Join(errs...)) from Validate.
Keep the method name AddonHookConfig.Validate as the entry point so future
fields can be added without changing the validation pattern.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/taxcode/service/hooks/addonhook.go (1)
74-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRate-card error-collection loop is duplicated with
planhook.go.This block (walk rate cards, check
TaxCodeReference(), appendtaxcode.NewTaxCodeReferencedByRateCardError, guard against zero-match anomaly) is essentially the same logic asplanhook.go'sPreDelete(Lines 77-92 there), just iterating one level shallower (noPhases). Extracting a small shared helper that takes[]productcatalog.RateCard+ tax code ID and returns[]errorwould remove this duplication across both hooks.♻️ Example shared helper
func collectRateCardTaxCodeErrors(taxCodeID string, rateCards []productcatalog.RateCard) []error { var errs []error for _, rateCard := range rateCards { ref := rateCard.AsMeta().TaxCodeReference() if ref == nil || *ref != taxCodeID { continue } errs = append(errs, taxcode.NewTaxCodeReferencedByRateCardError(taxCodeID, rateCard.Key())) } return errs }🤖 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/addonhook.go` around lines 74 - 91, The rate-card tax code reference check in addonhook.go duplicates the same PreDelete logic from planhook.go, so extract the shared loop into a small helper that collects errors from a slice of rate cards and re-use it in both hooks. Keep the helper focused on TaxCodeReference(), appending taxcode.NewTaxCodeReferencedByRateCardError, and preserving the zero-match anomaly handling currently done around affectedAddons.Items and errs.
🤖 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/addonhook.go`:
- Around line 74-91: The rate-card tax code reference check in addonhook.go
duplicates the same PreDelete logic from planhook.go, so extract the shared loop
into a small helper that collects errors from a slice of rate cards and re-use
it in both hooks. Keep the helper focused on TaxCodeReference(), appending
taxcode.NewTaxCodeReferencedByRateCardError, and preserving the zero-match
anomaly handling currently done around affectedAddons.Items and errs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33e234d3-78c4-40f5-8b4f-5f0d21f67860
📒 Files selected for processing (5)
openmeter/taxcode/errors.goopenmeter/taxcode/service/hooks/addonhook.goopenmeter/taxcode/service/hooks/addonhook_test.goopenmeter/taxcode/service/hooks/planhook.goopenmeter/taxcode/service/hooks/planhook_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@coderabbitai
Summary by CodeRabbit