refactor: start implementing gathering invoice #3804
Conversation
📝 WalkthroughWalkthroughAdds a new GatheringInvoice domain and end-to-end support: public models and converters, adapter CRUD (Create/Update/Delete/Get/List) and line diff/upsert, service APIs for pending-lines and listing, HTTP mapping/events, invoice-calculation split (legacy vs gathering), and widespread test migrations from Standard* → Gathering*. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Client as API Client
participant Service as GatheringInvoice Service
participant Adapter as Billing Adapter
participant DB as Database
participant EventBus as Event Bus
end
Client->>Service: CreatePendingInvoiceLines(ctx, input)
Service->>Service: Validate input
Service->>Adapter: CreateGatheringInvoice(ctx, input) (transaction)
Adapter->>DB: Insert/Upsert BillingInvoice + BillingInvoiceLines
Adapter->>Adapter: mapGatheringInvoiceFromDB(...)
Adapter-->>Service: GatheringInvoice result
Service->>EventBus: Publish GatheringInvoiceCreatedEvent
Service-->>Client: CreatePendingInvoiceLinesResult {Lines, Invoice}
sequenceDiagram
rect rgba(200,240,200,0.5)
participant Client as API Client
participant Service as GatheringInvoice Service
participant Adapter as Billing Adapter
participant DB as Database
end
Client->>Service: ListGatheringInvoices(ctx, input)
Service->>Service: Validate input
Service->>Adapter: ListGatheringInvoices(ctx, input) (transaction)
Adapter->>DB: Query BillingInvoices (filters + pagination)
Adapter->>DB: Fetch lines if expand requested
Adapter->>Adapter: mapGatheringInvoiceFromDB for each row
Adapter-->>Service: pagination.Result[GatheringInvoice]
Service-->>Client: Paginated gathering invoices
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
turip
force-pushed
the
refactor/start-implementing-gathering-invoice
branch
3 times, most recently
from
e1b43c5 to
115ac9c
Compare
turip
force-pushed
the
refactor/start-implementing-gathering-invoice
branch
2 times, most recently
from
60744f2 to
be3ecfe
Compare
turip
force-pushed
the
refactor/start-implementing-gathering-invoice
branch
from
1951b9a to
607e22e
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/httpdriver/invoiceline.go (1)
46-118:⚠️ Potential issue | 🟠 MajorGuard against nil CreatePendingInvoiceLines results.
CreatePendingInvoiceLinescan returnnil(e.g., zero‑valued lines). The current flow dereferencesresimmediately, which would panic in that valid case. A quick nil guard before building the response would make this safe.💡 Suggested guard
res, err := h.service.CreatePendingInvoiceLines(ctx, request) if err != nil { return CreatePendingLineResponse{}, fmt.Errorf("failed to create invoice lines: %w", err) } +if res == nil { + return CreatePendingLineResponse{}, nil +} out := CreatePendingLineResponse{ IsInvoiceNew: res.IsInvoiceNew, }
🤖 Fix all issues with AI agents
In `@openmeter/billing/adapter/gatheringinvoice.go`:
- Around line 98-190: UpdateGatheringInvoice is unconditionally calling
SetCollectionAt(in.NextCollectionAt.In(time.UTC)) which will panic or write
year‑0001 when collection_at is NULL; mirror CreateGatheringInvoice and
mapGatheringInvoiceFromDB by preserving nullable behavior: guard the nullable
NextCollectionAt by either calling SetNillableCollectionAt(...) with nil when
in.NextCollectionAt is unset or only invoke SetCollectionAt(...) when the
NextCollectionAt value is present, and ensure the same pattern is applied in the
other affected block (the second occurrence noted around lines 353-406); update
references in UpdateGatheringInvoice, CreateGatheringInvoice and
mapGatheringInvoiceFromDB accordingly.
In `@openmeter/billing/gatheringinvoice.go`:
- Around line 343-361: UpcomingChargeBase.Clone currently does a shallow copy
with out := i which leaves the Metadata map shared; update
UpcomingChargeBase.Clone to deep-copy the Metadata field (e.g., use maps.Clone
or equivalent) after copying annotations and before returning so the cloned
UpcomingChargeBase has its own map instance; keep the existing cloning of
Annotations (i.Annotations.Clone()), TaxConfig, and Subscription logic intact
and apply maps.Clone to i.Metadata into out.Metadata to prevent mutations on the
clone from affecting the original.
In `@openmeter/billing/service/invoicecalc/period.go`:
- Around line 10-12: The top-of-file doc comment still references the old
function name; update it to match the renamed function
CalculateStandardInvoiceServicePeriod by changing the comment from
"CalculateStandardInvoicePeriod calculates the period of the invoice based on
the lines." to a correct description that starts with
CalculateStandardInvoiceServicePeriod (e.g.,
"CalculateStandardInvoiceServicePeriod calculates the service period of the
invoice based on the lines."), ensuring the comment noun and verb agree with the
new function name and purpose.
🧹 Nitpick comments (5)
api/spec/src/billing/invoices.tsp (1)
709-709: Avoid TODO comments in API specs—use deprecation or external tracking.This TODO comment will likely leak into generated client SDKs and documentation, creating confusion for API consumers. Since the
invoicefield is still present and functional, consider one of these alternatives:
If deprecating soon: Use the
@deprecateddecorator with a message:`@deprecated`("This field will be removed in a future API version") `@visibility`(Lifecycle.Read) invoice: Invoice;If this is longer-term: Remove the comment from the spec and track it in your issue management system instead.
If V3 is a distinct API version: Consider creating version-specific response models when that work begins.
As per coding guidelines, the API should be easy to understand for users and multiple codegen toolchains depend on this spec.
openmeter/billing/service/stdinvoiceline.go (1)
60-63: Consider returning an empty result instead of nil.When
len(input.Lines) == 0, returning(nil, nil)means callers must handle a nil result. Returning an emptyCreatePendingInvoiceLinesResult(e.g., with an emptyLinesslice) might be more ergonomic and prevent potential nil pointer issues downstream.💡 Suggested change
return transcationForInvoiceManipulation(ctx, s, input.Customer, func(ctx context.Context) (*billing.CreatePendingInvoiceLinesResult, error) { if len(input.Lines) == 0 { - return nil, nil + return &billing.CreatePendingInvoiceLinesResult{ + Lines: []billing.UpcomingCharge{}, + }, nil }openmeter/billing/gatheringinvoice.go (1)
426-436: Default or validateline.Namespaceduring pending-line validation.
Line 426 only injects currency; if callers omit Namespace (common in tests),UpcomingChargeBase.Validatewill fail with “namespace required.” Consider defaulting fromCustomer.Namespaceand rejecting mismatches.♻️ Suggested tweak
for id, line := range c.Lines { // Note: this is for validation purposes, as Line is copied, we are not altering the struct itself line.Currency = c.Currency + if line.Namespace == "" { + line.Namespace = c.Customer.Namespace + } else if line.Namespace != c.Customer.Namespace { + errs = append(errs, fmt.Errorf("line.%d: namespace must match customer namespace", id)) + } if err := line.Validate(); err != nil { errs = append(errs, fmt.Errorf("line.%d: %w", id, err)) }openmeter/billing/service/gatheringinvoicependinglines.go (1)
610-639: Optional: validateGatheringInvoicein input.Validate for symmetry.
It would surface invalid/empty invoices earlier and make error sources clearer.♻️ Suggested validation addition
func (in createStandardInvoiceFromUpcomingChargesInput) Validate() error { var errs []error + if err := in.GatheringInvoice.Validate(); err != nil { + errs = append(errs, fmt.Errorf("gathering invoice: %w", err)) + } + if err := in.Customer.Validate(); err != nil { errs = append(errs, fmt.Errorf("customer: %w", err)) }openmeter/billing/httpdriver/gatheringinvoice.go (1)
17-60: Consider populating full customer details in the API invoice.
invoiceCustomeronly sets key + name right now, socustomerId(and possibly billing address / usage attribution) will be empty if clients expect parity with standard invoice responses. If that’s unintentional, wiring those fields fromcustomerwould avoid surprises.
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 (1)
openmeter/billing/gatheringinvoice.go (1)
413-449:⚠️ Potential issue | 🟠 MajorCurrency defaulting in validation doesn’t persist.
line.Currency = c.Currencyonly updates the loop copy, so the slice entries can still have empty currency. If callers rely on defaulting, this leaves invalid data downstream.🔧 Proposed fix
- for id, line := range c.Lines { - // Note: this is for validation purposes, as Line is copied, we are not altering the struct itself - line.Currency = c.Currency + for id := range c.Lines { + // Persist default currency on the actual slice element + c.Lines[id].Currency = c.Currency + line := c.Lines[id]
🤖 Fix all issues with AI agents
In `@openmeter/billing/adapter/gatheringinvoice.go`:
- Around line 141-145: The branch handling NextCollectionAt is inverted: instead
of calling updateQuery.SetCollectionAt when in.NextCollectionAt.IsZero() is
true, swap the branches so that when in.NextCollectionAt.IsZero() you call
updateQuery.ClearCollectionAt(), and when it's non-zero you call
updateQuery.SetCollectionAt(in.NextCollectionAt.In(time.UTC)); update the logic
around in.NextCollectionAt.IsZero(), keeping the same calls to SetCollectionAt
and ClearCollectionAt.
In `@openmeter/billing/adapter/gatheringlines.go`:
- Around line 232-242: The SubscriptionReference construction uses lo.FromPtr on
SubscriptionBillingPeriodFrom/To without nil checks which can produce zero
times; update the logic in gatheringlines.go where dbLine is mapped (the block
that sets line.Subscription and constructs billing.SubscriptionReference) to
validate that dbLine.SubscriptionBillingPeriodFrom and
SubscriptionBillingPeriodTo are non-nil before calling lo.FromPtr — if either is
nil, either skip setting Subscription.BillingPeriod (or set it to nil) or
surface/log an error/return so you don't silently create an invalid
ClosedPeriod; ensure you reference SubscriptionReference,
SubscriptionBillingPeriodFrom, SubscriptionBillingPeriodTo, and the mapping
block for the change.
In `@openmeter/billing/gatheringinvoice.go`:
- Around line 65-134: The Clone method currently does a shallow copy (clone :=
g) which leaves the Metadata map (on GatheringInvoice/GatheringInvoiceBase)
aliased between original and clone; update GatheringInvoice.Clone to deep-copy
the Metadata map (allocate a new map and copy each key/value) into the clone
before returning so mutations on the clone won't affect the original (this will
also fix WithoutDBState since it calls Clone); reference GatheringInvoice.Clone,
GatheringInvoice (and the Metadata field on GatheringInvoiceBase) when making
the change.
🧹 Nitpick comments (5)
openmeter/billing/gatheringinvoice.go (1)
24-63: Consider validatingCustomerID/Numberhere too (if required invariants).
If these fields are meant to be mandatory on aGatheringInvoiceBase, adding checks would keep model validation consistent and avoid partially‑initialized instances slipping through.openmeter/billing/stdinvoiceline.go (1)
189-221: Consider cloning metadata/annotations to avoid aliasing.
ToGatheringLineBasecurrently passes through the original maps, so later mutations on the gathering line could leak back into the StandardLine. A defensive copy keeps the conversion isolated.🔧 Suggested tweak
func (i StandardLine) ToGatheringLineBase() (GatheringLineBase, error) { if i.UsageBased == nil { return GatheringLineBase{}, errors.New("usage based line is required") } if i.UsageBased.Price == nil { return GatheringLineBase{}, errors.New("usage based line price is required") } + + metadata, err := models.Metadata(i.Metadata).Clone() + if err != nil { + return GatheringLineBase{}, fmt.Errorf("cloning metadata: %w", err) + } + + annotations, err := i.Annotations.Clone() + if err != nil { + return GatheringLineBase{}, fmt.Errorf("cloning annotations: %w", err) + } return GatheringLineBase{ ManagedResource: i.ManagedResource, - Metadata: i.Metadata, - Annotations: i.Annotations, + Metadata: metadata, + Annotations: annotations,test/billing/collection_test.go (1)
93-93: Minor inconsistency in price construction pattern.Here you're using
lo.FromPtr(productcatalog.NewPriceFrom(...))which is good. However, at lines 345 and 518, the code uses*productcatalog.NewPriceFrom(...)(direct dereference). Both work, butlo.FromPtris safer ifNewPriceFromcould ever return nil. Consider standardizing on one approach for consistency.📝 Suggested standardization
- Price: *productcatalog.NewPriceFrom(productcatalog.UnitPrice{Amount: alpacadecimal.NewFromFloat(1)}), + Price: lo.FromPtr(productcatalog.NewPriceFrom(productcatalog.UnitPrice{Amount: alpacadecimal.NewFromFloat(1)})),openmeter/billing/adapter/gatheringinvoice.go (1)
151-160: Hardcoded placeholder values for supplier fields.Setting
SupplierNameto "UNSET" andSupplierAddressCountryto "XX" as hardcoded placeholders works as a temporary hack, but these should ideally be documented or have a constant defined. The comments mention "Hack until we split the invoices table" which is helpful.Consider defining constants for these placeholder values to make them easier to find and update later:
const ( gatheringInvoicePlaceholderSupplierName = "UNSET" gatheringInvoicePlaceholderCountry = "XX" )openmeter/billing/service/stdinvoiceline.go (1)
24-24: Tracking the TODO.Good to see the TODO comment for future organization. Moving this to
gatheringinvoice.gowould help keep the gathering invoice logic together.Would you like me to open an issue to track moving this function to
gatheringinvoice.go?
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/httpdriver/invoiceline.go (1)
86-113:⚠️ Potential issue | 🟠 MajorAvoid post‑create failures that can cause duplicate pending lines.
CreatePendingInvoiceLinesis a non‑idempotent write. IfGetCustomerOverride(or subsequent mapping) fails afterward, the handler returns an error even though lines were persisted, so a client retry can duplicate data. Fetching the override before the write (or treating mapping failure as best‑effort) keeps the operation atomic from the caller’s view.🧭 One way to make the flow safer
- res, err := h.service.CreatePendingInvoiceLines(ctx, request) - if err != nil { - return CreatePendingLineResponse{}, fmt.Errorf("failed to create invoice lines: %w", err) - } - - if res == nil { - return CreatePendingLineResponse{}, fmt.Errorf("create pending invoice lines result is nil") - } - - out := CreatePendingLineResponse{ - IsInvoiceNew: res.IsInvoiceNew, - } - - mergedProfile, err := h.service.GetCustomerOverride(ctx, billing.GetCustomerOverrideInput{ + mergedProfile, err := h.service.GetCustomerOverride(ctx, billing.GetCustomerOverrideInput{ Customer: request.Customer, Expand: billing.CustomerOverrideExpand{ Customer: true, Apps: true, }, }) if err != nil { return CreatePendingLineResponse{}, fmt.Errorf("failed to get customer override: %w", err) } + + res, err := h.service.CreatePendingInvoiceLines(ctx, request) + if err != nil { + return CreatePendingLineResponse{}, fmt.Errorf("failed to create invoice lines: %w", err) + } + + if res == nil { + return CreatePendingLineResponse{}, fmt.Errorf("create pending invoice lines result is nil") + } + + out := CreatePendingLineResponse{ + IsInvoiceNew: res.IsInvoiceNew, + }
🤖 Fix all issues with AI agents
In `@openmeter/billing/gatheringinvoice.go`:
- Around line 280-331: Add a validation in GatheringLineBase.Validate to require
FeatureKey when the price is not flat: detect non‑flat prices using the same
predicate used in UsageBasedLine.Validate (e.g., Price.IsFlat() returning false
or the Price billing model check used there) and if the price is non‑flat ensure
i.FeatureKey is non‑nil/non‑empty, appending an error like "feature key is
required for non‑flat price" to errs; reference GatheringLineBase.Validate,
Price (and its IsFlat or billing model check) and the FeatureKey field to locate
where to add this guard.
In `@openmeter/billing/stdinvoiceline.go`:
- Around line 189-228: The ToGatheringLineBase method currently clones Metadata
and Annotations but returns TaxConfig, Subscription, and RateCardDiscounts by
reference which can cause later mutations to bleed back; update
ToGatheringLineBase to produce deep copies of i.TaxConfig, i.Subscription and
i.RateCardDiscounts (e.g., call existing Clone/DeepCopy helpers or create
shallow-copy constructors if no helpers exist) and use those copies in the
returned GatheringLineBase (handle nils and any clone errors similarly to
Annotations.Clone and propagate errors where appropriate).
🧹 Nitpick comments (1)
openmeter/billing/gatheringinvoice.go (1)
39-63: Consider validatingCustomerIDandNumberinGatheringInvoiceBase.Validate.
CreateGatheringInvoiceAdapterInput.Validatealready requires both, so mirroring the checks here keeps invariants consistent and prevents partially‑populated invoices from slipping through.🔧 Possible validation add‑on
if g.Namespace == "" { errs = append(errs, errors.New("namespace is required")) } + + if g.Number == "" { + errs = append(errs, errors.New("number is required")) + } + + if g.CustomerID == "" { + errs = append(errs, errors.New("customerID is required")) + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openmeter/billing/adapter/gatheringinvoice.go`:
- Around line 269-289: The validator must block attempts to update a record that
is not already a gathering invoice: in validateUpdateGatheringInvoiceRequest,
add a check that the existing invoice's type/status indicates it is a gathering
invoice (e.g., ensure existing.Type == billing.InvoiceTypeGathering or
existing.Status == billing.InvoiceStatusGathering depending on your model) and
return a billing.ValidationError (message like "cannot update non-gathering
invoice") if it is not; keep the existing Currency, Type, and CustomerID checks
but perform this new guard early using the validateUpdateGatheringInvoiceRequest
function and the existing variable.
2 participants
Overview
This PR is the first step of implementing Gathering Invoice and it's lines Gathering Lines. It changes the upcoming charge creation api to use the new type, while in the background we are still storing to the old database tables. Later changes will also change this behavior.
why? Right now we are using standard lines instead of Gathering Lines which is a validation nightmare: gathering lines have no totals, no quantities while std ones have. The only way to determine if a line is a std line or an upcoming charge is by the invoice.
Summary by CodeRabbit
New Features
Improvements
Tests