Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces a pluggable line-engine architecture to the billing system. Charge packages (flat-fee, credit-purchase, usage-based) now register their own Changes
Sequence DiagramsequenceDiagram
participant Client
participant BillingService
participant LineEngineRegistry
participant ChargeEngine as FlatFee/CreditPurchase<br/>LineEngine
participant RatingService
participant Adapter
Client->>BillingService: InvoicePendingLines(customer, asOf)
BillingService->>BillingService: gatherInScopeLines(customer, asOf)
BillingService->>LineEngineRegistry: Get(line.Engine)
LineEngineRegistry-->>BillingService: ChargeEngine
BillingService->>ChargeEngine: IsLineBillableAsOf(line, asOf)
ChargeEngine-->>BillingService: true/false
BillingService->>BillingService: groupGatheringLinesByEngine()
loop Per Engine Type
BillingService->>ChargeEngine: BuildStandardInvoiceLines(invoice, lines)
ChargeEngine->>ChargeEngine: Convert GatheringLines→StandardLines
ChargeEngine->>RatingService: GenerateDetailedLines()
RatingService-->>ChargeEngine: DetailedLines
ChargeEngine->>ChargeEngine: MergeGeneratedDetailedLines()
ChargeEngine-->>BillingService: StandardLines
end
BillingService->>Adapter: CreateStandardInvoice(lines)
Adapter-->>BillingService: StandardInvoice
BillingService-->>Client: StandardInvoice[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This diff introduces a significant new abstraction (line engines) with multiple implementations across charge packages, refactors core invoice-creation logic to dispatch through engines, and requires updates throughout the codebase (wiring, tests, HTTP driver, worker services, reconcilers). The changes are heterogeneous—each file touches different concerns (new interfaces, implementations, refactored control flow, data model updates, schema migrations)—and the logic density is moderate-to-high, particularly in the engine implementations and the refactored Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
feat/invoice-line-backend
branch
from
661b481 to
29f8fae
Compare
turip
force-pushed
the
feat/invoice-line-backend
branch
from
29f8fae to
122befc
Compare
turip
force-pushed
the
feat/invoice-line-backend
branch
from
122befc to
b328672
Compare
turip
force-pushed
the
feat/invoice-line-backend
branch
from
b328672 to
b9a6f6f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openmeter/billing/gatheringinvoice.go (1)
356-363:⚠️ Potential issue | 🟠 MajorRegenerate the derived equality helpers for
Engine.
GatheringLineBase.Equal()is still goderive-backed, so addingEnginehere without regeneratingbilling/derived.gen.goleaves engine changes invisible to equality/diff checks.As per coding guidelines, "Use Goderive annotations and run 'make generate' to regenerate billing/derived.gen.go".
Also applies to: 553-555
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/gatheringinvoice.go` around lines 356 - 363, GatheringLineBase has a new Engine field but the auto-generated equality helpers still omit it; update the goderive annotations and regenerate the derived helpers so GatheringLineBase.Equal() (and any other derived comparisons) include Engine — run the code generation step (e.g., add/ensure the proper //goderive:... annotations near the GatheringLineBase definition and run "make generate") to refresh billing/derived.gen.go so engine changes are detected; repeat the same regeneration for the other affected struct(s) mentioned around lines 553-555.openmeter/billing/service/gatheringinvoicependinglines.go (1)
262-286:⚠️ Potential issue | 🟠 MajorResolve the engine before doing rating-period work.
Right now every line still goes through
ResolveBillablePeriod(...)before its engine gets a say. That means flat-fee / credit-purchase lines can fail on feature-meter or rating prerequisites they don’t actually use, so the new engine-based billability is still partially coupled to the old invoice-line path. I’d flip this flow so the engine is resolved first, and only engines that need a resolved period require it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/gatheringinvoicependinglines.go` around lines 262 - 286, Flip the order so the engine is resolved before rating: call s.lineEngines.Get(line.Engine) first inside the slicesx.MapWithErr mapper, then ask the engine (eng) whether it requires a pre-resolved billable period (or attempt IsLineBillableAsOf with no ResolvedBillablePeriod), and only call s.ratingService.ResolveBillablePeriod(...) when the engine indicates it needs the period; finally pass lo.FromPtr(period) into eng.IsLineBillableAsOf only when you actually resolved one and otherwise pass nil/omit ResolvedBillablePeriod. Update the mapper around gatheringLineWithBillablePeriod, s.lineEngines.Get, s.ratingService.ResolveBillablePeriod, and eng.IsLineBillableAsOf to reflect this conditional flow.
🧹 Nitpick comments (2)
openmeter/billing/invoiceline.go (1)
207-224: Could we add branch-level tests forGetChargeID?Nice addition. Since this method now drives type-based branching plus nil/error paths, a small table-driven test set would help lock in behavior for standard/gathering/nil/invalid-type cases.
As per coding guidelines "
**/*_test.go: Make sure the tests are comprehensive and cover the changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/invoiceline.go` around lines 207 - 224, Add a table-driven test file for InvoiceLine.GetChargeID that exercises each branch and error path: create test cases for InvoiceLineTypeStandard with a non-nil standardLine (expect its ChargeID), InvoiceLineTypeGathering with a non-nil gatheringLine (expect its ChargeID), cases where standardLine or gatheringLine is nil (expect the corresponding "standard line is nil"/"gathering line is nil" error), and an invalid i.t value (expect the "invalid invoice line type" error). Use descriptive subtests and assert both returned pointer values and error messages; place the tests in a *_test.go alongside invoiceline.go and reference the InvoiceLine, GetChargeID, InvoiceLineTypeStandard, InvoiceLineTypeGathering, standardLine and gatheringLine symbols to locate the code under test.openmeter/billing/worker/subscriptionsync/service/base_test.go (1)
61-67: Please cover the engine-routed sync path in a real test.These setup changes only prove the service still builds. They won’t catch a missing line-engine registration or a bad
billing.Service.InvoicePendingLinesintegration, which is the risky part of this refactor.As per coding guidelines, "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
Also applies to: 81-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/base_test.go` around lines 61 - 67, The current test only constructs the service via New(Config{...}) but doesn’t exercise the engine-routed sync path; add a real unit/integration test that instantiates the service with a SubscriptionSyncAdapter that routes to the line-engine, mocks BillingService.InvoicePendingLines to expect and verify calls, and then invokes the service’s sync entrypoint (the method that triggers subscription sync) so the test asserts that the line-engine registration and BillingService.InvoicePendingLines integration are exercised; use the existing symbols New, Config, SubscriptionSyncAdapter, SubscriptionService and BillingService.InvoicePendingLines to locate and wire up the mocks and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/flatfee/lineengine/engine.go`:
- Around line 111-133: The code currently returns early with `continue` when
`realizations` is empty, skipping later materialization steps; instead, remove
the `continue` and only skip assigning credits when empty. Keep the call to
e.ratingService.GenerateDetailedLines(stdLine),
invoicecalc.MergeGeneratedDetailedLines(stdLine, ...), and stdLine.Validate()
always run; set stdLine.CreditsApplied to an empty slice or nil when
len(realizations) == 0 and only call convertCreditRealizations(realizations)
when realizations exist (i.e., if len(realizations) > 0 { stdLine.CreditsApplied
= convertCreditRealizations(realizations) }).
In `@openmeter/billing/service/lineengine.go`:
- Around line 23-25: Ensure Register on engineRegistry guards against a nil
billing.LineEngine: check if eng == nil at the start of engineRegistry.Register
and return a clear error instead of calling eng.GetLineEngineType(); then
proceed to call eng.GetLineEngineType() and engineType.Validate() only when eng
is non-nil. This prevents a panic when a nil engine is passed and keeps the
public API initialization path safe.
---
Outside diff comments:
In `@openmeter/billing/gatheringinvoice.go`:
- Around line 356-363: GatheringLineBase has a new Engine field but the
auto-generated equality helpers still omit it; update the goderive annotations
and regenerate the derived helpers so GatheringLineBase.Equal() (and any other
derived comparisons) include Engine — run the code generation step (e.g.,
add/ensure the proper //goderive:... annotations near the GatheringLineBase
definition and run "make generate") to refresh billing/derived.gen.go so engine
changes are detected; repeat the same regeneration for the other affected
struct(s) mentioned around lines 553-555.
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 262-286: Flip the order so the engine is resolved before rating:
call s.lineEngines.Get(line.Engine) first inside the slicesx.MapWithErr mapper,
then ask the engine (eng) whether it requires a pre-resolved billable period (or
attempt IsLineBillableAsOf with no ResolvedBillablePeriod), and only call
s.ratingService.ResolveBillablePeriod(...) when the engine indicates it needs
the period; finally pass lo.FromPtr(period) into eng.IsLineBillableAsOf only
when you actually resolved one and otherwise pass nil/omit
ResolvedBillablePeriod. Update the mapper around
gatheringLineWithBillablePeriod, s.lineEngines.Get,
s.ratingService.ResolveBillablePeriod, and eng.IsLineBillableAsOf to reflect
this conditional flow.
---
Nitpick comments:
In `@openmeter/billing/invoiceline.go`:
- Around line 207-224: Add a table-driven test file for InvoiceLine.GetChargeID
that exercises each branch and error path: create test cases for
InvoiceLineTypeStandard with a non-nil standardLine (expect its ChargeID),
InvoiceLineTypeGathering with a non-nil gatheringLine (expect its ChargeID),
cases where standardLine or gatheringLine is nil (expect the corresponding
"standard line is nil"/"gathering line is nil" error), and an invalid i.t value
(expect the "invalid invoice line type" error). Use descriptive subtests and
assert both returned pointer values and error messages; place the tests in a
*_test.go alongside invoiceline.go and reference the InvoiceLine, GetChargeID,
InvoiceLineTypeStandard, InvoiceLineTypeGathering, standardLine and
gatheringLine symbols to locate the code under test.
In `@openmeter/billing/worker/subscriptionsync/service/base_test.go`:
- Around line 61-67: The current test only constructs the service via
New(Config{...}) but doesn’t exercise the engine-routed sync path; add a real
unit/integration test that instantiates the service with a
SubscriptionSyncAdapter that routes to the line-engine, mocks
BillingService.InvoicePendingLines to expect and verify calls, and then invokes
the service’s sync entrypoint (the method that triggers subscription sync) so
the test asserts that the line-engine registration and
BillingService.InvoicePendingLines integration are exercised; use the existing
symbols New, Config, SubscriptionSyncAdapter, SubscriptionService and
BillingService.InvoicePendingLines to locate and wire up the mocks and
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a08c682-0a48-460c-89c4-6fd8b531769a
⛔ Files ignored due to path filters (10)
go.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/billinginvoiceline.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline/billinginvoiceline.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline/where.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceline_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (59)
app/common/billing.goapp/common/charges.gocmd/server/main.goopenmeter/billing/adapter/gatheringlines.goopenmeter/billing/adapter/stdinvoicelinemapper.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/charges/creditpurchase/lineengine/engine.goopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/flatfee/lineengine/engine.goopenmeter/billing/charges/flatfee/lineengine/helpers.goopenmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoicependinglines.goopenmeter/billing/charges/testutils/service.goopenmeter/billing/charges/usagebased/service/create.goopenmeter/billing/derived.gen.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/httpdriver/handler.goopenmeter/billing/httpdriver/invoice.goopenmeter/billing/httpdriver/invoiceline.goopenmeter/billing/invoiceline.goopenmeter/billing/lineengine.goopenmeter/billing/lineengine/engine.goopenmeter/billing/lineengine/splitlinegroup.goopenmeter/billing/lineengine/stdinvoice.goopenmeter/billing/service.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/service/invoice.goopenmeter/billing/service/invoicecalc/details.goopenmeter/billing/service/lineengine.goopenmeter/billing/service/quantitysnapshot.goopenmeter/billing/service/service.goopenmeter/billing/service/stdinvoiceline.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/worker/collect/collect.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchcharge.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeflatfee.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchchargeusagebased.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoice.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/ent/schema/billing.goopenmeter/server/router/router.goopenmeter/server/server_test.gotest/billing/adapter_test.gotest/billing/invoice_test.gotest/billing/subscription_test.gotest/billing/ubpflatfee_test.gotest/credits/sanity_test.gotest/subscription/framework_test.gotools/migrate/migrations/20260407133223_invoice-line-engine.down.sqltools/migrate/migrations/20260407133223_invoice-line-engine.up.sql
💤 Files with no reviewable changes (4)
- cmd/server/main.go
- openmeter/server/router/router.go
- openmeter/billing/charges/service.go
- openmeter/billing/charges/service/invoicependinglines.go
turip
force-pushed
the
feat/invoice-line-backend
branch
from
7efcf6d to
03170ea
Compare
tothandras
approved these changes
Apr 9, 2026
This was referenced Apr 9, 2026
This was referenced Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Note: this is the first part of a series of patches, the engine interface defintion has TODOs for the next steps.
This PR introduces a new billing line-engine architecture that routes pending and standard invoice lines through explicit
LineEngineTypeimplementations instead of keeping all behavior in the core billing service.It adds an
enginediscriminator to persisted billing invoice lines, wires a default invoice engine, and moves split-line handling, billability checks, and standard-line building behind engine-specific interfaces.It also extracts charge-specific invoice materialization for flat-fee and credit-purchase charges into dedicated line engines, removing the old charge-side
InvoicePendingLineswrapper and related post-create credit-allocation flow. In parallel, it restores billing-side tax snapshot hydration on invoice reads by backfilling scalar tax config fields and reattaching matching resolved tax-code references in the billing mapper.The overall goal is to make charge-backed billing flows own their own invoice-line behavior while shrinking the billing service toward orchestration and shared infrastructure concerns.
Notes for reviewer
The splitlinegroup still lives in billing's adapter. Eventually we should move that into the invoicelineengine.
Right now the charge adapters are seperate packages, once we see the structure those might end up being part of the service itself.
Usage-based has no credit_then_invoice support, thus there's no lineengine added.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor