fix(billing): route API invoice delete to line engines#4651
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
Greptile SummaryThis PR routes API invoice deletion through invoice-line engine behavior. The main changes are:
Confidence Score: 4/5This is close, but this delete guard should be fixed before merging.
openmeter/billing/httpdriver/invoice.go Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
openmeter/billing/httpdriver/invoice.go:476
**Credit purchases still pass**
This guard blocks usage-based charge lines, but active credit-purchase lines use `LineEngineTypeChargeCreditPurchase` and their mutable-line API hook also rejects with `ErrCannotUpdateChargeManagedLine`. A public API delete for an invoice with flat-fee and credit-purchase lines can pass this guard, then reach the later per-engine delete dispatch. If the flat-fee engine runs first, its manual-delete cleanup can execute before the credit-purchase engine rejects, leaving the invoice undeleted with partial cleanup already applied.
Reviews (3): Last reviewed commit: "fix(billing): support gathering invoice ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/service/invoiceupdate.go (1)
908-921: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant validation pass on already-validated grouped input.
GroupByLineEngine()already callsValidate()internally for every per-engine group before returning it (seestdinvoiceedit.go'sGroupByLineEngine, which doesif err := out[engine].Validate(); err != nil { ... }). CallinggroupedInput.Validate()again right after in the loop (Line 919) re-validates the exact same data for no additional benefit — it's just wasted work on every dispatch.♻️ Proposed fix: drop the redundant re-validation
for engineType, groupedInput := range changesByEngine { engine, err := s.lineEngines.Get(engineType) if err != nil { return fmt.Errorf("getting engine %s: %w", engineType, err) } - if err := groupedInput.Validate(); err != nil { - return fmt.Errorf("validating API invoice line delete input for engine %s: %w", engine.GetLineEngineType(), err) - } - engineResult, err := engine.OnMutableInvoiceLinesEditedViaAPI(ctx, groupedInput)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/service/invoiceupdate.go` around lines 908 - 921, `input.GroupByLineEngine()` already validates each grouped payload, so the extra `groupedInput.Validate()` inside the `invoiceupdate.go` dispatch loop is redundant. Remove that second validation in the loop over `changesByEngine`, and keep the existing error handling around `GroupByLineEngine()` and `s.lineEngines.Get()` so `groupedInput` is used directly after grouping.openmeter/billing/service/lineengine_test.go (1)
202-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding given/when/then intent comment.
Logic looks correct — non-deleted
line-1is the one routed to the API edit hook,deletedBySystemInputsstays empty, matching the production filter indeleteInvoice. Only nit: the sibling test file in this same PR (sync_credittheninvoice_test.go) adds given/when/then intent comments to its new tests, but this one doesn't. Worth a quick line or two for consistency.As per coding guidelines, "For service and lifecycle subtests, start each non-trivial subtest body with concise intent comments in given/when/then form."
📝 Suggested comment addition
func TestDeleteInvoiceAPIRequestDispatchesNonDeletedLinesToAPIEditHook(t *testing.T) { + // given: an invoice with one active line and one already-deleted line + // when: the invoice is deleted via ChangeSourceAPIRequest + // then: only the active line is routed to the API edit hook, no system-delete dispatch occurs invoiceEngine := &recordingLineEngine{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/service/lineengine_test.go` around lines 202 - 241, Add concise given/when/then intent comments at the start of this non-trivial test body for consistency with the service/lifecycle test guidelines. Update TestDeleteInvoiceAPIRequestDispatchesNonDeletedLinesToAPIEditHook to include a short setup/behavior/assertion comment sequence near the InvoiceStateMachine and deleteInvoice call so the test’s purpose is immediately clear.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/billing/service/invoiceupdate.go`:
- Around line 908-921: `input.GroupByLineEngine()` already validates each
grouped payload, so the extra `groupedInput.Validate()` inside the
`invoiceupdate.go` dispatch loop is redundant. Remove that second validation in
the loop over `changesByEngine`, and keep the existing error handling around
`GroupByLineEngine()` and `s.lineEngines.Get()` so `groupedInput` is used
directly after grouping.
In `@openmeter/billing/service/lineengine_test.go`:
- Around line 202-241: Add concise given/when/then intent comments at the start
of this non-trivial test body for consistency with the service/lifecycle test
guidelines. Update
TestDeleteInvoiceAPIRequestDispatchesNonDeletedLinesToAPIEditHook to include a
short setup/behavior/assertion comment sequence near the InvoiceStateMachine and
deleteInvoice call so the test’s purpose is immediately clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46f0bf0b-d861-45cb-abb1-d0a345f5420b
📒 Files selected for processing (4)
openmeter/billing/service/invoiceupdate.goopenmeter/billing/service/lineengine_test.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/billing/httpdriver/invoice_test.go (1)
74-146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNice, thorough table-driven coverage of
validateAPIGenericInvoiceDeleteSupported! One gap: the wrappervalidateAPIInvoiceDeleteSupported(which callsh.service.GetInvoiceByIdandAsGenericInvoice) isn't directly tested here — only the pure logic function is. A quick test using the handler's mocked service to cover theGetInvoiceByIderror path would close that gap.As per path instructions, "Make sure the tests are comprehensive and cover the changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/httpdriver/invoice_test.go` around lines 74 - 146, Add a test for validateAPIInvoiceDeleteSupported, not just validateAPIGenericInvoiceDeleteSupported, so the handler wrapper path is covered. Use the existing handler and mocked service around h.service.GetInvoiceById and AsGenericInvoice to verify the GetInvoiceById error path is exercised and asserted. Keep the current table-driven validateAPIGenericInvoiceDeleteSupported coverage, but extend test coverage to the wrapper function so the delete-support checks are fully comprehensive.openmeter/billing/httpdriver/invoice.go (1)
406-425: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffDouble full-invoice load on every API delete.
validateAPIInvoiceDeleteSupportedfetches the invoice withInvoiceExpandAll(all lines) purely to validate, thenh.service.DeleteInvoice(Line 374) runs and will need its own load to actually perform the deletion. That's two full line-expanded fetches per delete request.Since deletes aren't a hot path this is likely tolerable, but worth confirming there isn't a way to thread the already-loaded invoice/generic-lines into the delete call to avoid the redundant read, especially for invoices with many lines.
As per path instructions, "database operations (regardless of database) should be vetted for potential performance bottlenecks."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/httpdriver/invoice.go` around lines 406 - 425, The delete path in validateAPIInvoiceDeleteSupported and h.service.DeleteInvoice is loading the full invoice twice with InvoiceExpandAll, so check whether DeleteInvoice can accept the already-fetched invoice or generic lines from validateAPIInvoiceDeleteSupported. If possible, thread the loaded invoice/generic invoice through the delete flow in invoice.go to avoid the redundant full-line read; otherwise document why the second load is required and confirm it is acceptable for this path.openmeter/billing/invoiceline.go (1)
104-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo identical accessors on the same interface — intentional?
GetEngine()and the newGetLineEngineType()both returnLineEngineType, and their implementations inStandardLine/GatheringLineBaseare byte-for-byte identical (return i.Engine/return g.Engine). UnlessGetLineEngineType()is meant to satisfy a separate shared contract (e.g. matchinglineengine.Engine.GetLineEngineType()for polymorphic handling alongside the line-engine type), having two same-signature getters returning the same value on one interface is confusing and invites drift if only one gets updated later.Worth either documenting why both exist, or consolidating call sites onto one name.
As per path instructions, "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/invoiceline.go` around lines 104 - 105, The invoicing line interface currently exposes two identical getters, GetEngine() and GetLineEngineType(), both returning the same LineEngineType, which is confusing and risks divergence. In invoiceline.go and the matching StandardLine/GatheringLineBase implementations, either consolidate call sites onto a single accessor or add a clear comment explaining why both names must exist for separate contracts; keep the interface and implementations aligned so there is only one obvious source of truth for the engine type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/billing/httpdriver/invoice_test.go`:
- Around line 74-146: Add a test for validateAPIInvoiceDeleteSupported, not just
validateAPIGenericInvoiceDeleteSupported, so the handler wrapper path is
covered. Use the existing handler and mocked service around
h.service.GetInvoiceById and AsGenericInvoice to verify the GetInvoiceById error
path is exercised and asserted. Keep the current table-driven
validateAPIGenericInvoiceDeleteSupported coverage, but extend test coverage to
the wrapper function so the delete-support checks are fully comprehensive.
In `@openmeter/billing/httpdriver/invoice.go`:
- Around line 406-425: The delete path in validateAPIInvoiceDeleteSupported and
h.service.DeleteInvoice is loading the full invoice twice with InvoiceExpandAll,
so check whether DeleteInvoice can accept the already-fetched invoice or generic
lines from validateAPIInvoiceDeleteSupported. If possible, thread the loaded
invoice/generic invoice through the delete flow in invoice.go to avoid the
redundant full-line read; otherwise document why the second load is required and
confirm it is acceptable for this path.
In `@openmeter/billing/invoiceline.go`:
- Around line 104-105: The invoicing line interface currently exposes two
identical getters, GetEngine() and GetLineEngineType(), both returning the same
LineEngineType, which is confusing and risks divergence. In invoiceline.go and
the matching StandardLine/GatheringLineBase implementations, either consolidate
call sites onto a single accessor or add a clear comment explaining why both
names must exist for separate contracts; keep the interface and implementations
aligned so there is only one obvious source of truth for the engine type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b14cb22d-95f8-4e75-ab61-7ed220d86286
📒 Files selected for processing (5)
openmeter/billing/gatheringinvoice.goopenmeter/billing/httpdriver/invoice.goopenmeter/billing/httpdriver/invoice_test.goopenmeter/billing/invoiceline.goopenmeter/billing/stdinvoiceline.go
2 participants
Summary
Root Cause
API standard-invoice deletion previously skipped
OnMutableInvoiceLinesEditedViaAPI. That meant charge-backed lines were not asked to validate or perform their manual-delete side effects before the invoice entered deletion. Usage-based progressive invoice lines could therefore be deleted at invoice scope without surfacing the charge-managed-line rejection, while flat-fee-only invoice deletion did not exercise the same manual cleanup path as deleting the line through the invoice API.Validation
env GOCACHE=/private/tmp/openmeter-go-build-cache POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/billing/worker/subscriptionsync/service -run 'TestCreditThenInvoiceScenarios/TestDeleteStandardInvoice' -count=1 -v\n-env GOCACHE=/private/tmp/openmeter-go-build-cache POSTGRES_HOST=127.0.0.1 go vet -tags=dynamic ./openmeter/billing/service ./openmeter/billing/worker/subscriptionsync/service\n-env GOCACHE=/private/tmp/openmeter-go-build-cache POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/billing/service -run TestDeleteInvoice -count=1 -vSummary by CodeRabbit