fix(billing): merge tax-code Stripe+ID atomically - #4467
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces billing.TaxConfig (with resolved TaxCode snapshots), removes TaxCode from productcatalog, migrates line models/persistence to billing configs, updates adapters/HTTP/Stripe mappings to wrap/unwrap configs, refactors merge/stamping flows, updates tests, and improves taxcode conflict handling. ChangesTax Configuration Layering and Stamping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
openmeter/billing/adapter/stdinvoicelines.go (1)
329-333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame nil dereference risk as in gatheringlines.go:224.
At line 330,
*billing.FromProductCatalog(line.TaxConfig)dereferences the result. IfFromProductCatalogcan returnnil, this will panic. This is the same concern raised forgatheringlines.go.🤖 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/billing/adapter/stdinvoicelines.go` around lines 329 - 333, The code dereferences billing.FromProductCatalog(line.TaxConfig) when setting tax config in the stdinvoicelines flow, which can panic if FromProductCatalog returns nil; update the block that builds create (the code calling SetTaxConfig(...).SetNillableTaxCodeID(...).SetNillableTaxBehavior(...)) to first call billing.FromProductCatalog(line.TaxConfig) into a local variable, check that the result is non-nil before passing it to SetTaxConfig, and only call SetTaxConfig when that local is non-nil (still preserving the existing Nillable setters using line.TaxConfig fields as before).openmeter/billing/stdinvoiceline.go (1)
409-409:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame nil receiver concern as before.
At line 409,
i.TaxConfig.ToProductCatalog()will panic ifi.TaxConfigisnilandToProductCatalog()doesn't handlenilreceivers. This is the same issue flagged in multiple other files.🤖 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/billing/stdinvoiceline.go` at line 409, The assignment TaxConfig: i.TaxConfig.ToProductCatalog() can panic when i.TaxConfig is nil; update the struct construction to guard the call by checking i.TaxConfig for nil and only call ToProductCatalog() when non-nil (e.g., compute a local variable taxCfg := nil; if i.TaxConfig != nil { taxCfg = i.TaxConfig.ToProductCatalog() } and assign TaxConfig: taxCfg), ensuring ToProductCatalog is invoked only on non-nil receivers.openmeter/billing/httpdriver/invoiceline.go (1)
365-365:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame nil receiver concern for
ToProductCatalog()calls.At lines 365 and 376,
line.TaxConfig.ToProductCatalog()is called. Ifline.TaxConfigisnilandToProductCatalog()doesn't handlenilreceivers, this will panic. This is the same issue flagged in other files.Also applies to: 376-376
🤖 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/billing/httpdriver/invoiceline.go` at line 365, The call to line.TaxConfig.ToProductCatalog() can panic if line.TaxConfig is nil; update the invoiceline construction (where TaxConfig: mapTaxConfigToAPI(line.TaxConfig.ToProductCatalog()) appears) to defensively handle a nil TaxConfig by checking if line.TaxConfig == nil and passing nil into mapTaxConfigToAPI (or an appropriate empty value) instead of calling ToProductCatalog on a nil receiver; alternatively add a nil-safe ToProductCatalog receiver implementation, but prefer the explicit nil check near the use-site (referencing line.TaxConfig, ToProductCatalog, and mapTaxConfigToAPI) to avoid panics.openmeter/billing/adapter/stdinvoicelinemapper.go (1)
217-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame nil-safety concern as in gatheringlines.go.
At line 218,
billing.FromProductCatalog(lo.EmptyableToPtr(dbLine.TaxConfig))may receivenilif the tax config is empty. Please verify thatFromProductCataloghandlesnilinput correctly. Additionally, line 221's.ToProductCatalog()call may panic if the preceding result isniland the method doesn't handlenilreceivers.🤖 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/billing/adapter/stdinvoicelinemapper.go` around lines 217 - 221, The call chain around backfillTaxConfigReferences may pass or return nil: avoid passing a nil pointer into billing.FromProductCatalog and avoid calling .ToProductCatalog() on a nil result; update the code around backfillTaxConfigReferences(lo.EmptyableToPtr(dbLine.TaxConfig), dbLine.TaxBehavior, taxCodeFromDetailedLineV2Edge(dbLine)) so that you first guard lo.EmptyableToPtr(dbLine.TaxConfig) (use a non-nil zero-value or skip/backfill when TaxConfig is empty) or make billing.FromProductCatalog robust to nil, and ensure you only call .ToProductCatalog() after checking the returned value from backfillTaxConfigReferences is non-nil (or make ToProductCatalog a safe method on nil receivers); reference functions/types: backfillTaxConfigReferences, billing.FromProductCatalog, lo.EmptyableToPtr, dbLine.TaxConfig, taxCodeFromDetailedLineV2Edge, and ToProductCatalog when applying the fix.
🧹 Nitpick comments (1)
openmeter/taxcode/taxcode.go (1)
89-90: ⚡ Quick winClarify which fields are actually compared.
The comment says "all other fields are compared," but the implementation only compares
ID,Namespace,Key,Name,Description, andAppMappings. It doesn't compareMetadataorAnnotations, which might be intentional for semantic equality, but the comment is misleading.Consider rewording to explicitly list the compared fields or say "key semantic fields" instead of "all other fields."
📝 Suggested comment clarification
-// Equal returns true when both TaxCode values carry identical semantic data. -// ManagedModel timestamps (CreatedAt, UpdatedAt, DeletedAt) are excluded; all other fields are compared. +// Equal returns true when both TaxCode values carry identical semantic data. +// Compares ID, Namespace, Key, Name, Description, and AppMappings. +// ManagedModel timestamps (CreatedAt, UpdatedAt, DeletedAt), Metadata, and Annotations are excluded.🤖 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/taxcode.go` around lines 89 - 90, The comment for Equal in TaxCode is misleading because it claims "all other fields are compared" while the implementation only compares specific fields (ID, Namespace, Key, Name, Description, AppMappings) and excludes Metadata/Annotations and timestamps; update the comment for the Equal method (TaxCode.Equal) to explicitly list the fields being compared or replace "all other fields" with a phrase like "key semantic fields (ID, Namespace, Key, Name, Description, AppMappings)" so the doc matches the implementation.
🤖 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/billing/adapter/stdinvoicelinemapper.go`:
- Around line 345-367: The function backfillTaxConfigReferences calls
snapshottedTaxConfig.ToProductCatalog() without guarding for a nil receiver; add
a nil check for snapshottedTaxConfig at the top of backfillTaxConfigReferences
and handle the nil case (e.g., pass nil into productcatalog.BackfillTaxConfig or
short-circuit to return billing.FromProductCatalog(nil) as appropriate). Ensure
subsequent logic that inspects backfilledTaxConfig and resolvedTaxCode still
behaves correctly when backfilledTaxConfig is nil (the code paths that check
backfilledTaxConfig.TaxCodeID and backfilledTaxConfig.Stripe should only run
when backfilledTaxConfig != nil).
---
Duplicate comments:
In `@openmeter/billing/adapter/stdinvoicelinemapper.go`:
- Around line 217-221: The call chain around backfillTaxConfigReferences may
pass or return nil: avoid passing a nil pointer into billing.FromProductCatalog
and avoid calling .ToProductCatalog() on a nil result; update the code around
backfillTaxConfigReferences(lo.EmptyableToPtr(dbLine.TaxConfig),
dbLine.TaxBehavior, taxCodeFromDetailedLineV2Edge(dbLine)) so that you first
guard lo.EmptyableToPtr(dbLine.TaxConfig) (use a non-nil zero-value or
skip/backfill when TaxConfig is empty) or make billing.FromProductCatalog robust
to nil, and ensure you only call .ToProductCatalog() after checking the returned
value from backfillTaxConfigReferences is non-nil (or make ToProductCatalog a
safe method on nil receivers); reference functions/types:
backfillTaxConfigReferences, billing.FromProductCatalog, lo.EmptyableToPtr,
dbLine.TaxConfig, taxCodeFromDetailedLineV2Edge, and ToProductCatalog when
applying the fix.
In `@openmeter/billing/adapter/stdinvoicelines.go`:
- Around line 329-333: The code dereferences
billing.FromProductCatalog(line.TaxConfig) when setting tax config in the
stdinvoicelines flow, which can panic if FromProductCatalog returns nil; update
the block that builds create (the code calling
SetTaxConfig(...).SetNillableTaxCodeID(...).SetNillableTaxBehavior(...)) to
first call billing.FromProductCatalog(line.TaxConfig) into a local variable,
check that the result is non-nil before passing it to SetTaxConfig, and only
call SetTaxConfig when that local is non-nil (still preserving the existing
Nillable setters using line.TaxConfig fields as before).
In `@openmeter/billing/httpdriver/invoiceline.go`:
- Line 365: The call to line.TaxConfig.ToProductCatalog() can panic if
line.TaxConfig is nil; update the invoiceline construction (where TaxConfig:
mapTaxConfigToAPI(line.TaxConfig.ToProductCatalog()) appears) to defensively
handle a nil TaxConfig by checking if line.TaxConfig == nil and passing nil into
mapTaxConfigToAPI (or an appropriate empty value) instead of calling
ToProductCatalog on a nil receiver; alternatively add a nil-safe
ToProductCatalog receiver implementation, but prefer the explicit nil check near
the use-site (referencing line.TaxConfig, ToProductCatalog, and
mapTaxConfigToAPI) to avoid panics.
In `@openmeter/billing/stdinvoiceline.go`:
- Line 409: The assignment TaxConfig: i.TaxConfig.ToProductCatalog() can panic
when i.TaxConfig is nil; update the struct construction to guard the call by
checking i.TaxConfig for nil and only call ToProductCatalog() when non-nil
(e.g., compute a local variable taxCfg := nil; if i.TaxConfig != nil { taxCfg =
i.TaxConfig.ToProductCatalog() } and assign TaxConfig: taxCfg), ensuring
ToProductCatalog is invoked only on non-nil receivers.
---
Nitpick comments:
In `@openmeter/taxcode/taxcode.go`:
- Around line 89-90: The comment for Equal in TaxCode is misleading because it
claims "all other fields are compared" while the implementation only compares
specific fields (ID, Namespace, Key, Name, Description, AppMappings) and
excludes Metadata/Annotations and timestamps; update the comment for the Equal
method (TaxCode.Equal) to explicitly list the fields being compared or replace
"all other fields" with a phrase like "key semantic fields (ID, Namespace, Key,
Name, Description, AppMappings)" so the doc matches the implementation.
🪄 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: 8dfb5647-b6a9-4705-85d8-bcb5a4dc6126
⛔ Files ignored due to path filters (6)
openmeter/ent/db/billinginvoiceline.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_update.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**
📒 Files selected for processing (18)
api/v3/handlers/billingprofiles/convert.goopenmeter/billing/adapter/gatheringlines.goopenmeter/billing/adapter/stdinvoicelinemapper.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/httpdriver/invoiceline.goopenmeter/billing/service/invoicecalc/details.goopenmeter/billing/service/invoicecalc/taxconfig.goopenmeter/billing/service/invoicecalc/taxconfig_test.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/taxconfig.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/ent/schema/billing.goopenmeter/productcatalog/tax.goopenmeter/productcatalog/tax_test.goopenmeter/taxcode/taxcode.gotest/billing/taxcode_dual_write_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/billingprofiles/convert.go
4ca0e9e to
7a181a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
openmeter/billing/taxconfig.go (1)
86-111: ⚡ Quick winReuse the embedded
Clone()andslices.Cloneto keep this honest.This re-implements
productcatalog.TaxConfig.Clone()by hand forBehavior/Stripe/TaxCodeID. Ifproductcatalog.TaxConfigever grows a field, this clone will silently drop it. Delegating to the embeddedClone()keeps them in sync. Also, theAppMappingscopy can useslices.Cloneper the standard-helpers guideline.♻️ Proposed refactor
func (c TaxConfig) Clone() TaxConfig { - out := TaxConfig{} - - if c.Behavior != nil { - out.Behavior = lo.ToPtr(*c.Behavior) - } - - if c.Stripe != nil { - out.Stripe = lo.ToPtr(c.Stripe.Clone()) - } - - if c.TaxCodeID != nil { - out.TaxCodeID = lo.ToPtr(*c.TaxCodeID) - } + out := TaxConfig{ + TaxConfig: c.TaxConfig.Clone(), + } if c.TaxCode != nil { tc := *c.TaxCode - tc.AppMappings = append(taxcode.TaxCodeAppMappings(nil), c.TaxCode.AppMappings...) + tc.AppMappings = slices.Clone(c.TaxCode.AppMappings) if c.TaxCode.Description != nil { tc.Description = lo.ToPtr(*c.TaxCode.Description) } out.TaxCode = &tc } return out }Add
"slices"to the imports (and drop the now-unusedtaxcodereference here if it isn't used elsewhere in the file).As per coding guidelines: "use
slices.Clone(s)... instead of creating local ... wrappers."🤖 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/billing/taxconfig.go` around lines 86 - 111, The custom TaxConfig.Clone implementation duplicates productcatalog.TaxConfig.Clone and risks dropping future fields; refactor TaxConfig.Clone to delegate to the embedded productcatalog.TaxConfig.Clone (call that Clone first and assign its result to out) and then only deep-copy the mutable nested slices/ptrs that need special handling; replace the manual AppMappings copy with slices.Clone(c.TaxCode.AppMappings) and remove the custom taxcode.TaxCodeAppMappings usage, ensuring Behavior, Stripe, TaxCodeID and TaxCode are preserved by first calling the embedded Clone and then applying any required deep copies using slices.Clone and existing Clone methods.
🤖 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/billing/taxconfig.go`:
- Around line 43-62: The Equality check in TaxConfig.Equal wrongly treats both
TaxCode == nil as unequal; update the logic in TaxConfig.Equal so that after
confirming embedded c.TaxConfig.Equal(&v.TaxConfig) returns true, handle the
TaxCode pair: if both c.TaxCode and v.TaxCode are nil return true, if only one
is nil return false, otherwise return c.TaxCode.Equal(v.TaxCode). Make this
change inside the TaxConfig.Equal method to ensure two unresolved (both-nil) tax
codes are considered equal.
- Around line 121-148: MergeTaxConfigs currently drops the resolved TaxCode only
in the both-non-nil branch but returns clones that preserve TaxCode snapshots in
the single-side branches; make behavior consistent by clearing the TaxCode
snapshot and TaxCodeID on single-side clones so they match the two-sided merge
contract: in MergeTaxConfigs, when returning a clone from overrides.Clone() or
base.Clone(), set c.TaxCode = nil and c.TaxCodeID = nil before returning (refer
to MergeTaxConfigs, TaxConfig, Clone, TaxCode, TaxCodeID).
---
Nitpick comments:
In `@openmeter/billing/taxconfig.go`:
- Around line 86-111: The custom TaxConfig.Clone implementation duplicates
productcatalog.TaxConfig.Clone and risks dropping future fields; refactor
TaxConfig.Clone to delegate to the embedded productcatalog.TaxConfig.Clone (call
that Clone first and assign its result to out) and then only deep-copy the
mutable nested slices/ptrs that need special handling; replace the manual
AppMappings copy with slices.Clone(c.TaxCode.AppMappings) and remove the custom
taxcode.TaxCodeAppMappings usage, ensuring Behavior, Stripe, TaxCodeID and
TaxCode are preserved by first calling the embedded Clone and then applying any
required deep copies using slices.Clone and existing Clone methods.
🪄 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: 3875bce6-cb8d-406d-9d15-96ab4462b3de
⛔ Files ignored due to path filters (6)
openmeter/ent/db/billinginvoiceline.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_update.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**
📒 Files selected for processing (18)
api/v3/handlers/billingprofiles/convert.goopenmeter/billing/adapter/gatheringlines.goopenmeter/billing/adapter/stdinvoicelinemapper.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/httpdriver/invoiceline.goopenmeter/billing/service/invoicecalc/details.goopenmeter/billing/service/invoicecalc/taxconfig.goopenmeter/billing/service/invoicecalc/taxconfig_test.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/taxconfig.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/ent/schema/billing.goopenmeter/productcatalog/tax.goopenmeter/productcatalog/tax_test.goopenmeter/taxcode/taxcode.gotest/billing/taxcode_dual_write_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/billingprofiles/convert.go
🚧 Files skipped from review as they are similar to previous changes (16)
- openmeter/billing/adapter/gatheringlines.go
- openmeter/billing/adapter/stdinvoicelines.go
- test/billing/taxcode_dual_write_test.go
- openmeter/ent/schema/billing.go
- openmeter/billing/service/invoicecalc/details.go
- openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go
- openmeter/taxcode/taxcode.go
- openmeter/billing/adapter/stdinvoicelinemapper.go
- openmeter/billing/httpdriver/invoiceline.go
- openmeter/billing/gatheringinvoice.go
- openmeter/billing/stdinvoiceline.go
- openmeter/billing/service/invoicecalc/taxconfig.go
- openmeter/productcatalog/tax.go
- openmeter/billing/service/stdinvoicestate.go
- openmeter/billing/service/invoicecalc/taxconfig_test.go
- openmeter/productcatalog/tax_test.go
…fig snapshot at sync
528a770 to
78041fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/taxcode.go`:
- Around line 111-118: The current equality check compares t.AppMappings and
v.AppMappings by index which is order-sensitive; change it to be
order-insensitive by comparing them as multisets: build a frequency map (e.g.,
map[AppMapping]int) for t.AppMappings and decrement/validate counts while
iterating v.AppMappings (or sort both slices by a stable key and compare), then
return false if any counts mismatch or leftover entries remain; update the
equality block that references t.AppMappings and v.AppMappings accordingly.
🪄 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: 797f60e7-578e-45c1-a48d-23b59a6cdd97
⛔ Files ignored due to path filters (6)
openmeter/ent/db/billinginvoiceline.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_update.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**
📒 Files selected for processing (19)
api/v3/handlers/billingprofiles/convert.goopenmeter/app/stripe/appinvoice.goopenmeter/billing/adapter/gatheringlines.goopenmeter/billing/adapter/stdinvoicelinemapper.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/httpdriver/invoiceline.goopenmeter/billing/service/invoicecalc/taxconfig.goopenmeter/billing/service/invoicecalc/taxconfig_test.goopenmeter/billing/stdinvoice.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/taxconfig.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/ent/schema/billing.goopenmeter/productcatalog/tax.goopenmeter/productcatalog/tax_test.goopenmeter/taxcode/errors.goopenmeter/taxcode/service/taxcode.goopenmeter/taxcode/taxcode.gotest/billing/taxcode_dual_write_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/billingprofiles/convert.go
@coderabbitai
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests