refactor: splitlinegroup gathering types #3851
Conversation
📝 WalkthroughWalkthroughIntroduces generic invoice/line abstractions, adds UpdateGatheringInvoice flow with invoicability checks and transactional persistence, renames adapter line-type enums to adapter-specific variants, makes clone/map APIs error-aware, and refactors subscription-sync, adapter, and service layers to handle standard vs gathering invoices and lines. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller
participant Sync as SubscriptionSync
participant Svc as BillingService
participant Adapter as BillingAdapter
participant DB as Database
Client->>Sync: ApplyPatches(linePatches)
Sync->>Sync: RouteByInvoiceType(gathering / standard / immutable)
alt Gathering
Sync->>Svc: UpdateGatheringInvoice(ctx, input)
Svc->>Adapter: GetGatheringInvoiceById(ctx, id)
Adapter->>DB: Query invoice + lines (+workflow config)
DB-->>Adapter: rows
Adapter-->>Svc: GatheringInvoice
Svc->>Svc: EditFn(invoice) → Normalize → Validate/Invoicability
Svc->>Adapter: UpsertGatheringInvoice(ctx, invoice)
Adapter->>DB: Persist invoice + lines
DB-->>Adapter: OK
Adapter-->>Svc: OK
Svc-->>Sync: OK
else Standard
Sync->>Svc: updateMutableStandardInvoice(ctx, invoice, patches)
Svc->>Adapter: UpsertStandardInvoice(ctx, invoice)
Adapter->>DB: Persist
DB-->>Adapter: OK
Adapter-->>Svc: OK
Svc-->>Sync: OK
end
Sync-->>Client: Done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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/splitlinegroup-gathering-types
branch
4 times, most recently
from
05b9cc0 to
8813eec
Compare
|
Still wip, moving to ready for review to see the rabbit results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openmeter/billing/service/stdinvoicestate.go (1)
691-718:⚠️ Potential issue | 🟠 MajorFinalize uses a stale clone; issued/due timestamps won’t reach the app.
FinalizeStandardInvoicestill gets the original clone created before IssuedAt/DueAt/external IDs are updated, so downstream apps won’t see the latest state. Re-clone (or update the clone) after the mutations.💡 Suggested fix
- results, err := app.FinalizeStandardInvoice(ctx, clonedInvoice) + finalizedInvoice, err := m.Invoice.Clone() + if err != nil { + return nil, err + } + results, err := app.FinalizeStandardInvoice(ctx, finalizedInvoice)openmeter/billing/invoicelinesplitgroup.go (1)
250-276:⚠️ Potential issue | 🟠 Major
SumNetAmountsilently discards errors fromForEachChild.Line 253 assigns the error to
_, but the callback on line 260 can return a real error fromAsStandardLine(). If that ever fails, you get a silently incorrect sum with no indication anything went wrong.Consider returning
(alpacadecimal.Decimal, error)or at least logging the error.Proposed fix
-func (h *SplitLineHierarchy) SumNetAmount(in SumNetAmountInput) alpacadecimal.Decimal { +func (h *SplitLineHierarchy) SumNetAmount(in SumNetAmountInput) (alpacadecimal.Decimal, error) { netAmount := alpacadecimal.Zero - _ = h.ForEachChild(ForEachChildInput{ + err := h.ForEachChild(ForEachChildInput{ PeriodEndLTE: in.PeriodEndLTE, Callback: func(child LineWithInvoiceHeader) error { if child.Invoice.AsInvoice().Type() != InvoiceTypeStandard { return nil } stdLine, err := child.Line.AsInvoiceLine().AsStandardLine() if err != nil { return err } netAmount = netAmount.Add(stdLine.Totals.Amount) if in.IncludeCharges { netAmount = netAmount.Add(stdLine.Totals.ChargesTotal) } return nil }, }) + if err != nil { + return alpacadecimal.Zero, fmt.Errorf("iterating children: %w", err) + } - return netAmount + return netAmount, nil }
🤖 Fix all issues with AI agents
In `@openmeter/billing/adapter/invoicelinesplitgroup.go`:
- Around line 225-234: The code in mapSplitLineHierarchyLinesFromDB reads
dbLine.Edges.BillingInvoice.Status without checking that
dbLine.Edges.BillingInvoice is non-nil; add a nil guard in
mapSplitLineHierarchyLinesFromDB (inside the MapWithErr lambda) to check dbLine
!= nil and dbLine.Edges.BillingInvoice != nil and return a clear error if
missing (e.g., "missing BillingInvoice edge on BillingInvoiceLine"); this makes
mapSplitLineHierarchyLinesFromDB robust even if callers forget to preload the
BillingInvoice edge and points to the failing record for easier debugging.
In `@openmeter/billing/invoice.go`:
- Around line 164-173: Invoice.Validate currently dereferences i.standardInvoice
/ i.gatheringInvoice and can panic if those pointers are nil; update
Invoice.Validate to use the existing nil-safe accessors (AsStandardInvoice and
AsGatheringInvoice) instead of direct field access: call i.AsStandardInvoice()
when i.t == InvoiceTypeStandard and if it returns the standard invoice, run its
Validate(), otherwise return the accessor error; do the same for
InvoiceTypeGathering with i.AsGatheringInvoice(); keep the default case
returning fmt.Errorf("invalid invoice type: %s", i.t).
- Around line 24-26: Billing's InvoiceType values include "credit-note" but the
API expects "credit_note", and simply casting invoice.Type to api.InvoiceType
(in openmeter/billing/httpdriver/invoice.go) preserves the hyphen; add a small
conversion function (e.g., mapBillingInvoiceTypeToAPI or convertInvoiceType)
that switches known billing values (InvoiceTypeStandard, InvoiceTypeCreditNote,
InvoiceTypeGathering) to the API strings (replace hyphen with underscore for
credit note) and use that function when building the response (replace the
direct cast Type: api.InvoiceType(invoice.Type) with Type:
mapBillingInvoiceTypeToAPI(invoice.Type)).
In `@openmeter/billing/invoiceline.go`:
- Around line 175-184: The Validate() method can dereference nil pointers on
i.standardLine or i.gatheringLine; update InvoiceLine.Validate() to guard before
calling Validate(): for InvoiceLineTypeStandard, check i.standardLine != nil (or
use the existing AsStandardLine() guard) and return a clear error if nil,
otherwise call i.standardLine.Validate(); do the same for
InvoiceLineTypeGathering with i.gatheringLine (or AsGatheringLine()); ensure the
default case still returns fmt.Errorf("invalid invoice line type: %s", i.t).
- Around line 214-231: InvoiceLine.AsGenericLine currently returns a value for
standardInvoiceLineGenericWrapper but a pointer for
gatheringInvoiceLineGenericWrapper, causing inconsistency; update the standard
branch in AsGenericLine to return a pointer instead (use
&standardInvoiceLineGenericWrapper{StandardLine: i.standardLine}) so both
branches return the same type (pointer) that implements GenericInvoiceLine;
reference InvoiceLine.AsGenericLine, standardInvoiceLineGenericWrapper,
gatheringInvoiceLineGenericWrapper, and GenericInvoiceLine to locate and make
the change.
In `@openmeter/billing/stdinvoiceline.go`:
- Around line 399-405: The SetPrice method on StandardLine silently no-ops when
UsageBased is nil; change it to either initialize UsageBased before assigning
(e.g., allocate a UsageBased struct and then set i.UsageBased.Price =
price.Clone()) or change the API to return an error so callers are notified
(update GenericInvoiceLine.SetPrice and callers accordingly), and ensure
behavior aligns with GatheringLineBase.SetPrice by documenting or enforcing the
invariant that UsageBased is non-nil before setting the price; reference
SetPrice, StandardLine, UsageBased, productcatalog.Price,
GatheringLineBase.SetPrice, and GenericInvoiceLine.SetPrice when making the
change.
🧹 Nitpick comments (12)
openmeter/billing/adapter/stdinvoicelinemapper.go (1)
114-116: Type constant updated toInvoiceLineAdapterTypeUsageBased— minor note on the return pattern.The constant rename aligns with the broader refactor. One thing worth noting (pre-existing, not introduced here): line 115 returns
(invoiceLine, error)— a partially-constructed object alongside an error. The caller discards it, so it's harmless, but returningnilwith the error would be more idiomatic Go.Optional: return nil instead of partial object on error
if dbLine.Type != billing.InvoiceLineAdapterTypeUsageBased { - return invoiceLine, fmt.Errorf("only usage based lines can be top level lines [line_id=%s]", dbLine.ID) + return nil, fmt.Errorf("only usage based lines can be top level lines [line_id=%s]", dbLine.ID) }openmeter/billing/gatheringinvoice_test.go (1)
11-32: Solid test for theInvoiceAtAccessorinterface!Covers both direct wrapper access and the interface-asserted path, plus the dual-interface check (
GenericInvoiceLine→InvoiceAtAccessor). One optional thought: sinceGatheringLineis stored by value in the wrapper, you could add a quick assertion that the originalline.InvoiceAtis not modified afterwrapper.SetInvoiceAt(target)— that would document the value-copy semantics and guard against accidental changes to pointer semantics later.openmeter/billing/worker/subscriptionsync/service/feehelper.go (1)
35-39: Stale error message after refactor.Now that we're using
GetPrice()on a generic line interface, the message"line misses usage based metadata"doesn't match the actual check (price is nil). Same issue on line 56. A quick update would help future debugging.✏️ Suggested wording
- return alpacadecimal.Zero, fmt.Errorf("line misses usage based metadata") + return alpacadecimal.Zero, fmt.Errorf("line has no price set")And similarly for
setFlatFeePerUnitAmount:- return fmt.Errorf("line misses usage based metadata") + return fmt.Errorf("line has no price set")openmeter/billing/stdinvoice.go (2)
270-283: Looks good! Clean accessor methods for theGenericInvoiceReaderinterface.One thing to note:
GetInvoiceID()(Line 278) does the same thing as the existingInvoiceID()method onStandardInvoice(Line 322). Might be worth having one delegate to the other to keep things DRY — though since one lives onStandardInvoiceBaseand the other onStandardInvoice, the duplication is understandable.
480-491: On error,MapWithErrreturns the original receivercinstead of a zero value.Line 487 returns
c, erron failure. This means callers get back the original lines alongside the error. It's a bit inconsistent with the pattern inClone()(Line 499) andRemoveMetaForCompare()(Line 375), where a zero value is returned on error. If a caller ever accidentally ignores the error, they'd still have the original data — which might be fine, but could also be surprising.Not a blocker, just something to keep in mind for consistency.
openmeter/billing/adapter/stdinvoicelines.go (1)
840-852: Variable shadowing:dbGatheringLinesis reused as a loop variable.On Line 840, the
rangevariabledbGatheringLinesshadows the outerdbGatheringLinesdeclared on Line 831. This compiles fine and works correctly, but it makes the code harder to read. Consider renaming the loop variable to something likeinvoiceLinesorlinesInInvoice.♻️ Suggested rename
- for invoiceID, dbGatheringLines := range dbGatheringLinesByInvoiceID { + for invoiceID, dbLinesForInvoice := range dbGatheringLinesByInvoiceID { schemaLevel, found := invoiceSchemaLevelByID[invoiceID] if !found { return nil, fmt.Errorf("schema level not found for invoice [id=%s]", invoiceID) } - mappedLines, err := tx.mapGatheringInvoiceLinesFromDB(schemaLevel, dbGatheringLines) + mappedLines, err := tx.mapGatheringInvoiceLinesFromDB(schemaLevel, dbLinesForInvoice) if err != nil { return nil, fmt.Errorf("mapping gathering lines: %w", err) }openmeter/billing/worker/subscriptionsync/service/invoiceupdate.go (1)
193-220: Two-step conversionAsInvoiceLine().AsStandardLine()to get the standard line for mutable updates.This works but feels a bit roundabout. The
GenericInvoiceLineis first converted back to anInvoiceLine(sum type), then unwrapped to aStandardLine. The error handling is good though — if the line isn't actually a standard line, it errors appropriately with a clear message.openmeter/billing/gatheringinvoice.go (2)
587-635: Several accessors onGatheringLineshadow promoted methods fromGatheringLineBase.
GetID(),GetInvoiceAt(), andGetChildUniqueReferenceID()are already available through the embeddedGatheringLineBase. The re-declarations are harmless and arguably make the intent clearer, but they do add surface area to maintain. Just a heads-up — totally fine to keep them if you prefer the explicitness.
275-290:WithNormalizedValuescould reuseMapWithErrfromGatheringInvoiceLinesinstead of callingslicesx.MapWithErrdirectly.Lines 260-273 already define
MapWithErron the type, which handles theIsAbsent()check and wraps the result. This new method repeats that same pattern.Proposed simplification
func (l GatheringInvoiceLines) WithNormalizedValues() (GatheringInvoiceLines, error) { - if l.IsAbsent() { - return l, nil - } - - out, err := slicesx.MapWithErr(l.OrEmpty(), func(l GatheringLine) (GatheringLine, error) { + return l.MapWithErr(func(l GatheringLine) (GatheringLine, error) { return l.WithNormalizedValues() }) - if err != nil { - return GatheringInvoiceLines{}, err - } - - return GatheringInvoiceLines{ - Option: mo.Some(GatheringLines(out)), - }, nil }openmeter/billing/worker/subscriptionsync/service/sync.go (2)
641-712:lineFromSubscritionRateCardcleanly converts toGatheringLine.The construction of
GatheringLine/GatheringLineBaseis straightforward and all fields are mapped correctly. TheServicePeriodconversion viaToClosedPeriod()at line 653 properly bridges the two period types.Tiny pre-existing nit:
lineFromSubscritionRateCardhas a typo — missing 'p' in "Subscription". If you're touching it anyway, might be worth a rename tolineFromSubscriptionRateCard. No pressure though.
835-848: TheInvoiceAtAccessortype assertion +SetInvoiceAtpattern is repeated 3 times.This same block appears at lines 839–848, 939–945, and 983–988. Extracting a small helper would reduce noise and make the intent clearer.
Possible helper
func setInvoiceAtIfGathering(line billing.GenericInvoiceLine, invoiceAt time.Time, invoiceByID InvoiceByID) error { if !invoiceByID.IsGatheringInvoice(line.GetInvoiceID()) { return nil } invoiceAtAccessor, ok := line.(billing.InvoiceAtAccessor) if !ok { return fmt.Errorf("line is not an invoice at accessor: %T", line) } invoiceAtAccessor.SetInvoiceAt(invoiceAt) return nil }Then each call site becomes:
if err := setInvoiceAtIfGathering(targetLine, expectedLine.InvoiceAt, invoiceByID); err != nil { return nil, err }openmeter/billing/worker/subscriptionsync/service/sync_test.go (1)
3164-3165: Inconsistent error handling forClone()across test helpers.Some
EditFncallbacks use expliciterr+s.NoError(err)(here, lines 3164–3165 and 3245–3246), while others uselo.Must(line.Clone())(lines 3336, 3449, 3706). Both work fine in tests, but picking one pattern would improve readability.Minor note: if
Clone()fails at line 3164,s.NoError(err)marks the test as failed, but theEditFnstill returnsnil, soUpdateInvoiceproceeds with a potentially nilupdatedLine— which could cause a confusing nil-pointer panic downstream. Usinglo.Mustor returning the error fromEditFnwould be a bit cleaner:Option A: return the error
- updatedLine, err = line.Clone() - s.NoError(err) + var cloneErr error + updatedLine, cloneErr = line.Clone() + if cloneErr != nil { + return cloneErr + } return nilOption B: use lo.Must consistently
- updatedLine, err = line.Clone() - s.NoError(err) + updatedLine = lo.Must(line.Clone()) return nilAlso applies to: 3245-3246
turip
force-pushed
the
refactor/splitlinegroup-gathering-types
branch
from
08921d0 to
65fdeaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@openmeter/billing/adapter/stdinvoicelines.go`:
- Around line 820-852: The filter and grouping closures access
line.Edges.BillingInvoice.Status and .ID without nil checks which can panic if
the edge is nil; update the dbStandardLines and dbGatheringLines filters and the
GroupBy key function to first check that line != nil and line.Edges != nil and
line.Edges.BillingInvoice != nil, and then either skip the entry (return false /
empty key) or return a clear error (e.g., "orphaned invoice line [id=...]")
before calling tx.mapStandardInvoiceLinesFromDB and
tx.mapGatheringInvoiceLinesFromDB; ensure you reference the existing variables
and methods (dbStandardLines, dbGatheringLines, dbGatheringLinesByInvoiceID,
invoiceSchemaLevelByID, mapStandardInvoiceLinesFromDB,
mapGatheringInvoiceLinesFromDB, WithBillingInvoice) when making the changes.
In `@openmeter/billing/invoicelinesplitgroup.go`:
- Around line 198-207: NewLineWithInvoiceHeader currently returns a
value-wrapped standardInvoiceLineGenericWrapper but a pointer-wrapped
gatheringInvoiceLineGenericWrapper, which is inconsistent with AsGenericLine
(which returns pointer wrappers) and NewLineOrHierarchy; change
NewLineWithInvoiceHeader to construct and return pointers for both wrappers (use
&standardInvoiceLineGenericWrapper{...} and
&gatheringInvoiceLineGenericWrapper{...}) and then update the corresponding
Clone methods on standardInvoiceLineGenericWrapper and
gatheringInvoiceLineGenericWrapper so they both return the same pointer/value
style (preferably pointer to match AsGenericLine), and ensure NewLineOrHierarchy
and any callers (e.g., AsGenericLine, Clone) are consistent with this
pointer-based wrapper convention.
In `@openmeter/ent/schema/billing.go`:
- Around line 364-366: The schema's field.Enum("type") using
billing.InvoiceLineAdapterType("") must match the actual stored strings
("flat_fee" and "usage_based") in the DB; currently the codebase/DB used "fee"
vs "flat_fee" mismatch will cause existing rows to break. Update the enum
mapping so billing.InvoiceLineAdapterType uses the exact adapter strings
("flat_fee", "usage_based") (or update the InvoiceLineAdapterType definition to
return those values), and ensure the migration that renames/converts legacy
"fee" rows to "flat_fee" (the migrations
20250605131637_migrate-flat-fees-to-ubp-flat-fees.up.sql and
20250731141420_billing-migrate-flat-fee-lines.up.sql) has run before deploying
this schema change; if not present, add a migration to convert any "fee" or
legacy InvoiceLineType values to "flat_fee" and verify existing records align
with field.Enum("type") and billing.InvoiceLineAdapterType prior to deployment.
🧹 Nitpick comments (10)
openmeter/billing/adapter/stdinvoicelinediff_test.go (1)
154-158: Minor typos in test name and comment (pre-existing).Line 154:
"hieararchy"→"hierarchy"and Line 158:"tirgger"→"trigger". These look pre-existing so no pressure, but easy to fix if you're already touching this file.✏️ Typo fixes
- t.Run("a line is updated in the existing line hieararchy", func(t *testing.T) { + t.Run("a line is updated in the existing line hierarchy", func(t *testing.T) { base := cloneLines(template) snapshotAsDBState(t, base) - // ID change should tirgger a delete/update + // ID change should trigger a delete/updateopenmeter/billing/service/service.go (1)
156-162: Looks good — clean convenience wrapper.The delegation pattern is straightforward. One small note: the pre-existing typo
transcation(missing thes→ should betransaction) carries over here. Not introduced by this PR, but worth a quick rename if you're touching these signatures anyway.openmeter/billing/invoice.go (1)
117-132: Unreachable return inNewInvoice— required by the compiler but worth a comment.Line 131 (
return Invoice{}) can never execute given the type constraint[T StandardInvoice | GatheringInvoice], but the compiler needs it. A small// unreachablecomment would help future readers understand it's intentional.openmeter/billing/invoiceline.go (1)
222-239: Both branches now return pointers — previously flagged inconsistency resolved.However, there's a subtle semantic difference worth being aware of: the standard wrapper holds a
*StandardLine(pointer — mutations propagate to original), while the gathering wrapper holds aGatheringLinevalue copy (mutations are isolated). This seems intentional given how these types are used elsewhere, but it meansAsGenericLine()has different mutation semantics depending on the line type.openmeter/billing/worker/subscriptionsync/service/invoiceupdate.go (1)
338-359: Immutable path assumes all updated lines are standard lines.Lines 339-342 convert
targetStateto a standard line viaAsInvoiceLine().AsStandardLine(). If a gathering line somehow ends up in the immutable path, this would error out. This seems correct given that immutable invoices should only be standard invoices, but there's no explicit guard at the top ofupdateImmutableInvoiceasserting this invariant.openmeter/billing/worker/subscriptionsync/service/sync.go (2)
826-848:InvoiceAtAccessortype assertion pattern for gathering invoices.Lines 840-845 do a runtime type assertion to check if the cloned line supports
SetInvoiceAt. This is a consequence ofInvoiceAtAccessorbeing a separate optional interface. The pattern is repeated in the hierarchy paths too (Lines 940-943, 984-987).This works, but it's a bit fragile — if a new line type is added that doesn't implement
InvoiceAtAccessor, this will error at runtime rather than compile time. Since this is a WIP refactor, this might be fine for now. Just flagging it as something to keep an eye on.
898-956: Cross-type period comparisons:Period.EndvsClosedPeriod.To.Throughout this method,
existingHierarchy.Group.ServicePeriod(typePeriodwith.Start/.End) is compared againstexpectedLine.ServicePeriod(typeClosedPeriodwith.From/.To). For example:
- Line 905:
...ServicePeriod.End.Equal(expectedLine.ServicePeriod.To)- Line 913:
...ServicePeriod.End.Before(expectedLine.ServicePeriod.To)- Line 952:
...ServicePeriod.End = expectedLine.ServicePeriod.ToThese are semantically correct, but the mixed field naming (
.Endvs.To) across the two period types adds cognitive overhead. This is an existing design choice in the codebase (not introduced by this PR), so just a note for awareness.openmeter/billing/stdinvoiceline.go (3)
241-248: Minor: consider a helper for theClosedPeriod → Periodback-conversion.
ToClosedPeriod()handles the forward direction, but the reverse (lines 244-247) is done inline. If this pattern repeats elsewhere, aPeriodFromClosedPeriodconstructor could reduce duplication. Totally fine as-is for now though.
805-821:GetReferencedFeatureKeyserror could be more informative.When
UsageBasedis nil (line 810), the error doesn't indicate which line is the culprit. Adding the index or line ID would help debugging:Optional improvement
- for _, line := range c { + for idx, line := range c { if line.UsageBased == nil { - return nil, fmt.Errorf("usage based line is required") + return nil, fmt.Errorf("line[%d]: usage based line is required", idx) }
265-304:TaxConfigandSubscriptionare shared by pointer, not cloned.
MetadataandAnnotationsare carefully cloned (lines 276-281), butTaxConfig(line 297) andSubscription(line 300) are pointer fields that get copied by reference. If the resultingGatheringLineBaseor the originalStandardLinemutates either, the other will see the change.Consider cloning them for consistency:
Suggested fix
- TaxConfig: i.TaxConfig, - RateCardDiscounts: i.RateCardDiscounts, + TaxConfig: lo.Ternary(i.TaxConfig != nil, lo.ToPtr(*i.TaxConfig), nil), + RateCardDiscounts: i.RateCardDiscounts.Clone(), ChildUniqueReferenceID: i.ChildUniqueReferenceID, - Subscription: i.Subscription, + Subscription: lo.Ternary(i.Subscription != nil, i.Subscription.Clone(), nil),
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@openmeter/billing/invoicelinesplitgroup.go`:
- Around line 389-408: ChildUniqueReferenceID and ServicePeriod assume i.line is
non-nil when i.t == LineOrHierarchyTypeLine and can panic; add the same nil
guard used by AsGenericLine: if i.t == LineOrHierarchyTypeLine then check if
i.line == nil and return nil (for ChildUniqueReferenceID) or return
timeutil.ClosedPeriod{} (for ServicePeriod) before calling
i.line.GetChildUniqueReferenceID() / i.line.GetServicePeriod(); keep the
existing behavior for LineOrHierarchyTypeHierarchy unchanged.
- Around line 250-279: Update the SumNetAmount docstring on SplitLineHierarchy
to state that non-standard (gathering) invoice lines are intentionally filtered
out (see the Invoice.AsInvoice().Type() check against InvoiceTypeStandard in
SumNetAmount and the ForEachChild callback) because gathering lines represent
unfinalized amounts and must not be included in minimum/net amount calculations;
mirror the wording/rationale used in discountusage.go (comment about gathering
lines not holding usage discounts) so future readers understand why the check
exists and that IncludeCharges only applies to standard lines.
In `@openmeter/billing/service/gatheringinvoice.go`:
- Around line 162-163: Update the inline comment that references the old
misspelled function name: change "TranscationForGatheringInvoiceManipulation" to
the corrected "TransactionForGatheringInvoiceManipulation" in
gatheringinvoice.go near the block that auto-deletes invoices with no lines (the
comment above the Auto delete the invoice... line) so the comment matches the
renamed function.
🧹 Nitpick comments (6)
openmeter/billing/service/lock.go (1)
15-17: Tiny nit: the wrapping closure is redundantSince
fnalready matches thefunc(ctx context.Context) errorsignature, you could pass it directly instead of wrapping it. Totally minor though — feel free to ignore!✨ Optional simplification
- err := transactionForInvoiceManipulationNoValue(ctx, s, customerID, func(ctx context.Context) error { - return fn(ctx) - }) + err := transactionForInvoiceManipulationNoValue(ctx, s, customerID, fn)openmeter/billing/adapter/stdinvoicelinediff_test.go (1)
363-367:lo.Mustfor cloning in tests — totally fine, small thought though.Using
lo.Mustmeans a clone failure panics with a stack trace rather than a clean test failure message. It's perfectly acceptable in test helpers, but if you ever want slightly friendlier output, you could mirror thesnapshotAsDBStatepattern (acceptt *testing.Tand userequire.NoError). Totally optional — the current approach is concise and clear.Optional: slightly friendlier test failures
-func cloneLines(lines []*billing.StandardLine) []*billing.StandardLine { +func cloneLines(t *testing.T, lines []*billing.StandardLine) []*billing.StandardLine { + t.Helper() return lo.Map(lines, func(line *billing.StandardLine, _ int) *billing.StandardLine { - return lo.Must(line.Clone()) + cloned, err := line.Clone() + require.NoError(t, err) + return cloned }) }This would require updating all call sites to pass
t, similar to what was done forsnapshotAsDBState.openmeter/billing/service/gatheringinvoice.go (1)
178-206: Mixed error types inerrors.Join— might make unwrapping tricky.
checkIfGatheringLinesAreInvoicablereturns a mix of plainfmt.Errorfwrapped errors andbilling.ValidationErrorvalues viaerrors.Join. When a caller tries toerrors.Asorerrors.Isagainstbilling.ValidationError, the joined error will match if any contained error is aValidationError, but it won't distinguish which lines had validation failures vs. which had unexpected errors (like fromResolveBillablePeriod).If that's intentional and callers just care about "did anything fail," this is fine. But if you ever need to separate validation errors from unexpected errors, you might want to split them or wrap the whole thing in a single
ValidationError. Just something to keep in mind.openmeter/billing/adapter/stdinvoicelines.go (2)
833-865: Gathering lines: iteration order overdbGatheringLinesByInvoiceIDmap is non-deterministic.The
for invoiceID, dbGatheringLinesForInvoice := range dbGatheringLinesByInvoiceIDloop iterates over a map, which means the order ofgatheringLinesin the output slice is non-deterministic across runs. If downstream consumers depend on stable ordering (e.g., for diffing, snapshotting, or deterministic test assertions), this could cause subtle flakiness.If ordering doesn't matter here, a brief comment noting that would help future readers. Otherwise, consider sorting the invoice IDs first.
929-941: Output assembly covers all three categories — groups, standard lines, and gathering lines.Capacity calculation on line 929 includes all three, which is correct. The mapping lambdas are straightforward.
One small nit:
standardLinesare[]*billing.StandardLine(pointer) whilegatheringLinesare[]billing.GatheringLine(value). TheNewLineOrHierarchygeneric constraint[T *StandardLine | GatheringLine | *SplitLineHierarchy]handles this, but the pointer-vs-value asymmetry between the two line types is a bit of a mental speed bump for readers. Might be worth a brief comment or TODO if unifying this is on the roadmap.openmeter/billing/invoicelinesplitgroup.go (1)
183-207: The generic constructorNewLineWithInvoiceHeaderlooks clean.The type switch on the generic constraint is a nice pattern. Both wrappers now use pointer construction (
&standardInvoiceLineGenericWrapper{...}and&gatheringInvoiceLineGenericWrapper{...}) — consistent here. 👍One thing: lines 204-206 — the default branch returns a zero-value
LineWithInvoiceHeader{}. Since the generic constraint is exhaustive (StandardLineWithInvoiceHeader | GatheringLineWithInvoiceHeader), this path is unreachable. It's fine as a safety net, but apanic("unreachable")would make intent clearer and catch future constraint changes.Suggested tweak
case GatheringLineWithInvoiceHeader: return LineWithInvoiceHeader{Line: &gatheringInvoiceLineGenericWrapper{GatheringLine: v.Line}, Invoice: v.Invoice} } - return LineWithInvoiceHeader{} + panic("unreachable: unhandled LineWithInvoiceHeader type")
2 participants
Overview
The major thing this PR does is it replaces the SplitLineGroupHierarchy's way of addressing invoice lines with header from this:
To this:
This has numerous implications:
GenericInvoice* vs InvoiceLine/Invoice
The system uses both union types and interface definitions. The duality is a safety measure:
This makes sure that we don't have to do type assertions at random places like
gathering, ok := (GenericLine)(l).(GatheringLine). Instead this is performed at a single location where we can unit test the conversion itself.Adapter changes
The adapter of splitlinegroup has been fully refactored to support gathering and standard lines independently.
Notes for reviewer
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests