feat: implement credit-only flat fee handling#3919
Draft
turip wants to merge 3 commits intofeat/credit-purchase-chargesfrom
Draft
feat: implement credit-only flat fee handling#3919turip wants to merge 3 commits intofeat/credit-purchase-chargesfrom
turip wants to merge 3 commits intofeat/credit-purchase-chargesfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an advance-credit-only flow: new input types and validation, service and adapter methods to advance credit-only charges, per-type dispatch helper, flat-fee credit-only advancement implementation, error for unsupported credit-purchase advancement, and tests for future/past invoice timing. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service
participant Adapter
participant Orchestrator
participant Repo as Repository
Client->>Service: AdvanceCreditOnlyCharges(input)
Service->>Service: Validate(input)
Service->>Adapter: withBillingTransactionForInvoiceManipulation(customerID)*
Service->>Repo: GetChargesByIDs(ctx, chargeIDs, expand=Realizations)
Repo-->>Service: charges
Service->>Service: mapChargesByType(charges, handlers)
alt FlatFeeCharge (CreditOnly)
Service->>Orchestrator: AdvanceCreditOnlyCharge(ctx, charge)
Orchestrator->>Adapter: OnFlatFeeAssignedToInvoice(charge, ...)
Adapter-->>Orchestrator: creditRealizations
Orchestrator->>Adapter: CreateCreditRealizations(realizations)
Adapter-->>Orchestrator: created realizations
Orchestrator->>Adapter: UpdateFlatFeeCharge(finalizedCharge)
Adapter-->>Orchestrator: updated charge
Orchestrator-->>Service: advancedCharge
else UsageBased or CreditPurchase
Service-->>Client: error (unsupported / cannot advance)
end
Service-->>Client: charges, error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 3
🧹 Nitpick comments (2)
openmeter/billing/charges/charge.go (1)
428-435: ValidateChargeIDsentries here as wellNice addition overall. One small tweak:
Validate()currently checks onlyNamespace, soChargeIDslike[""]fail later in the adapter path. Fast-failing here keeps errors cleaner and avoids unnecessary transaction work.♻️ Suggested tweak
func (i AdvanceCreditOnlyChargesInput) Validate() error { var errs []error if i.Namespace == "" { errs = append(errs, fmt.Errorf("namespace is required")) } + + for idx, id := range i.ChargeIDs { + if id == "" { + errs = append(errs, fmt.Errorf("charge ID [%d]: cannot be empty", idx)) + } + } return errors.Join(errs...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/charge.go` around lines 428 - 435, The Validate() method on AdvanceCreditOnlyChargesInput currently only checks Namespace; update AdvanceCreditOnlyChargesInput.Validate() to also iterate over i.ChargeIDs and append an error for any empty string entries (e.g., "charge id at index N is empty") so invalid ChargeIDs like [""] fail fast; return errors.Join(errs...) as before. Ensure you reference the ChargeIDs field and include index information in the error messages to aid debugging.openmeter/billing/charges/service/service_test.go (1)
343-379: Add one failure-path case for mismatched credit realization totalsA useful hardening test here: make
onFlatFeeAssignedToInvoicereturn realizations that do not sum toAmountAfterProration, then assert:
AdvanceCreditOnlyCharges/CreateChargesreturns an error- charge status stays non-final
- no realizations are persisted
That will lock in the rollback/data-integrity behavior for this critical flow.
As per coding guidelines,**/*_test.go: 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: 400-452
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/service_test.go` around lines 343 - 379, Add a negative test that exercises the mismatch path by making the FlatFeeTestHandler.onFlatFeeAssignedToInvoice return CreditRealizationCreateInput items whose summed Amount does NOT equal input.AmountAfterProration, then call Charges.AdvanceCreditOnlyCharges (or Charges.CreateCharges) with that flatFeeChargeID and assert the call returns an error, the charge status remains non-final (not charges.ChargeStatusFinal via res[0].AsFlatFeeCharge()), and no CreditRealizations are persisted on the charge state (State.CreditRealizations is empty); place this as a new s.Run (e.g. "fail on mismatched credit totals") alongside the existing "advance the charge" test and reuse the same handler hooks to simulate the mismatch.
🤖 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/service/advance.go`:
- Around line 30-38: The current mapChargesByType call only provides a flatFee
handler so mixed ChargeIDs containing usage-based or credit-purchase charges
cause mapChargesByType to return ErrUnsupported and abort; update the
handlerByType passed to mapChargesByType to include pass-through handlers for
other charge kinds (e.g., usageBased and creditPurchase) that simply return the
incoming charge unchanged and nil error, so only flatFee charges are processed
by s.flatFeeOrchestrator.AdvanceCreditOnlyCharge while other types are no-ops;
reference mapChargesByType, handlerByType, flatFee, usageBased, creditPurchase,
ChargeIDs, and s.flatFeeOrchestrator.AdvanceCreditOnlyCharge when making the
change.
In `@openmeter/billing/charges/service/flatfee/creditonly.go`:
- Around line 18-55: The current finalize flow in creditonly.go (the non-final
guard on charge.Status, the clock.Now().Before check, the call to
s.flatFeeHandler.OnFlatFeeAssignedToInvoice, then
s.adapter.CreateCreditRealizations and s.adapter.UpdateFlatFeeCharge that sets
ChargeStatusFinal and State.CreditRealizations) is not concurrency safe and
allows double-realization; make the finalization atomic by either acquiring a
row-level lock or reloading-and-conditional-updating the charge before creating
realizations: reload the charge (or SELECT ... FOR UPDATE) and verify
charge.Status is still not ChargeStatusFinal, then create realizations and
perform a conditional UpdateFlatFeeCharge that only sets
Status=ChargeStatusFinal if the precondition (previous status and/or missing
CreditRealizations) holds (or use a DB transaction that creates realizations and
updates the charge atomically); also ensure CreateCreditRealizations is
idempotent or checks for existing realizations to avoid duplicates if it must be
retried.
In `@openmeter/billing/charges/service/service_test.go`:
- Around line 292-453: Add explicit assertions in
TestFlatFeeCreditOnlyFutureInvoiceAt and TestFlatFeeCreditOnlyPastInvoiceAt to
verify credit-only charges bypass the invoicing stack: after calls to
Charges.CreateCharges and Charges.AdvanceCreditOnlyCharges (and after obtaining
flatFeeChargeID/updatedFlatFeeCharge), assert that no gathering records, no
invoices, and no standard invoice lines were created for the namespace/charge
(use the test helpers or repository query functions you have for
gatherings/invoices/invoice lines to check by Namespace and ChargeID); place
these checks right after the existing status/realization assertions in each test
to ensure no invoice artifacts exist.
---
Nitpick comments:
In `@openmeter/billing/charges/charge.go`:
- Around line 428-435: The Validate() method on AdvanceCreditOnlyChargesInput
currently only checks Namespace; update AdvanceCreditOnlyChargesInput.Validate()
to also iterate over i.ChargeIDs and append an error for any empty string
entries (e.g., "charge id at index N is empty") so invalid ChargeIDs like [""]
fail fast; return errors.Join(errs...) as before. Ensure you reference the
ChargeIDs field and include index information in the error messages to aid
debugging.
In `@openmeter/billing/charges/service/service_test.go`:
- Around line 343-379: Add a negative test that exercises the mismatch path by
making the FlatFeeTestHandler.onFlatFeeAssignedToInvoice return
CreditRealizationCreateInput items whose summed Amount does NOT equal
input.AmountAfterProration, then call Charges.AdvanceCreditOnlyCharges (or
Charges.CreateCharges) with that flatFeeChargeID and assert the call returns an
error, the charge status remains non-final (not charges.ChargeStatusFinal via
res[0].AsFlatFeeCharge()), and no CreditRealizations are persisted on the charge
state (State.CreditRealizations is empty); place this as a new s.Run (e.g. "fail
on mismatched credit totals") alongside the existing "advance the charge" test
and reuse the same handler hooks to simulate the mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42b8511f-f535-4d9e-ae5b-052f9c1935eb
📒 Files selected for processing (8)
openmeter/billing/charges/charge.goopenmeter/billing/charges/chargeflatfee.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/flatfee/create.goopenmeter/billing/charges/service/flatfee/creditonly.goopenmeter/billing/charges/service/helpers.goopenmeter/billing/charges/service/service_test.go
Comment on lines
+18
to
+55
| if charge.Status == charges.ChargeStatusFinal { | ||
| return charge, nil | ||
| } | ||
|
|
||
| if clock.Now().Before(charge.Intent.InvoiceAt) { | ||
| return charge, nil | ||
| } | ||
|
|
||
| // Let's realize the charge from credits | ||
| createCreditRealizations, err := s.flatFeeHandler.OnFlatFeeAssignedToInvoice(ctx, charges.OnFlatFeeAssignedToInvoiceInput{ | ||
| Charge: charge, | ||
| ServicePeriod: charge.Intent.ServicePeriod, | ||
| PreTaxTotalAmount: charge.Intent.AmountAfterProration, | ||
| }) | ||
| if err != nil { | ||
| return charge, err | ||
| } | ||
|
|
||
| // Sanity check: we are in credit only mode so let's make sure that the returned realizations are covering for the entire amount | ||
| totalCreditRealizationAmount := alpacadecimal.Zero | ||
| for _, realization := range createCreditRealizations { | ||
| totalCreditRealizationAmount = totalCreditRealizationAmount.Add(realization.Amount) | ||
| } | ||
|
|
||
| if !totalCreditRealizationAmount.Equals(charge.Intent.AmountAfterProration) { | ||
| return charge, fmt.Errorf("credit realizations do not cover the entire amount: %s != %s", totalCreditRealizationAmount, charge.Intent.AmountAfterProration) | ||
| } | ||
|
|
||
| createdCreditRealizations, err := s.adapter.CreateCreditRealizations(ctx, charge.GetChargeID(), createCreditRealizations) | ||
| if err != nil { | ||
| return charge, err | ||
| } | ||
|
|
||
| charge.State.CreditRealizations = createdCreditRealizations | ||
| charge.Status = charges.ChargeStatusFinal | ||
|
|
||
| _, err = s.adapter.UpdateFlatFeeCharge(ctx, charge) | ||
| if err != nil { |
Contributor
There was a problem hiding this comment.
Concurrency gap can double-realize credits
This flow is not idempotent under concurrent calls for the same charge: two requests can both pass the non-final guard and each create credit realizations before one marks the charge final. That can lead to duplicate credit application.
Please make the finalization path atomic (for example: lock/reload the charge row before proceeding, or use a compare-and-swap/conditional update strategy with idempotency safeguards on realization creation).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/service/flatfee/creditonly.go` around lines 18 -
55, The current finalize flow in creditonly.go (the non-final guard on
charge.Status, the clock.Now().Before check, the call to
s.flatFeeHandler.OnFlatFeeAssignedToInvoice, then
s.adapter.CreateCreditRealizations and s.adapter.UpdateFlatFeeCharge that sets
ChargeStatusFinal and State.CreditRealizations) is not concurrency safe and
allows double-realization; make the finalization atomic by either acquiring a
row-level lock or reloading-and-conditional-updating the charge before creating
realizations: reload the charge (or SELECT ... FOR UPDATE) and verify
charge.Status is still not ChargeStatusFinal, then create realizations and
perform a conditional UpdateFlatFeeCharge that only sets
Status=ChargeStatusFinal if the precondition (previous status and/or missing
CreditRealizations) holds (or use a DB transaction that creates realizations and
updates the charge atomically); also ensure CreateCreditRealizations is
idempotent or checks for existing realizations to avoid duplicates if it must be
retried.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openmeter/billing/charges/charge.go (1)
428-435: AddChargeIDselement validation in the new input validatorNice addition overall. One small gap:
Validate()checks namespace, but malformed IDs (empty strings) are only caught deeper inGetChargesByIDsInput.Validate().Suggested tweak
func (i AdvanceCreditOnlyChargesInput) Validate() error { var errs []error if i.Namespace == "" { errs = append(errs, fmt.Errorf("namespace is required")) } + + for idx, id := range i.ChargeIDs { + if id == "" { + errs = append(errs, fmt.Errorf("charge ID [%d]: cannot be empty", idx)) + } + } return errors.Join(errs...) }As per coding guidelines,
**/*.go: “make readability and maintainability a priority.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/charge.go` around lines 428 - 435, The AdvanceCreditOnlyChargesInput.Validate() currently only checks Namespace; add validation for the ChargeIDs slice: ensure ChargeIDs is not nil/empty and iterate over ChargeIDs to append an error for any empty string IDs (similar to logic in GetChargesByIDsInput.Validate()), so invalid or blank IDs are caught early; update the error messages to be descriptive (e.g., "charge id at index X is empty") and return errors.Join(errs...).openmeter/billing/charges/service/helpers.go (1)
60-60: Tiny readability nit: rename thehandlerByTypeparameter
func mapChargesByType(in charges.Charges, handlerByType handlerByType)is a bit hard to scan because the var and type share the same identifier. Renaming the param tohandlerswould make this easier to read.As per coding guidelines,
**/*.go: “make readability and maintainability a priority.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/helpers.go` at line 60, Rename the parameter named handlerByType in the function mapChargesByType to a clearer name (e.g., handlers) to avoid shadowing the handlerByType type; update the signature func mapChargesByType(in charges.Charges, handlers handlerByType) (charges.Charges, error) and adjust all uses inside mapChargesByType to reference handlers instead of handlerByType so the code is easier to read and matches the naming guideline.
🤖 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/service/advance.go`:
- Around line 22-30: GetChargesByIDs can return a partial result; update the
caller in advance.go (the code invoking s.adapter.GetChargesByIDs and storing
the result in res) to detect missing IDs by comparing the returned charges' IDs
against the requested input.ChargeIDs and if any IDs are absent return a
descriptive error instead of proceeding; ensure you reference the adapter method
s.adapter.GetChargesByIDs, the input type charges.GetChargesByIDsInput and the
variable res to locate where to add the check.
---
Nitpick comments:
In `@openmeter/billing/charges/charge.go`:
- Around line 428-435: The AdvanceCreditOnlyChargesInput.Validate() currently
only checks Namespace; add validation for the ChargeIDs slice: ensure ChargeIDs
is not nil/empty and iterate over ChargeIDs to append an error for any empty
string IDs (similar to logic in GetChargesByIDsInput.Validate()), so invalid or
blank IDs are caught early; update the error messages to be descriptive (e.g.,
"charge id at index X is empty") and return errors.Join(errs...).
In `@openmeter/billing/charges/service/helpers.go`:
- Line 60: Rename the parameter named handlerByType in the function
mapChargesByType to a clearer name (e.g., handlers) to avoid shadowing the
handlerByType type; update the signature func mapChargesByType(in
charges.Charges, handlers handlerByType) (charges.Charges, error) and adjust all
uses inside mapChargesByType to reference handlers instead of handlerByType so
the code is easier to read and matches the naming guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6c549f3-8791-4cae-a860-6590c019bd43
📒 Files selected for processing (9)
openmeter/billing/charges/charge.goopenmeter/billing/charges/chargeflatfee.goopenmeter/billing/charges/errors.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/flatfee/create.goopenmeter/billing/charges/service/flatfee/creditonly.goopenmeter/billing/charges/service/helpers.goopenmeter/billing/charges/service/service_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/billing/charges/service/service_test.go
- openmeter/billing/charges/service.go
- openmeter/billing/charges/service/flatfee/creditonly.go
Comment on lines
+22
to
+30
| res, err := s.adapter.GetChargesByIDs(ctx, charges.GetChargesByIDsInput{ | ||
| Namespace: input.Namespace, | ||
| ChargeIDs: input.ChargeIDs, | ||
| Expands: charges.Expands{charges.ExpandRealizations}, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Missing charge IDs are silently ignored right now
At Line 22, GetChargesByIDs can return a partial set when some requested IDs do not exist. This method currently continues without detecting that, so callers can get a “successful” response with skipped IDs.
Suggested fix (detect and fail on missing IDs)
res, err := s.adapter.GetChargesByIDs(ctx, charges.GetChargesByIDsInput{
Namespace: input.Namespace,
ChargeIDs: input.ChargeIDs,
Expands: charges.Expands{charges.ExpandRealizations},
})
if err != nil {
return nil, err
}
+
+ requested := make(map[string]struct{}, len(input.ChargeIDs))
+ for _, id := range input.ChargeIDs {
+ requested[id] = struct{}{}
+ }
+
+ for _, ch := range res {
+ chargeID, err := ch.GetChargeID()
+ if err != nil {
+ return nil, err
+ }
+ delete(requested, chargeID.ID)
+ }
+
+ for missingID := range requested {
+ return nil, charges.NewChargeNotFoundError(input.Namespace, missingID)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/service/advance.go` around lines 22 - 30,
GetChargesByIDs can return a partial result; update the caller in advance.go
(the code invoking s.adapter.GetChargesByIDs and storing the result in res) to
detect missing IDs by comparing the returned charges' IDs against the requested
input.ChargeIDs and if any IDs are absent return a descriptive error instead of
proceeding; ensure you reference the adapter method s.adapter.GetChargesByIDs,
the input type charges.GetChargesByIDsInput and the variable res to locate where
to add the check.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/adapter.go`:
- Line 24: The concrete adapter is missing the ListCharges method required by
the charges.Adapter interface; add a method with signature ListCharges(ctx
context.Context, input ListChargesInput) (Charges, error) on the adapter
receiver (func (a *adapter) ListCharges(...)) that performs the adapter’s
existing listing logic (e.g., validate input, call the underlying
store/repository method, convert/store results into the Charges return type) so
the compile-time assertion var _ charges.Adapter = (*adapter)(nil) succeeds;
ensure error propagation and context usage mirror other adapter methods.
In `@openmeter/billing/charges/service/advance.go`:
- Around line 34-59: The second transaction.Run block containing the actual
charge-handling logic (calling s.adapter.GetChargesByIDs and mapChargesByType
with handlerByType for flatFee, usageBased, and creditPurchase) is currently
unreachable because an earlier transaction wrapper wasn't removed/merged; fix
this by removing the redundant first transaction or merging the logic so only
this transaction.Run is executed, ensuring you keep the GetChargesByIDs call,
the mapChargesByType dispatch, the flatFee branch that calls
s.flatFeeOrchestrator.AdvanceCreditOnlyCharge, the usageBased branch returning
fmt.Errorf("usage based charges are not supported": %w, charges.ErrUnsupported),
and the creditPurchase branch returning
charges.ErrCannotAdvanceCreditPurchaseCharge.WithAttrs(charge.ErrorAttributes()).
- Around line 21-32: The first transaction.Run block is incomplete and makes the
second block unreachable: add a proper return at the end of the lambda (return
out, nil) and implement the empty callback passed to
withBillingTransactionForInvoiceManipulation so it returns the intended charges
instead of (nil, nil), or remove/merge this entire first transaction.Run into
the subsequent transaction.Run block so only one fully-implemented transaction
flow remains; ensure the callback passed to
withBillingTransactionForInvoiceManipulation returns charges.Charges and error,
and do not use an early return that prevents the second transaction.Run from
executing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7122753-a33f-42b9-9cbf-0b6cab0d4fff
📒 Files selected for processing (10)
openmeter/billing/charges/adapter.goopenmeter/billing/charges/charge.goopenmeter/billing/charges/chargeflatfee.goopenmeter/billing/charges/errors.goopenmeter/billing/charges/service.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/flatfee/create.goopenmeter/billing/charges/service/flatfee/creditonly.goopenmeter/billing/charges/service/helpers.goopenmeter/billing/charges/service/service_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/billing/charges/charge.go
- openmeter/billing/charges/chargeflatfee.go
- openmeter/billing/charges/service/flatfee/create.go
- openmeter/billing/charges/service/helpers.go
| GetChargesByIDs(ctx context.Context, input GetChargesByIDsInput) (Charges, error) | ||
| CreateCharges(ctx context.Context, input CreateChargeInputs) (Charges, error) | ||
| UpdateFlatFeeCharge(ctx context.Context, charge FlatFeeCharge) (FlatFeeCharge, error) | ||
| ListCharges(ctx context.Context, input ListChargesInput) (Charges, error) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "adapter.go" -type f | head -20Repository: openmeterio/openmeter
Length of output: 882
🏁 Script executed:
rg "ListCharges" -A 5 --type goRepository: openmeterio/openmeter
Length of output: 1229
🏁 Script executed:
ast-grep --pattern $'func (a *adapter) ListCharges($$$) {
$$$
}'Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
cat ./openmeter/billing/charges/adapter/adapter.goRepository: openmeterio/openmeter
Length of output: 1550
🏁 Script executed:
rg "ListCharges" ./openmeter/billing/charges/adapter/ -A 10Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
wc -l ./openmeter/billing/charges/adapter/adapter.goRepository: openmeterio/openmeter
Length of output: 114
🏁 Script executed:
cat -n ./openmeter/billing/charges/adapter/adapter.go | tail -50Repository: openmeterio/openmeter
Length of output: 1448
Add the ListCharges implementation to the concrete adapter
The interface method is defined, but the concrete adapter struct in adapter/adapter.go is missing the implementation. The var _ charges.Adapter = (*adapter)(nil) check on line 44 will catch this at compile time, but you'll need to add the method to make it compile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/adapter.go` at line 24, The concrete adapter is
missing the ListCharges method required by the charges.Adapter interface; add a
method with signature ListCharges(ctx context.Context, input ListChargesInput)
(Charges, error) on the adapter receiver (func (a *adapter) ListCharges(...))
that performs the adapter’s existing listing logic (e.g., validate input, call
the underlying store/repository method, convert/store results into the Charges
return type) so the compile-time assertion var _ charges.Adapter =
(*adapter)(nil) succeeds; ensure error propagation and context usage mirror
other adapter methods.
Comment on lines
+21
to
+32
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (charges.Charges, error) { | ||
| out := make(charges.Charges, 0) | ||
| for _, customerID := range input.CustomerIDs { | ||
| charges, err := withBillingTransactionForInvoiceManipulation(ctx, s, customerID, func(ctx context.Context) (charges.Charges, error) { | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| out = append(out, charges...) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Critical: This code block has structural issues that will cause problems.
A few things going on here:
-
Missing return statement: The lambda passed to
transaction.Run(lines 21-32) iterates and appends toout, but never returns it. The function signature expects(charges.Charges, error)but there's noreturn out, nilbefore the closing}). -
Empty callback: The callback passed to
withBillingTransactionForInvoiceManipulation(lines 24-25) has an empty bodyfunc(ctx context.Context) (charges.Charges, error) { }. Based on context snippet 1 frominvoice.go, this will return(nil, nil), making the entire loop a no-op. -
Early return: Since line 21 uses
return transaction.Run(...), the second transaction block (lines 34-59) is completely unreachable.
This looks like incomplete code — perhaps the first block was intended to be removed or merged with the second block?
🐛 Suggested structure (assuming the second block is the intended logic)
func (s *service) AdvanceCreditOnlyCharges(ctx context.Context, input charges.AdvanceCreditOnlyChargesInput) (charges.Charges, error) {
if err := input.Validate(); err != nil {
return nil, err
}
- if len(input.CustomerIDs) == 0 {
+ if len(input.ChargeIDs) == 0 {
return nil, nil
}
- return transaction.Run(ctx, s.adapter, func(ctx context.Context) (charges.Charges, error) {
- out := make(charges.Charges, 0)
- for _, customerID := range input.CustomerIDs {
- charges, err := withBillingTransactionForInvoiceManipulation(ctx, s, customerID, func(ctx context.Context) (charges.Charges, error) {
- })
- if err != nil {
- return nil, err
- }
-
- out = append(out, charges...)
- }
- })
-
return transaction.Run(ctx, s.adapter, func(ctx context.Context) (charges.Charges, error) {
res, err := s.adapter.GetChargesByIDs(ctx, charges.GetChargesByIDsInput{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/service/advance.go` around lines 21 - 32, The first
transaction.Run block is incomplete and makes the second block unreachable: add
a proper return at the end of the lambda (return out, nil) and implement the
empty callback passed to withBillingTransactionForInvoiceManipulation so it
returns the intended charges instead of (nil, nil), or remove/merge this entire
first transaction.Run into the subsequent transaction.Run block so only one
fully-implemented transaction flow remains; ensure the callback passed to
withBillingTransactionForInvoiceManipulation returns charges.Charges and error,
and do not use an early return that prevents the second transaction.Run from
executing.
Comment on lines
+34
to
+59
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (charges.Charges, error) { | ||
| res, err := s.adapter.GetChargesByIDs(ctx, charges.GetChargesByIDsInput{ | ||
| Namespace: input.Namespace, | ||
| ChargeIDs: input.ChargeIDs, | ||
| Expands: charges.Expands{charges.ExpandRealizations}, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return mapChargesByType(res, handlerByType{ | ||
| flatFee: func(charge charges.FlatFeeCharge) (charges.FlatFeeCharge, error) { | ||
| if charge.Intent.SettlementMode != productcatalog.CreditOnlySettlementMode { | ||
| return charge, nil | ||
| } | ||
|
|
||
| return s.flatFeeOrchestrator.AdvanceCreditOnlyCharge(ctx, charge) | ||
| }, | ||
| usageBased: func(charge charges.UsageBasedCharge) (charges.UsageBasedCharge, error) { | ||
| return charge, fmt.Errorf("usage based charges are not supported: %w", charges.ErrUnsupported) | ||
| }, | ||
| creditPurchase: func(charge charges.CreditPurchaseCharge) (charges.CreditPurchaseCharge, error) { | ||
| return charge, charges.ErrCannotAdvanceCreditPurchaseCharge.WithAttrs(charge.ErrorAttributes()) | ||
| }, | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
This block contains the actual logic but is currently unreachable.
Assuming the first transaction block (lines 21-32) is removed or fixed, this second block looks correct and handles the three charge types appropriately:
- FlatFee: advances credit-only charges
- UsageBased: returns unsupported error
- CreditPurchase: returns the specific error
Once the structural issue above is resolved, this logic should work as intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/service/advance.go` around lines 34 - 59, The
second transaction.Run block containing the actual charge-handling logic
(calling s.adapter.GetChargesByIDs and mapChargesByType with handlerByType for
flatFee, usageBased, and creditPurchase) is currently unreachable because an
earlier transaction wrapper wasn't removed/merged; fix this by removing the
redundant first transaction or merging the logic so only this transaction.Run is
executed, ensuring you keep the GetChargesByIDs call, the mapChargesByType
dispatch, the flatFee branch that calls
s.flatFeeOrchestrator.AdvanceCreditOnlyCharge, the usageBased branch returning
fmt.Errorf("usage based charges are not supported": %w, charges.ErrUnsupported),
and the creditPurchase branch returning
charges.ErrCannotAdvanceCreditPurchaseCharge.WithAttrs(charge.ErrorAttributes()).
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
1 participant
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
Implement credit-only charges. These should bypass the invoicing stack altogether.
Summary by CodeRabbit
New Features
Tests