feat(productcatalog): eager load tax_code on plan rate cards and expo…#3945
Open
borbelyr-kong wants to merge 14 commits intomainfrom
Open
feat(productcatalog): eager load tax_code on plan rate cards and expo…#3945borbelyr-kong wants to merge 14 commits intomainfrom
borbelyr-kong wants to merge 14 commits intomainfrom
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
borbelyr-kong
added
the
release-note/ignore
borbelyr-kong
force-pushed
the
feat/tax-codes-v3-add-fk-keys
branch
from
360fb25 to
47540a1
Compare
turip
reviewed
Mar 20, 2026
| return ratecard, nil | ||
| } | ||
|
|
||
| func fromTaxCodeRow(r *entdb.TaxCode) (taxcode.TaxCode, error) { |
Member
There was a problem hiding this comment.
let's move this to taxcode adapter
Comment on lines
+409
to
+411
| if m, ok := lo.Find(tc.AppMappings, func(m taxcode.TaxCodeAppMapping) bool { | ||
| return m.AppType == app.AppTypeStripe | ||
| }); ok { |
Member
There was a problem hiding this comment.
let's add a helper for this on the TaxConfig object
Comment on lines
+238
to
+240
| // getOrCreateTaxCode looks up a TaxCode by its Stripe app mapping. If none exists, | ||
| // it creates one with a key derived from the Stripe code. | ||
| func (s service) getOrCreateTaxCode(ctx context.Context, namespace string, stripeCode string) (taxcode.TaxCode, error) { |
Member
There was a problem hiding this comment.
Let's move this to the tax code service.
…se in RateCardMeta
…rate card write path
…umn and TaxCode entity on plan rate card read
…Stripe tax codes on plan creation
borbelyr-kong
force-pushed
the
feat/tax-codes-double-write-plan
branch
from
ebf9126 to
b9a9453
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openmeter/productcatalog/ratecard.go (1)
142-181:⚠️ Potential issue | 🟡 Minor
Equal()doesn't compare the newTaxCodefield.The
Equal()method compares all other fields inRateCardMetabut omits the newTaxCodefield. If two rate cards should be considered different when their tax codes differ, this needs updating.If the intentional behavior is to ignore
TaxCodeduring equality checks (e.g., because it's only for display/eager loading), adding a comment would clarify that decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/ratecard.go` around lines 142 - 181, The Equal method on RateCardMeta omits the new TaxCode field; update RateCardMeta.Equal to compare TaxCode (use the same pointer-safe comparison style as FeatureKey/FeatureID, e.g., lo.FromPtr(r.TaxCode) vs lo.FromPtr(v.TaxCode)) and return false if they differ, or if the intended semantics are to ignore TaxCode add a clarifying comment above RateCardMeta.Equal explaining why it is excluded; reference the RateCardMeta.Equal method and the TaxCode field when making the change.openmeter/productcatalog/tax.go (1)
88-99:⚠️ Potential issue | 🟡 MinorKeep the cloning pattern consistent across all pointer fields.
Good catch on deep-copying
TaxCodeID! ThoughBehavioris also a pointer field, it's assigned directly instead of being deep-copied likeStripe. For consistency and maintainability, all pointer fields should follow the same cloning pattern.🛠️ Suggested fix
func (c TaxConfig) Clone() TaxConfig { - out := TaxConfig{ - Behavior: c.Behavior, - } + out := TaxConfig{} + + if c.Behavior != nil { + out.Behavior = lo.ToPtr(*c.Behavior) + } if c.Stripe != nil { out.Stripe = lo.ToPtr(c.Stripe.Clone()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/tax.go` around lines 88 - 99, The Clone method for TaxConfig assigns the pointer field Behavior directly rather than deep-copying it like Stripe and TaxCodeID; update TaxConfig.Clone to deep-copy Behavior as well (e.g. in the same pattern used for Stripe/TaxCodeID: check if c.Behavior != nil and set out.Behavior to a new pointer holding a cloned or dereferenced value using lo.ToPtr or the Behavior.Clone() method as appropriate) so all pointer fields are cloned consistently in TaxConfig.Clone.openmeter/productcatalog/plan/adapter/mapping.go (1)
339-341:⚠️ Potential issue | 🟡 MinorError variable may be nil in this error message.
The
errvariable referenced here comes from theParsePtrOrNil()call on line 300. If that succeeded but we hit this default case,errwill benil, leading to a confusing error message likeinvalid RateCard type foo: <nil>.🐛 Suggested fix
default: - return nil, fmt.Errorf("invalid RateCard type %s: %w", r.Type, err) + return nil, fmt.Errorf("invalid RateCard type %s", r.Type) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/adapter/mapping.go` around lines 339 - 341, The default error return in the switch uses err (from ParsePtrOrNil) which may be nil, producing messages like "invalid RateCard type foo: <nil>"; update the default case in mapping.go to avoid appending a nil error—e.g., check if err != nil and return fmt.Errorf("invalid RateCard type %s: %w", r.Type, err) otherwise return fmt.Errorf("invalid RateCard type %s", r.Type); ensure you reference the ParsePtrOrNil result and avoid shadowing the err variable.
🧹 Nitpick comments (5)
openmeter/taxcode/service.go (1)
125-171: Consider consolidating duplicate input types.
GetTaxCodeByAppMappingInputandGetOrCreateByAppMappingInputhave identical structures and validation logic. You could define a shared base type or use a single input type with a type alias if the semantic distinction matters:type AppMappingLookupInput struct { Namespace string AppType app.AppType TaxCode string } type GetTaxCodeByAppMappingInput = AppMappingLookupInput type GetOrCreateByAppMappingInput = AppMappingLookupInputThat said, keeping them separate is totally fine if you anticipate them diverging in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/service.go` around lines 125 - 171, Replace the duplicated structs and validation by introducing a single shared type (e.g., AppMappingLookupInput) containing Namespace, AppType and TaxCode, move the Validate() implementation to that type (Validate on AppMappingLookupInput) and then create type aliases GetTaxCodeByAppMappingInput = AppMappingLookupInput and GetOrCreateByAppMappingInput = AppMappingLookupInput so existing code referring to GetTaxCodeByAppMappingInput and GetOrCreateByAppMappingInput continues to work; ensure you remove the duplicate Validate methods for the old names and keep references to AppType.Validate() and models.NewNillableGenericValidationError(errors.Join(...)) unchanged.api/v3/handlers/billingprofiles/convert.go (1)
69-71: FIXME noted — consider tracking this.The goverter ignore for
TaxCodeIDmakes sense as a temporary measure. Just a friendly nudge to ensure this gets tracked (issue/ticket) so it doesn't get forgotten once the OpenAPI schema is updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/billingprofiles/convert.go` around lines 69 - 71, The goverter ignore on TaxCodeID in the ConvertBillingTaxConfigToTaxConfig declaration is a temporary workaround; create and reference a tracking ticket/issue for removing the "// goverter:ignore TaxCodeID" once the OpenAPI schema is updated, then update this comment to include the issue number (e.g., "TODO: remove goverter:ignore TaxCodeID — tracked in ISSUE-1234") and add a brief note in the function comment for ConvertBillingTaxConfigToTaxConfig so reviewers can find and remove the ignore when the schema change lands.openmeter/productcatalog/plan/service/service_test.go (1)
214-216: Consider adding a nil check for robustness.The extraction logic assumes
TaxConfigandTaxCodeIDare populated. While the test setup guarantees this, a quickrequire.NotNilwould make the test more self-documenting and catch regressions early if the tax code resolution ever fails silently.taxConfig := draftPlanV1.Phases[0].RateCards[0].AsMeta().TaxConfig require.NotNil(t, taxConfig, "TaxConfig should be populated after plan creation") require.NotNil(t, taxConfig.TaxCodeID, "TaxCodeID should be resolved during plan creation") taxCodeID := taxConfig.TaxCodeID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service/service_test.go` around lines 214 - 216, Add explicit nil assertions before extracting TaxCodeID from draftPlanV1 to make the test robust: retrieve taxConfig via draftPlanV1.Phases[0].RateCards[0].AsMeta().TaxConfig, call require.NotNil(t, taxConfig, "TaxConfig should be populated after plan creation") and require.NotNil(t, taxConfig.TaxCodeID, "TaxCodeID should be resolved during plan creation"), then assign taxCodeID := taxConfig.TaxCodeID; this ensures TaxConfig and TaxCodeID are validated before use in the test.openmeter/productcatalog/plan/service/taxcode_test.go (1)
23-38: Consider adding test coverage forUsageBasedRateCard.The current tests comprehensively cover
FlatFeeRateCardscenarios, but the production code inresolveTaxCodeshas different handling forUsageBasedRateCard(BillingCadence is required and non-pointer). Adding a test case with a usage-based rate card + tax config would validate that code path.Something like:
t.Run("UsageBasedWithStripeCode", func(t *testing.T) { ubp := &productcatalog.UsageBasedRateCard{ RateCardMeta: productcatalog.RateCardMeta{ Key: features[0].Key, Name: features[0].Name, TaxConfig: &productcatalog.TaxConfig{ Stripe: &productcatalog.StripeTaxConfig{Code: "txcd_ubp_test"}, }, // ... price config }, BillingCadence: MonthPeriod, // non-pointer } // ... test assertions })Based on learnings: In
productcatalog.UsageBasedRateCard, theBillingCadencefield is a non-pointerisodate.Period, while inproductcatalog.FlatFeeRateCard,BillingCadenceis a pointer type.Also applies to: 67-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service/taxcode_test.go` around lines 23 - 38, Tests only exercise FlatFeeRateCard but production resolveTaxCodes also handles UsageBasedRateCard (with non-pointer BillingCadence); add a test covering UsageBasedRateCard to validate that path. Create a helper similar to newTestFlatRateCard (e.g., newTestUsageBasedRateCard) that returns a *productcatalog.UsageBasedRateCard with RateCardMeta including a TaxConfig (StripeTaxConfig.Code set) and a non-pointer BillingCadence (e.g., MonthPeriod), then add a t.Run case (e.g., "UsageBasedWithStripeCode") asserting resolveTaxCodes returns the expected stripe tax code mapping for that feature; reference resolveTaxCodes, productcatalog.UsageBasedRateCard, RateCardMeta, and StripeTaxConfig in the new test.openmeter/productcatalog/plan/service/plan.go (1)
185-240: Consider extracting the rate card rebuild logic to reduce duplication.The rate card type-switching and rebuild pattern (lines 212-236) duplicates what's already in
resolveFeatures(lines 120-142, 154-176). The existing FIXME on line 37 mentions this is "a bit brittle."This isn't blocking, but consolidating into a shared helper (e.g.,
rebuildRateCardWithMeta(rc RateCard, meta RateCardMeta) (RateCard, error)) would reduce duplication and make future rate card type additions easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service/plan.go` around lines 185 - 240, The rate-card rebuild switch in resolveTaxCodes duplicates logic from resolveFeatures; extract that logic into a shared helper (e.g., rebuildRateCardWithMeta(rc productcatalog.RateCard, meta productcatalog.RateCardMeta) (productcatalog.RateCard, error)) that inspects rc.Type(), uses rc.GetBillingCadence() when needed, validates required billing cadence for UsageBasedRateCardType, constructs and returns the appropriate productcatalog.FlatFeeRateCard or productcatalog.UsageBasedRateCard with the provided meta, and returns an error for unsupported types or missing cadence; then call this helper from both resolveTaxCodes and resolveFeatures and keep the existing rc.Merge(newRC) call and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/productcatalog/ratecard.go`:
- Around line 83-84: The RateCard.Clone() method currently omits the newly added
TaxCode field so cloned RateCard instances lose the eagerly-loaded tax
reference; update the Clone() implementation(s) that copy RateCard data (look
for the Clone method on RateCard and any helper clone functions in the 93-140
region) to also copy the TaxCode pointer (preserving nil vs non-nil) into the
cloned struct, or add a clarifying comment in Clone() that omitting TaxCode is
intentional if it must remain read-only/eager-only.
In `@openmeter/productcatalog/tax_test.go`:
- Around line 156-191: Add a test case that exercises TaxConfig.Clone(): create
a TaxConfig with pointer fields (e.g., TaxCodeID), call Clone() and assert the
cloned value is equal via Equal(), then mutate the clone (change the TaxCodeID)
and assert the original is unchanged and pointers are not the same; place this
alongside the existing Equal()/MergeTaxConfigs() cases so the test suite catches
any shallow-copy regressions in TaxConfig.Clone().
In `@openmeter/taxcode/service/taxcode.go`:
- Around line 58-87: The code in GetOrCreateByAppMapping calls
s.adapter.GetTaxCodeByAppMapping and treats any error as "not found" — change
this so you only proceed to create when the error is an explicit not-found; if
err != nil and !models.IsNotFoundError(err) (or the project’s equivalent
not-found checker) return the error immediately, otherwise continue to build the
key and call s.adapter.CreateTaxCode; keep the existing concurrent-create
conflict handling using models.IsGenericConflictError and the subsequent
GetTaxCodeByAppMapping call.
---
Outside diff comments:
In `@openmeter/productcatalog/plan/adapter/mapping.go`:
- Around line 339-341: The default error return in the switch uses err (from
ParsePtrOrNil) which may be nil, producing messages like "invalid RateCard type
foo: <nil>"; update the default case in mapping.go to avoid appending a nil
error—e.g., check if err != nil and return fmt.Errorf("invalid RateCard type %s:
%w", r.Type, err) otherwise return fmt.Errorf("invalid RateCard type %s",
r.Type); ensure you reference the ParsePtrOrNil result and avoid shadowing the
err variable.
In `@openmeter/productcatalog/ratecard.go`:
- Around line 142-181: The Equal method on RateCardMeta omits the new TaxCode
field; update RateCardMeta.Equal to compare TaxCode (use the same pointer-safe
comparison style as FeatureKey/FeatureID, e.g., lo.FromPtr(r.TaxCode) vs
lo.FromPtr(v.TaxCode)) and return false if they differ, or if the intended
semantics are to ignore TaxCode add a clarifying comment above
RateCardMeta.Equal explaining why it is excluded; reference the
RateCardMeta.Equal method and the TaxCode field when making the change.
In `@openmeter/productcatalog/tax.go`:
- Around line 88-99: The Clone method for TaxConfig assigns the pointer field
Behavior directly rather than deep-copying it like Stripe and TaxCodeID; update
TaxConfig.Clone to deep-copy Behavior as well (e.g. in the same pattern used for
Stripe/TaxCodeID: check if c.Behavior != nil and set out.Behavior to a new
pointer holding a cloned or dereferenced value using lo.ToPtr or the
Behavior.Clone() method as appropriate) so all pointer fields are cloned
consistently in TaxConfig.Clone.
---
Nitpick comments:
In `@api/v3/handlers/billingprofiles/convert.go`:
- Around line 69-71: The goverter ignore on TaxCodeID in the
ConvertBillingTaxConfigToTaxConfig declaration is a temporary workaround; create
and reference a tracking ticket/issue for removing the "// goverter:ignore
TaxCodeID" once the OpenAPI schema is updated, then update this comment to
include the issue number (e.g., "TODO: remove goverter:ignore TaxCodeID —
tracked in ISSUE-1234") and add a brief note in the function comment for
ConvertBillingTaxConfigToTaxConfig so reviewers can find and remove the ignore
when the schema change lands.
In `@openmeter/productcatalog/plan/service/plan.go`:
- Around line 185-240: The rate-card rebuild switch in resolveTaxCodes
duplicates logic from resolveFeatures; extract that logic into a shared helper
(e.g., rebuildRateCardWithMeta(rc productcatalog.RateCard, meta
productcatalog.RateCardMeta) (productcatalog.RateCard, error)) that inspects
rc.Type(), uses rc.GetBillingCadence() when needed, validates required billing
cadence for UsageBasedRateCardType, constructs and returns the appropriate
productcatalog.FlatFeeRateCard or productcatalog.UsageBasedRateCard with the
provided meta, and returns an error for unsupported types or missing cadence;
then call this helper from both resolveTaxCodes and resolveFeatures and keep the
existing rc.Merge(newRC) call and error handling.
In `@openmeter/productcatalog/plan/service/service_test.go`:
- Around line 214-216: Add explicit nil assertions before extracting TaxCodeID
from draftPlanV1 to make the test robust: retrieve taxConfig via
draftPlanV1.Phases[0].RateCards[0].AsMeta().TaxConfig, call require.NotNil(t,
taxConfig, "TaxConfig should be populated after plan creation") and
require.NotNil(t, taxConfig.TaxCodeID, "TaxCodeID should be resolved during plan
creation"), then assign taxCodeID := taxConfig.TaxCodeID; this ensures TaxConfig
and TaxCodeID are validated before use in the test.
In `@openmeter/productcatalog/plan/service/taxcode_test.go`:
- Around line 23-38: Tests only exercise FlatFeeRateCard but production
resolveTaxCodes also handles UsageBasedRateCard (with non-pointer
BillingCadence); add a test covering UsageBasedRateCard to validate that path.
Create a helper similar to newTestFlatRateCard (e.g., newTestUsageBasedRateCard)
that returns a *productcatalog.UsageBasedRateCard with RateCardMeta including a
TaxConfig (StripeTaxConfig.Code set) and a non-pointer BillingCadence (e.g.,
MonthPeriod), then add a t.Run case (e.g., "UsageBasedWithStripeCode") asserting
resolveTaxCodes returns the expected stripe tax code mapping for that feature;
reference resolveTaxCodes, productcatalog.UsageBasedRateCard, RateCardMeta, and
StripeTaxConfig in the new test.
In `@openmeter/taxcode/service.go`:
- Around line 125-171: Replace the duplicated structs and validation by
introducing a single shared type (e.g., AppMappingLookupInput) containing
Namespace, AppType and TaxCode, move the Validate() implementation to that type
(Validate on AppMappingLookupInput) and then create type aliases
GetTaxCodeByAppMappingInput = AppMappingLookupInput and
GetOrCreateByAppMappingInput = AppMappingLookupInput so existing code referring
to GetTaxCodeByAppMappingInput and GetOrCreateByAppMappingInput continues to
work; ensure you remove the duplicate Validate methods for the old names and
keep references to AppType.Validate() and
models.NewNillableGenericValidationError(errors.Join(...)) unchanged.
🪄 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: 76d2fa06-08c7-4b11-96ce-1bad47181fca
📒 Files selected for processing (29)
api/v3/handlers/billingprofiles/convert.goapp/common/productcatalog.gocmd/billing-worker/wire.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/productcatalog/plan/adapter/mapping.goopenmeter/productcatalog/plan/adapter/phase.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/tax.goopenmeter/productcatalog/tax_test.goopenmeter/productcatalog/testutils/env.goopenmeter/server/server_test.goopenmeter/subscription/addon/diff/apply_test.goopenmeter/subscription/testutils/service.goopenmeter/taxcode/adapter/mapping.goopenmeter/taxcode/adapter/taxcode.goopenmeter/taxcode/repository.goopenmeter/taxcode/service.goopenmeter/taxcode/service/taxcode.goopenmeter/taxcode/taxcode.gotest/billing/subscription_suite.gotest/customer/testenv.go
Comment on lines
+58
to
+87
| func (s *service) GetOrCreateByAppMapping(ctx context.Context, input taxcode.GetOrCreateByAppMappingInput) (taxcode.TaxCode, error) { | ||
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (taxcode.TaxCode, error) { | ||
| // Try to find an existing TaxCode with this app mapping. | ||
| tc, err := s.adapter.GetTaxCodeByAppMapping(ctx, taxcode.GetTaxCodeByAppMappingInput(input)) | ||
| if err == nil { | ||
| return tc, nil | ||
| } | ||
|
|
||
| // Not found — create a new TaxCode. | ||
| key := fmt.Sprintf("%s_%s", input.AppType, input.TaxCode) | ||
|
|
||
| tc, err = s.adapter.CreateTaxCode(ctx, taxcode.CreateTaxCodeInput{ | ||
| Namespace: input.Namespace, | ||
| Key: key, | ||
| Name: input.TaxCode, | ||
| AppMappings: taxcode.TaxCodeAppMappings{ | ||
| {AppType: input.AppType, TaxCode: input.TaxCode}, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| // Another request may have created it concurrently. | ||
| if models.IsGenericConflictError(err) { | ||
| return s.adapter.GetTaxCodeByAppMapping(ctx, taxcode.GetTaxCodeByAppMappingInput(input)) | ||
| } | ||
|
|
||
| return taxcode.TaxCode{}, err | ||
| } | ||
|
|
||
| return tc, nil | ||
| }) |
Contributor
There was a problem hiding this comment.
Missing "not found" check before creation.
The current logic proceeds to create a new TaxCode on any error from GetTaxCodeByAppMapping, not just "not found" errors. If there's a transient DB error or other failure, this would incorrectly attempt to create a new entity instead of propagating the error.
Consider checking for "not found" explicitly before proceeding with creation:
🛡️ Suggested fix
func (s *service) GetOrCreateByAppMapping(ctx context.Context, input taxcode.GetOrCreateByAppMappingInput) (taxcode.TaxCode, error) {
return transaction.Run(ctx, s.adapter, func(ctx context.Context) (taxcode.TaxCode, error) {
// Try to find an existing TaxCode with this app mapping.
tc, err := s.adapter.GetTaxCodeByAppMapping(ctx, taxcode.GetTaxCodeByAppMappingInput(input))
if err == nil {
return tc, nil
}
+
+ // Only proceed to creation if the error indicates "not found".
+ if !models.IsGenericNotFoundError(err) {
+ return taxcode.TaxCode{}, err
+ }
// Not found — create a new TaxCode.
key := fmt.Sprintf("%s_%s", input.AppType, input.TaxCode)📝 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 (s *service) GetOrCreateByAppMapping(ctx context.Context, input taxcode.GetOrCreateByAppMappingInput) (taxcode.TaxCode, error) { | |
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (taxcode.TaxCode, error) { | |
| // Try to find an existing TaxCode with this app mapping. | |
| tc, err := s.adapter.GetTaxCodeByAppMapping(ctx, taxcode.GetTaxCodeByAppMappingInput(input)) | |
| if err == nil { | |
| return tc, nil | |
| } | |
| // Not found — create a new TaxCode. | |
| key := fmt.Sprintf("%s_%s", input.AppType, input.TaxCode) | |
| tc, err = s.adapter.CreateTaxCode(ctx, taxcode.CreateTaxCodeInput{ | |
| Namespace: input.Namespace, | |
| Key: key, | |
| Name: input.TaxCode, | |
| AppMappings: taxcode.TaxCodeAppMappings{ | |
| {AppType: input.AppType, TaxCode: input.TaxCode}, | |
| }, | |
| }) | |
| if err != nil { | |
| // Another request may have created it concurrently. | |
| if models.IsGenericConflictError(err) { | |
| return s.adapter.GetTaxCodeByAppMapping(ctx, taxcode.GetTaxCodeByAppMappingInput(input)) | |
| } | |
| return taxcode.TaxCode{}, err | |
| } | |
| return tc, nil | |
| }) | |
| func (s *service) GetOrCreateByAppMapping(ctx context.Context, input taxcode.GetOrCreateByAppMappingInput) (taxcode.TaxCode, error) { | |
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (taxcode.TaxCode, error) { | |
| // Try to find an existing TaxCode with this app mapping. | |
| tc, err := s.adapter.GetTaxCodeByAppMapping(ctx, taxcode.GetTaxCodeByAppMappingInput(input)) | |
| if err == nil { | |
| return tc, nil | |
| } | |
| // Only proceed to creation if the error indicates "not found". | |
| if !models.IsGenericNotFoundError(err) { | |
| return taxcode.TaxCode{}, err | |
| } | |
| // Not found — create a new TaxCode. | |
| key := fmt.Sprintf("%s_%s", input.AppType, input.TaxCode) | |
| tc, err = s.adapter.CreateTaxCode(ctx, taxcode.CreateTaxCodeInput{ | |
| Namespace: input.Namespace, | |
| Key: key, | |
| Name: input.TaxCode, | |
| AppMappings: taxcode.TaxCodeAppMappings{ | |
| {AppType: input.AppType, TaxCode: input.TaxCode}, | |
| }, | |
| }) | |
| if err != nil { | |
| // Another request may have created it concurrently. | |
| if models.IsGenericConflictError(err) { | |
| return s.adapter.GetTaxCodeByAppMapping(ctx, taxcode.GetTaxCodeByAppMappingInput(input)) | |
| } | |
| return taxcode.TaxCode{}, err | |
| } | |
| return tc, nil | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/taxcode/service/taxcode.go` around lines 58 - 87, The code in
GetOrCreateByAppMapping calls s.adapter.GetTaxCodeByAppMapping and treats any
error as "not found" — change this so you only proceed to create when the error
is an explicit not-found; if err != nil and !models.IsNotFoundError(err) (or the
project’s equivalent not-found checker) return the error immediately, otherwise
continue to build the key and call s.adapter.CreateTaxCode; keep the existing
concurrent-create conflict handling using models.IsGenericConflictError and the
subsequent GetTaxCodeByAppMapping call.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/productcatalog/adapter/taxconfig.go`:
- Around line 11-39: BackfillTaxConfig must also populate the TaxCodeID when
rebuilding a nil cfg: in BackfillTaxConfig, after creating cfg (cfg =
&productcatalog.TaxConfig{}) and before returning, set cfg.TaxCodeID (if it's
empty/nil) from the provided taxcode object (use tc.GetId() when tc != nil),
similar to how cfg.Behavior and cfg.Stripe are set so the backfill path
preserves the TaxCodeID field consistent with the dual-write path.
In `@openmeter/productcatalog/addon/adapter/mapping.go`:
- Around line 102-115: The FromPlanRateCardRow path never backfills legacy tax
fields from the eagerly-loaded TaxCode like the addon path does; update the
FromPlanRateCardRow handling to call r.Edges.TaxCodeOrErr(), map it via
taxcodeadapter.MapTaxCodeFromEntity, set meta.TaxCode, and then call
productcatalogadapter.BackfillTaxConfig(meta.TaxConfig, r.TaxBehavior,
meta.TaxCode) (same sequence as in the addon branch) so plan-phase rate cards
receive the same backfilled TaxConfig and TaxCode values.
In `@openmeter/productcatalog/addon/service/addon.go`:
- Around line 503-506: In UpdateAddon, move the call to s.resolveTaxCodes(ctx,
params.Namespace, params.RateCards) so it runs after the existing add-on is
loaded and after the draft-only/status validation (i.e., after the code that
enforces update allowed only for draft status), not before; this prevents
running the get-or-create write on failing/unauthorized updates and avoids
creating orphan tax codes when the update is rejected. Ensure resolveTaxCodes
remains using params.Namespace and params.RateCards and only executes once the
status check on the loaded add-on passes.
In `@openmeter/productcatalog/addon/service/taxcode_test.go`:
- Around line 527-533: The test is missing an assertion for the backfilled
TaxCodeID; update the test block that reads tc := backfillRC.AsMeta().TaxConfig
to also require/not-nil and assert the expected TaxCodeID value (e.g.,
require.NotNil or require.Equal on tc.TaxCodeID) so the dedicated-column read
path preserves TaxConfig.TaxCodeID; reference the existing variables backfillRC,
tc, and TaxConfig.TaxCodeID when adding the assertion.
🪄 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: 5a0996d7-7bdd-4ea3-b897-1a83eb8c662c
📒 Files selected for processing (22)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/productcatalog/adapter/taxconfig.goopenmeter/productcatalog/addon/adapter/addon.goopenmeter/productcatalog/addon/adapter/mapping.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/service_test.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/plan/adapter/mapping.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/tax.goopenmeter/productcatalog/tax_test.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/addon/repo/subscriptionaddon.goopenmeter/subscription/testutils/service.gotest/billing/subscription_suite.gotest/customer/testenv.go
✅ Files skipped from review due to trivial changes (3)
- cmd/billing-worker/wire_gen.go
- cmd/jobs/internal/wire_gen.go
- cmd/server/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/productcatalog/plan/adapter/plan.go
- openmeter/productcatalog/plan/adapter/mapping.go
- app/common/productcatalog.go
- openmeter/productcatalog/plan/service/taxcode_test.go
Comment on lines
+102
to
+115
| // Map TaxCode if eagerly loaded. | ||
| taxCodeRow, err := r.Edges.TaxCodeOrErr() | ||
| if err == nil { | ||
| tc, err := taxcodeadapter.MapTaxCodeFromEntity(taxCodeRow) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid tax code for rate card %s: %w", r.ID, err) | ||
| } | ||
|
|
||
| meta.TaxCode = &tc | ||
| } | ||
|
|
||
| // Backfill legacy TaxConfig fields from new columns and TaxCode entity. | ||
| meta.TaxConfig = productcatalogadapter.BackfillTaxConfig(meta.TaxConfig, r.TaxBehavior, meta.TaxCode) | ||
|
|
Contributor
There was a problem hiding this comment.
Please apply the same backfill to FromPlanRateCardRow.
This fixes add-on rate cards, but the expanded plan path in this file still never looks at r.Edges.TaxCode or calls BackfillTaxConfig. So plan phase rate cards that rely on tax_code_id / tax_behavior will still miss the backfilled tax data when GetAddon(..., Expand.PlanAddons=true) is used, even though addonEagerLoadActivePlans now loads TaxCode for them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/productcatalog/addon/adapter/mapping.go` around lines 102 - 115,
The FromPlanRateCardRow path never backfills legacy tax fields from the
eagerly-loaded TaxCode like the addon path does; update the FromPlanRateCardRow
handling to call r.Edges.TaxCodeOrErr(), map it via
taxcodeadapter.MapTaxCodeFromEntity, set meta.TaxCode, and then call
productcatalogadapter.BackfillTaxConfig(meta.TaxConfig, r.TaxBehavior,
meta.TaxCode) (same sequence as in the addon branch) so plan-phase rate cards
receive the same backfilled TaxConfig and TaxCode values.
Comment on lines
+503
to
+506
|
|
||
| if err := s.resolveTaxCodes(ctx, params.Namespace, params.RateCards); err != nil { | ||
| return nil, fmt.Errorf("failed to resolve tax codes for ratecards in add-on: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Validate before resolving tax codes.
resolveTaxCodes does a get-or-create write. In UpdateAddon, this runs before we even load the existing add-on and enforce draft-only updates, so a rejected update still hits the DB and may leave orphan tax codes behind if the tax-code service is not enlisted in the same transaction. Moving this block below the status validation keeps the failure path side-effect free.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/productcatalog/addon/service/addon.go` around lines 503 - 506, In
UpdateAddon, move the call to s.resolveTaxCodes(ctx, params.Namespace,
params.RateCards) so it runs after the existing add-on is loaded and after the
draft-only/status validation (i.e., after the code that enforces update allowed
only for draft status), not before; this prevents running the get-or-create
write on failing/unauthorized updates and avoids creating orphan tax codes when
the update is rejected. Ensure resolveTaxCodes remains using params.Namespace
and params.RateCards and only executes once the status check on the loaded
add-on passes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/productcatalog/addon/service/taxcode_test.go (1)
433-443: Consider assertingTaxCodeIDfor completeness.The test verifies
BehaviorandStripeare preserved on read-back, but doesn't checkTaxCodeID. Since this test covers the full round-trip (create → get), assertingTaxCodeIDwould ensure the resolution path is fully validated.🧪 Suggested assertion to add
tc := getFirstAddonRCTaxConfig(t, fetched) require.NotNil(t, tc) + +// TaxCodeID should be present +require.NotNil(t, tc.TaxCodeID, "TaxCodeID must be resolved and preserved") // Behavior should be present require.NotNil(t, tc.Behavior)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/addon/service/taxcode_test.go` around lines 433 - 443, The test checks Behavior and Stripe on the read-back tax config but omits asserting the TaxCodeID; update the test after calling getFirstAddonRCTaxConfig (variable tc) to assert that tc.TaxCodeID is present and equals the expected tax code identifier used when creating the resource (e.g., assert.NotNil(tc.TaxCodeID) and assert.Equal(t, expectedTaxCodeID, tc.TaxCodeID) where expectedTaxCodeID is the tax code ID variable from the create/setup step).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/productcatalog/addon/service/taxcode_test.go`:
- Around line 433-443: The test checks Behavior and Stripe on the read-back tax
config but omits asserting the TaxCodeID; update the test after calling
getFirstAddonRCTaxConfig (variable tc) to assert that tc.TaxCodeID is present
and equals the expected tax code identifier used when creating the resource
(e.g., assert.NotNil(tc.TaxCodeID) and assert.Equal(t, expectedTaxCodeID,
tc.TaxCodeID) where expectedTaxCodeID is the tax code ID variable from the
create/setup step).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b745e11-2fcc-4347-94b5-4db7707ddf5c
📒 Files selected for processing (5)
openmeter/productcatalog/adapter/taxconfig.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/testutils/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/productcatalog/adapter/taxconfig.go
- openmeter/productcatalog/plan/service/taxcode_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.
Summary by CodeRabbit
Release Notes
New Features
Tests