refactor: remove recursive line lookups

[turip]

Overview

Now that we have only two levels of line hierarchy, let's remove the recursive lookup strategy and expand the lines where we know we need a second level.

Seperate detailed lines mapper is added so that it will be more visible what changes when we move to seperate types.

This is a prep for this PR: #3514

There's a slight behavior change compared to the previous version, now on Upsert deleted detailed lines are not returned. This will not cause any issues going forward.

Notes for reviewer

There are no additional tests as the whole invoicing test suite relies on this codepath.

Summary by CodeRabbit

• Bug Fixes

  • Fixed invoice updates to properly include lines marked for deletion in results

• Improvements

  • Restructured invoice line data to display child lines hierarchically within parent lines
  • Enhanced line information with comprehensive field mapping and detailed data population

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors invoice line expansion and mapping logic by replacing expandLineItems with expandLineItemsWithDetailedLines, introducing per-line detailed mapping with validation, changing the mapInvoiceLineFromDB signature to directly process DB objects, removing deletion filters, and restructuring parent-child line associations through a new Children collection pattern.

Changes

Cohort / File(s)	Change Summary
Invoice Line Mapping Core openmeter/billing/adapter/invoicelinemapper.go	Refactored mapInvoiceLineFromDB signature from (ctx context.Context, in mapInvoiceLineFromDBInput) to (dbLines []*db.BillingInvoiceLine), replacing context-dependent batch processing with per-line detailed mapping. Added mapInvoiceDetailedLineWithoutReferences to return DetailedLine objects with expanded field population (ManagedResource, Period, Metadata, Annotations, InvoiceID, Status, Currency, TaxConfig, RateCardDiscounts, Totals, ExternalIDs, Subscription). Removed obsolete reference resolution and deduplication logic; added per-line validation and error handling.
Invoice Expansion & Filtering openmeter/billing/adapter/invoice.go	Replaced expandLineItems call with expandLineItemsWithDetailedLines in expandInvoiceLineItems. Removed deletion filter logic (!in.ExpandedFields.DeletedLines) in UpdateInvoice, allowing deleted lines to be included in returned invoices. Simplified mapInvoiceFromDB call to a.mapInvoiceLineFromDB(invoice.Edges.BillingInvoiceLines).
Line Retrieval & Association openmeter/billing/adapter/invoicelines.go	Introduced fetchLinesInput struct encapsulating Namespace, LineIDs, and IncludeDeleted. Implemented new expandLineItemsWithDetailedLines function applying detailed line expansion with deletion filtering. Updated UpsertInvoiceLines and ListInvoiceLines to use detailed lines pathway. Modified AssociateLinesToInvoice to filter deleted lines during association and use fetchLinesInput for retrieval.
Line Hierarchy & Type Changes openmeter/billing/adapter/invoicelinesplitgroup.go, openmeter/billing/invoiceline.go	Changed Line field in LineWithInvoiceHeader from pointer (\*Line) to value (Line). Updated ChildrenWithIDReuse to skip deleted children, enforce flat-fee line compatibility when reusing, preserve ConfigID and timestamps, and retain database ID. Removed fields from ListInvoiceLinesAdapterInput: InvoiceAtBefore, ParentLineIDs, ParentLineIDsIncludeParent.
Test Updates test/billing/adapter_test.go	Removed mergeDBFields helper. Updated ListInvoiceLines calls to use LineIDs instead of ParentLineIDs. Changed assertions from validating 4 individual lines to validating 1 top-level line with 3 Children and updated reference name assertions accordingly.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

The changes involve significant signature refactoring of core mapping functions, interconnected logic across multiple files, new detailed line handling with validation paths, altered deletion filtering behavior, and restructured parent-child associations requiring careful review of per-line error propagation and hierarchy preservation.

Possibly related PRs

• refactor(billing): use interface based parent relationships #3232: Directly modifies invoice line mapping in invoicelinemapper.go with similar refactoring of per-line mapping and DB-to-API conversion logic.
• refactor: allow single discount per type #2639: Makes overlapping code-level changes to invoicelinemapper.go and invoicelines.go, specifically modifying mapInvoiceLineFromDB and expandLineItems variants.
• refactor: detailed line provisioning logic #3516: Introduces/modifies detailed-line handling paths and changes invoice line mapping/expansion logic with similar DetailedLine struct usage.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title “refactor: remove recursive line lookups” clearly and concisely captures the primary change of the pull request, which replaces the recursive line lookup strategy with explicit two‐level mapping and a detailed‐lines mapper, aligning directly with the described objectives.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch cleanup/remove-recursive-line-lookups

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (6)

openmeter/billing/invoiceline.go (1)

732-782: Robust ID reuse safeguards LGTM (skip deleted, enforce flat-fee, copy ConfigID)

These checks prevent reusing deleted children and ensure correct config reuse. Solid alignment with detailed-lines design.

Consider prefixing errors with context (e.g., "ChildrenWithIDReuse: ...") for easier tracing.

openmeter/billing/adapter/invoicelines.go (3)

270-279: Clarify comment: only top-level lines can be returned deleted

The fetch includes deleted top-level lines, but detailed children are always filtered to non-deleted. Update the comment to avoid confusion.

Apply this diff:

-        // We will include deleted lines, as we need to return all the lines even if the edit function marked them as deleted.
+        // We include deleted top-level lines (if any were marked deleted during edit).
+        // Note: detailed children are always returned non-deleted by design.

---

559-572: New fetchLinesInput LGTM

Minimal validation is fine for internal use; consider enforcing non-empty LineIDs if future callers may pass empty.

---

593-596: Generalize fetchLines error message

“not all lines were created” is misleading outside upsert. Prefer “not all requested lines were found”.

Apply this diff:

-    if len(dbLines) != len(in.LineIDs) {
-        return nil, fmt.Errorf("not all lines were created")
-    }
+    if len(dbLines) != len(in.LineIDs) {
+        return nil, fmt.Errorf("not all requested lines were found")
+    }

openmeter/billing/adapter/invoicelinemapper.go (2)

49-97: Deduplicate LineBase mapping via a shared helper.

The LineBase population is duplicated across both mappers. Extracting a helper reduces drift and eases future changes.

Example:

func mapLineBaseFromDB(dbLine *db.BillingInvoiceLine) billing.LineBase {
  return billing.LineBase{
    ManagedResource: models.NewManagedResource(models.ManagedResourceInput{
      Namespace: dbLine.Namespace, ID: dbLine.ID,
      CreatedAt: dbLine.CreatedAt.In(time.UTC),
      UpdatedAt: dbLine.UpdatedAt.In(time.UTC),
      DeletedAt: convert.TimePtrIn(dbLine.DeletedAt, time.UTC),
      Name: dbLine.Name, Description: dbLine.Description,
    }),
    Metadata: dbLine.Metadata, Annotations: dbLine.Annotations,
    InvoiceID: dbLine.InvoiceID, Status: dbLine.Status, ManagedBy: dbLine.ManagedBy,
    Period: billing.Period{Start: dbLine.PeriodStart.In(time.UTC), End: dbLine.PeriodEnd.In(time.UTC)},
    ParentLineID: dbLine.ParentLineID, SplitLineGroupID: dbLine.SplitLineGroupID,
    ChildUniqueReferenceID: dbLine.ChildUniqueReferenceID,
    InvoiceAt: dbLine.InvoiceAt.In(time.UTC), Type: dbLine.Type, Currency: dbLine.Currency,
    TaxConfig: lo.EmptyableToPtr(dbLine.TaxConfig),
    RateCardDiscounts: lo.FromPtr(dbLine.RatecardDiscounts),
    Totals: billing.Totals{Amount: dbLine.Amount, ChargesTotal: dbLine.ChargesTotal,
      DiscountsTotal: dbLine.DiscountsTotal, TaxesInclusiveTotal: dbLine.TaxesInclusiveTotal,
      TaxesExclusiveTotal: dbLine.TaxesExclusiveTotal, TaxesTotal: dbLine.TaxesTotal, Total: dbLine.Total},
    ExternalIDs: billing.LineExternalIDs{Invoicing: lo.FromPtr(dbLine.InvoicingAppExternalID)},
  }
}

Then in both mappers: LineBase: mapLineBaseFromDB(dbLine),

Also applies to: 150-198

---

115-118: Improve error message with line context.

Include the line ID to ease diagnosis.

-    return nil, fmt.Errorf("manual usage based line is missing")
+    return nil, fmt.Errorf("usage-based config is missing [line_id=%s]", dbLine.ID)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb7aeac and d85e71f.

📒 Files selected for processing (6)

• openmeter/billing/adapter/invoice.go (2 hunks)
• openmeter/billing/adapter/invoicelinemapper.go (2 hunks)
• openmeter/billing/adapter/invoicelines.go (12 hunks)
• openmeter/billing/adapter/invoicelinesplitgroup.go (1 hunks)
• openmeter/billing/invoiceline.go (3 hunks)
• test/billing/adapter_test.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (5)

openmeter/billing/adapter/invoicelinesplitgroup.go (1)

openmeter/billing/invoiceline.go (1)

• Line (311-325)

openmeter/billing/adapter/invoicelines.go (4)

openmeter/ent/db/billinginvoiceline/where.go (4)

• Namespace (72-74)
• ID (17-19)
• DeletedAtIsNil (423-425)
• IDIn (32-34)

pkg/slicesx/map.go (2)

• Map (8-21)
• MapWithErr (25-49)

openmeter/ent/db/billinginvoiceline_query.go (1)

• BillingInvoiceLineQuery (30-52)

openmeter/ent/db/billinginvoiceline.go (2)

• BillingInvoiceLine (29-113)
• BillingInvoiceLine (260-285)

openmeter/billing/adapter/invoicelinemapper.go (6)

openmeter/ent/db/billinginvoiceline.go (2)

• BillingInvoiceLine (29-113)
• BillingInvoiceLine (260-285)

openmeter/billing/invoiceline.go (8)

• Line (311-325)
• NewLineChildren (682-689)
• LineBase (128-157)
• Period (83-86)
• SubscriptionReference (236-241)
• UsageBasedLine (784-797)
• DetailedLine (329-329)
• FlatFeeLine (289-298)

pkg/models/model.go (3)

• ManagedResource (23-31)
• NewManagedResource (75-97)
• ManagedResourceInput (45-53)

pkg/convert/time.go (1)

• TimePtrIn (5-10)

pkg/timeutil/closedperiod.go (1)

• ClosedPeriod (8-11)

pkg/slicesx/map.go (1)

• MapWithErr (25-49)

openmeter/billing/invoiceline.go (1)

openmeter/ent/db/billinginvoiceline/where.go (2)

• DeletedAt (87-89)
• ChildUniqueReferenceID (178-180)

openmeter/billing/adapter/invoice.go (1)

openmeter/ent/db/billinginvoiceline.go (1)

• BillingInvoiceLines (781-781)

⏰ 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). (4)

• GitHub Check: E2E
• GitHub Check: Quickstart
• GitHub Check: Code Generators
• GitHub Check: Analyze (go)

🔇 Additional comments (8)

openmeter/billing/adapter/invoicelinesplitgroup.go (1)

210-219: Pointer/value assignment LGTM

Using Line: line (not &line) matches mapInvoiceLineWithoutReferences returning a pointer. Consistent with updated value-based usage across the PR.

openmeter/billing/adapter/invoice.go (2)

66-81: Switch to detailed-lines expansion LGTM

Using expandLineItemsWithDetailedLines aligns with new mapping and ensures children are present while excluding deleted detailed lines.

Confirm this semantic: Expand.DeletedLines governs only top-level lines; detailed children remain filtered to non-deleted.

---

739-745: Mapper signature update LGTM

Directly mapping invoice.Edges.BillingInvoiceLines matches the new mapInvoiceLineFromDB API.

openmeter/billing/adapter/invoicelines.go (3)

464-468: List path aligned with detailed-lines expansion

Using expandLineItemsWithDetailedLines ensures children are present and non-deleted; matches new API expectations.

---

533-539: Guard against associating deleted lines LGTM

The DeletedAtIsNil filter prevents linking deleted lines to invoices.

---

586-590: Mapper/ordering and hierarchy expansion LGTM

• Expanding detailed lines before mapping ensures complete inputs.
• Preserving input order via MapWithErr over LineIDs is good.
• Split-line hierarchy expansion after mapping is correct.

Also applies to: 601-614, 618-625

openmeter/billing/invoiceline.go (1)

930-940: The struct migration is complete and properly implemented. All callers in openmeter/billing/service/invoice.go and tests have been updated to use the new fields (CustomerID, InvoiceIDs, InvoiceStatuses, Statuses, IncludeDeleted, LineIDs). The adapter implementation in openmeter/billing/adapter/invoicelines.go correctly integrates all new fields into the query logic. No lingering references to the removed fields exist in the business logic—ParentLineIDs references are confined to auto-generated ent entity mutation code, which is independent of the adapter input struct.

Likely an incorrect or invalid review comment.

openmeter/billing/adapter/invoicelinemapper.go (1)

111-118: Top-level type enforcement: confirm data invariants/migrations.

Failing on non-usage-based top-level lines will abort the whole mapping. Ensure all callers only pass usage-based parents (and historical invoices don’t contain fee-typed top-level lines), or gate earlier.

• Confirm migrations/queries guarantee: top-level = usage-based only.
• If not guaranteed, consider returning a clearer error at the call site or filtering earlier.