feat: refactor getInvoiceByID#3863
Conversation
📝 WalkthroughWalkthroughSplit invoice handling into explicit standard vs gathering flows, introduce typed expand enums and Expand[T] helpers, add split-line hierarchy handling in mappers, and rename/add adapter/service methods (e.g., GetStandardInvoiceById, ListStandardInvoices) with corresponding input/expand type updates and getter renames (InvoiceID → GetInvoiceID). Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/HTTP
participant Service as BillingService
participant Adapter as Adapter/DB
rect rgba(200,200,255,0.5)
Client->>Service: GetInvoiceById(GetInvoiceByIdInput)
Service->>Adapter: GetInvoiceType(invoiceID)
Adapter-->>Service: InvoiceType (Standard|Gathering)
alt type == Standard
Service->>Service: validate expands
Service->>Adapter: GetStandardInvoiceById(GetStandardInvoiceByIdInput)
Adapter-->>Service: StandardInvoice
Service->>Service: resolveWorkflowApps & statusDetails
Service-->>Client: Standard Invoice (with expands)
else type == Gathering
Service->>Adapter: GetGatheringInvoiceById(GetGatheringInvoiceByIdInput)
Adapter-->>Service: GatheringInvoice
alt expand includes calculate live-data
Service->>Service: calculateGatheringInvoiceAsStandardInvoice
Service-->>Client: Calculated Standard Invoice
else
Service-->>Client: Gathering Invoice
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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/get-invoice-by-id
branch
4 times, most recently
from
8896621 to
ae687ab
Compare
turip
force-pushed
the
refactor/get-invoice-by-id
branch
from
ae687ab to
1e98e0d
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openmeter/billing/worker/collect/collect.go (1)
165-177:⚠️ Potential issue | 🟠 MajorPre-existing data race:
erris shared across goroutines.This isn't introduced by your PR, but since you're touching this file — line 168 uses
_, err = ...(assignment, not declaration), so all goroutines in the batch write to the same outererrvariable concurrently. This is a race condition.A quick fix would be to use
:=for a localerrand send it directly toerrChan:🔒 Suggested fix
go func() { defer wg.Done() - _, err = a.CollectCustomerInvoice(ctx, CollectCustomerInvoiceInput{ + _, err := a.CollectCustomerInvoice(ctx, CollectCustomerInvoiceInput{ CustomerID: customerID, AsOf: time.Now(), })openmeter/billing/gatheringinvoice.go (1)
585-599:⚠️ Potential issue | 🟠 MajorPreserve
SplitLineHierarchyinGatheringLine.Clone.
Clonecurrently drops the newSplitLineHierarchyfield, so callers usingClone/WithoutDBState/WithNormalizedValueswill silently lose it. Please carry it through (or deep‑clone if needed).🛠️ Suggested patch
return GatheringLine{ GatheringLineBase: base, DBState: g.DBState, + SplitLineHierarchy: g.SplitLineHierarchy, }openmeter/billing/service/invoice.go (1)
188-229:⚠️ Potential issue | 🟡 MinorDeleted lines never surface in live-data calc, even when requested.
inScopeLinesfilters out deleted lines unconditionally, so even if callers requestInvoiceExpandDeletedLines, those lines never make it into the response. Consider keeping a full line set for output while still using non-deleted lines for calculation.💡 Suggested adjustment
- inScopeLines := lo.FilterMap(invoice.Lines.OrEmpty(), func(line billing.GatheringLine, _ int) (*billing.StandardLine, bool) { - return lo.ToPtr(line.AsStandardLine()), line.DeletedAt == nil - }) + allLines := lo.Map(invoice.Lines.OrEmpty(), func(line billing.GatheringLine, _ int) *billing.StandardLine { + return lo.ToPtr(line.AsStandardLine()) + }) + inScopeLines := lo.Filter(allLines, func(line *billing.StandardLine, _ int) bool { + return line.DeletedAt == nil + }) @@ - } else { - // Emulate deleted_at filtering - out.Lines = billing.NewStandardInvoiceLines( - lo.Filter(out.Lines.OrEmpty(), func(line *billing.StandardLine, _ int) bool { - if !in.Expand.Has(billing.InvoiceExpandDeletedLines) && line.DeletedAt != nil { - return false - } - - return true - }), - ) - } + } else { + if in.Expand.Has(billing.InvoiceExpandDeletedLines) { + out.Lines = billing.NewStandardInvoiceLines(allLines) + } else { + out.Lines = billing.NewStandardInvoiceLines(inScopeLines) + } + }
🤖 Fix all issues with AI agents
In `@openmeter/billing/adapter/invoicelinesplitgroup.go`:
- Line 316: Rename the function setSplitLineHierarchForLines to
setSplitLineHierarchyForLines (adding the missing "y" in "Hierarchy") and update
all call sites to use the new name; specifically change the references in the
invoice.go, gatheringinvoice.go, and the two places in stdinvoicelines.go to
call setSplitLineHierarchyForLines instead of setSplitLineHierarchForLines,
preserving the same signature and generic type parameter so no other logic is
affected.
In `@openmeter/billing/httpdriver/invoice.go`:
- Around line 751-758: mapInvoiceExpandsToEntity currently ignores the
deletedLines expand, causing expand=deletedLines to be dropped; update the
function to also check for api.InvoiceExpandDeletedLines (using slices.Contains)
and include billing.InvoiceExpandDeletedLines in the returned
billing.InvoiceExpands builder (alongside the existing InvoiceExpandLines
mapping) so deletedLines is forwarded for list and get endpoints while
preserving the nil return for empty input.
In `@openmeter/billing/invoice.go`:
- Around line 416-431: The validation branch guarded by OnlyGathering currently
appends errors referring to "standard invoices"; update the error messages in
the block that checks i.OnlyGathering (the appended errors to outErr for
i.Statuses, i.ExtendedStatuses, i.ExternalIDs, and i.DraftUntilLTE) to say
"cannot be set for gathering invoices" (or similar wording referencing gathering
invoices) so the messages correctly reflect the OnlyGathering condition.
In `@openmeter/billing/service/stdinvoice.go`:
- Line 78: The error message string contains a typo: "workload apps" should be
"workflow apps"; update the fmt.Errorf call that returns
billing.StandardInvoice{} (the line using fmt.Errorf("error resolving workload
apps for invoice [%s]: %w", input.Invoice.ID, err)) to use "workflow apps"
instead so the message reads "error resolving workflow apps for invoice [%s]:
%w".
In `@openmeter/billing/stdinvoice.go`:
- Around line 959-989: ListStandardInvoicesInput.Validate is missing pagination
validation; call the embedded Page validation like in ListGatheringInvoicesInput
to prevent invalid/oversized pages reaching DB. Update
ListStandardInvoicesInput.Validate to invoke i.Page.Validate(), and if it
returns an error append it to errs using a prefixed fmt.Errorf("page: %w", err)
before returning errors.Join(errs...), ensuring the pagination guardrail is
applied.
In `@test/subscription/scenario_firstofmonth_test.go`:
- Around line 429-443: The test currently finds a non-gathering invoice but then
calls AsGatheringInvoice() on it; change the predicate in the lo.Find call to
select the gathering invoice (use i.Type() == billing.InvoiceTypeGathering) so
that the returned invoice can be safely converted via AsGatheringInvoice() and
the subsequent assertions on gatheringInvoice.NextCollectionAt and
gatheringInvoice.Lines are valid.
🧹 Nitpick comments (10)
pkg/expand/expand.go (1)
37-43:With()doesn't deduplicate — intentional?Calling
With(value)on a slice that already containsvaluewill produce duplicates. This won't breakHas()orValidate(), but it could be surprising for callers who expect set semantics. If that's the intent, a quick comment would help; otherwise, a dedup or guard would make this more robust.💡 Optional: guard against duplicates
func (e Expand[T]) With(value T) Expand[T] { + if e.Has(value) { + return e + } out := make(Expand[T], len(e)) copy(out, e) out = append(out, value) return out }openmeter/billing/invoice.go (1)
329-349: Duplicated time-range validation betweenListInvoicesInputandListInvoicesAdapterInput.Both
Validate()methods repeat the sameIssuedAfter/IssuedBefore,CreatedAfter/CreatedBefore, andPeriodStartAfter/PeriodStartBeforechecks. Consider extracting a shared helper to keep these in sync. Not urgent, but worth noting for maintainability.openmeter/billing/validators/customer/customer.go (1)
81-93: Minor:DeletedAtcheck on gathering invoices may be redundant.If
IncludeDeleteddefaults tofalseinListInvoicesInput, soft-deleted gathering invoices won't appear in the results, making thegatheringInvoice.DeletedAt != nilcheck on line 87 a no-op. Not harmful — just FYI in case you want to keep the code lean. If there's a scenario whereIncludeDeletedgets flipped totruelater, this guard would become useful, so totally fine to keep it as defensive coding.openmeter/billing/adapter/gatheringinvoice.go (1)
437-439: Pointer-into-slice pattern works here but is worth a small note.Taking
&mappedLines[idx]creates pointers directly into the backing array ofmappedLines. This is fine sincemappedLinesisn't modified afterward, but it's a pattern that can bite if someone later appends tomappedLines(triggering reallocation). A brief comment might save future readers some head-scratching, but no action needed.openmeter/billing/service/stdinvoice.go (1)
53-69: Optional: the validate → type-check preamble is duplicated acrossUpdateStandardInvoiceandGetStandardInvoiceById.Both methods share the same validate-input →
GetInvoiceType→ assert-standard pattern (lines 14–29 and 54–69). If more standard-invoice methods follow the same pattern, consider extracting a small helper likeensureStandardInvoice(ctx, invoiceID)to keep things DRY.openmeter/billing/worker/subscriptionsync/service/sync.go (1)
154-162: TheAsGenericInvoice()→GetID()dance is a bit verbose but looks correct.If
billing.Invoiceever gets a convenienceGetID()method, this could be simplified. For now, the per-item error handling on the conversion is solid.openmeter/billing/worker/subscriptionsync/service/invoiceupdate.go (1)
303-305: PreferGetInvoiceIDover deprecatedInvoiceID().
SinceInvoiceID()is marked deprecated onStandardInvoice, switch toGetInvoiceIDhere to avoid relying on the legacy accessor.♻️ Suggested patch
invoice, err := u.billingService.GetStandardInvoiceById(ctx, billing.GetStandardInvoiceByIdInput{ - Invoice: invoice.InvoiceID(), + Invoice: invoice.GetInvoiceID(), Expand: billing.StandardInvoiceExpandAll, })test/billing/invoice_test.go (1)
309-314: Add a quickSplitLineHierarchyassertion when usingExpandAll.
GatheringInvoiceExpandAllnow includesSplitLineHierarchy; a small assertion here would lock the new expand behavior down.
As per coding guidelines: /*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.openmeter/billing/adapter/invoice.go (1)
765-777: Possible N+1 when expanding split-line hierarchy.
expandSplitLineHierarchylooks like it could hit the DB per invoice/line. If this is called from list endpoints withExpandLines, consider batching by invoice IDs (or gating behind a dedicated expand flag) to avoid N+1s.As per coding guidelines: Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
openmeter/billing/service/invoice.go (1)
76-95: Consider caching app lookups in ListInvoices.
resolveWorkflowAppspulls three apps per standard invoice; on large lists this can become chatty. A simple memoization per app ID (or batch fetch) would keep this snappy.As per coding guidelines: Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/subscription/scenario_firstofmonth_test.go`:
- Around line 429-443: The comment above the assertion is stale (mentions
"gathering invoice") while the code now calls invoice.AsStandardInvoice() and
uses standardInvoice; update the comment to accurately describe that we're
asserting properties of the standard invoice (e.g., that
standardInvoice.CollectionAt equals firstOfNextMonth and all
standardInvoice.Lines have InvoiceAt before that collection date) so readers
aren’t confused by the mismatch between the comment and the use of
AsStandardInvoice()/standardInvoice and Lines.Get().
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/adapter/invoice.go (1)
596-615:⚠️ Potential issue | 🟠 MajorSplit-line hierarchy is dropped on the “updatedLines” fast‑path.
When
ExpandedFieldsincludes lines, this branch bypassesmapStandardInvoiceFromDB, soSplitLineHierarchynever gets attached. That’s a user‑visible regression for callers relying on the hierarchy data.✅ Suggested fix: enrich updatedLines before returning
- updatedInvoice.Lines = updatedLines + // Ensure split-line hierarchy is available when lines are expanded + hierarchyByLineID, err := tx.expandSplitLineHierarchy(ctx, in.Namespace, updatedLines.AsGenericLines()) + if err != nil { + return in, err + } + updatedLines, err = withSplitLineHierarchyForLines[*billing.StandardLine](updatedLines, hierarchyByLineID) + if err != nil { + return in, err + } + updatedInvoice.Lines = updatedLines
🤖 Fix all issues with AI agents
In `@openmeter/billing/httpdriver/invoice.go`:
- Around line 68-69: The code unconditionally appends
InvoiceExpandCalculateGatheringInvoiceWithLiveData in both ListInvoices and
GetInvoice via mapInvoiceExpandsToEntity(lo.FromPtr(input.Expand)).With(...);
either document that this is intentional (add a short comment above these lines
explaining that live-data recalculation must always be available for gathering
invoices and why) or make it opt-in by only calling
.With(InvoiceExpandCalculateGatheringInvoiceWithLiveData) when the incoming
expand set (from lo.FromPtr(input.Expand) / mapInvoiceExpandsToEntity)
explicitly requests it (e.g., check the parsed expands for that flag before
calling .With). Ensure you update both ListInvoices and GetInvoice and reference
InvoiceExpandCalculateGatheringInvoiceWithLiveData, mapInvoiceExpandsToEntity,
lo.FromPtr(input.Expand), and the .With(...) usage.
In `@openmeter/billing/service/invoice.go`:
- Around line 290-357: Add focused unit tests for Service.GetInvoiceById
covering both InvoiceTypeStandard and InvoiceTypeGathering branches and the
calculate-as-standard path: create tests that (1) assert GetStandardInvoiceById
is called and returned invoice is wrapped when adapter.GetInvoiceType returns
billing.InvoiceTypeStandard and Expand contains
InvoiceExpandLines/InvoiceExpandDeletedLines; (2) assert GetGatheringInvoiceById
is used and returned directly when adapter.GetInvoiceType returns
billing.InvoiceTypeGathering without the calculate flag; (3) assert when Expand
includes InvoiceExpandCalculateGatheringInvoiceWithLiveData the service calls
calculateGatheringInvoiceAsStandardInvoice and returns the calculated standard
invoice (mock calculateGatheringInvoiceAsStandardInvoice to return a valid
billing.StandardInvoice); and (4) assert error handling when
calculateGatheringInvoiceAsStandardInvoice returns nil or an error. Use
mocks/stubs for adapter.GetInvoiceType, GetStandardInvoiceById,
GetGatheringInvoiceById, and calculateGatheringInvoiceAsStandardInvoice and
include cases for InvoiceExpandLines and InvoiceExpandDeletedLines toggled to
verify expand propagation.
🧹 Nitpick comments (6)
pkg/expand/expand.go (1)
18-31: Consider returning early when the expand list is empty.Minor thought:
Validate()iterates even wheneis empty. Not a real issue since expansion lists are tiny, but just noting it.Also worth mentioning:
Validate()allows duplicate values in the expand list — it only checks membership againstValues(). If duplicates are undesirable, you could validate that too. But this is likely fine for the use case.openmeter/billing/adapter/gatheringinvoice.go (1)
437-446: The pointer-to-value round-trip works but is a bit noisy.The double
lo.Map(value→ptr, then ptr→value) is needed to satisfy thesplitLineSettableLinesinterface. It's functionally correct, just a tad verbose. IfGatheringLinemethods were defined on value receivers this wouldn't be needed, but that's a broader design choice — no action needed right now.openmeter/billing/worker/advance/advance.go (1)
17-17: Field type broadened frombilling.InvoiceServicetobilling.Service.This is a wider interface than needed — the worker only uses invoice-related methods. If
billing.Serviceincludes unrelated capabilities (profiles, overrides, etc.), keeping the dependency narrow via a focused interface would be cleaner. That said, if the oldInvoiceServiceinterface was removed as part of this refactor, this is pragmatic.#!/bin/bash # Check if billing.InvoiceService still exists or was removed rg -n "type InvoiceService interface" --type=goAlso applies to: 57-57
openmeter/billing/invoice.go (1)
68-88: Consider adding a comment explaining whyInvoiceExpandAllonly includesLines.Looking at the pattern across the codebase,
GatheringInvoiceExpandAllpulls in all values viaValues(), whileStandardInvoiceExpandAll(likeInvoiceExpandAll) includes onlyLinesand has a comment:// Deleted lines are not expanded by default.Adding a similar explanatory comment to
InvoiceExpandAllwould clarify why the other expand types (DeletedLines,CalculateGatheringInvoiceWithLiveData) aren't included, making the "all" naming less confusing. The "ExpandAll" naming convention seems intentionally mean "all default expansions" rather than "all possible ones," so a quick comment would help future readers understand the intent.openmeter/billing/service/invoice.go (1)
58-95: Potential N+1 app lookups in ListInvoices.Line 76–92 resolve apps/status per invoice; on large lists this can get expensive. Consider batching app fetches or skipping resolveWorkflowApps when Workflow.Apps is already populated (e.g., for calculated gathering invoices).
As per coding guidelines, “Performance should be a priority in critical code paths. Anything related to … database operations … should be vetted for potential performance bottlenecks.”openmeter/billing/adapter/invoice.go (1)
765-781: Potential N+1 when expanding split‑line hierarchy.
expandSplitLineHierarchyis called per invoice; on large list queries this may amplify DB work. Consider batching or deferring hierarchy expansion to only the required endpoints.
As per coding guidelines, “Performance should be a priority in critical code paths. Anything related to … database operations … should be vetted for potential performance bottlenecks.”
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openmeter/billing/service/invoice.go`:
- Around line 58-96: The converted gathering invoices are immediately replaced
with a StandardInvoice that already has Apps populated (via
createStandardInvoiceHeaderFromGatheringInvoice), but the following Type() ==
InvoiceTypeStandard branch then redundantly calls resolveWorkflowApps and
resolveStatusDetails; to fix this, after setting invoices.Items[i] =
billing.NewInvoice(*calculatedStdInvoice) in the
calculateGatheringInvoiceAsStandardInvoice flow, add a continue to skip the
subsequent StandardInvoice handling (thereby avoiding resolveWorkflowApps and
resolveStatusDetails for invoices converted by
calculateGatheringInvoiceAsStandardInvoice).
🧹 Nitpick comments (4)
openmeter/billing/adapter/invoice.go (2)
69-84:expandInvoiceLineItemsacceptsStandardInvoiceExpandsbut is called with constructed values fromListInvoicestoo.This helper takes
billing.StandardInvoiceExpands(Line 69), but it's also invoked fromListInvoices(Line 236) where you construct aStandardInvoiceExpandsfromInvoiceExpands. This works, but conceptually the helper is no longer "standard-invoice-specific" — it's a shared utility. If gathering invoices ever need line expansion through a different path, this coupling could get confusing.Not a blocker at all, just something to keep in mind if the expand types diverge further.
235-239: The expansion translation fromInvoiceExpands→StandardInvoiceExpandsis a bit dense.This one-liner does a lot — it constructs a
StandardInvoiceExpands, conditionally addsStandardInvoiceExpandLines, and then conditionally addsStandardInvoiceExpandDeletedLines:query = tx.expandInvoiceLineItems(query, billing. StandardInvoiceExpands{billing.StandardInvoiceExpandLines}. SetOrUnsetIf(input.Expand.Has(billing.InvoiceExpandDeletedLines), billing.StandardInvoiceExpandDeletedLines))It works correctly, but the line wrapping makes it a bit hard to parse at a glance. Consider extracting the expand construction into a local variable for readability.
Proposed readability tweak
- if input.Expand.Has(billing.InvoiceExpandLines) { - query = tx.expandInvoiceLineItems(query, billing. - StandardInvoiceExpands{billing.StandardInvoiceExpandLines}. - SetOrUnsetIf(input.Expand.Has(billing.InvoiceExpandDeletedLines), billing.StandardInvoiceExpandDeletedLines)) - } + if input.Expand.Has(billing.InvoiceExpandLines) { + lineExpand := billing.StandardInvoiceExpands{billing.StandardInvoiceExpandLines}. + SetOrUnsetIf(input.Expand.Has(billing.InvoiceExpandDeletedLines), billing.StandardInvoiceExpandDeletedLines) + query = tx.expandInvoiceLineItems(query, lineExpand) + }openmeter/billing/service/stdinvoice.go (1)
42-76: The "workload" typo from the previous review is fixed — nice!One observation:
GetStandardInvoiceByIdmakes two DB round trips — firstGetInvoiceType(Line 49), thenadapter.GetStandardInvoiceById(Line 60). The adapter already filters out gathering invoices viaStatusNEQ(Gathering), so it would returnNotFoundfor gathering invoices anyway. The separate type check gives a better error message but doubles the DB calls for every standard invoice fetch.Not necessarily something to change now, but worth keeping in mind if this becomes a hot path.
openmeter/billing/service/invoice.go (1)
384-417:withLockedInvoiceStateMachinedoes 3 DB round trips before actual work.
GetInvoiceById→GetInvoiceType(DB hit#1) → actual fetch (DB hit#2)- Inside transaction:
GetStandardInvoiceById(DB hit#3)The first two are just to get the customer ID for the transaction lock scope. This is the existing pattern and seems intentional — just noting it since this is a path used by advance, approve, retry, delete, and update operations. If these become latency-sensitive, a lighter "get customer ID by invoice ID" adapter method could help.
| for i := range invoices.Items { | ||
| invoiceID := invoices.Items[i].ID | ||
| if invoices.Items[i].Type() == billing.InvoiceTypeGathering && input.Expand.Has(billing.InvoiceExpandCalculateGatheringInvoiceWithLiveData) { | ||
| gatheringInvoice, err := invoices.Items[i].AsGatheringInvoice() | ||
| if err != nil { | ||
| return billing.ListInvoicesResponse{}, fmt.Errorf("converting invoice to gathering invoice: %w", err) | ||
| } | ||
|
|
||
| invoices.Items[i], err = s.resolveWorkflowApps(ctx, invoices.Items[i]) | ||
| if err != nil { | ||
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving workflow apps [%s]: %w", invoiceID, err) | ||
| } | ||
| calculatedStdInvoice, err := s.calculateGatheringInvoiceAsStandardInvoice(ctx, recalculateGatheringInvoiceInput{ | ||
| Invoice: gatheringInvoice, | ||
| Expand: input.Expand, | ||
| }) | ||
| if err != nil { | ||
| return billing.ListInvoicesResponse{}, fmt.Errorf("calculating gathering invoice as standard invoice: %w", err) | ||
| } | ||
|
|
||
| invoices.Items[i], err = s.resolveStatusDetails(ctx, invoices.Items[i]) | ||
| if err != nil { | ||
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving status details for invoice [%s]: %w", invoiceID, err) | ||
| invoices.Items[i] = billing.NewInvoice(*calculatedStdInvoice) | ||
| } | ||
|
|
||
| if input.Expand.RecalculateGatheringInvoice { | ||
| invoices.Items[i], err = s.recalculateGatheringInvoice(ctx, recalculateGatheringInvoiceInput{ | ||
| Invoice: invoices.Items[i], | ||
| Expand: input.Expand, | ||
| }) | ||
| if invoices.Items[i].Type() == billing.InvoiceTypeStandard { | ||
| stdInvoice, err := invoices.Items[i].AsStandardInvoice() | ||
| if err != nil { | ||
| return billing.ListInvoicesResponse{}, fmt.Errorf("converting invoice to standard invoice: %w", err) | ||
| } | ||
|
|
||
| invoiceID := stdInvoice.ID | ||
|
|
||
| stdInvoice, err = s.resolveWorkflowApps(ctx, stdInvoice) | ||
| if err != nil { | ||
| return billing.ListInvoicesResponse{}, fmt.Errorf("error recalculating gathering invoice [%s]: %w", invoiceID, err) | ||
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving workflow apps [%s]: %w", invoiceID, err) | ||
| } | ||
|
|
||
| stdInvoice, err = s.resolveStatusDetails(ctx, stdInvoice) | ||
| if err != nil { | ||
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving status details for invoice [%s]: %w", invoiceID, err) | ||
| } | ||
|
|
||
| invoices.Items[i] = billing.NewInvoice(stdInvoice) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the implementation of createStandardInvoiceHeaderFromGatheringInvoice
rg -n 'func.*createStandardInvoiceHeaderFromGatheringInvoice' --type=go -A 50Repository: openmeterio/openmeter
Length of output: 3958
🏁 Script executed:
#!/bin/bash
# Also search for where Type is set in the context of gathering to standard conversion
rg -n 'InvoiceTypeStandard' openmeter/billing/service/invoice.go --type=go -B 2 -A 2Repository: openmeterio/openmeter
Length of output: 962
🏁 Script executed:
#!/bin/bash
# Check the invoice structure and how Type() is determined
rg -n 'func.*Type\(\)' openmeter/billing/ --type=goRepository: openmeterio/openmeter
Length of output: 406
🏁 Script executed:
#!/bin/bash
# Check how the Type() method works on Invoice
rg -n 'func (i Invoice) Type()' openmeter/billing/invoice.go -A 10Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Verify resolveWorkflowApps implementation to confirm it unconditionally overwrites Apps
rg -n 'func.*resolveWorkflowApps' openmeter/billing/service/invoice.go -A 30Repository: openmeterio/openmeter
Length of output: 1357
🏁 Script executed:
#!/bin/bash
# Check calculateGatheringInvoiceAsStandardInvoice to see if it calls resolveWorkflowApps
rg -n 'func.*calculateGatheringInvoiceAsStandardInvoice' openmeter/billing/service/invoice.go -A 50 | head -100Repository: openmeterio/openmeter
Length of output: 2170
🏁 Script executed:
#!/bin/bash
# Let's also check the Invoice.Type() method to confirm how it determines type
rg -n 'func.*Invoice.*Type\(\)' openmeter/billing/ --type=go -A 5Repository: openmeterio/openmeter
Length of output: 797
Watch out for redundant app resolution on converted gathering invoices.
When a gathering invoice gets converted to standard (via calculateGatheringInvoiceAsStandardInvoice at line 65), it already has its Apps populated from the customer override—set during createStandardInvoiceHeaderFromGatheringInvoice and tucked into the new StandardInvoice.
But because the converted invoice now has Type() == InvoiceTypeStandard, it triggers the second if block at line 76, which calls resolveWorkflowApps. This makes 3 fresh GetApp calls and overwrites the already-correct Apps with duplicates. It's wasteful, especially since gathering invoices with live data calculation are the default.
The resolveStatusDetails call after is harmless (it short-circuits for StandardInvoiceStatusGathering), but both are unnecessary overhead.
A quick continue after line 73 skips the redundant work:
Fix
for i := range invoices.Items {
if invoices.Items[i].Type() == billing.InvoiceTypeGathering && input.Expand.Has(billing.InvoiceExpandCalculateGatheringInvoiceWithLiveData) {
// ... gathering → standard conversion ...
invoices.Items[i] = billing.NewInvoice(*calculatedStdInvoice)
+ continue
}
if invoices.Items[i].Type() == billing.InvoiceTypeStandard {📝 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.
| for i := range invoices.Items { | |
| invoiceID := invoices.Items[i].ID | |
| if invoices.Items[i].Type() == billing.InvoiceTypeGathering && input.Expand.Has(billing.InvoiceExpandCalculateGatheringInvoiceWithLiveData) { | |
| gatheringInvoice, err := invoices.Items[i].AsGatheringInvoice() | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("converting invoice to gathering invoice: %w", err) | |
| } | |
| invoices.Items[i], err = s.resolveWorkflowApps(ctx, invoices.Items[i]) | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving workflow apps [%s]: %w", invoiceID, err) | |
| } | |
| calculatedStdInvoice, err := s.calculateGatheringInvoiceAsStandardInvoice(ctx, recalculateGatheringInvoiceInput{ | |
| Invoice: gatheringInvoice, | |
| Expand: input.Expand, | |
| }) | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("calculating gathering invoice as standard invoice: %w", err) | |
| } | |
| invoices.Items[i], err = s.resolveStatusDetails(ctx, invoices.Items[i]) | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving status details for invoice [%s]: %w", invoiceID, err) | |
| invoices.Items[i] = billing.NewInvoice(*calculatedStdInvoice) | |
| } | |
| if input.Expand.RecalculateGatheringInvoice { | |
| invoices.Items[i], err = s.recalculateGatheringInvoice(ctx, recalculateGatheringInvoiceInput{ | |
| Invoice: invoices.Items[i], | |
| Expand: input.Expand, | |
| }) | |
| if invoices.Items[i].Type() == billing.InvoiceTypeStandard { | |
| stdInvoice, err := invoices.Items[i].AsStandardInvoice() | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("converting invoice to standard invoice: %w", err) | |
| } | |
| invoiceID := stdInvoice.ID | |
| stdInvoice, err = s.resolveWorkflowApps(ctx, stdInvoice) | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("error recalculating gathering invoice [%s]: %w", invoiceID, err) | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving workflow apps [%s]: %w", invoiceID, err) | |
| } | |
| stdInvoice, err = s.resolveStatusDetails(ctx, stdInvoice) | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving status details for invoice [%s]: %w", invoiceID, err) | |
| } | |
| invoices.Items[i] = billing.NewInvoice(stdInvoice) | |
| } | |
| } | |
| for i := range invoices.Items { | |
| if invoices.Items[i].Type() == billing.InvoiceTypeGathering && input.Expand.Has(billing.InvoiceExpandCalculateGatheringInvoiceWithLiveData) { | |
| gatheringInvoice, err := invoices.Items[i].AsGatheringInvoice() | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("converting invoice to gathering invoice: %w", err) | |
| } | |
| calculatedStdInvoice, err := s.calculateGatheringInvoiceAsStandardInvoice(ctx, recalculateGatheringInvoiceInput{ | |
| Invoice: gatheringInvoice, | |
| Expand: input.Expand, | |
| }) | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("calculating gathering invoice as standard invoice: %w", err) | |
| } | |
| invoices.Items[i] = billing.NewInvoice(*calculatedStdInvoice) | |
| continue | |
| } | |
| if invoices.Items[i].Type() == billing.InvoiceTypeStandard { | |
| stdInvoice, err := invoices.Items[i].AsStandardInvoice() | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("converting invoice to standard invoice: %w", err) | |
| } | |
| invoiceID := stdInvoice.ID | |
| stdInvoice, err = s.resolveWorkflowApps(ctx, stdInvoice) | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving workflow apps [%s]: %w", invoiceID, err) | |
| } | |
| stdInvoice, err = s.resolveStatusDetails(ctx, stdInvoice) | |
| if err != nil { | |
| return billing.ListInvoicesResponse{}, fmt.Errorf("error resolving status details for invoice [%s]: %w", invoiceID, err) | |
| } | |
| invoices.Items[i] = billing.NewInvoice(stdInvoice) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@openmeter/billing/service/invoice.go` around lines 58 - 96, The converted
gathering invoices are immediately replaced with a StandardInvoice that already
has Apps populated (via createStandardInvoiceHeaderFromGatheringInvoice), but
the following Type() == InvoiceTypeStandard branch then redundantly calls
resolveWorkflowApps and resolveStatusDetails; to fix this, after setting
invoices.Items[i] = billing.NewInvoice(*calculatedStdInvoice) in the
calculateGatheringInvoiceAsStandardInvoice flow, add a continue to skip the
subsequent StandardInvoice handling (thereby avoiding resolveWorkflowApps and
resolveStatusDetails for invoices converted by
calculateGatheringInvoiceAsStandardInvoice).
2 participants
Overview
This patch moves the GetInvoice/ListInvoice types to the new union types. Given standard invoice handling is complex we are just using the Union type based listing to list entities.
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Refactor