refactor(billing): use interface based parent relationships #3232
Conversation
This patch changes the invoidelinediff to use interface based parent/entity relationsips in anticipation of the Line/DetailedLine split, where the two types would behave the same but would be different go types implementing the same interfaces.
📝 WalkthroughWalkthroughMoves per-line identity into a new models.ManagedResource, adds model getters, and refactors invoice-line discount diffs to a parent-oriented generic model; updates mapping, HTTP driver, worker code, and tests to use ManagedResource and parent-bound discount types. 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
the
release-note/ignore
![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
🧹 Nitpick comments (17)
pkg/models/model.go (1)
167-178: ManagedModel getters align with interface-based accessThese accessors make timestamp reads explicit without exposing fields. Consider documenting that these return UTC-normalized values as produced by NewManagedResource (minor).
openmeter/billing/adapter/invoicelines.go (1)
190-192: Verify Parent Bindings for DiscountsPlease manually confirm that moving to d.Parent.GetNamespace()/GetID() never encounters a missing or empty parent, and that all downstream reads load discounts off the parent line:
- File openmeter/billing/adapter/invoicelines.go
• Lines 190–192 and 236–238: calls to SetNamespace(d.Parent.GetNamespace()) and SetLineID(d.Parent.GetID()). Ensured.Parentis always non-nil and implements these getters.- Wrapper types
usageLineDiscountMangedWithLine&amountLineDiscountMangedWithLine: verify theirParentfield is always populated at creation time.- Mapping/queries that load discounts (e.g.,
With(LineUsageDiscounts|LineAmountDiscounts),Edges.LineUsageDiscounts,Edges.LineAmountDiscounts): confirm they expect discounts on parent lines, not children.If there’s any risk of a missing parent ID/namespace here, add a defensive check to fail fast.
openmeter/billing/service/lineservice/usagebasedline_test.go (1)
53-56: Minimal ManagedResource construction in test is fine; consider setting timestamps to reduce future couplingYou’re only setting ID/Name, which is enough for this unit test. To future-proof against potential Validate() calls, you could also set Namespace and non-zero CreatedAt/UpdatedAt (in UTC).
Apply this diff if you want slightly more robust test data:
ManagedResource: models.NewManagedResource(models.ManagedResourceInput{ ID: "fake-line", - Name: "feature", + Name: "feature", + Namespace: "test-ns", + CreatedAt: time.Now(), + UpdatedAt: time.Now(), }),test/billing/suite.go (2)
341-346: ManagedResource: set only Namespace/Name — OK; optional: add timestamps for consistencyThis matches the refactor goal. Optional: set CreatedAt/UpdatedAt to non-zero UTC values to avoid surprises if Validate() is invoked in the future.
ManagedResource: models.NewManagedResource(models.ManagedResourceInput{ Namespace: namespace, Name: "Test item1", + CreatedAt: time.Now().UTC(), + UpdatedAt: time.Now().UTC(), }),
367-372: Same note for the second line’s ManagedResourceConsider adding non-zero timestamps to align with ManagedModel expectations, even if not strictly required today.
ManagedResource: models.NewManagedResource(models.ManagedResourceInput{ Namespace: namespace, Name: "Test item2", + CreatedAt: time.Now().UTC(), + UpdatedAt: time.Now().UTC(), }),openmeter/billing/worker/subscription/sync_test.go (1)
443-446: Setting only Name in ManagedResource is sufficient for this test; optional: add Namespace/timestampsIf future code paths call resource validation, a minimal Namespace and non-zero timestamps will help. Not required for current assertions.
ManagedResource: models.NewManagedResource(models.ManagedResourceInput{ - Name: "UBP - unit", + Name: "UBP - unit", + Namespace: "test-ns", + CreatedAt: time.Now().UTC(), + UpdatedAt: time.Now().UTC(), }),openmeter/billing/invoice_test.go (1)
17-21: Good migration to ManagedResource; minor note on field promotion in assertionsUsing models.NewManagedResource to populate LineBase identity is consistent with the refactor. The test later relies on promoted fields (e.g., lines[i].Description) from the embedded ManagedResource, which is fine today. If you plan to move towards interface-based composition, consider accessing via the embedded field explicitly (ManagedResource.Description) or a getter if one exists to reduce coupling to embedding.
Also applies to: 29-34, 41-46, 55-59
test/app/stripe/invoice_test.go (1)
300-302: Consistent switch to ManagedResource across test line initializationsAll affected test cases now construct LineBase.ManagedResource via models.NewManagedResource with the expected Name. This keeps tests aligned with the new identity model without changing behavior of downstream assertions that use Name/Description.
To reduce repetition and improve readability, consider a tiny helper within the test:
// near the top of the file func mr(name string) models.ManagedResource { return models.NewManagedResource(models.ManagedResourceInput{Name: name}) } // usage: LineBase: billing.LineBase{ ManagedResource: mr("Fee"), // ... }Also applies to: 318-321, 339-342, 357-360, 377-380, 414-417, 1115-1117
openmeter/billing/adapter/invoicelinediff_test.go (2)
392-400: Remove unused testing.T parameter from helpert is unused in mapLineDiscountsToIDs. Dropping it simplifies the helper without impacting call sites beyond this file.
Apply this diff:
-func mapLineDiscountsToIDs(t *testing.T, discounts []withParent[billing.AmountLineDiscountManaged, *billing.Line]) []string { - return lo.Map(discounts, func(d withParent[billing.AmountLineDiscountManaged, *billing.Line], _ int) string { +func mapLineDiscountsToIDs(discounts []withParent[billing.AmountLineDiscountManaged, *billing.Line]) []string { + return lo.Map(discounts, func(d withParent[billing.AmountLineDiscountManaged, *billing.Line], _ int) string { if d.Discount.Description != nil { return *d.Discount.Description } return d.Discount.GetID() }) }Additionally, update the only call site accordingly (see next comment).
402-408: Align wrapper helper signature with removed parameterAdjust mapLineDiscountDiffToIDs to match the simplified helper and update its usages.
Apply this diff:
-func mapLineDiscountDiffToIDs(t *testing.T, in diff[withParent[billing.AmountLineDiscountManaged, *billing.Line]]) idDiff { +func mapLineDiscountDiffToIDs(in diff[withParent[billing.AmountLineDiscountManaged, *billing.Line]]) idDiff { return idDiff{ - ToCreate: mapLineDiscountsToIDs(t, in.ToCreate), - ToUpdate: mapLineDiscountsToIDs(t, in.ToUpdate), - ToDelete: mapLineDiscountsToIDs(t, in.ToDelete), + ToCreate: mapLineDiscountsToIDs(in.ToCreate), + ToUpdate: mapLineDiscountsToIDs(in.ToUpdate), + ToDelete: mapLineDiscountsToIDs(in.ToDelete), } }You’ll also need to update the call in requireDiffWithoutChildren to drop the t argument:
// current: requireIdDiffMatches(t, expected.AmountDiscounts, mapLineDiscountDiffToIDs(t, actual.AmountDiscounts), prefix+": AmountDiscounts") // suggested: requireIdDiffMatches(t, expected.AmountDiscounts, mapLineDiscountDiffToIDs(actual.AmountDiscounts), prefix+": AmountDiscounts")openmeter/billing/service/lineservice/detailedlines.go (1)
140-143: ManagedResource initialization is aligned; consider avoiding shadowing for readabilitySetting Namespace from the parent and Name from the input is correct and consistent with the refactor. Minor readability nit: later in this function a local variable named line shadows the parameter line, which can be confusing when reading the struct literal that references line.Namespace et al. Consider renaming the local variable (e.g., detailedLine or child) to avoid shadowing.
Example (outside this hunk):
detailedLine := &billing.Line{ LineBase: billing.LineBase{ ManagedResource: models.NewManagedResource(models.ManagedResourceInput{ Namespace: line.Namespace, Name: in.Name, }), // ... }, // ... } if err := detailedLine.Validate(); err != nil { return nil, err } return detailedLine, nilopenmeter/billing/adapter/invoicelinediff.go (3)
71-83: Harden against empty parent IDs being added to AffectedLineIDsIf an implementation mistakenly returns ok=true with an empty parent ID, the current code would add an empty string to the affected set. Guard against this to avoid accidental no-op updates or downstream surprises.
Apply this diff:
func getIDAndParentID(e entityParent) []string { out := make([]string, 0, 2) if e.GetID() != "" { out = append(out, e.GetID()) } - if parentID, ok := e.GetParentID(); ok { - out = append(out, parentID) + if parentID, ok := e.GetParentID(); ok && parentID != "" { + out = append(out, parentID) } return out }
85-91: Avoid adding empty parent IDsSame concern as above for the parent-only helper.
Apply this diff:
func getParentIDAsSlice(e entityParent) []string { - if parentID, ok := e.GetParentID(); ok { - return []string{parentID} + if parentID, ok := e.GetParentID(); ok && parentID != "" { + return []string{parentID} } return nil }
98-101: Typo in type alias names: “Manged” → “Managed” (add corrected aliases without breaking changes)The current aliases have a small typo. Add corrected aliases alongside the existing ones to keep this change non-breaking.
Apply this diff:
type ( usageLineDiscountMangedWithLine = withParent[billing.UsageLineDiscountManaged, *billing.Line] amountLineDiscountMangedWithLine = withParent[billing.AmountLineDiscountManaged, *billing.Line] + // Backward-compatible aliases with corrected spelling + usageLineDiscountManagedWithLine = usageLineDiscountMangedWithLine + amountLineDiscountManagedWithLine = amountLineDiscountMangedWithLine )test/billing/discount_test.go (1)
157-159: Minor naming inconsistency: RatecardDiscounts vs RateCardDiscountsThe test references SplitLineHierarchy.Group.RatecardDiscounts while other places use RateCardDiscounts. If both are intentional due to differing types, ignore; otherwise, consider aligning the casing for consistency.
openmeter/billing/httpdriver/invoiceline.go (1)
427-429: Fix copy-paste error in error message ("amount" vs "usage")The error path for mapping usage discounts still says "amount discounts".
Apply this diff:
- return nil, fmt.Errorf("failed to map amount discounts: %w", err) + return nil, fmt.Errorf("failed to map usage discounts: %w", err)openmeter/billing/invoiceline.go (1)
172-175: Validate still requires Namespace; ensure upstream injects itWith identity now under ManagedResource, keeping the Namespace requirement is fine, but it relies on upstream (e.g., service/http mapping) to set Namespace before calling LineBase.Validate(). Tests often pass Name-only lines; please confirm service guarantees namespace injection prior to validation.
If useful, we could add a helper to derive/set Namespace from request context or CustomerID consistently before validation.
📜 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/invoicelinediff.go(9 hunks)openmeter/billing/adapter/invoicelinediff_test.go(3 hunks)openmeter/billing/adapter/invoicelinemapper.go(1 hunks)openmeter/billing/adapter/invoicelines.go(2 hunks)openmeter/billing/httpdriver/invoiceline.go(4 hunks)openmeter/billing/invoice_test.go(2 hunks)openmeter/billing/invoiceline.go(4 hunks)openmeter/billing/service/lineservice/detailedlines.go(2 hunks)openmeter/billing/service/lineservice/usagebasedline_test.go(1 hunks)openmeter/billing/worker/subscription/sync.go(2 hunks)openmeter/billing/worker/subscription/sync_test.go(1 hunks)pkg/models/model.go(3 hunks)test/app/custominvoicing/invocing_test.go(1 hunks)test/app/stripe/invoice_test.go(7 hunks)test/billing/adapter_test.go(1 hunks)test/billing/collection_test.go(6 hunks)test/billing/discount_test.go(2 hunks)test/billing/invoice_test.go(19 hunks)test/billing/suite.go(2 hunks)test/billing/tax_test.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:
test/billing/suite.gotest/app/custominvoicing/invocing_test.gotest/app/stripe/invoice_test.goopenmeter/billing/worker/subscription/sync.gotest/billing/invoice_test.gotest/billing/discount_test.gotest/billing/collection_test.go
🧬 Code Graph Analysis (18)
test/billing/tax_test.go (2)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/billing/invoiceline.go (1)
Period(82-85)
test/billing/adapter_test.go (1)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)
openmeter/billing/worker/subscription/sync_test.go (1)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)
test/billing/suite.go (2)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/billing/invoiceline.go (1)
Period(82-85)
test/app/custominvoicing/invocing_test.go (2)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/ent/db/app/where.go (1)
Name(90-92)
openmeter/billing/service/lineservice/usagebasedline_test.go (1)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)
openmeter/billing/adapter/invoicelinemapper.go (2)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)pkg/convert/time.go (1)
TimePtrIn(5-10)
openmeter/billing/adapter/invoicelinediff.go (3)
openmeter/billing/invoicelinediscount.go (2)
UsageLineDiscountManaged(254-257)AmountLineDiscountManaged(104-107)openmeter/billing/invoiceline.go (1)
Line(305-319)openmeter/billing/service/lineservice/service.go (1)
Line(206-221)
openmeter/billing/httpdriver/invoiceline.go (2)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/billing/invoiceline.go (3)
InvoiceLineTypeUsageBased(34-34)ManuallyManagedLine(68-68)InvoiceLineStatusValid(48-48)
test/app/stripe/invoice_test.go (3)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/ent/db/billinginvoiceline/where.go (1)
Name(92-94)openmeter/ent/db/app/where.go (1)
Name(90-92)
openmeter/billing/worker/subscription/sync.go (2)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/productcatalog/ratecard.go (1)
RateCard(29-42)
test/billing/invoice_test.go (1)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)
openmeter/billing/service/lineservice/detailedlines.go (1)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)
openmeter/billing/invoice_test.go (3)
openmeter/billing/invoiceline.go (4)
Line(305-319)LineBase(127-156)Period(82-85)FlatFeeLine(283-292)pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/ent/db/billinginvoiceflatfeelineconfig/where.go (1)
Index(79-81)
test/billing/discount_test.go (2)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/billing/invoiceline.go (1)
Period(82-85)
test/billing/collection_test.go (1)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)
openmeter/billing/adapter/invoicelinediff_test.go (3)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)openmeter/billing/invoiceline.go (2)
LineBase(127-156)Line(305-319)openmeter/billing/invoicelinediscount.go (1)
AmountLineDiscountManaged(104-107)
openmeter/billing/invoiceline.go (1)
pkg/models/model.go (3)
ManagedResource(23-31)NewManagedResource(75-97)ManagedResourceInput(45-53)
⏰ 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: Code Generators
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (29)
pkg/models/model.go (2)
33-44: Accessor methods on ManagedResource look goodAdding GetID/GetDescription/GetName exposes read-only access cleanly. Value receivers are fine here; they’ll work for both value and pointer instances.
216-219: NamespacedModel.GetNamespace is consistent with the new interface patternThis enables using NamespacedModel (and ManagedResource via embedding) as a parent provider for namespace reads.
openmeter/billing/invoice_test.go (1)
10-10: LGTM: models import aligns with ManagedResource usageImporting models is necessary for constructing ManagedResource; change looks correct.
test/app/custominvoicing/invocing_test.go (1)
183-185: Adoption of ManagedResource for line identity looks correctReplacing the direct LineBase.Name assignment with ManagedResource initialization matches the new identity model. The rest of the LineBase setup is unchanged and consistent.
openmeter/billing/adapter/invoicelinediff_test.go (1)
33-36: Line identity via ManagedResource IDs is correctInitializing LineBase.ManagedResource with explicit IDs for parent and child lines preserves deterministic diff behavior and keeps the tests readable. No issues found.
Also applies to: 42-45, 51-54, 61-64
openmeter/billing/service/lineservice/detailedlines.go (1)
12-12: LGTM: models importImporting models is required for ManagedResource construction; no issues here.
test/billing/adapter_test.go (1)
101-104: Migration to ManagedResource in test helper looks correctInitializing LineBase.ManagedResource via models.NewManagedResource with Namespace and Name aligns with the new identity model and keeps the helper focused. No issues spotted.
test/billing/tax_test.go (1)
209-214: LGTM: line identity via ManagedResource is correctly setUsing models.NewManagedResource with Namespace and Name for CreatePendingInvoiceLines is consistent with the refactor and should preserve downstream behavior (including Name-based assertions).
openmeter/billing/adapter/invoicelinemapper.go (1)
133-141: Correct mapping of DB identity into ManagedResource (with UTC normalization)Passing Namespace, ID, Name, Description, and normalized CreatedAt/UpdatedAt/DeletedAt into models.NewManagedResource is the right way to capture line identity. This keeps identity concerns encapsulated and avoids leaking top-level fields.
openmeter/billing/adapter/invoicelinediff.go (3)
412-420: Generic result type for discount handling is clear and extensibleThe handleLineDiscountsResult wrapper cleanly couples diffs with affected line IDs, which keeps the call sites simple and enables parent-generic reuse. No issues.
417-447: Discount create/update/delete handling correctly propagates affected line IDs on updatesFor operationUpdate, setting affected IDs to both the line and its parent via getIDAndParentID(parentLine) makes sense given UpdatedAt propagation semantics. Create/delete paths for discounts are covered either by line create/delete or child parent updates elsewhere.
If your intent is to also “touch” the parent when discounts are created/deleted during an update operation (not full line create/delete), this is already achieved because updateDiffs.IsEmpty() will be false for create/delete diffs. If you intended different behavior, please clarify.
452-505: Diff computation for discount updates is sound
- Deletes anything missing from the current set,
- Creates entries with empty IDs,
- Updates when IDs match but contents differ.
This aligns with common diff patterns and should be stable.
openmeter/billing/worker/subscription/sync.go (1)
591-596: ManagedResource composition is correct for subscription-driven linesFeeding Namespace, Name, and Description into models.NewManagedResource centralizes identity/labels. This keeps the worker aligned with the new LineBase shape. No issues.
test/billing/discount_test.go (2)
93-99: ManagedResource migration in tests looks correctInitializing LineBase with models.NewManagedResource and setting Period is aligned with the refactor. No additional fields (ID/timestamps) are required for these test scenarios.
Also applies to: 97-97
290-297: LGTM on second ManagedResource usageSame pattern applied consistently: ManagedResource with Namespace/Name and explicit Period. This keeps tests in sync with the new identity model.
openmeter/billing/httpdriver/invoiceline.go (4)
23-23: Importing models is expected for ManagedResource mappingThe new import is required and appropriate for constructing ManagedResource.
141-151: Creating lines with ManagedResource is correct and future-proofPopulating LineBase via models.NewManagedResource with Namespace/Name/Description cleanly migrates identity under ManagedResource. The rest of the mapping (status/type/period) is unchanged and looks good.
553-571: Simulation mapping omits Namespace; verify expectationsmapSimulationLineToEntity sets ID/Name/Description but not Namespace. If simulation code paths don’t require validation of Namespace, this is fine; otherwise, it could trip Validate() which requires a Namespace at the LineBase level.
Would you like me to scan the service layer to confirm Namespace is injected before any validation on simulation paths?
629-686: Merging updates into ManagedResource-backed fields is soundAssigning Metadata/Name/Description into the embedded ManagedResource through LineBase preserves the new identity model and maintains backward compatibility for field access. No issues spotted.
test/billing/invoice_test.go (3)
181-184: LGTM: tests now construct line identities via ManagedResourceUsing models.NewManagedResource for Name-only lines aligns with the refactor and keeps tests focused on behavior rather than IDs/timestamps.
238-245: Good: expected line uses full ManagedResource (ID/Namespace/timestamps)Populating ID/Namespace and CreatedAt/UpdatedAt on the expected object ensures deterministic comparisons. NewManagedResource will normalize to UTC, which is correctly accounted for here.
644-649: Namespace injection for tests using Name-only ManagedResourceSeveral tests create lines with only Name set in ManagedResource. That’s fine if the service layer injects the Namespace from the provided Customer before validation. If not, LineBase.Validate() (which requires Namespace) could fail.
I can check the service implementation path (CreatePendingInvoiceLines) to ensure namespace is applied to test-provided lines before validation. Do you want me to run that check?
Also applies to: 1418-1421, 1456-1463, 1492-1499, 1636-1643, 2318-2321, 2700-2707, 2736-2743, 3190-3196, 3357-3364, 3478-3485, 3636-3643, 3732-3739, 3846-3853
openmeter/billing/invoiceline.go (4)
126-129: Embedding ManagedResource in LineBase: solid migrationThis cleanly centralizes identity (ID/Namespace/Name/Description/timestamps). Field promotion maintains backward-compatible field access.
162-167: New helper GetParentID is a useful convenienceSimple and avoids nil-pointer checks at call sites. No issues.
616-627: Constructors populate ManagedResource correctlyNewFlatFeeLine initializes ManagedResource with Namespace/ID/timestamps/Name/Description via NewManagedResource. This keeps identity cohesive and consistent. Good use of UTC normalization via constructor.
678-686: Usage-based flat-fee constructor follows the same patternSame ManagedResource construction and UTC normalization. This keeps both constructors aligned.
test/billing/collection_test.go (3)
19-20: Importing models for ManagedResource is expectedTests now depend on models.NewManagedResource; import is correct.
85-88: Tests correctly set ManagedResource on linesUsing models.NewManagedResource for Name (and Namespace in one case) aligns with the refactor. The rest of the line construction (period/invoiceAt/type/managedBy) remains unchanged.
Also applies to: 100-103, 345-348, 448-451, 492-496
85-88: Confirm Namespace handling for Name-only linesLike in other test files, some lines only set Name in ManagedResource. Ensure the service assigns Namespace from the Customer before validation; otherwise LineBase.Validate() would fail on missing Namespace.
If you want, I can scan the service path to verify the injection order and add a guard test to catch regressions.
Also applies to: 100-103, 345-348, 448-451, 492-496
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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(10 hunks)openmeter/billing/adapter/invoicelines.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
openmeter/billing/adapter/invoicelinediff.go (2)
openmeter/billing/invoicelinediscount.go (2)
UsageLineDiscountManaged(254-257)AmountLineDiscountManaged(104-107)openmeter/billing/invoiceline.go (1)
Line(305-319)
openmeter/billing/adapter/invoicelines.go (3)
openmeter/ent/schema/billing.go (10)
BillingInvoiceLineUsageDiscount(758-760)BillingInvoiceLineUsageDiscount(762-769)BillingInvoiceLineUsageDiscount(771-795)BillingInvoiceLineUsageDiscount(797-806)BillingInvoiceLineUsageDiscount(808-816)BillingInvoiceLineDiscount(675-677)BillingInvoiceLineDiscount(679-686)BillingInvoiceLineDiscount(688-735)BillingInvoiceLineDiscount(737-746)BillingInvoiceLineDiscount(748-756)openmeter/ent/db/billinginvoicelineusagediscount.go (2)
BillingInvoiceLineUsageDiscount(19-51)BillingInvoiceLineUsageDiscount(74-93)openmeter/ent/db/billinginvoicelinediscount.go (2)
BillingInvoiceLineDiscount(19-63)BillingInvoiceLineDiscount(86-105)
⏰ 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: E2E
- GitHub Check: Quickstart
- GitHub Check: Analyze (go)
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (8)
openmeter/billing/adapter/invoicelinediff.go (5)
256-257: Good: parent affected set updated on line delete.Adding the parent ID to AffectedLineIDs when deleting a line is correct and keeps UpdatedAt in sync on the parent.
300-305: Good: parent affected set updated on base updates.Bumping the parent’s UpdatedAt when the base changes maintains the intended propagation.
328-329: Good: capturing parent updates for child create/delete.Marking the parent as affected when child lines are created/deleted ensures consistency without re-traversing hierarchy.
Also applies to: 345-346, 373-374
386-388: Generic parent-bound discount handling is sound.Passing the parent line into the generic discount diffing keeps the flow uniform and enables reuse for future DetailedLine.
Also applies to: 392-395, 401-403
445-446: Good: updating both the line and its parent on discount changes.Using getIDAndParentID ensures UpdatedAt is bumped for the line and its immediate parent when discounts change.
openmeter/billing/adapter/invoicelines.go (3)
180-192: Good: attach discount identity to the parent line (namespace + line ID).Deriving Namespace and LineID from the parent line eliminates ambiguity and aligns with the interface-based parent approach.
215-217: Good: consistent soft-delete on discounts.Using MarkDeleted to set DeletedAt on the discount entity keeps deletion semantics uniform across usage and amount discounts.
Also applies to: 261-263
236-239: Good: persist SourceDiscount for amount discounts.Populating SourceDiscount with the typed Reason improves traceability and matches the ent schema.
| type entityParent interface { | ||
| // Get the ID of the parent entity | ||
| GetID() string | ||
| // Get the parent ID of the parent (if available) | ||
| GetParentID() (string, bool) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
*Confirm interface conformance for billing.Line (and tighten comment wording).
- Ensure that *billing.Line implements:
- GetID() string
- GetParentID() (string, bool) — returning ok=false when parent is unset or empty.
- Minor: the comments read as “parent of the parent” which can be confusing. Prefer “entity ID” and “entity’s parent ID”.
[suggest_nitpick]
Run this to verify conformance and presence of getters:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check entityParent definition (for reference)
rg -n -C2 --type=go 'type\s+entityParent\s+interface' openmeter | sed -n '1,200p'
# Verify *billing.Line methods used by entityParent
rg -nP -C3 --type=go '^\s*func\s*\(\s*\*?Line\s*\)\s+GetID\s*\(\s*\)\s+string\b' openmeter/billing
rg -nP -C3 --type=go '^\s*func\s*\(\s*\*?Line\s*\)\s+GetParentID\s*\(\s*\)\s*\(\s*string\s*,\s*bool\s*\)\b' openmeter/billing
# Also verify GetNamespace() used in upsert flows
rg -nP -C3 --type=go '^\s*func\s*\(\s*\*?Line\s*\)\s+GetNamespace\s*\(\s*\)\s+string\b' openmeter/billingLength of output: 527
*Implement entityParent on billing.Line and clarify comments
- *billing.Line is missing the interface methods—please add:
func (l *Line) GetID() string { return l.ID } func (l *Line) GetParentID() (string, bool) { if l.ParentID == "" { return "", false } return l.ParentID, true }
- In openmeter/billing/adapter/invoicelinediff.go (lines 64–69), update the interface docs to:
// entityParent represents an entity with an ID and an optional parent ID. type entityParent interface { // GetID returns the entity’s ID. GetID() string // GetParentID returns the entity’s parent ID (ok=false if unset). GetParentID() (string, bool) }
These changes ensure *billing.Line conforms to entityParent and improve comment clarity.
🤖 Prompt for AI Agents
In openmeter/billing/adapter/invoicelinediff.go around lines 64 to 69, update
the interface comment to: "entityParent represents an entity with an ID and an
optional parent ID." and change the method docs to "GetID returns the entity’s
ID." and "GetParentID returns the entity’s parent ID (ok=false if unset)." Also
implement entityParent on *billing.Line by adding methods on the Line pointer
receiver: GetID returning the Line's ID string, and GetParentID returning the
ParentID and a boolean indicating whether ParentID is set (false when empty);
ensure no other behavior changes.
| for _, discount := range items { | ||
| out.diff.NeedsDelete(withLine[T]{ | ||
| Discount: discount, | ||
| Line: line, | ||
| out.diff.NeedsDelete(withParent[T, P]{ | ||
| Entity: discount, | ||
| Parent: parentLine, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Bug: discount delete uses “current items” instead of DB items.
When deleting a line, the delete branch only iterates over the “items” (current payload). Discounts present only in the DB (which is the typical case on line deletion) won’t be marked deleted. This will leave active discounts dangling on a deleted line.
Switch to using dbItems in the delete branch.
Apply:
- case operationDelete:
- for _, discount := range items {
+ case operationDelete:
+ for _, discount := range dbItems {
out.diff.NeedsDelete(withParent[T, P]{
Entity: discount,
Parent: parentLine,
})
}Follow-up: please add a test that deletes a line with existing discounts (not present in the input payload) and asserts those discounts are soft-deleted (DeletedAt set).
📝 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 _, discount := range items { | |
| out.diff.NeedsDelete(withLine[T]{ | |
| Discount: discount, | |
| Line: line, | |
| out.diff.NeedsDelete(withParent[T, P]{ | |
| Entity: discount, | |
| Parent: parentLine, | |
| }) | |
| } | |
| case operationDelete: | |
| for _, discount := range dbItems { | |
| out.diff.NeedsDelete(withParent[T, P]{ | |
| Entity: discount, | |
| Parent: parentLine, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In openmeter/billing/adapter/invoicelinediff.go around lines 431 to 436, the
delete branch currently iterates over the incoming payload "items" (so discounts
only in DB are missed) — change that loop to iterate over dbItems so discounts
that exist only in the DB are marked for deletion; update the
out.diff.NeedsDelete calls to use withParent wrapping each dbItem and parentLine
as before. Also add a unit test that deletes an invoice line while DB-only
discounts exist: run the diff logic and assert those discounts are soft-deleted
(DeletedAt is set).
| SetNamespace(d.Parent.GetNamespace()). | ||
| SetLineID(d.Parent.GetID()). | ||
| SetReason(discount.Reason.Type()). |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
*Verify getters on billing.Line used here.
This code depends on d.Parent.GetNamespace() and d.Parent.GetID(). Ensure these methods exist on *billing.Line.
Run:
Also applies to: 236-237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm GetNamespace() and GetID() on *billing.Line
rg -nP -C3 --type=go '^\s*func\s*\(\s*\*?Line\s*\)\s+GetNamespace\s*\(\s*\)\s+string\b' openmeter/billing
rg -nP -C3 --type=go '^\s*func\s*\(\s*\*?Line\s*\)\s+GetID\s*\(\s*\)\s+string\b' openmeter/billingLength of output: 127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for type Line definitions in openmeter/billing..."
rg -nP 'type\s+Line\b' -n openmeter/billing
echo; echo "Listing methods on Line in openmeter/billing..."
rg -nP --type=go 'func\s*\(\s*\*?Line\s*\)\s+\w+' -n openmeter/billing
echo; echo "Searching for any GetNamespace occurrences in entire repo..."
rg -nP 'GetNamespace' -n .
echo; echo "Searching for any GetID occurrences in entire repo..."
rg -nP 'GetID' -n .Length of output: 511
Billing.Line is missing GetNamespace/GetID methods
The billing.Line type (both the struct in invoiceline.go and the interface in service.go) does not define GetNamespace() or GetID(). Invoking these will result in compile errors. Please either:
- Add these getters on
*billing.Line, for example:func (l *Line) GetNamespace() string { return l.Namespace } func (l *Line) GetID() string { return l.ID }
- Or update these calls to use the existing API (e.g.
l.Namespace,l.ID, orl.Namespace()/l.ID()if those methods exist).
Affected locations:
- openmeter/billing/invoiceline.go:190–192
- openmeter/billing/invoiceline.go:236–237
🤖 Prompt for AI Agents
In openmeter/billing/invoicelines.go around lines 190-192 (and likewise update
lines 236-237), the code calls d.Parent.GetNamespace() and d.Parent.GetID() but
the billing.Line type does not expose GetNamespace/GetID methods; add receiver
methods on *billing.Line: GetNamespace() string returning the Namespace field
and GetID() string returning the ID field, or alternatively replace the calls
with the existing public fields or methods (e.g., d.Parent.Namespace /
d.Parent.ID or the existing accessors) so the code compiles and consistently
uses the billing.Line API.
2 participants
Overview
This patch changes the invoidelinediff to use interface based parent/entity relationsips in anticipation of the Line/DetailedLine split, where the two types would behave the same but would be different go types implementing the same interfaces.
Summary by CodeRabbit
New Features
Refactor
Tests