chore: move invoice pending lines to gathering types #3847
Conversation
📝 WalkthroughWalkthroughAdds adapter operations and domain helpers: a transactional hard-delete for gathering invoice lines (with cascading removal of usage-based configs) and a new API to fetch split-line-group headers; plus related gathering-line helpers, namespace-aware feature resolution, and test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Adapter as Adapter\n(HardDeleteGatheringInvoiceLines)
participant DB as Database
participant UBP as UBPConfigs\n(usage-based configs)
Client->>Adapter: HardDeleteGatheringInvoiceLines(invoiceID, lineIDs)
Adapter->>DB: Begin transaction
DB-->>Adapter: TX started
Adapter->>DB: Query invoice header by invoiceID
DB-->>Adapter: Invoice (status)
Adapter->>DB: Fetch lines by invoiceID & lineIDs (with usage-based relation)
DB-->>Adapter: Lines + UsageBasedLineIDs
Adapter->>DB: Delete invoice lines WHERE id IN lineIDs
DB-->>Adapter: Deleted lines count
Adapter->>UBP: Delete usage-based configs WHERE id IN UsageBasedLineIDs
UBP-->>Adapter: Deleted configs count
Adapter->>DB: Commit transaction
DB-->>Adapter: TX committed
Adapter-->>Client: Success / Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
chore/move-invoice-pending-lines-to-gathering-types
branch
2 times, most recently
from
0b9f917 to
f8cf5ea
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/service/gatheringinvoicependinglines.go (1)
854-883:⚠️ Potential issue | 🟠 MajorVerify detailed line persistence ordering before app upsert.
The code structure looks correct in theory: detailed lines are created during
calculateInvoice(triggered at line 889) and persisted at line 902 beforeadvanceUntilStateStableadvances to the sync phase. However, the failing test suggests detailed lines don't have IDs when the app'sUpsertStandardInvoiceruns.Worth checking:
- Does
updateInvoiceat line 902 actually persist the complete detailed line tree with assigned IDs?- When
advanceUntilStateStableeventually reachessyncDraftInvoice, what invoice data does the app receive—is it freshly loaded from the DB or in-memory state that may have lost DetailedLines?- The comment at lines 900–901 promises IDs are available for the app—if that's not holding true, there's a gap in either persistence or state handoff that needs clarifying.
Consider explicitly verifying that
updateInvoicepersists DetailedLines with IDs before the state machine advances, or add a refresh/reload step to confirm the data is durable before sync.
🤖 Fix all issues with AI agents
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 82-88: The current ListGatheringInvoices call in
gatheringinvoicependinglines.go only expands live lines
(billing.GatheringInvoiceExpandLines) which omits intentionally deleted lines;
update the billing.ListGatheringInvoicesInput passed to
s.adapter.ListGatheringInvoices so the Expand slice includes the deleted-lines
expand flag (e.g., add a GatheringInvoiceExpandDeletedLines or the equivalent
constant provided by the billing package) or conditionally include that expand
when IncludePendingLines is set, ensuring the returned existingGatheringInvoices
contains deleted lines so subsequent association/movement logic in this function
can consider them for audit/tracing.
- Around line 1016-1033: The code assigns line.Metadata directly into the new
billing.StandardLine (convertedLine) causing shared aliasing; change it to use a
cloned copy (like clonedAnnotations) instead of line.Metadata. Locate the
convertedLine construction where StandardLineBase.Metadata is set and replace
Metadata: line.Metadata with a cloned map (e.g., create a new map[string]string
and copy each key/value from line.Metadata, or call an existing clone helper) so
modifications to gathering lines won’t affect the new StandardLine; keep
clonedAnnotations usage as-is.
In `@openmeter/billing/worker/subscriptionsync/service/sync.go`:
- Around line 500-503: The current loop over hierarchy.Lines skips entries with
line.Line.DeletedAt != nil which causes missing annotations on soft-deleted
split lines; change the logic in the check that compares line.Line.Period.End to
servicePeriod.End so it either (a) does not filter out deleted lines when
calling s.lineHasAnnotation(line.Line, annotation), or (b) first tries active
lines and if none match falls back to checking deleted lines as well; update the
code paths referencing hierarchy.Lines, line.Line.Period.End, servicePeriod.End,
line.Line.DeletedAt and s.lineHasAnnotation to ensure deleted lines are
considered for annotation detection.
🧹 Nitpick comments (2)
openmeter/billing/service/gatheringinvoicependinglines.go (1)
1053-1068: Use a set for line-ID filtering to avoid O(n²) scans.
This is in a DB-heavy path and can be simplified + sped up with a map.As per coding guidelines, performance should be a priority in critical code paths, including database operations.♻️ Suggested refactor
lineIDsToRemove := lo.Map(linesToRemove, func(l billing.GatheringLine, _ int) string { return l.ID }) +lineIDSet := make(map[string]struct{}, len(lineIDsToRemove)) +for _, id := range lineIDsToRemove { + lineIDSet[id] = struct{}{} +} nrLinesRemoved := 0 invoiceLinesWithRemovedLines := lo.Filter(invoice.Lines.OrEmpty(), func(l billing.GatheringLine, _ int) bool { - if slices.Contains(lineIDsToRemove, l.ID) { + if _, ok := lineIDSet[l.ID]; ok { nrLinesRemoved++ return false }openmeter/billing/service/featuremeter.go (1)
33-33: Minor:lo.Uniqmay be redundant ifGetReferencedFeatureKeysalready deduplicates.Based on the AI summary,
StandardLines.GetReferencedFeatureKeys()collects unique feature keys. If that's the case, thislo.Uniqis a no-op. That said, it's cheap and acts as a safety net if future implementations oflinesFeatureGetterdon't deduplicate, so totally fine to keep.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 581-586: The warning message in the block checking
splitLine.PreSplitAtLine.DeletedAt != nil is misleading; update the
s.logger.WarnContext call in the same block (the one using "pre split line is
nil, we are not creating empty lines") to accurately state that the pre-split
line was deleted/soft-deleted (e.g., "pre-split line is soft-deleted, skipping
creation of empty lines") while keeping the existing context keys ("line",
line.Line.ID, "period_start", line.Line.ServicePeriod.From, "period_end",
line.Line.ServicePeriod.To).
In `@openmeter/billing/service/stdinvoicestate.go`:
- Around line 455-462: After a successful DB save via m.Service.updateInvoice
(the block guarding m.NeedsDBSave), reset the flag so subsequent loop iterations
don't perform redundant writes: set m.NeedsDBSave = false immediately after
assigning m.Invoice = updatedInvoice (and only on no-error path); ensure you
update the same receiver/struct field (m.NeedsDBSave) so future transitions that
do not call requireDBSave don't trigger an unnecessary update.
🧹 Nitpick comments (5)
openmeter/billing/service/stdinvoicestate.go (1)
850-866:allOfrunsrequireDBSaveeven when a prior action (e.g.,calculateInvoice) fails — worth a quick note.This is currently safe because
FireAndActivatepropagates the error and the loop inAdvanceUntilStateStableexits before reaching the DB save block. But it's a subtle invariant:NeedsDBSavecan betrueeven when the invoice is in an error state. If you reset the flag after save (per my comment above), this becomes a non-issue since the flag is ephemeral per iteration anyway.Just flagging for awareness — no action needed unless you want to make
allOfshort-circuit on error.openmeter/billing/service/gatheringinvoicependinglines.go (2)
858-894: Multiple fetches and cross-references after conversion — worth a brief sanity check.The flow here is:
- Convert gathering lines → standard lines (in-memory)
- Hard-delete gathering lines from DB
- Update + re-fetch the standard invoice (to get split-line hierarchies expanded)
- Re-resolve the new lines from the persisted invoice by matching IDs
Step 4 (lines 887-894) does an O(n×m) search with
slices.Containsinsidelo.Filter. For typical billing workloads this is fine, but if line counts grow significantly, consider using a set fornewLineIDs.
441-515: Conversion function validates correctly, but consider usingStandardLine.ToGatheringLineBase()instead.The function does require
UsageBasedto be non-nil (lines 447-448), but in practice, everyStandardLinecreated in the codebase sets this field — even flat-fee lines are created withUsageBasedcontaining aFlatPrice. The risk is theoretical rather than practical.Worth noting: this validation logic is already implemented in
StandardLine.ToGatheringLineBase()on line 191-198 ofstdinvoiceline.go. The current code duplicates that logic. Since the function is marked as temporary (TODO on line 442), it might make sense to call the existing method instead of reimplementing the conversion.test/billing/invoice_test.go (2)
2083-2085: Consider usingrequire.NotEmptyinstead ofs.Failfor early termination.
s.Fail(...)records the failure but continues execution, which means the callback will keep running and might produce confusing follow-on errors or even panic on the empty ID.require.NotEmpty(s.T(), detailedLine.ID)would stop the test immediately. Same applies to the discount ID check on lines 2090-2092.That said, this is inside a callback returning
(*result, error), so you could alsoreturn nil, fmt.Errorf(...)to propagate cleanly. Either way works — just flagging the subtle behavior difference.Suggested alternative
- if detailedLine.ID == "" { - s.Fail("detailed line id is empty") - } + require.NotEmpty(s.T(), detailedLine.ID, "detailed line id should not be empty")Also applies to: 2090-2092
2139-2148: Sames.Failvsrequire.NotEmptyconsideration as above.These guards in the "create end of period invoice" test callback have the same behavior:
s.Failmarks failure without stopping execution. Worth aligning with the approach used elsewhere in the test suite for consistency.
turip
force-pushed
the
chore/move-invoice-pending-lines-to-gathering-types
branch
from
a5686b5 to
67fd5f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@openmeter/billing/adapter/gatheringlines.go`:
- Around line 47-49: The error message in the gathering invoice validation uses
invoiceHeader.Currency which isn't selected and will be zero-valued; update the
fmt.Errorf call in the block that checks invoiceHeader.Status against
billing.StandardInvoiceStatusGathering to reference the actual validated field
(invoiceHeader.Status) instead of invoiceHeader.Currency (keep invoiceID.ID and
invoiceID.Namespace as-is), so the error accurately reflects the validated
status.
- Around line 62-64: The code maps existingLines to usageBasedLineConfigIDs by
accessing line.Edges.UsageBasedLine.ID which can panic if UsageBasedLine is nil;
update the mapping in the function that builds usageBasedLineConfigIDs to either
skip lines with nil line.Edges.UsageBasedLine (filtering them out) or detect any
nil and return a descriptive error, e.g., iterate existingLines, check if line
== nil or line.Edges == nil or line.Edges.UsageBasedLine == nil before reading
.ID, and handle accordingly; reference the variables/structs
usageBasedLineConfigIDs, existingLines, db.BillingInvoiceLine and
Edges.UsageBasedLine when applying the fix.
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 873-877: Fix the typo in the comment above the call to
s.resolveSplitLineGroupHeadersForLines: change "exapanded" to "expanded" in the
comment that references split line group headers and snapshotting (the comment
immediately preceding the call to resolveSplitLineGroupHeadersForLines with
convertResults.LinesAssociated).
- Around line 1109-1151: Add an early-return to avoid an unnecessary DB call
when no split group IDs exist: inside resolveSplitLineGroupHeadersForLines,
after computing splitLineGroupIDs (the lo.Uniq(...) result), check if
len(splitLineGroupIDs) == 0 and return nil immediately so you skip calling
s.adapter.GetSplitLineGroupHeaders; keep all existing error handling and
subsequent mapping logic unchanged.
In `@test/billing/adapter_test.go`:
- Line 633: Subtest title strings contain a double space; locate the s.Run calls
that use the title "Given a customer and billing profile exists" (the subtest
registration using s.Run) and remove the extra space so it reads "Given a
customer and billing profile exists", and apply the same correction to the other
s.Run occurrence with the same typo elsewhere in this test file.
🧹 Nitpick comments (7)
openmeter/billing/adapter/gatheringinvoice.go (1)
224-231: Consistent deleted-line filtering — nice.The same filtering pattern appears in both
ListGatheringInvoices(line 226) andGetGatheringInvoiceById(line 351). It's concise enough to keep inline, but if you ever add a third call site, consider extracting a small helper likeapplyDeletedLineFilter(q, expand)to keep things DRY.openmeter/billing/service/gatheringinvoicependinglines.go (1)
1064-1079: O(n*m) lookup inremoveLinesFromGatheringInvoice— fine for small sets, heads up for scale.
slices.Contains(lineIDsToRemove, l.ID)insidelo.Filteris a linear scan per line. If line counts stay small (typical for a single invoice), this is totally fine. If you ever expect larger sets, amap[string]struct{}would be the easy fix.Optional: use a set for O(1) lookups
- lineIDsToRemove := lo.Map(linesToRemove, func(l billing.GatheringLine, _ int) string { return l.ID }) + lineIDsToRemoveSet := lo.SliceToMap(linesToRemove, func(l billing.GatheringLine) (string, struct{}) { return l.ID, struct{}{} }) nrLinesRemoved := 0 invoiceLinesWithoutRemovedLines := lo.Filter(invoice.Lines.OrEmpty(), func(l billing.GatheringLine, _ int) bool { - if slices.Contains(lineIDsToRemove, l.ID) { + if _, ok := lineIDsToRemoveSet[l.ID]; ok { nrLinesRemoved++ return false } return true }) - if nrLinesRemoved != len(lineIDsToRemove) { - return fmt.Errorf("lines to remove[%d] must contain the same number of lines as line IDs to remove[%d]", nrLinesRemoved, len(lineIDsToRemove)) + if nrLinesRemoved != len(lineIDsToRemoveSet) { + return fmt.Errorf("lines to remove[%d] must contain the same number of lines as line IDs to remove[%d]", nrLinesRemoved, len(lineIDsToRemoveSet)) }openmeter/billing/invoicelinesplitgroup.go (1)
348-362: Consider validatingSplitLineGroupIDsis non-empty.The
Validate()method checksNamespacebut notSplitLineGroupIDs. Calling the adapter with an empty slice would produce a valid but pointless DB query. If callers are expected to guard this themselves, that's fine — just noting the asymmetry.Optional: add a check
func (i GetSplitLineGroupHeadersInput) Validate() error { var errs []error if i.Namespace == "" { errs = append(errs, errors.New("namespace is required")) } + + if len(i.SplitLineGroupIDs) == 0 { + errs = append(errs, errors.New("split line group IDs are required")) + } return errors.Join(errs...) }openmeter/billing/service/stdinvoicestate.go (1)
168-170: Wrapping single functions inallOfis unnecessary but harmless.
allOf(out.syncDraftInvoice)andallOf(out.finalizeInvoice)don't gain anything from theallOfwrapper since there's only one function. If the intent is to make it easy to add more hooks later (likerequireDBSave), that's fair — just a minor style nit.Also applies to: 229-231
test/billing/adapter_test.go (1)
743-861: Negative test cases look solid.Testing both the "wrong invoice type" and "line doesn't belong to invoice" rejection paths is great. One small suggestion: asserting on specific error content (e.g., checking if the error message contains a known substring) would make these tests more robust against false positives where any error satisfies
s.Error(err).openmeter/billing/adapter/gatheringlines.go (1)
75-78: Count mismatch check is good — but consider deduplicatinglineIDsinput.If the caller passes duplicate line IDs, the DB delete would only remove unique rows, causing the count check to fail with a confusing error. A
lo.Uniq(lineIDs)at the top would make this more robust.Proposed fix
if len(lineIDs) == 0 { return nil } + lineIDs = lo.Uniq(lineIDs)openmeter/billing/gatheringinvoice.go (1)
436-438:GetPrice()returns a pointer from a value receiver — intentional?Since
GatheringLineBaseis a value receiver,&i.Pricepoints to the copy's field. The caller gets a valid pointer, but it's disconnected from the original struct. If the intent is to avoid copyingPrice, consider using a pointer receiver. If it's just for interface compliance, this is fine as-is.
2 participants
Overview
This patch moves the standard invoice creation to the new gathering invoice API (and fixes an issue with the state machine that prevented line ID allocation in certian cases).
The new deletion function is essential, as this is how we can model the move.
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests