fix: validate billing discount correlation IDs - #4891
Open
turip wants to merge 1 commit into
Open
Conversation
turip
added
kind/bug
Contributor
📝 WalkthroughWalkthroughProduct-catalog discounts now remain separate from billing discounts until conversion. Billing conversion assigns and validates correlation IDs, preserves them during replacements, and applies the flow to customer charges and invoice lines. ChangesDiscount identity and conversion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant API
participant HTTPDriver
participant BillingDiscounts
participant InvoiceLine
API->>HTTPDriver: provide product-catalog discounts
HTTPDriver->>BillingDiscounts: convert or replace discounts
BillingDiscounts->>InvoiceLine: apply billing discounts with correlation IDs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![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: 1
🤖 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.
Inline comments:
In `@openmeter/billing/charges/service/featureid_test.go`:
- Around line 298-299: Update the persisted-charge assertions in
openmeter/billing/charges/service/featureid_test.go at lines 298-299 and
344-345: after the fetched charge is loaded, assert that
fetched.Intent.GetEffectiveIntent().PercentageDiscounts and
fetched.Intent.GetEffectiveIntent().Discounts.Usage are non-nil and each has a
non-empty CorrelationID, rather than checking only the originally returned
charge.
🪄 Autofix
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 Plus
Run ID: 140b0b55-eec3-4556-a69d-455ab034777c
📒 Files selected for processing (13)
api/v3/handlers/billinginvoices/convert.goapi/v3/handlers/billinginvoices/convert_test.goapi/v3/handlers/customers/charges/convert.goopenmeter/billing/README.mdopenmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/service/featureid_test.goopenmeter/billing/charges/usagebased/service/create.goopenmeter/billing/discount.goopenmeter/billing/discount_test.goopenmeter/billing/httpdriver/deprecations.goopenmeter/billing/httpdriver/discounts.goopenmeter/billing/httpdriver/invoiceline.goopenmeter/billing/httpdriver/invoiceline_test.go
Comment on lines
+298
to
+299
| s.Require().NotNil(flatFeeCharge.Intent.GetEffectiveIntent().PercentageDiscounts) | ||
| s.NotEmpty(flatFeeCharge.Intent.GetEffectiveIntent().PercentageDiscounts.CorrelationID) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert correlation IDs after the persisted-charge fetch.
The current assertions only inspect the returned charge. A persistence mapping that drops a correlation ID can still pass these tests.
openmeter/billing/charges/service/featureid_test.go#L298-L299: Assert thatfetched.Intent.GetEffectiveIntent().PercentageDiscountsis non-nil and has a non-emptyCorrelationID.openmeter/billing/charges/service/featureid_test.go#L344-L345: Assert thatfetched.Intent.GetEffectiveIntent().Discounts.Usageis non-nil and has a non-emptyCorrelationID.
As per coding guidelines, “Suggest missing tests only when the PR introduces behavior without meaningful coverage, or when the current tests would pass despite a concrete regression in the changed code.”
📍 Affects 1 file
openmeter/billing/charges/service/featureid_test.go#L298-L299(this comment)openmeter/billing/charges/service/featureid_test.go#L344-L345
🤖 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/charges/service/featureid_test.go` around lines 298 - 299,
Update the persisted-charge assertions in
openmeter/billing/charges/service/featureid_test.go at lines 298-299 and
344-345: after the fetched charge is loaded, assert that
fetched.Intent.GetEffectiveIntent().PercentageDiscounts and
fetched.Intent.GetEffectiveIntent().Discounts.Usage are non-nil and each has a
non-empty CorrelationID, rather than checking only the originally returned
charge.
Source: Coding guidelines
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+39
to
+41
| if d.CorrelationID == "" { | ||
| errs = append(errs, errors.New("correlation ID is required")) | ||
| } |
Contributor
There was a problem hiding this comment.
If persisted flat-fee or usage-based charges have discounts without correlation IDs, the new validation rejects them during UpdateCharge before the adapters reach their defensive backfill, causing charge advancement and patch operations to fail with correlation ID is required.
Knowledge Base Used: Billing: Invoices, Charges, Tax, and Currency
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/discount.go
Line: 39-41
Comment:
**Legacy charge repair is blocked**
If persisted flat-fee or usage-based charges have discounts without correlation IDs, the new validation rejects them during `UpdateCharge` before the adapters reach their defensive backfill, causing charge advancement and patch operations to fail with `correlation ID is required`.
**Knowledge Base Used:** [Billing: Invoices, Charges, Tax, and Currency](https://app.greptile.com/openmeter/-/custom-context/knowledge-base/openmeterio/openmeter/-/docs/billing.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
chrisgacsal
approved these changes
Aug 10, 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.
Summary
Root cause
The original correlation-ID repair allocated IDs at charge lifecycle and persistence boundaries, but the billing discount validators still inherited product-catalog validation and therefore did not enforce billing identity. API conversion paths could also construct billing discount types directly from unowned request data. That made an uncorrelated billing-owned discount valid to the type's validator and left correctness dependent on every caller remembering a later fallback.
Impact
Billing-owned discounts now reject missing correlation IDs. Product-catalog discounts remain valid without billing identity, and create/update boundaries explicitly materialize or reconcile billing correlation IDs. Existing defensive upserts remain in place for compatibility.
Validation
go test -tags=dynamic ./openmeter/billing/... ./api/v3/handlers/billinginvoices ./api/v3/handlers/customers/chargesgolangci-lint run --config .golangci-fast.yaml ./openmeter/billing/... ./api/v3/handlers/billinginvoices/... ./api/v3/handlers/customers/charges/...Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
The PR establishes correlation IDs as required billing-owned identity for percentage and usage discounts.
Confidence Score: 4/5
The legacy charge-update compatibility failure should be fixed before merging because affected charges cannot advance or accept patches.
The new validators reject empty correlation IDs at the start of adapter updates, while the adapters' explicit legacy-data backfills execute only after validation and are therefore unreachable for the entities they are intended to repair.
Files Needing Attention: openmeter/billing/discount.go and the flat-fee and usage-based charge adapters
Important Files Changed
Sequence Diagram
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: validate billing discount correlati..." | Re-trigger Greptile
Context used: