Conversation
📝 WalkthroughWalkthroughReplace bespoke per-field invoice-line diffs with a parent-aware, ID-driven entitydiff framework; migrate diff generation, nested children reconciliation, upsert plumbing, tests, and small billing helpers/types to use entitydiff.Diff and nested parent wrappers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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
refactory/entitydiff
branch
9 times, most recently
from
3ddde39 to
69a9416
Compare
turip
force-pushed
the
refactory/entitydiff
branch
2 times, most recently
from
ded8fec to
83cb770
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
openmeter/billing/adapter/invoicelines.go (3)
291-320: OnConflict should ignore created_at for config upsertsFor BillingInvoiceFlatFeeLineConfig upserts you call ResolveWithNewValues() but don’t ignore created_at, unlike invoice line upserts where created_at is protected. This can silently “time-travel” created_at on updates.
Apply this diff:
return upsertWithOptions(ctx, a.db, in, upsertInput[detailedLineWithParent, *db.BillingInvoiceFlatFeeLineConfigCreate]{ @@ UpsertItems: func(ctx context.Context, tx *db.Client, items []*db.BillingInvoiceFlatFeeLineConfigCreate) error { return tx.BillingInvoiceFlatFeeLineConfig. CreateBulk(items...). OnConflict( sql.ConflictColumns(billinginvoiceflatfeelineconfig.FieldID), - sql.ResolveWithNewValues(), + sql.ResolveWithNewValues(), + sql.ResolveWith(func(u *sql.UpdateSet) { + u.SetIgnore(billinginvoiceflatfeelineconfig.FieldCreatedAt) + }), ). UpdateIndex(). Exec(ctx) },
322-349: Same created_at issue for usage-based config upsertsBillingInvoiceUsageBasedLineConfig upserts also call ResolveWithNewValues() without ignoring created_at. Please align with the pattern used for lines.
UpsertItems: func(ctx context.Context, tx *db.Client, items []*db.BillingInvoiceUsageBasedLineConfigCreate) error { return tx.BillingInvoiceUsageBasedLineConfig. CreateBulk(items...). OnConflict( sql.ConflictColumns(billinginvoiceusagebasedlineconfig.FieldID), - sql.ResolveWithNewValues(), + sql.ResolveWithNewValues(), + sql.ResolveWith(func(u *sql.UpdateSet) { + u.SetIgnore(billinginvoiceusagebasedlineconfig.FieldCreatedAt) + }), ).Exec(ctx) },
274-282: Bump updated_at for both parent and child invoice‐line recordsThe current update only touches IDs in
lineDiffs.AffectedLineIDs, which represent parent lines impacted by any change. Child (“detailed”) lines that have their own discounts or other nested updates live inlineDiffs.DetailedLineAffectedLineIDsbut are never included in this timestamp bump. To ensure consumers relying on each line’supdated_atsee the most recent change, merge both sets before issuing theUPDATE.• Location:
openmeter/billing/adapter/invoicelines.go, around lines 274–282
• Change: collect both parent and child IDs, then callUpdate().SetUpdatedAt(…)on the combined listProposed diff:
- if !lineDiffs.AffectedLineIDs.IsEmpty() { - err := tx.db.BillingInvoiceLine.Update(). - SetUpdatedAt(clock.Now().In(time.UTC)). - Where(billinginvoiceline.IDIn(lineDiffs.AffectedLineIDs.AsSlice()...)). - Exec(ctx) - if err != nil { - return nil, fmt.Errorf("updating updated_at for lines: %w", err) - } - } + { + // bump timestamp on both parent and detailed child lines + ids := append([]string{}, lineDiffs.AffectedLineIDs.AsSlice()...) + ids = append(ids, lineDiffs.DetailedLineAffectedLineIDs.AsSlice()...) + if len(ids) > 0 { + if err := tx.db.BillingInvoiceLine.Update(). + SetUpdatedAt(clock.Now().In(time.UTC)). + Where(billinginvoiceline.IDIn(ids...)). + Exec(ctx); err != nil { + return nil, fmt.Errorf("updating updated_at for lines: %w", err) + } + } + }If omitting child‐line timestamp bumps was intentional, please add a comment clarifying that only parent lines should reflect nested changes.
openmeter/billing/adapter/upsert.go (1)
28-47: Deletes are silently skipped when MarkDeleted is nil. Fail fast to avoid data drift.If itemDiff.Delete is non-empty and opts.MarkDeleted is not provided, the delete path becomes a no-op with no error. That can leave orphaned rows and violate uniqueness constraints when paired with creates.
Recommend erroring out early:
-// Delete must be first, as we might have a constraint that prevents us from creating the item if not deleted before. -if len(itemDiff.Delete) > 0 && opts.MarkDeleted != nil { +// Delete must be first, as we might have a constraint that prevents us from creating the item if not deleted before. +if len(itemDiff.Delete) > 0 { + if opts.MarkDeleted == nil { + return fmt.Errorf("upsert: %d deletes requested but MarkDeleted handler is nil", len(itemDiff.Delete)) + } // We formulate delete as a soft delete update, so that any changes happening alongside the deletion are persisted // to the database. - toDelete, err := slicesx.MapWithErr(itemDiff.Delete, func(item T) (T, error) { + toDelete, err := slicesx.MapWithErr(itemDiff.Delete, func(item T) (T, error) { return opts.MarkDeleted(ctx, item) })Note: this adds a fmt import.
🧹 Nitpick comments (17)
openmeter/billing/invoiceline.go (1)
327-330: Exported alias: add proper doc comment and clarify intentDetailedLine is exported. Please add a doc comment that starts with “DetailedLine …” to satisfy linters and make the transitional nature explicit.
Apply this diff:
-// Temporary type alias until we move the detailed line to its own entity, for now it just enhances -// readability. +// DetailedLine is a temporary alias for Line. It improves readability for nested/detailed diffs +// and will be removed once DetailedLine becomes its own entity (see OM-1060). type DetailedLine = Lineopenmeter/billing/invoicelinediscount.go (1)
51-53: Accessor looks good; add doc comment for exported methodGetDescription mirrors GetChildUniqueReferenceID and is non-mutating. Add a short doc comment to satisfy exported-symbol linting.
+// GetDescription returns the optional description of the discount. func (i LineDiscountBase) GetDescription() *string { return i.Description }pkg/entitydiff/parent.go (2)
18-22: Document wrapper semanticsPlease add a brief doc comment explaining that Parent is carried for context only and is not used for identity. This prevents confusion when these wrappers appear in diffs.
+// EqualerWithParent wraps an EqualerEntity with its parent. Parent is carried for context +// (e.g., writes), while identity and equality delegate to the inner Entity. type EqualerWithParent[T EqualerEntity[T], P Entity] struct { Entity T Parent P }
35-42: Optional helper for single itemsConsider adding a constructor for a single WithParent to reduce boilerplate at call sites that don’t start from a slice.
func NewEqualersWithParent[T EqualerEntity[T], P Entity](entity []T, parent P) []EqualerWithParent[T, P] { return lo.Map(entity, func(item T, _ int) EqualerWithParent[T, P] { return EqualerWithParent[T, P]{ Entity: item, Parent: parent, } }) } + +// NewWithParent wraps a single Entity with its Parent. +func NewWithParent[T Entity, P Entity](entity T, parent P) WithParent[T, P] { + return WithParent[T, P]{Entity: entity, Parent: parent} +}openmeter/billing/adapter/invoicelines.go (1)
159-172: ParentLineID re-assignment: safe but add a quick guard?We rely on the diff generator ensuring detailed lines are fee-typed. If an unexpected type leaks in, accessing line.FlatFee in subsequent steps could panic. A light guard (e.g., verify line.Type == InvoiceLineTypeFee and line.FlatFee != nil when building the detailed diffs) would make this path more robust.
Would you like me to patch diffInvoiceLines to assert children are fee-typed before producing DetailedLine diffs?
openmeter/billing/adapter/invoicelinediff_test.go (2)
160-169: Fix typos in test descriptions/commentsMinor spelling issues reduce clarity.
- 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) { @@ - // ID change should tirgger a delete/update + // ID change should trigger a delete/update
356-367: Assert updated_at bump policy in tests (optional)Given the adapter bumps updated_at only for AffectedLineIDs (parents), consider adding a test that changes only a detailed line’s discount and asserts whether the parent and/or child’s updated_at should change. This will lock the intended policy.
I can add a focused test that exercises only DetailedLineAmountDiscounts changes and checks which line IDs are included in AffectedLineIDs vs DetailedLineAffectedLineIDs and how the adapter applies updated_at.
pkg/entitydiff/diff.go (3)
44-61: Append ordering is counterintuitive (parameter 'a' precedes receiver 'd'). Consider clarifying or flipping.Current behavior returns a new Diff where entries from 'a' come before entries from the receiver 'd'. At call sites like acc = acc.Append(next), this causes latest segments to be placed first, effectively reversing chronological order. If intentional, consider documenting it; otherwise, it’s easy to accidentally rely on the wrong order.
Suggested change (receiver entries first, then 'a'):
func (d *Diff[T]) Append(a Diff[T]) Diff[T] { out := Diff[T]{ - Delete: make([]T, 0, len(a.Delete)+len(d.Delete)), - Update: make([]DiffUpdate[T], 0, len(a.Update)+len(d.Update)), - Create: make([]T, 0, len(a.Create)+len(d.Create)), + Delete: make([]T, 0, len(d.Delete)+len(a.Delete)), + Update: make([]DiffUpdate[T], 0, len(d.Update)+len(a.Update)), + Create: make([]T, 0, len(d.Create)+len(a.Create)), } - out.Delete = append(out.Delete, a.Delete...) - out.Delete = append(out.Delete, d.Delete...) + out.Delete = append(out.Delete, d.Delete...) + out.Delete = append(out.Delete, a.Delete...) - out.Update = append(out.Update, a.Update...) - out.Update = append(out.Update, d.Update...) + out.Update = append(out.Update, d.Update...) + out.Update = append(out.Update, a.Update...) - out.Create = append(out.Create, a.Create...) - out.Create = append(out.Create, d.Create...) + out.Create = append(out.Create, d.Create...) + out.Create = append(out.Create, a.Create...) return out }
67-81: Union is fine; minor nit: zero-value slices can be nil.Using nil slices instead of empty literals (e.g., Create: nil) would avoid unnecessary allocations without changing semantics. Optional.
83-154: Stability and idempotency in diffByID: avoid map iteration nondeterminism and redundant deletes.
- Order: The second pass iterates a map (dbStateByID), which produces nondeterministic ordering of Delete entries for “missing in expected”. This can make logs/tests flaky.
- Redundant deletes: When an item is already soft-deleted in DB, adding it again to Delete may generate noop updates or failures, depending on downstream logic.
Proposed improvements:
- Iterate dbState slice to maintain caller-provided order.
- Skip deletion for items already marked deleted.
- for dbID, dbItemState := range dbStateByID { - // If the expected state does not contain the item we need to delete it - if _, ok := expectedItemsByID[dbID]; !ok { - diff.Delete = append(diff.Delete, dbItemState) - } - } + for _, dbItemState := range dbState { + if dbItemState == nil { + continue // defensive: ignore nils if they slip in + } + // If the expected state does not contain the item we need to delete it + if _, ok := expectedItemsByID[dbItemState.GetID()]; !ok { + if !dbItemState.IsDeleted() { + diff.Delete = append(diff.Delete, dbItemState) + } + } + }Optional guardrails (document or enforce):
- Detect duplicate IDs in inputs (SliceToMap keeps the last occurrence). If duplicates can occur, consider validating and returning an error to avoid surprising outcomes.
Would you like a quick validation utility to scan for duplicate IDs in your pipelines/tests?
openmeter/billing/adapter/upsert.go (1)
39-47: Order-of-operations guarantee for bulk UpsertItems.You’re building a single bulk payload with delete-then-create-then-update ordering. Depending on the implementation of UpsertItems (e.g., one INSERT ... ON CONFLICT per batch vs multiple statements), the DB might not honor intra-batch ordering for uniqueness checks. If constraints require deletes to be visible before creates, consider issuing deletes in a separate UpsertItems call (or transaction step) to guarantee visibility.
Would you like me to scan the repo to confirm UpsertItems semantics (bulk INSERT vs per-row) and recommend a safe split if needed?
openmeter/billing/adapter/invoicelinediff.go (6)
49-56: Deriving dbState from per-line DBState is reasonable; minor prealloc nit.You can preallocate to len(lines) to avoid a couple of reslices:
- dbState := []*billing.Line{} + dbState := make([]*billing.Line, 0, len(lines))
72-81: Append usage reverses chronological order.Because Diff.Append currently puts the new diff before the receiver’s entries, successive appends make latest changes appear first. If this is not intentional, either switch to the proposed Append ordering or append in the opposite direction at call sites.
96-104: Detailed-line update preconditions: enrich error context; avoid hard-failing if possible.The current errors lack context and will abort the entire diff for any single problematic detailed line. If business rules guarantee DetailedLine is always FlatFee, this is fine; otherwise:
- Include IDs to aid debugging.
- Consider downgrading to a top-level line update or recording a validation issue instead of erroring.
Example improvement:
- if detailedLine.ExpectedState == nil || detailedLine.ExpectedState.FlatFee == nil { - return fmt.Errorf("detailed line expected state is nil or flat fee is nil") - } + if detailedLine.ExpectedState == nil || detailedLine.ExpectedState.FlatFee == nil { + return fmt.Errorf("detailed line update unsupported: missing expected flat fee (lineID=%s, parentID=%s)", + item.ExpectedState.GetID(), item.DBState.GetID()) + } - if detailedLine.DBState == nil || detailedLine.DBState.FlatFee == nil { - return fmt.Errorf("detailed line db state is nil or flat fee is nil") - } + if detailedLine.DBState == nil || detailedLine.DBState.FlatFee == nil { + return fmt.Errorf("detailed line update unsupported: missing DB flat fee (lineID=%s, parentID=%s)", + item.DBState.GetID(), item.DBState.GetID()) + }
105-116: Parent assignment in ExpectedState uses DB parent; use Expected parent for clarity.IDs should match, but semantically the Expected side should carry the Expected parent. This also aligns with GetDetailedLineDiffWithParentID, which reads ExpectedState.Parent.
- ExpectedState: detailedLineWithParent{ - Entity: detailedLine.ExpectedState, - Parent: item.DBState, - }, + ExpectedState: detailedLineWithParent{ + Entity: detailedLine.ExpectedState, + Parent: item.ExpectedState, + },
197-209: CreateLine initializes detailed-line and discount creates.If DetailedLine can ever have usage-based discounts in the future, consider mirroring usage-discount handling here for symmetry. For now, OK.
246-268: GetDetailedLineDiffWithParentID mutates entities to set ParentLineID. Consider avoiding side effects.The mapping sets ParentLineID directly on the referenced entities. If those instances are reused elsewhere, this introduces implicit side effects. Optionally, set ParentLineID on shallow copies to keep the original values untouched.
📜 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 (8)
openmeter/billing/adapter/invoicelinediff.go(2 hunks)openmeter/billing/adapter/invoicelinediff_test.go(15 hunks)openmeter/billing/adapter/invoicelines.go(7 hunks)openmeter/billing/adapter/upsert.go(4 hunks)openmeter/billing/invoiceline.go(1 hunks)openmeter/billing/invoicelinediscount.go(1 hunks)pkg/entitydiff/diff.go(1 hunks)pkg/entitydiff/parent.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-21T08:32:31.689Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
Applied to files:
openmeter/billing/adapter/invoicelinediff_test.go
🧬 Code graph analysis (7)
openmeter/billing/invoicelinediscount.go (3)
openmeter/ent/db/billinginvoiceline/where.go (1)
Description(97-99)openmeter/ent/db/billinginvoicelinediscount/where.go (1)
Description(101-103)openmeter/ent/db/billinginvoicelineusagediscount/where.go (1)
Description(101-103)
openmeter/billing/invoiceline.go (1)
openmeter/billing/service/lineservice/service.go (1)
Line(206-221)
openmeter/billing/adapter/invoicelinediff_test.go (2)
openmeter/billing/invoiceline.go (4)
Line(311-325)DetailedLine(329-329)InvoiceLineTypeUsageBased(35-35)UsageBasedLine(808-821)pkg/entitydiff/diff.go (3)
Entity(10-13)Diff(26-30)DiffUpdate(15-18)
pkg/entitydiff/parent.go (1)
pkg/entitydiff/diff.go (2)
Entity(10-13)EqualerEntity(202-205)
openmeter/billing/adapter/invoicelinediff.go (4)
pkg/entitydiff/parent.go (3)
EqualerWithParent(18-21)WithParent(5-8)NewEqualersWithParent(35-42)openmeter/billing/invoicelinediscount.go (2)
UsageLineDiscountManaged(258-261)AmountLineDiscountManaged(108-111)openmeter/billing/invoiceline.go (3)
Line(311-325)DetailedLine(329-329)LineBase(128-157)pkg/entitydiff/diff.go (6)
Diff(26-30)DiffByID(171-200)DiffByIDInput(156-163)DiffUpdate(15-18)DiffByIDEqualer(213-229)Entity(10-13)
openmeter/billing/adapter/invoicelines.go (3)
openmeter/billing/invoiceline.go (2)
DetailedLine(329-329)Line(311-325)openmeter/ent/db/billinginvoiceline/where.go (2)
ParentLineID(158-160)ID(17-19)pkg/entitydiff/diff.go (3)
Union(67-81)Entity(10-13)Diff(26-30)
openmeter/billing/adapter/upsert.go (2)
pkg/entitydiff/diff.go (3)
Entity(10-13)Diff(26-30)DiffUpdate(15-18)pkg/slicesx/map.go (1)
MapWithErr(25-49)
⏰ 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). (5)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Code Generators
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (20)
pkg/entitydiff/parent.go (1)
31-33: Equality ignores Parent; confirm move semantics are intentionalEqualerWithParent.Equal delegates to the inner entity and ignores Parent. If an entity is “moved” between parents but remains otherwise equal, this wrapper will consider items equal. Confirm this is intentional for all usages (e.g., discounts or nested lines). If moves should be treated as updates (or delete+create), tests should cover that case explicitly.
Would you like me to draft a focused test that “moves” a child between parents and asserts the desired diff (update vs. delete+create)?
openmeter/billing/adapter/invoicelines.go (1)
222-225: Nice: union of amount-discount diffsUnioning amount discounts from parent lines and detailed lines via entitydiff.Union centralizes upsert handling and avoids duplication.
openmeter/billing/adapter/invoicelinediff_test.go (2)
319-331: Generic mapper reads well and is reusablemapDiffToIDs abstracts ID/description mapping cleanly. Good use of lo.FromPtrOr to keep IDs stable when descriptions are absent.
17-33: idDiff and expanded expectations: good structureThe new idDiff and separation of top-level vs. detailed expectations (including DetailedLineAffectedLineIDs) make assertions precise and future-proof as the diff model evolves.
pkg/entitydiff/diff.go (6)
10-13: Minimal Entity contract is clear and composable.GetID and IsDeleted are sufficient for ID-driven diffs and soft-delete flows. No concerns.
15-31: Diff/DiffUpdate shape looks good for public consumption.The structs are small PODs with explicit fields, easy to pass across boundaries. No concerns.
32-42: Append helpers are convenient; pointer receiver aligns with mutating usage.The Needs* helpers are straightforward and make call sites readable.
63-66: IsEmpty is correct and fast.Covers all three dimensions. No concerns.
165-201: Callback application order is sensible; error aggregation with errors.Join is good.Once diffByID produces stable ordering, callback invocation becomes stable as well. No changes requested.
213-229: Equaler-based updates are correctly filtered to only non-equal pairs.One caveat: if T is a pointer type and a nil sneaks in, calling Equal on a nil receiver could panic. If inputs are guaranteed non-nil, fine; otherwise add a defensive check.
openmeter/billing/adapter/upsert.go (6)
7-9: Importing entitydiff aligns this module with the new public diff API.No concerns.
19-23: Signature migration to entitydiff.Diff[T] looks correct.Constrained T to entitydiff.Entity, and carried through generics cleanly.
25-26: Capacity preallocation matches Create+Update+Delete.Minor optimization that avoids reallocations.
49-58: Create path looks correct and matches new Diff API.Straightforward mapping to CreateBulkType.
60-69: Update path uses ExpectedState, which is consistent with idempotent upserts.Assuming DiffOnly contains non-deleted ExpectedState entries (as produced by DiffByIDEqualer), this is correct. If other producers may populate Update with deleted ExpectedState, add a guard to skip IsDeleted() items.
71-77: Single call to UpsertItems keeps DB traffic low.If you adopt the “separate deletes” advice, this would become two calls (delete-bulk then create/update-bulk). Otherwise, this is fine.
openmeter/billing/adapter/invoicelinediff.go (4)
14-20: Type aliases cleanly express parent-aware equalers for nested diffs.Good use of EqualerWithParent/WithParent to carry context.
22-41: invoiceLineDiff structure is cohesive and future-proof for nested diffs.Clear separation of top-level, discounts, and detailed-line changes with affected ID tracking.
122-137: Discount diffs and affected ID tracking look correct.Good separation: detailed discount changes bubble up AffectedLineIDs and DetailedLineAffectedLineIDs conditionally on IsDeleted flags.
156-176: DeleteLine correctly propagates deletions to discounts and children.Soft-delete model is respected; no concerns.
turip
force-pushed
the
refactory/entitydiff
branch
from
f9e73b3 to
34218e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
openmeter/billing/adapter/invoicelinediff.go (1)
60-76: Top-level update assumes UsageBased lines only; verify domain or make comparison union-safeYou now hard-error if UsageBased is nil on either side, which enforces “top-level must be UsageBased.” The model (billing.Line) allows FlatFee or UsageBased. If FlatFee top-level lines are valid in any pathway, this will abort diffing.
Optional union-safe pattern:
- if item.ExpectedState.UsageBased == nil { - return fmt.Errorf("expected state usage based is nil") - } - if item.DBState.UsageBased == nil { - return fmt.Errorf("db state usage based is nil") - } - if !item.ExpectedState.LineBase.Equal(item.DBState.LineBase) || !item.ExpectedState.UsageBased.Equal(item.DBState.UsageBased) { + baseChanged := !item.ExpectedState.LineBase.Equal(item.DBState.LineBase) + var lineChanged bool + switch { + case item.ExpectedState.UsageBased != nil || item.DBState.UsageBased != nil: + lineChanged = (item.ExpectedState.UsageBased == nil) != (item.DBState.UsageBased == nil) || + (item.ExpectedState.UsageBased != nil && !item.ExpectedState.UsageBased.Equal(item.DBState.UsageBased)) + case item.ExpectedState.FlatFee != nil || item.DBState.FlatFee != nil: + lineChanged = (item.ExpectedState.FlatFee == nil) != (item.DBState.FlatFee == nil) || + (item.ExpectedState.FlatFee != nil && !item.ExpectedState.FlatFee.Equal(item.DBState.FlatFee)) + default: + lineChanged = false + } + if baseChanged || lineChanged { diff.Line.NeedsUpdate(item) }If the domain truly forbids FlatFee at the top level, consider replacing the nil checks with a single guard that returns a typed error annotated with the line ID to make troubleshooting easier.
🧹 Nitpick comments (5)
pkg/entitydiff/diff.go (2)
45-62: Make Append a value receiver; current pointer receiver is misleading since the method is pureAppend does not mutate the receiver; it constructs and returns a new Diff. Using a pointer receiver suggests mutation and can confuse call sites. Prefer a value receiver for clarity.
-func (d *Diff[T]) Append(a Diff[T]) Diff[T] { +func (d Diff[T]) Append(a Diff[T]) Diff[T] { out := Diff[T]{ Delete: make([]T, 0, len(a.Delete)+len(d.Delete)), Update: make([]DiffUpdate[T], 0, len(a.Update)+len(d.Update)), Create: make([]T, 0, len(a.Create)+len(d.Create)), }
108-119: Avoid parameter shadowing to improve readabilityThe local variable named dbState shadows the function parameter dbState. Rename the local to dbItem (or similar) to reduce cognitive load and prevent accidental misuse.
- dbState, ok := dbStateByID[expected.GetID()] + dbItem, ok := dbStateByID[expected.GetID()] if !ok { if expected.IsDeleted() { // If the expected state is deleted, but we don't have it in the db, we can skip it continue } // If the expected state is not deleted, but we don't have it in the db, we need to create it diff.Create = append(diff.Create, expected) continue } - if expected.IsDeleted() { - if !dbState.IsDeleted() { + if expected.IsDeleted() { + if !dbItem.IsDeleted() { // If the expected state is deleted, but we have it in the db, we need to delete it - diff.Delete = append(diff.Delete, dbState) + diff.Delete = append(diff.Delete, dbItem) continue } // If the expected state is deleted, and we have it in the db, but it's deleted, we can skip it continue } diff.UpdateCandidates = append(diff.UpdateCandidates, DiffUpdate[T]{ - DBState: dbState, + DBState: dbItem, ExpectedState: expected, })openmeter/billing/adapter/invoicelinediff.go (3)
14-20: Type alias duplication: consider reusing one alias for AmountLineDiscountManagedamountLineDiscountManagedWithLine and detailedLineAmountDiscountWithParent resolve to the same underlying type. Keeping one alias improves consistency and avoids confusion.
- amountLineDiscountManagedWithLine = entitydiff.EqualerWithParent[billing.AmountLineDiscountManaged, *billing.Line] + amountLineDiscountManagedWithLine = entitydiff.EqualerWithParent[billing.AmountLineDiscountManaged, *billing.Line] // ... - detailedLineAmountDiscountWithParent = entitydiff.EqualerWithParent[billing.AmountLineDiscountManaged, *billing.Line] + detailedLineAmountDiscountWithParent = amountLineDiscountManagedWithLine
95-99: Consistency: use GetID() everywhere instead of mixing .ID and GetID()The file mixes direct field access (ID) and GetID(). Prefer GetID() for consistency with the Entity interface and to preserve future encapsulation.
- diff.AffectedLineIDs.Add(item.DBState.GetID()) + diff.AffectedLineIDs.Add(item.DBState.GetID()) // ... - diff.AffectedLineIDs.Add(item.DBState.ID) + diff.AffectedLineIDs.Add(item.DBState.GetID()) // ... - diff.DetailedLineAffectedLineIDs.Add(detailedLine.DBState.ID) + diff.DetailedLineAffectedLineIDs.Add(detailedLine.DBState.GetID())Also applies to: 125-127, 137-145
254-275: Avoid mutating source entities when projecting ParentLineID; consider cloning or delaying the projectionGetDetailedLineDiffWithParentID mutates the DetailedLine pointers (Create/Delete/Update projections) by setting ParentLineID in-place. If these objects are shared elsewhere in the pipeline, this can cause subtle side effects.
Two alternatives:
- Clone before mutation (if a Clone/Copy exists on DetailedLine, use it; otherwise shallow-copy the struct value).
- Defer setting ParentLineID to the “apply” stage rather than during diff projection.
Example using shallow copies:
- Create: lo.Map(d.DetailedLine.Create, func(item detailedLineWithParent, _ int) *billing.DetailedLine { - item.Entity.ParentLineID = lo.ToPtr(item.Parent.GetID()) - return item.Entity - }), + Create: lo.Map(d.DetailedLine.Create, func(item detailedLineWithParent, _ int) *billing.DetailedLine { + cl := *item.Entity + cl.ParentLineID = lo.ToPtr(item.Parent.GetID()) + return &cl + }), - Delete: lo.Map(d.DetailedLine.Delete, func(item detailedLineWithParent, _ int) *billing.DetailedLine { - item.Entity.ParentLineID = lo.ToPtr(item.Parent.GetID()) - return item.Entity - }), + Delete: lo.Map(d.DetailedLine.Delete, func(item detailedLineWithParent, _ int) *billing.DetailedLine { + cl := *item.Entity + cl.ParentLineID = lo.ToPtr(item.Parent.GetID()) + return &cl + }), - Update: lo.Map(d.DetailedLine.Update, func(item entitydiff.DiffUpdate[detailedLineWithParent], _ int) entitydiff.DiffUpdate[*billing.DetailedLine] { - item.ExpectedState.Entity.ParentLineID = lo.ToPtr(item.ExpectedState.Parent.GetID()) - return entitydiff.DiffUpdate[*billing.DetailedLine]{ - DBState: item.DBState.Entity, - ExpectedState: item.ExpectedState.Entity, - } - }), + Update: lo.Map(d.DetailedLine.Update, func(item entitydiff.DiffUpdate[detailedLineWithParent], _ int) entitydiff.DiffUpdate[*billing.DetailedLine] { + exp := *item.ExpectedState.Entity + exp.ParentLineID = lo.ToPtr(item.ExpectedState.Parent.GetID()) + return entitydiff.DiffUpdate[*billing.DetailedLine]{ + DBState: item.DBState.Entity, + ExpectedState: &exp, + } + }),
📜 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 (2)
openmeter/billing/adapter/invoicelinediff.go(2 hunks)pkg/entitydiff/diff.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-21T08:32:31.689Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
Applied to files:
openmeter/billing/adapter/invoicelinediff.go
🧬 Code graph analysis (1)
openmeter/billing/adapter/invoicelinediff.go (5)
pkg/entitydiff/parent.go (3)
EqualerWithParent(18-21)WithParent(5-8)NewEqualersWithParent(35-42)openmeter/billing/invoicelinediscount.go (2)
UsageLineDiscountManaged(258-261)AmountLineDiscountManaged(108-111)openmeter/billing/invoiceline.go (3)
Line(311-325)DetailedLine(329-329)LineBase(128-157)pkg/entitydiff/diff.go (6)
Diff(27-31)DiffByID(172-201)DiffByIDInput(157-164)DiffUpdate(16-19)DiffByIDEqualer(214-230)Entity(11-14)pkg/set/set.go (2)
Set(5-8)New(10-16)
⏰ 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). (8)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Code Generators
- GitHub Check: Migration Checks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
pkg/entitydiff/diff.go (1)
213-229: Confirm Equal() semantics include deletion/restore state, or “undelete” may be missedIf DBState is marked deleted and ExpectedState is not (same ID), diffByID pushes an UpdateCandidate. Whether this becomes an Update depends on DBState.Equal(ExpectedState). Verify the Equal implementations for your entities include deletion/restore semantics; if they don’t, “undelete” will be silently ignored.
Would you like me to draft a table-driven test covering: create, delete, update (content), undelete, and “no-op” scenarios across both Equaler and non-Equaler flows?
openmeter/billing/adapter/invoicelinediff.go (1)
80-89: Nice use of parent-aware equalers for discount diffsThe EqualerWithParent wrappers keep parent context handy while leveraging entity equality. This reads clean and composes well with DiffByIDEqualer.
Also applies to: 130-136
turip
force-pushed
the
refactory/entitydiff
branch
2 times, most recently
from
fbcad57 to
3c17fa4
Compare
turip
changed the base branch from
main
to
chore/remove-flat-fee-line-constructor
turip
force-pushed
the
refactory/entitydiff
branch
from
3c17fa4 to
bb56651
Compare
turip
force-pushed
the
refactory/entitydiff
branch
2 times, most recently
from
1886e79 to
6a3da86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/app/stripe/invoice_test.go (2)
751-769: Guard ParentLineID dereference to avoid panicParentLineID may be nil. Add a check before dereferencing.
- lineToRemove := getLine("UBP - FLAT per any usage") + lineToRemove := getLine("UBP - FLAT per any usage") s.NotNil(lineToRemove, "line ID to remove is not found") + s.Require().NotNil(lineToRemove.ParentLineID, "parent line id is nil for %s", lineToRemove.ID) ... - ok = updateInvoice.Lines.RemoveByID(*lineToRemove.ParentLineID) + ok = updateInvoice.Lines.RemoveByID(*lineToRemove.ParentLineID)
780-785: Fix Stripe invoice update filter to use the Stripe line IDComparing Stripe line IDs against OM line IDs is incorrect.
- Data: lo.Filter(stripeInvoice.Lines.Data, func(line *stripe.InvoiceLineItem, _ int) bool { - return line.ID != lineToRemove.ID - }), + Data: lo.Filter(stripeInvoice.Lines.Data, func(line *stripe.InvoiceLineItem, _ int) bool { + return line.ID != stripeLineIDToRemove + }),
♻️ Duplicate comments (1)
pkg/entitydiff/diff.go (1)
185-213: Preflight ID validation to surface duplicates/empties (follow‑up on prior review)Add lightweight checks in DiffByID to detect duplicate or empty IDs in inputs and surface them as joined errors, in addition to the diff.
@@ -import ( - "errors" +import ( + "errors" + "fmt" @@ func DiffByID[T Entity](input DiffByIDInput[T]) error { - diff := diffByID(input.ExpectedState, input.DBState) + // Preflight validation: catch duplicate/empty IDs early + var preflightErrs []error + if d := duplicateIDs(input.DBState); len(d) > 0 { + preflightErrs = append(preflightErrs, fmt.Errorf("dbState has duplicate IDs: %v", d)) + } + if d := duplicateIDs(input.ExpectedState); len(d) > 0 { + preflightErrs = append(preflightErrs, fmt.Errorf("expectedState has duplicate IDs: %v", d)) + } + if idx := emptyIDs(input.DBState); len(idx) > 0 { + preflightErrs = append(preflightErrs, fmt.Errorf("dbState contains entities with empty IDs at indexes: %v", idx)) + } + + diff := diffByID(input.ExpectedState, input.DBState) var errs []error + if len(preflightErrs) > 0 { + errs = append(errs, preflightErrs...) + } @@ return errors.Join(errs...) } + +// helpers +func duplicateIDs[T Entity](items []T) []string { + seen := make(map[string]struct{}, len(items)) + var dups []string + for _, it := range items { + id := it.GetID() + if id == "" { + continue + } + if _, ok := seen[id]; ok { + dups = append(dups, id) + continue + } + seen[id] = struct{}{} + } + return lo.Uniq(dups) +} + +func emptyIDs[T Entity](items []T) []int { + var idxs []int + for i, it := range items { + if it.GetID() == "" && !it.IsDeleted() { + idxs = append(idxs, i) + } + } + return idxs +}
🧹 Nitpick comments (4)
openmeter/billing/worker/subscription/suitebase_test.go (1)
61-63: Slugify and cap test Namespace to avoid invalid chars/lengthsuiteName/testName may contain spaces, slashes, or unicode and can make Namespace invalid or overly long. Safer to slugify and cap length.
Apply e.g.:
- s.Namespace = fmt.Sprintf("t-%s-%s-%s", suiteName, testName, ulid.Make().String()) + safeSuite := slugify(suiteName) + safeTest := slugify(testName) + s.Namespace = fmt.Sprintf("t-%s-%s-%s", safeSuite, safeTest, ulid.Make().String()) + +// helper (in this file) +func slugify(s string) string { + s = strings.ToLower(strings.ReplaceAll(s, " ", "-")) + re := regexp.MustCompile(`[^a-z0-9\-]+`) + out := strings.Trim(re.ReplaceAllString(s, "-"), "-") + if len(out) > 40 { // cap to keep final length reasonable + out = out[:40] + } + return out +}openmeter/billing/adapter/invoicelines.go (1)
300-318: Redundant UpdateIndex after ResolveWithNewValuesResolveWithNewValues already sets all fields from the insert payload. UpdateIndex() is likely redundant.
- OnConflict( - sql.ConflictColumns(billinginvoiceflatfeelineconfig.FieldID), - sql.ResolveWithNewValues(), - ). - UpdateIndex(). - Exec(ctx) + OnConflict( + sql.ConflictColumns(billinginvoiceflatfeelineconfig.FieldID), + sql.ResolveWithNewValues(), + ).Exec(ctx)openmeter/billing/adapter/invoicelinediff_test.go (1)
160-166: Fix minor typos in test names/comments“hieararchy” → “hierarchy”, “tirgger” → “trigger”.
openmeter/billing/adapter/invoicelinediff.go (1)
126-126: Minor: Prefer GetID() method for consistency.Line 126 directly accesses
item.PersistedState.IDwhile the rest of the codebase uses theGetID()method (e.g., line 96, 267).Apply this diff for consistency:
if !item.ExpectedState.IsDeleted() { - diff.AffectedLineIDs.Add(item.PersistedState.ID) + diff.AffectedLineIDs.Add(item.PersistedState.GetID()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openmeter/billing/adapter/invoicelinediff.go(2 hunks)openmeter/billing/adapter/invoicelinediff_test.go(15 hunks)openmeter/billing/adapter/invoicelines.go(7 hunks)openmeter/billing/adapter/upsert.go(4 hunks)openmeter/billing/invoiceline.go(1 hunks)openmeter/billing/invoicelinediscount.go(1 hunks)openmeter/billing/worker/subscription/suitebase_test.go(1 hunks)pkg/entitydiff/diff.go(1 hunks)pkg/entitydiff/parent.go(1 hunks)test/app/stripe/invoice_test.go(3 hunks)test/billing/invoice_test.go(1 hunks)test/billing/suite.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/billing/invoicelinediscount.go
- openmeter/billing/invoiceline.go
- test/billing/invoice_test.go
🧰 Additional context used
🧬 Code graph analysis (8)
test/app/stripe/invoice_test.go (2)
openmeter/billing/service/lineservice/service.go (1)
Lines(223-223)openmeter/ent/db/billinginvoiceline/where.go (1)
ParentLineID(158-160)
test/billing/suite.go (2)
openmeter/billing/invoiceline.go (4)
NewFlatFeeLine(635-677)NewFlatFeeLineInput(596-620)Period(83-86)ManuallyManagedLine(69-69)openmeter/productcatalog/price.go (1)
InArrearsPaymentTerm(21-21)
pkg/entitydiff/parent.go (1)
pkg/entitydiff/diff.go (2)
Entity(11-14)EqualerEntity(215-218)
openmeter/billing/adapter/invoicelines.go (4)
openmeter/billing/invoiceline.go (2)
DetailedLine(329-329)Line(311-325)openmeter/ent/db/billinginvoiceline/where.go (2)
ParentLineID(158-160)ID(17-19)openmeter/ent/db/billinginvoiceflatfeelineconfig/where.go (2)
ID(14-16)Index(79-81)pkg/entitydiff/diff.go (3)
Union(70-84)Entity(11-14)Diff(29-33)
openmeter/billing/adapter/invoicelinediff_test.go (2)
openmeter/billing/invoiceline.go (4)
Line(311-325)DetailedLine(329-329)InvoiceLineTypeUsageBased(35-35)UsageBasedLine(768-781)pkg/entitydiff/diff.go (3)
Entity(11-14)Diff(29-33)DiffUpdate(16-21)
openmeter/billing/worker/subscription/suitebase_test.go (2)
openmeter/ent/db/billinginvoicesplitlinegroup/where.go (1)
Namespace(70-72)openmeter/ent/db/billinginvoiceline/where.go (1)
Namespace(72-74)
openmeter/billing/adapter/invoicelinediff.go (5)
pkg/entitydiff/parent.go (3)
EqualerNestedEntity(18-21)NestedEntity(5-8)NewEqualersWithParent(35-42)openmeter/billing/invoicelinediscount.go (2)
UsageLineDiscountManaged(258-261)AmountLineDiscountManaged(108-111)openmeter/billing/invoiceline.go (3)
Line(311-325)DetailedLine(329-329)LineBase(128-157)pkg/entitydiff/diff.go (6)
Diff(29-33)DiffByID(184-213)DiffByIDInput(169-176)DiffUpdate(16-21)DiffByIDEqualer(226-242)Entity(11-14)pkg/set/set.go (2)
Set(5-8)New(10-16)
openmeter/billing/adapter/upsert.go (2)
pkg/entitydiff/diff.go (3)
Entity(11-14)Diff(29-33)DiffUpdate(16-21)pkg/slicesx/map.go (1)
MapWithErr(25-49)
⏰ 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). (9)
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Artifacts / Container image
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
pkg/entitydiff/parent.go (1)
5-42: Wrappers look goodDelegations and helper are straightforward and correct.
test/billing/suite.go (1)
393-423: Constructor-based line creation is cleaner and consistentGood move to NewFlatFeeLine; inputs cover prior fields, reduces manual wiring.
openmeter/billing/adapter/invoicelines.go (1)
156-172: Confirm detailed diff item type matches upsert configEnsure GetDetailedLineDiffWithParentID returns entitydiff.Diff[*billing.Line] so it matches lineUpsertConfig’s type parameter; otherwise generics won’t align at compile time.
openmeter/billing/adapter/upsert.go (1)
25-33: Row order reliance in single bulk upsertCombining Delete, Create, Update into one CreateBulk relies on per-row order for final state on duplicate IDs. This is fine on Postgres, but verify ent/driver preserves insertion order across the generated VALUES list.
If uncertain, consider issuing three separate upserts (Delete, then Create, then Update) to remove ordering assumptions.
openmeter/billing/adapter/invoicelinediff.go (5)
14-19: LGTM! Well-structured type aliases for nested entity diffing.The type aliases correctly leverage the entitydiff framework:
- Usage and amount discounts use
EqualerNestedEntityfor equality-based diffing- Detailed lines use
NestedEntityfor parent tracking without requiring Equaler implementation at the wrapper level
22-41: LGTM! Clean consolidation using entitydiff types.The struct effectively consolidates the previous per-field diff tracking into a unified entitydiff-based approach, with proper separation of top-level lines and detailed lines with their respective discounts.
66-72: Verify that line type transitions are not expected in updates.The validation requires both
UsageBasedto be non-nil for updates. If a line can transition between types (e.g., FlatFee ↔ UsageBased), these checks would reject valid updates.Does the business logic support line type transitions during updates? If so, this validation is too strict. If not, consider adding a comment explaining that line type changes are handled as delete+create.
164-252: LGTM! Consistent cascade operations for nested entities.The helper methods correctly handle cascading operations for line creation/deletion, properly wrapping entities with their parent references and maintaining consistency across top-level lines, detailed lines, and their respective discounts.
254-275: Method logic is sound but depends on fixing the parent assignment bug.The flattening logic correctly transforms nested
detailedLineWithParentstructures into regular*billing.DetailedLineby extracting theParentLineIDfromParent.GetID(). However, line 267 will read the incorrect parent ID for updates until the bug on line 121 is fixed.Ensure the parent assignment bug on line 121 is fixed before merging, as this method relies on correct parent references.
turip
force-pushed
the
refactory/entitydiff
branch
from
0834f3d to
390fa82
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/invoicelines.go (1)
291-320: Risk: unconditional UpdateIndex() may overwrite/NULL index on conflictOnConflict with ResolveWithNewValues already updates fields set on the create builder. Adding UpdateIndex() forces index updates even when not set, which can zero-out or null the column (depending on EXCLUDED semantics and nullability). Safer to rely on ResolveWithNewValues alone.
Apply this diff to avoid unintended index updates:
return upsertWithOptions(ctx, a.db, in, upsertInput[detailedLineWithParent, *db.BillingInvoiceFlatFeeLineConfigCreate]{ Create: func(tx *db.Client, lineWithParent detailedLineWithParent) (*db.BillingInvoiceFlatFeeLineConfigCreate, error) { line := lineWithParent.Entity @@ create := tx.BillingInvoiceFlatFeeLineConfig.Create(). SetNamespace(line.Namespace). SetPerUnitAmount(line.FlatFee.PerUnitAmount). SetCategory(line.FlatFee.Category). SetPaymentTerm(line.FlatFee.PaymentTerm). SetID(line.FlatFee.ConfigID). SetNillableIndex(line.FlatFee.Index) return create, nil }, UpsertItems: func(ctx context.Context, tx *db.Client, items []*db.BillingInvoiceFlatFeeLineConfigCreate) error { return tx.BillingInvoiceFlatFeeLineConfig. CreateBulk(items...). OnConflict( sql.ConflictColumns(billinginvoiceflatfeelineconfig.FieldID), sql.ResolveWithNewValues(), - ). - UpdateIndex(). - Exec(ctx) + ).Exec(ctx) }, })
🧹 Nitpick comments (4)
pkg/entitydiff/diff.go (1)
86-167: Core diffing logic is correct; consider optional duplicate ID validation.The logic correctly handles entities by ID, including proper filtering of empty IDs (lines 93-95) to prevent map key collisions. Deletion, creation, and update paths are sound.
As noted in a previous review, adding preflight validation to detect duplicate IDs in both
dbStateandexpectedStatewould surface silent overwrites early. This is a recommended enhancement rather than a critical fix, as the current implementation is functional.Based on learnings.
openmeter/billing/adapter/invoicelinediff.go (1)
65-88: LGTM! Nil-safety is correctly implemented.The nil checks for
UsageBasedon bothExpectedStateandPersistedState(lines 66-72) properly guard against panics before callingEqualon line 74. The discount diffing logic usingNewEqualersWithParentandDiffByIDEqualeris correct.As noted in previous reviews, the nil-safety concern has been addressed.
Optional: Consider more descriptive error messages.
The error messages on lines 67 and 71 could include the line ID for easier debugging:
return fmt.Errorf("expected state usage based is nil for line %s", item.ExpectedState.GetID())openmeter/billing/adapter/invoicelinediff_test.go (1)
319-331: Optional: consider labeling updates by ExpectedStatemapDiffToIDs uses PersistedState to label updates. If you want messages to reflect the post-change identity (e.g., after description edits), switch to ExpectedState; current approach is consistent and stable though.
pkg/entitydiff/parent.go (1)
1-42: Parent-aware wrappers LGTMNestedEntity/EqualerNestedEntity delegate identity/deletion and equality correctly. NewEqualersWithParent is handy.
If useful, add a non-equaler constructor (NewWithParent) mirroring NewEqualersWithParent for plain Entity wrappers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
openmeter/billing/adapter/invoicelinediff.go(2 hunks)openmeter/billing/adapter/invoicelinediff_test.go(15 hunks)openmeter/billing/adapter/invoicelines.go(7 hunks)openmeter/billing/adapter/upsert.go(4 hunks)openmeter/billing/invoiceline.go(1 hunks)openmeter/billing/invoicelinediscount.go(1 hunks)openmeter/billing/worker/subscription/suitebase_test.go(1 hunks)pkg/entitydiff/diff.go(1 hunks)pkg/entitydiff/parent.go(1 hunks)test/app/stripe/invoice_test.go(3 hunks)test/billing/invoice_test.go(1 hunks)test/billing/suite.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/billing/worker/subscription/suitebase_test.go
- test/billing/suite.go
- test/app/stripe/invoice_test.go
🧰 Additional context used
🧬 Code graph analysis (8)
pkg/entitydiff/parent.go (1)
pkg/entitydiff/diff.go (2)
Entity(11-14)EqualerEntity(215-218)
openmeter/billing/adapter/invoicelinediff_test.go (2)
openmeter/billing/invoiceline.go (4)
Line(311-325)DetailedLine(329-329)InvoiceLineTypeUsageBased(35-35)UsageBasedLine(768-781)pkg/entitydiff/diff.go (3)
Entity(11-14)Diff(29-33)DiffUpdate(16-21)
test/billing/invoice_test.go (2)
openmeter/billing/invoiceline.go (4)
NewFlatFeeLine(635-677)NewFlatFeeLineInput(596-620)Period(83-86)ManuallyManagedLine(69-69)openmeter/productcatalog/price.go (1)
InAdvancePaymentTerm(20-20)
openmeter/billing/adapter/invoicelines.go (4)
openmeter/billing/invoiceline.go (2)
DetailedLine(329-329)Line(311-325)openmeter/ent/db/billinginvoiceline/where.go (2)
ParentLineID(158-160)ID(17-19)openmeter/ent/db/billinginvoiceflatfeelineconfig/where.go (2)
ID(14-16)Index(79-81)pkg/entitydiff/diff.go (3)
Union(70-84)Entity(11-14)Diff(29-33)
openmeter/billing/adapter/upsert.go (2)
pkg/entitydiff/diff.go (3)
Entity(11-14)Diff(29-33)DiffUpdate(16-21)pkg/slicesx/map.go (1)
MapWithErr(25-49)
openmeter/billing/adapter/invoicelinediff.go (5)
pkg/entitydiff/parent.go (3)
EqualerNestedEntity(18-21)NestedEntity(5-8)NewEqualersWithParent(35-42)openmeter/billing/invoicelinediscount.go (2)
UsageLineDiscountManaged(258-261)AmountLineDiscountManaged(108-111)openmeter/billing/invoiceline.go (3)
Line(311-325)DetailedLine(329-329)LineBase(128-157)pkg/entitydiff/diff.go (6)
Diff(29-33)DiffByID(184-213)DiffByIDInput(169-176)DiffUpdate(16-21)DiffByIDEqualer(226-242)Entity(11-14)pkg/set/set.go (2)
Set(5-8)New(10-16)
openmeter/billing/invoicelinediscount.go (3)
openmeter/ent/db/billinginvoiceline/where.go (1)
Description(97-99)openmeter/ent/db/billinginvoicelineusagediscount/where.go (1)
Description(101-103)openmeter/ent/db/billinginvoicelinediscount/where.go (1)
Description(101-103)
openmeter/billing/invoiceline.go (1)
openmeter/billing/service/lineservice/service.go (1)
Line(206-221)
⏰ 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). (6)
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Code Generators
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
pkg/entitydiff/diff.go (3)
11-33: LGTM! Clean interface and type definitions.The
Entityinterface is minimal and well-defined. TheDiffUpdateandDifftypes clearly distinguish between persisted and expected states. Documentation is helpful.
35-84: LGTM! Diff operations are correctly implemented.The mutation methods and combination logic are clean. Pre-allocating capacity in
Appendis a good practice.
178-242: LGTM! Public API is well-structured.The callback-based
DiffByIDallows flexible handling of diff operations while maintaining error aggregation.DiffByIDEqualerappropriately filters update candidates to only include actual changes via theEqualmethod.openmeter/billing/adapter/invoicelinediff.go (5)
13-20: LGTM! Clean type aliases for nested entities.Using type aliases for the nested entity wrappers improves readability throughout the file. The choice between
EqualerNestedEntity(for discounts) andNestedEntity(for detailed lines) is appropriate based on whether equality checking is needed.
22-41: LGTM! Well-structured diff container.The separation between top-level
Linediffs and nestedDetailedLinediffs is clear. Usingset.Setto track affected line IDs forUpdatedAtpropagation is a good design choice.
43-58: LGTM! Clear setup with helpful context.Building
dbStatefrom individualline.DBStatefields is straightforward. The comment explaining the per-line approach and future considerations is helpful.
90-149: LGTM! Nested diffing is correctly implemented.The nested
DiffByIDforChildrenproperly handles the parent-child relationship. Critically, line 121 correctly setsExpectedState.Parenttoitem.ExpectedState(notitem.PersistedState), which ensures that parent ID propagation and potential reparent operations work correctly. This fix addresses the critical concern raised in previous reviews.The logic for tracking affected line IDs (lines 125-127, 138-144) and nested discount diffing (lines 130-145) is sound.
As noted in previous reviews, the parent assignment bug has been resolved.
164-275: LGTM! Helper methods are correctly structured.The
Delete*andCreate*helpers consistently wrap entities with their parents, maintaining the parent-child relationships throughout the diff process.
GetDetailedLineDiffWithParentIDcorrectly unwraps the parent context and setsParentLineIDfromParent.GetID()(line 267), ensuring the parent ID is properly propagated to the final diff result.openmeter/billing/invoicelinediscount.go (1)
51-53: Accessor addition LGTMNon-mutating getter is fine and aligns with usage in entitydiff-based tests.
openmeter/billing/invoiceline.go (1)
327-330: Type alias is safe and clearAlias improves readability without behavioral change.
test/billing/invoice_test.go (1)
646-660: Builder usage LGTMSwitch to NewFlatFeeLine(NewFlatFeeLineInput) matches the new construction path and keeps expectations intact.
openmeter/billing/adapter/invoicelinediff_test.go (2)
17-33: Restructured diff expectations look goodidDiff and expanded expectations (DetailedLine*, affected IDs) match the new nested entitydiff model.
356-367: Diff assertions read wellGeneric requireIdDiffMatches usage against all components (including nested) is succinct and maintainable.
openmeter/billing/adapter/upsert.go (1)
19-26: Entitydiff-based upsert integration looks correct
- T constrained to entitydiff.Entity and Diff[T] adoption is clean.
- Delete-as-soft-update path is sensible; create/update paths materialize payloads from items and ExpectedState as expected.
Please confirm all call sites that pass a delete diff also provide a MarkDeleted implementation; otherwise deletes will be skipped by design.
Also applies to: 32-47, 60-69
openmeter/billing/adapter/invoicelines.go (1)
56-63: Upsert flow ordering and unions LGTM
- Config upserts before line upserts are correct.
- Restoring ParentLineID before detailed upserts prevents FK issues.
- Using entitydiff.Union for amount discounts is appropriate.
Also applies to: 158-172, 222-225
2 participants
Overview
This change splits the previous version of the linediffer into two seperate parts:
The specific implementation is structured in a way that it is super easy to split the Line/DetailedLine entities into two going forward.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests