refactor: disambiguate line children #3237
Conversation
📝 WalkthroughWalkthroughThe PR replaces the LineChildren wrapper with a new InvoiceLines type and converts Line.Children from an optional wrapper to a plain slice across the billing domain. Constructors and method usages are updated accordingly. Supporting adapters, services, mappers, and tests are adjusted to iterate over slices directly (no OrEmpty/MustGet), and lo.Map is used where applicable. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
turip
added
release-note/ignore
turip
force-pushed
the
reafactor/disambiguate-line-children
branch
from
09c5016 to
9b4d970
Compare
LineChildren type was used for two reasons: - Collection of invoice lines (with the option of being expanded) - Collection of detailed lines (expand does not make sense) This patch makes sure that we have seperate types as perparation for the introduction of specific detailed line types.
turip
force-pushed
the
reafactor/disambiguate-line-children
branch
from
9b4d970 to
daa356d
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: 2
🧹 Nitpick comments (6)
openmeter/billing/invoice.go (1)
523-614: Optional: expose IsEmpty() to clarify “present vs. empty” semanticsGiven NewInvoiceLines may wrap nil with Some(nil), having a small helper improves readability and avoids subtle presence checks across the codebase.
Proposed addition (outside this hunk, in the same type):
func (c InvoiceLines) IsEmpty() bool { s, ok := c.Get() return !ok || len(s) == 0 }openmeter/billing/worker/subscription/sync_test.go (1)
3496-3502: Repeat mapping: OK, consider extracting helper to reduce duplicationYou perform the same child ConfigID clearing in multiple places. A small helper could simplify tests and reduce churn if the shape changes again.
Example:
func clearChildrenFlatFeeConfigIDs(line *billing.Line) *billing.Line { line.Children = lo.Map(line.Children, func(child *billing.Line, _ int) *billing.Line { if child.FlatFee != nil { child.FlatFee.ConfigID = "" } return child }) return line }openmeter/billing/service/invoicecalc/gatheringrealtime.go (1)
17-34: Using lo.Map over children aligns with slice-based refactor; consider clock usage for determinism
- The switch from the wrapper’s Map to
lo.Mapover the slice is correct and type-safe withChildrenas a plain slice.- Optional: replace
time.Now()with the project clock to maintain determinism in tests and consistency with the rest of the codebase.Apply this diff to use the shared clock and normalize to UTC:
@@ -import ( - "time" - - "github.com/oklog/ulid/v2" - "github.com/samber/lo" - - "github.com/openmeterio/openmeter/openmeter/billing" -) +import ( + "time" + + "github.com/oklog/ulid/v2" + "github.com/samber/lo" + "github.com/openmeterio/openmeter/pkg/clock" + + "github.com/openmeterio/openmeter/openmeter/billing" +) @@ - if child.CreatedAt.IsZero() { - child.CreatedAt = time.Now() - } + if child.CreatedAt.IsZero() { + child.CreatedAt = clock.Now().In(time.UTC) + } @@ - if child.UpdatedAt.IsZero() { - child.UpdatedAt = time.Now() - } + if child.UpdatedAt.IsZero() { + child.UpdatedAt = clock.Now().In(time.UTC) + }openmeter/billing/service/lineservice/service.go (2)
158-171: Avoid variable shadowing: rename loop variableThe inner loop variable named line shadows the function parameter line, which harms readability and can lead to bugs on future edits. Rename the inner binding to child.
Apply this diff:
- for _, line := range line.Children { - if line.DeletedAt != nil { + for _, child := range line.Children { + if child.DeletedAt != nil { continue } - lineSvc, err := s.FromEntity(line) + lineSvc, err := s.FromEntity(child) if err != nil { - return fmt.Errorf("creating line service: %w", err) + return fmt.Errorf("creating line service: %w", err) } if err := lineSvc.UpdateTotals(); err != nil { - return fmt.Errorf("updating totals for line[%s]: %w", line.ID, err) + return fmt.Errorf("updating totals for line[%s]: %w", child.ID, err) } }
173-179: Fix typo in commentMinor: “syncorinzed” -> “synchronized”.
Apply this diff:
-// The usageBasedLine will never be syncorinzed directly to stripe or other apps, only the detailed lines. +// The usageBasedLine will never be synchronized directly to Stripe or other apps, only the detailed lines.openmeter/billing/httpdriver/invoiceline.go (1)
186-202: Consider nil safety for direct slice access.The function now operates on a direct slice instead of an optional wrapper. While the implementation is correct, consider adding a nil check at the beginning for defensive programming:
func mapDetailedLinesToAPI(children billing.LineChildren) (*[]api.InvoiceDetailedLine, error) { + if children == nil { + return &[]api.InvoiceDetailedLine{}, nil + } out := make([]api.InvoiceDetailedLine, 0, len(children))This would make the function more robust against potential nil inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
openmeter/billing/adapter/invoice.go(3 hunks)openmeter/billing/adapter/invoicelinediff.go(4 hunks)openmeter/billing/adapter/invoicelinediff_test.go(1 hunks)openmeter/billing/adapter/invoicelinemapper.go(1 hunks)openmeter/billing/adapter/invoicelines.go(1 hunks)openmeter/billing/httpdriver/invoice.go(1 hunks)openmeter/billing/httpdriver/invoiceline.go(7 hunks)openmeter/billing/invoice.go(6 hunks)openmeter/billing/invoice_test.go(1 hunks)openmeter/billing/invoiceline.go(7 hunks)openmeter/billing/service/invoice.go(4 hunks)openmeter/billing/service/invoicecalc/gatheringrealtime.go(2 hunks)openmeter/billing/service/lineservice/service.go(2 hunks)openmeter/billing/worker/subscription/sync_test.go(4 hunks)openmeter/notification/internal/rule.go(1 hunks)test/billing/adapter_test.go(10 hunks)test/billing/discount_test.go(4 hunks)test/billing/invoice_test.go(8 hunks)test/billing/tax_test.go(1 hunks)test/billing/ubpflatfee_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (13)
openmeter/notification/internal/rule.go (2)
openmeter/billing/invoice.go (1)
NewInvoiceLines(527-534)openmeter/billing/invoiceline.go (1)
Line(305-319)
openmeter/billing/httpdriver/invoice.go (2)
openmeter/billing/service/lineservice/service.go (1)
Lines(223-223)openmeter/billing/invoice.go (1)
NewInvoiceLines(527-534)
openmeter/billing/adapter/invoicelines.go (1)
openmeter/billing/service/lineservice/service.go (1)
Lines(223-223)
openmeter/billing/service/lineservice/service.go (3)
openmeter/billing/invoice.go (1)
NewInvoiceLines(527-534)openmeter/billing/invoiceline.go (1)
Line(305-319)openmeter/billing/totals.go (1)
Totals(9-26)
openmeter/billing/service/invoicecalc/gatheringrealtime.go (1)
openmeter/billing/invoiceline.go (1)
Line(305-319)
openmeter/billing/service/invoice.go (1)
openmeter/billing/invoice.go (2)
NewInvoiceLines(527-534)InvoiceLines(523-525)
test/billing/adapter_test.go (3)
openmeter/ent/db/billinginvoiceline/where.go (3)
ID(17-19)ChildUniqueReferenceID(178-180)Amount(108-110)openmeter/billing/invoiceline.go (1)
Line(305-319)openmeter/billing/invoicelinediscount.go (1)
LineDiscounts(352-355)
openmeter/billing/worker/subscription/sync_test.go (2)
openmeter/billing/invoiceline.go (1)
Line(305-319)openmeter/billing/invoice.go (1)
NewInvoiceLines(527-534)
openmeter/billing/httpdriver/invoiceline.go (3)
openmeter/billing/invoiceline.go (2)
LineChildren(710-710)InvoiceLineTypeFee(32-32)openmeter/billing/invoice.go (3)
Invoice(331-342)InvoiceLines(523-525)NewInvoiceLines(527-534)openmeter/ent/db/ent.go (1)
ValidationError(259-262)
openmeter/billing/adapter/invoice.go (1)
openmeter/billing/invoice.go (2)
InvoiceLines(523-525)NewInvoiceLines(527-534)
openmeter/billing/invoiceline.go (1)
pkg/slicesx/map.go (1)
Map(8-21)
test/billing/invoice_test.go (4)
openmeter/billing/service/lineservice/service.go (2)
Lines(223-223)Line(206-221)openmeter/billing/invoice.go (1)
NewInvoiceLines(527-534)openmeter/billing/invoiceline.go (2)
Line(305-319)NewLineChildren(712-719)openmeter/billing/totals.go (1)
Totals(9-26)
openmeter/billing/invoice.go (3)
openmeter/billing/invoiceline.go (3)
InvoiceLineTypeUsageBased(34-34)Line(305-319)InvoiceLineStatusValid(48-48)pkg/models/validator.go (1)
Validate(16-26)openmeter/billing/validationissue.go (1)
ValidationWithFieldPrefix(131-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (59)
openmeter/billing/invoice.go (6)
335-335: Good API rename: Lines → InvoiceLinesSwitching the top-level lines collection to the new InvoiceLines wrapper aligns with the stated PR objective and improves semantic clarity at the invoice level.
413-416: Direct iteration over child slice looks correct and nil-safeIterating over line.Children directly (plain slice) removes wrapper ceremony and remains safe even when nil. This matches the refactor goal and keeps FlattenLinesByID straightforward.
480-481: Re-wrapping after in-place sort is fine; double-check presence semanticsRe-wrapping the in-place sorted slice via NewInvoiceLines(lines) is fine. One nuance: NewInvoiceLines coerces empty to nil and returns mo.Some(nil). That means SortLines will still treat “empty but present” as present. If any call sites rely on “absent means not expanded,” ensure they don't accidentally consider “present but empty (nil slice)” as expanded.
Would you like a helper like InvoiceLines.IsEmpty() to make this intent explicit across call sites?
515-517: Recursive child sorting for usage-based lines LGTMRecursively calling sortLines on usage-based children keeps ordering consistent across hierarchies. The nil child slice path is safe.
1051-1052: SimulateInvoiceInput: Lines type change is consistentSwitching to InvoiceLines in the simulation input matches the new model and keeps API symmetry with Invoice.Lines.
1083-1086: Validation via length-only check is clearerRequiring lines by checking len(i.Lines.OrEmpty()) == 0 reads better than optional presence checks, especially with NewInvoiceLines coercion.
openmeter/billing/invoice_test.go (1)
72-75: Test change to direct slice access is correctAccessing children directly as a slice matches the refactor. Given this test expects two children on lines[1], this is a safe and readable change.
openmeter/notification/internal/rule.go (1)
187-187: Constructor rename to NewInvoiceLines aligns with new APIThe switch from NewLineChildren to NewInvoiceLines for invoice-level collections is correct and consistent with the new type.
openmeter/billing/worker/subscription/sync_test.go (2)
3488-3492: Swapping to lo.Map over direct slice is the right moveThis mirrors the model change (children -> []*Line) and keeps mutation localized. Good use of a transformation rather than manual loops.
4131-4132: Direct child assertions are clearer and safe (length asserted above)Using direct indexing after s.Len(line.Children, 1) is concise and robust.
openmeter/billing/httpdriver/invoice.go (1)
386-387: SimulateInvoice: NewInvoiceLines usage is correctConstructing the request with billing.NewInvoiceLines(lines) matches the new public API. Downstream MapInvoiceToAPI already handles sorting and mapping correctly.
test/billing/tax_test.go (1)
263-268: Dropping OrEmpty() for direct child slice access is correctDirectly using the slice aligns with the refactor. The preceding length assertion makes the nil vs empty distinction safe here.
openmeter/billing/adapter/invoicelinediff_test.go (1)
464-467: Iterating over direct children slice is safe and clearerRanging a nil slice yields zero iterations, so this is behaviorally equivalent without the wrapper and simpler to read.
openmeter/billing/adapter/invoicelines.go (3)
159-160: Flattening children via direct slice is correct (nil-safe)Returning
input.Lines[idx].Childrenworks fine—lo.FlatMapgracefully handles nil returns.
165-167: Parent linkage restoration with direct slice looks goodRe-linking
ParentLineIDby iteratingline.Childrenkeeps semantics intact post-refactor and remains nil-safe.
159-160: No leftover wrapper calls found
Ran the provided ripgrep searches for.Children.(OrEmpty|MustGet)andNewLineChildren()—no matches were returned. It looks like all old wrapper usages have been removed across the repo.test/billing/ubpflatfee_test.go (2)
113-115: Direct child indexing in tests: LGTMSwitching from
MustGet()to length assertion + direct indexing matches the new slice semantics and keeps the test robust.
194-196: Repeat of the pattern: LGTMSame here—asserting length then indexing is concise and safe.
openmeter/billing/service/invoicecalc/gatheringrealtime.go (1)
7-7: New lo import is justifiedUsed by the new
lo.Mapon children; all good.openmeter/billing/adapter/invoicelinemapper.go (2)
73-77: Switch to slice append for Children is correct and nil-safeUsing append on a slice is the right replacement for the previous wrapper’s Append; it handles nil slices and avoids allocation when capacity allows. No functional issues spotted here.
73-79: Confirm downstream expectations for nil vs empty ChildrenWith the wrapper removed, parents with no children will have Children == nil (and thus omitted in JSON due to omitempty). Verify that API consumers and serializers rely on this behavior and don’t require an explicit empty array.
If helpful, I can scan the codebase for any logic that assumes non-nil Children and propose follow-ups.
openmeter/billing/adapter/invoicelinediff.go (3)
182-194: Creation flow for new children reads wellIterating over workItem.Children directly is consistent with the new slice semantics; range over nil is a no-op, so this path remains safe.
258-261: Good: delete children on parent deletionExecuting deleteLineChildren unconditionally in the deletion branch is the correct behavior and avoids stale descendants.
364-372: Direct iteration over DBState.Children is consistentIterating the saved DB state’s children is correct for deletion. No issues spotted.
test/billing/discount_test.go (4)
160-163: LGTM: direct access to Children in testsReplacing OrEmpty() with direct indexing is correct under the new slice semantics.
364-365: LGTM: zero-children assertionUsing require.Len(invoiceLine.Children, 0) aligns with nil-safe slice semantics.
406-408: LGTM: detailed line accessAccessing the first child directly reflects the updated representation. No issues.
438-440: LGTM: detailed line accessConsistent with other changes; test intent remains the same.
openmeter/billing/service/lineservice/service.go (2)
150-151: Correct switch to NewInvoiceLines for associationBuilding Invoice.Lines via NewInvoiceLines with existing + new lineEntities is correct and preserves the optional presence semantics for top-level invoice lines.
182-190: LGTM: totals aggregation over ChildrenMapping over line.Children to sum Totals while skipping deleted lines is consistent and clear.
openmeter/billing/adapter/invoice.go (5)
556-556: Initialize updatedLines as InvoiceLinesGood adaptation to the new API.
570-571: Wrap upsert results via NewInvoiceLinesCorrect construction and preserves nil-for-empty semantics for tests/equality.
578-581: Filter out deleted lines while preserving option semanticsThis keeps response payloads aligned with expand flags. Looks good.
764-765: mapInvoiceFromDB: NewInvoiceLines assignment is correctConsistent with the public API change; no issues spotted.
555-582: Verify removal of legacy children logicThe grep results show that references to the old
LineChildren/NewLineChildrentype and calls toChildren.OrEmpty()still exist throughout both production code and tests. If this migration’s goal is to consolidate around the new top-levelInvoiceLinesAPI, please review and clean up any remaining legacy constructs.Key locations to inspect:
- Domain model (children type definitions and constructors):
•openmeter/billing/invoiceline.go:710–714(type LineChildren []*Line,func NewLineChildren(...))
•openmeter/billing/invoiceline.go:386–394(defaulting toNewLineChildren(nil))- Adapter logic:
•openmeter/billing/adapter/invoice.go:555–582(snippet under review still filters onupdatedLines.OrEmpty())- Core services & HTTP drivers:
• Multiple occurrences inopenmeter/billing/service/...andopenmeter/billing/httpdriver/...- Tests:
•test/billing/adapter_test.go
•test/billing/invoice_test.go
•openmeter/billing/invoice_test.go
•openmeter/billing/adapter/invoicelinediff_test.go
• plus others underworker/subscription/sync_test.goPlease confirm that each of these is either intentionally retained (e.g., for backward-compatibility or event-marshalling) or remove/replace them with the new
InvoiceLinesAPI to avoid surprises post-migration.test/billing/invoice_test.go (5)
317-317: LGTM!The line correctly updates the
Linesfield to use the newbilling.NewInvoiceLinesconstructor, which aligns with the PR's objective of replacingLineChildrenwithInvoiceLinesat the top-level invoice representation.
388-390: New test assertions correctly verify invoice field presence.The test correctly validates that
Linesis not present when no expansion is requested, and that the workflow and app fields are populated as expected. This properly tests the newInvoiceLineswrapper behavior.Also applies to: 420-423
1664-1664: Direct child line access is now correctly implemented.The code correctly accesses child lines directly via
line.Children[index]instead of the previous wrapper methods. This aligns with the refactor whereLine.Childrenis now a plain slice type (LineChildren []*Line).Also applies to: 2424-2426, 2460-2462, 2519-2521
3030-3031: Child line iteration and access patterns are properly updated.The direct slice iteration over
line.Childrenand accessing children via index is correct. The validation logic for detailed lines properly handles the new slice-basedLineChildrentype.Also applies to: 3040-3056, 3517-3522
3916-3924: Line sorting implementation correctly handles the new structure.The sorting logic properly manipulates
line.Childrenas a direct slice and correctly reconstructs it usingbilling.NewLineChildren. The use ofbilling.NewInvoiceLinesfor the top-level invoice lines is also appropriate.openmeter/billing/httpdriver/invoiceline.go (1)
694-771: LGTM! The function correctly returns the new InvoiceLines type.The migration from
LineChildrentoInvoiceLinesis properly implemented throughout the function, including all error paths and the successful return usingbilling.NewInvoiceLines(out).openmeter/billing/service/invoice.go (2)
138-139: Consistent migration to InvoiceLines type.The service correctly uses
billing.NewInvoiceLinesfor line wrapping andbilling.InvoiceLines{}for empty initialization. The filtering logic for deleted lines is properly maintained with the new type.Also applies to: 193-194, 196-205
924-924: SimulateInvoice correctly uses NewInvoiceLines.Both the
UpdateInvoiceandSimulateInvoicefunctions properly construct invoice lines usingbilling.NewInvoiceLines, maintaining consistency with the new type system.Also applies to: 1107-1120
test/billing/adapter_test.go (4)
143-143: Direct slice append is the correct approach.The change from
out.Children.Append(line)toout.Children = append(out.Children, line)correctly handles the new plain slice type.
228-235: Test assertions properly updated for direct slice access.All test assertions have been correctly updated to use direct slice access (
lines[x].Children) instead of the previous wrapper methods. The element matching and count validations remain functionally equivalent.Also applies to: 238-248, 250-256, 271-277, 301-309, 311-323
327-332: Detailed line handling correctly uses direct slice manipulation.The test correctly handles detailed lines as a direct slice, including sorting and validation operations. The use of
strings.Comparefor sorting byChildUniqueReferenceIDis appropriate.Also applies to: 370-378
449-450: Discount access patterns correctly updated.All discount-related test assertions have been properly updated to access discounts through the direct slice (
lines[0].Children[0].Discounts) instead of through wrapper methods.Also applies to: 495-496, 506-511, 519-520, 589-590
openmeter/billing/invoiceline.go (12)
370-373: LGTM! Clean migration to lo.Map for child transformation.The use of
lo.Mapfor transforming children is cleaner than the previous wrapper-basedMapmethod.
385-395: Proper handling of empty children slices.The direct length check
len(out.Children) == 0and subsequent initialization withNewLineChildren(nil)correctly ensures consistent nil/empty slice behavior.
429-434: Child cloning correctly uses lo.Map.The cloning logic properly uses
lo.Mapto transform children, maintaining the parent-child relationship correctly.
467-488: Validation correctly iterates over direct slice.The validation logic properly checks
len(i.Children) > 0and iterates directly over the slice with proper error reporting including the index.
566-571: DisassociateChildren simplified correctly.The function now simply assigns an empty
LineChildren{}to both the line and its DBState, which is cleaner than the previous implementation.
710-719: NewLineChildren correctly returns plain slice.The constructor properly handles nil normalization for empty slices, which helps with test equality checks as noted in the comment.
721-725: Validation uses lo.Map appropriately.The validation method correctly uses
lo.Mapwith proper error prefixing for each child line.
727-731: GetByID correctly operates on value receiver.The method properly uses
lo.FindOrElseon the slice value, which is appropriate for the new plain slice type.
733-744: RemoveByID correctly mutates slice in place.The method properly uses
lo.Filterto remove the matching element and reassigns the filtered result to the receiver.
746-750: GetByChildUniqueReferenceID uses value receiver correctly.The method properly operates on a value receiver and uses
lo.FindOrElsefor the lookup.
754-796: ChildrenWithIDReuse correctly handles the new LineChildren type.The function properly operates on the direct slice (
c.Children) and correctly preserves IDs and timestamps when reusing existing children.
895-897: Pending line validation correctly checks direct slice length.The validation properly uses
len(line.Children) > 0instead of the previous wrapper-based check.
Overview
LineChildren type was used for two reasons:
Historically this reuse made sense as we were having the progressively billed lines represented as Lines, but now the progressive billing part is captured in a SplitLineGroup, thus we don't need this amount of generic coding.
This patch makes sure that we have separate types as preparation for the introduction of specific DetailedLine types where the collection would need to either contain Line or DetailedLine types.
Motivation
Why should I care? You should not, but this #3219 would require to split the Lines/DetailedLines before so that the adapter change is reviewable (e.g not a rewrite, it just contains the new mapping rules)
The split cannot happen until the same type is used for both invoice level Lines and the forhtcoming DetailedLines (as in it's current state the code would allow to have children of detailed lines, which is a logic error, but allowed by the DB schema).