Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end usage-based billing: new domain models, adapter and service layers, DB mappers, wiring into existing charges service, and test scaffolding—enabling creation, retrieval, and transformation of usage-based charges and intents. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Service as UsageBasedService
participant Adapter
participant MetaAdapter
participant DB as EntDB
participant Gathering as GatheringLine
Caller->>Service: Create(ctx, CreateInput{Intents})
Service->>Service: Validate input
Service->>Adapter: CreateCharges(ctx, input)
Adapter->>MetaAdapter: Create charge metas
MetaAdapter->>DB: Persist meta records
DB-->>MetaAdapter: Meta IDs
Adapter->>DB: Bulk create ChargeUsageBased rows
DB-->>Adapter: Created charge rows
Adapter-->>Service: []usagebased.Charge
Service->>Gathering: Transform charges → GatheringLine (per settlement mode)
Gathering-->>Service: ChargeWithGatheringLine
Service-->>Caller: []ChargeWithGatheringLine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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/usage-based-charges
branch
from
78b8394 to
3533b26
Compare
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![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: 5
🤖 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/charge.go`:
- Around line 96-102: The AsUsageBasedCharge method lacks a nil-check before
dereferencing c.usageBased; update AsUsageBasedCharge to first verify c.t ==
meta.ChargeTypeUsageBased (already present) and then check if c.usageBased ==
nil, returning an appropriate error like "usage based charge is nil" if so,
otherwise return *c.usageBased; mirror the pattern used in AsFlatFeeCharge to
avoid a potential nil pointer dereference.
In `@openmeter/billing/charges/usagebased/adapter/charge.go`:
- Around line 98-145: The code incorrectly pairs DB entities to input metas by
slice index; change the logic in the entutils.TransactingRepo callback so you
first build a map metaByID from input.Charges keyed by meta.ID, then iterate the
returned entities and for each entity use entity.ID to look up the corresponding
meta (use MapUsageBasedChargeFromDB(entity, metaByID[entity.ID],
input.Expands)); collect mapped charges, group by ID as before, and finally use
the existing final loop to restore caller order and report missing IDs. Ensure
you handle missing metas when mapping entities and propagate errors from
MapUsageBasedChargeFromDB.
In `@openmeter/billing/charges/usagebased/charge.go`:
- Around line 149-151: The struct field RealizationRun.Type currently uses the
JSON tag `status` causing incorrect serialization; update the tag on the Type
field in the RealizationRun definition from `json:"status"` to `json:"type"` so
API payloads and round-trips expose the run kind under the correct key.
- Around line 80-103: Add a pre-check in Intent.Validate to reject non-flat
prices with an empty FeatureKey: before calling i.Price.Validate(), check the
Price for non-flat (e.g., not a flat/one-time price) and if its FeatureKey is
empty append an error like "price: feature key required for non-flat prices";
reference the Price struct/field (i.Price.FeatureKey) and the Intent.Validate
method so this mirrors the rule enforced by billing.GatheringLineBase.Validate
and prevents persisting charges that will later fail gathering line validation.
In `@openmeter/billing/charges/usagebased/service/create.go`:
- Around line 34-43: The code is returning invoice-managed gathering lines for
non-credit usage-based charges (via gatheringLineFromUsageBasedCharge) which
causes those charges to enter invoice allocation before the usage-based
lifecycle is wired; change the behavior to keep GatheringLineToCreate nil until
the usage-based lifecycle/hooks are implemented or explicitly enabled: in the
function that maps charges (where ChargeWithGatheringLine is constructed) return
ChargeWithGatheringLine{Charge: charge} with no GatheringLineToCreate for
non-credit-only charges (or gate creation behind a clear boolean/feature flag on
the charges service), and apply the same fix to the other places in this file
that currently call gatheringLineFromUsageBasedCharge so no invoice-managed
lines are emitted prematurely.
🪄 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: 37179185-314b-41cd-9c7a-79598494205b
⛔ Files ignored due to path filters (54)
openmeter/ent/db/charge.gois excluded by!**/ent/db/**openmeter/ent/db/charge/charge.gois excluded by!**/ent/db/**openmeter/ent/db/charge/where.gois excluded by!**/ent/db/**openmeter/ent/db/charge_create.gois excluded by!**/ent/db/**openmeter/ent/db/charge_query.gois excluded by!**/ent/db/**openmeter/ent/db/charge_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage/chargeusagebasedruninvoicedusage.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment/chargeusagebasedrunpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_update.gois excluded by!**/ent/db/**openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.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/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (20)
openmeter/billing/charges/charge.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/get.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/service.goopenmeter/billing/charges/service/service_test.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/adapter.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/adapter/mapper.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/create.goopenmeter/billing/charges/usagebased/service/get.goopenmeter/billing/charges/usagebased/service/service.goopenmeter/ent/schema/charges.goopenmeter/ent/schema/chargesusagebased.gotools/migrate/migrations/20260316153605_charge_usage_based.down.sqltools/migrate/migrations/20260316153605_charge_usage_based.up.sql
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
openmeter/billing/charges/usagebased/charge.go (2)
149-151:⚠️ Potential issue | 🟠 Major
RealizationRun.Typeis exposed under the wrong JSON field.
Typeis currently tagged asstatus, so serialized payloads won’t round-trip correctly.💡 Suggested fix
- Type RealizationRunType `json:"status"` + Type RealizationRunType `json:"type"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/charge.go` around lines 149 - 151, The RealizationRun struct exposes the Type field under the wrong JSON tag; update the struct field declaration for RealizationRun.Type in charge.go to use the correct JSON tag (json:"type") instead of json:"status" so serialized/deserialized payloads round-trip correctly; ensure the change is applied to the Type field of the RealizationRun type and run tests or serialization checks to confirm no other code relies on the old "status" tag.
80-103:⚠️ Potential issue | 🟠 MajorPlease fail fast when non-flat prices are missing
FeatureKey.Right now this can pass
Intent.Validate()and fail later downstream when building/validating invoice/gathering lines.💡 Suggested fix
func (i Intent) Validate() error { var errs []error @@ if i.InvoiceAt.IsZero() { errs = append(errs, fmt.Errorf("invoice at is required")) } + if i.Price.Type() != productcatalog.FlatPriceType && i.FeatureKey == "" { + errs = append(errs, fmt.Errorf("feature key is required for non-flat prices")) + } + if err := i.Price.Validate(); err != nil { errs = append(errs, fmt.Errorf("price: %w", err)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/charge.go` around lines 80 - 103, The Intent.Validate() flow must fail fast when a non-flat price is missing FeatureKey: add an explicit check in Intent.Validate (before calling i.Price.Validate or before returning) that detects a non-flat price (e.g., using Price.Type or a helper like Price.IsFlat()) and returns an immediate error if Price.FeatureKey is empty (e.g., "price: non-flat price requires feature key"); update the validation path around the existing Intent.Validate(), Price.Validate(), and FeatureKey references so the missing FeatureKey is caught early rather than downstream.openmeter/billing/charges/charge.go (1)
96-102:⚠️ Potential issue | 🔴 Critical
AsUsageBasedCharge()can panic on nil pointer.It checks type, but not
c.usageBasedbefore dereferencing.💡 Suggested fix
func (c Charge) AsUsageBasedCharge() (usagebased.Charge, error) { if c.t != meta.ChargeTypeUsageBased { return usagebased.Charge{}, fmt.Errorf("charge is not a usage based charge") } + if c.usageBased == nil { + return usagebased.Charge{}, fmt.Errorf("usage based charge is nil") + } + return *c.usageBased, nil }🤖 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 96 - 102, AsUsageBasedCharge currently checks c.t against meta.ChargeTypeUsageBased but dereferences c.usageBased without nil-check, which can panic; update AsUsageBasedCharge to also validate c.usageBased != nil (in addition to c.t == meta.ChargeTypeUsageBased) and return a descriptive error (e.g., "usageBased data is nil for usage-based charge") instead of dereferencing when nil so the function safely returns (usagebased.Charge{}, error).openmeter/billing/charges/usagebased/adapter/charge.go (1)
122-129:⚠️ Potential issue | 🔴 CriticalIndex-based pairing assumes query returns results in input order — this is a bug.
The
IDIn(...)query has no ordering guarantee, but line 124 pairsentities[idx]withinput.Charges[idx]. If the DB returns rows in a different order (or fewer rows), you'll map the wrong meta to the wrong entity.The fix from the previous review is still needed — build a
metaByIDmap first, then look up byentity.ID:+ metaByID := make(map[string]meta.Charge, len(input.Charges)) + for _, chargeMeta := range input.Charges { + metaByID[chargeMeta.ID] = chargeMeta + } + entitiesMapped := make([]usagebased.Charge, 0, len(entities)) - for idx, entity := range entities { - charge, err := MapUsageBasedChargeFromDB(entity, input.Charges[idx], input.Expands) + for _, entity := range entities { + chargeMeta, ok := metaByID[entity.ID] + if !ok { + return nil, fmt.Errorf("charge meta not found for usage based charge [id=%s]", entity.ID) + } + charge, err := MapUsageBasedChargeFromDB(entity, chargeMeta, input.Expands) if err != nil { return nil, err } entitiesMapped = append(entitiesMapped, charge) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/adapter/charge.go` around lines 122 - 129, The loop currently pairs DB rows to input charges by index (entities[idx] with input.Charges[idx]) which is incorrect because IDIn queries are unordered; instead build a map from input charge ID to its metadata (e.g., metaByID from input.Charges keyed by charge.ID) and then iterate entities, looking up the matching meta via entity.ID and passing that to MapUsageBasedChargeFromDB; ensure you handle missing meta (returning an error) and update the code that appends to entitiesMapped to use the looked-up meta rather than input.Charges[idx].
🧹 Nitpick comments (1)
openmeter/billing/charges/usagebased/adapter/mapper.go (1)
68-93: Edge loading error handling is strict but potentially confusing.The pattern here returns an error when edges aren't loaded (lines 69-71, 78-80, 87-89), but then proceeds to use those edges anyway if they are loaded. This works, but the control flow is a bit subtle:
CreditAllocationsOrErr()returns an error if not loaded- You check for
NotLoadedErrorand return early- But if there's no error, you proceed
This is correct, but the variable naming (
err) being reused without checking the actual error value in the happy path might confuse future readers. Consider adding a brief comment or restructuring slightly.That said, this matches similar patterns elsewhere, so it's fine as-is if that's the convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/adapter/mapper.go` around lines 68 - 93, The edge-loading checks reuse the same err variable for multiple EdgeOrErr calls (CreditAllocationsOrErr, InvoicedUsageOrErr, PaymentOrErr), which makes the control flow subtle; rename each error variable (e.g., errAlloc, errInvoice, errPayment) or explicitly check err != nil before using lo.ErrorsAs for each call and add a one-line comment above each block explaining "return on NotLoadedError for this edge"; update the blocks that set run.CreditsAllocated, run.InvoiceUsage, and run.Payment to reference the new error variables so the intent is explicit and future readers won't be confused.
🤖 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/service_test.go`:
- Around line 781-864: The test named TestUsageBasedPartialCreditRealizations
only creates and fetches a usage-based charge but does not exercise partial
credit realizations; either rename it to TestUsageBasedCreateAndGet or extend it
to follow the full lifecycle used by TestFlatFeePartialCreditRealizations: after
creating the charge with Charges.Create and verifying the gathering invoice via
BillingService.ListGatheringInvoices and mustGetChargeByID, create an invoice
for the servicePeriod, apply partial credit realizations to that invoice,
approve the payment (manual approval flow already configured), settle the
invoice and assert settlement/realization state and amounts (mirror the
assertions and helper calls used in TestFlatFeePartialCreditRealizations).
Ensure you update the test function name
(TestUsageBasedPartialCreditRealizations) or add the additional
invoice/realization/assert calls around usageBasedChargeID and related helpers
accordingly.
In `@openmeter/billing/charges/usagebased/service/create.go`:
- Line 47: Rename the parameter in function gatheringLineFromUsageBasedCharge
from flatFee to charge (its type is usagebased.Charge) and update all internal
references to use charge instead of flatFee, including uses in the return type
handling for usagebased.ChargeWithGatheringLine and any variable accesses or
field lookups that currently reference flatFee.
---
Duplicate comments:
In `@openmeter/billing/charges/charge.go`:
- Around line 96-102: AsUsageBasedCharge currently checks c.t against
meta.ChargeTypeUsageBased but dereferences c.usageBased without nil-check, which
can panic; update AsUsageBasedCharge to also validate c.usageBased != nil (in
addition to c.t == meta.ChargeTypeUsageBased) and return a descriptive error
(e.g., "usageBased data is nil for usage-based charge") instead of dereferencing
when nil so the function safely returns (usagebased.Charge{}, error).
In `@openmeter/billing/charges/usagebased/adapter/charge.go`:
- Around line 122-129: The loop currently pairs DB rows to input charges by
index (entities[idx] with input.Charges[idx]) which is incorrect because IDIn
queries are unordered; instead build a map from input charge ID to its metadata
(e.g., metaByID from input.Charges keyed by charge.ID) and then iterate
entities, looking up the matching meta via entity.ID and passing that to
MapUsageBasedChargeFromDB; ensure you handle missing meta (returning an error)
and update the code that appends to entitiesMapped to use the looked-up meta
rather than input.Charges[idx].
In `@openmeter/billing/charges/usagebased/charge.go`:
- Around line 149-151: The RealizationRun struct exposes the Type field under
the wrong JSON tag; update the struct field declaration for RealizationRun.Type
in charge.go to use the correct JSON tag (json:"type") instead of json:"status"
so serialized/deserialized payloads round-trip correctly; ensure the change is
applied to the Type field of the RealizationRun type and run tests or
serialization checks to confirm no other code relies on the old "status" tag.
- Around line 80-103: The Intent.Validate() flow must fail fast when a non-flat
price is missing FeatureKey: add an explicit check in Intent.Validate (before
calling i.Price.Validate or before returning) that detects a non-flat price
(e.g., using Price.Type or a helper like Price.IsFlat()) and returns an
immediate error if Price.FeatureKey is empty (e.g., "price: non-flat price
requires feature key"); update the validation path around the existing
Intent.Validate(), Price.Validate(), and FeatureKey references so the missing
FeatureKey is caught early rather than downstream.
---
Nitpick comments:
In `@openmeter/billing/charges/usagebased/adapter/mapper.go`:
- Around line 68-93: The edge-loading checks reuse the same err variable for
multiple EdgeOrErr calls (CreditAllocationsOrErr, InvoicedUsageOrErr,
PaymentOrErr), which makes the control flow subtle; rename each error variable
(e.g., errAlloc, errInvoice, errPayment) or explicitly check err != nil before
using lo.ErrorsAs for each call and add a one-line comment above each block
explaining "return on NotLoadedError for this edge"; update the blocks that set
run.CreditsAllocated, run.InvoiceUsage, and run.Payment to reference the new
error variables so the intent is explicit and future readers won't be confused.
🪄 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: 5c18e408-9069-4d0a-8102-e6152b9a6dd8
⛔ Files ignored due to path filters (54)
openmeter/ent/db/charge.gois excluded by!**/ent/db/**openmeter/ent/db/charge/charge.gois excluded by!**/ent/db/**openmeter/ent/db/charge/where.gois excluded by!**/ent/db/**openmeter/ent/db/charge_create.gois excluded by!**/ent/db/**openmeter/ent/db/charge_query.gois excluded by!**/ent/db/**openmeter/ent/db/charge_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage/chargeusagebasedruninvoicedusage.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment/chargeusagebasedrunpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_update.gois excluded by!**/ent/db/**openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.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/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (20)
openmeter/billing/charges/charge.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/get.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/service.goopenmeter/billing/charges/service/service_test.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/adapter.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/adapter/mapper.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/create.goopenmeter/billing/charges/usagebased/service/get.goopenmeter/billing/charges/usagebased/service/service.goopenmeter/ent/schema/charges.goopenmeter/ent/schema/chargesusagebased.gotools/migrate/migrations/20260316153605_charge_usage_based.down.sqltools/migrate/migrations/20260316153605_charge_usage_based.up.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openmeter/billing/charges/usagebased/adapter/charge.go (1)
122-149:⚠️ Potential issue | 🔴 CriticalMap DB rows to metas by
entity.ID, not slice position.
Line 124assumesquery.All(ctx)result order matchesinput.Charges. WithIDIn(...)and no explicit ordering, that is unsafe and can bind the wrongmeta.Chargeto a row.💡 Proposed fix
- entitiesMapped := make([]usagebased.Charge, 0, len(entities)) - for idx, entity := range entities { - charge, err := MapUsageBasedChargeFromDB(entity, input.Charges[idx], input.Expands) + metaByID := make(map[string]meta.Charge, len(input.Charges)) + for _, chargeMeta := range input.Charges { + metaByID[chargeMeta.ID] = chargeMeta + } + + entitiesMapped := make([]usagebased.Charge, 0, len(entities)) + for _, entity := range entities { + chargeMeta, ok := metaByID[entity.ID] + if !ok { + return nil, fmt.Errorf("charge meta not found for usage based charge [id=%s]", entity.ID) + } + + charge, err := MapUsageBasedChargeFromDB(entity, chargeMeta, input.Expands) if err != nil { return nil, err } entitiesMapped = append(entitiesMapped, charge) }Use this to verify the positional mapping assumption and missing explicit ordering:
#!/bin/bash set -euo pipefail file="openmeter/billing/charges/usagebased/adapter/charge.go" echo "=== GetByIDs block ===" sed -n '93,156p' "$file" echo echo "=== Positional mapping callsite ===" rg -nP 'MapUsageBasedChargeFromDB\(entity,\s*input\.Charges\[idx\]' "$file" echo echo "=== Query section (check for explicit ordering) ===" rg -n -C2 'ChargeUsageBased\.Query|IDIn\(|All\(ctx\)|Order\(' "$file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/adapter/charge.go` around lines 122 - 149, The current loop calls MapUsageBasedChargeFromDB(entity, input.Charges[idx], ...) assuming entities are in same order as input.Charges; instead build a map of input charges by ID and, when iterating the DB rows (entities), look up the corresponding input charge by entity.ID before calling MapUsageBasedChargeFromDB; update the code that creates entitiesMapped (and subsequently entitiesByID) to use that lookup, return an error if a DB row has no matching input charge, and keep the final verification that the number of mapped charges equals len(input.Charges). Ensure you touch the MapUsageBasedChargeFromDB call site, the entitiesMapped construction, and the lookup logic using entity.ID rather than the slice index.
🤖 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/service_test.go`:
- Around line 863-864: The assertion s.NotEmpty(usageBasedChargeID) is outside
the s.Run subtest and can panic if the subtest calls FailNow; move the
s.NotEmpty(usageBasedChargeID) check inside the s.Run closure (alongside the
existing s.NoError checks) or remove it entirely — alternatively rely on the
existing mustGetChargeByID/GetChargeID validation at line 860–865; update the
test so any assertion that depends on the subtest's success (usageBasedChargeID)
executes inside the subtest to avoid the "subtest may have called FailNow on a
parent test" panic.
In `@openmeter/billing/charges/usagebased/charge.go`:
- Around line 178-180: The error message for the meter value check is
misleading: the code only rejects negative values via r.MeterValue.IsNegative(),
so update the error text appended to errs in that conditional to reflect the
actual rule (e.g., use "meter value must be non-negative" or "meter value must
not be negative") so the message matches the check in the function where
r.MeterValue.IsNegative() is used.
---
Duplicate comments:
In `@openmeter/billing/charges/usagebased/adapter/charge.go`:
- Around line 122-149: The current loop calls MapUsageBasedChargeFromDB(entity,
input.Charges[idx], ...) assuming entities are in same order as input.Charges;
instead build a map of input charges by ID and, when iterating the DB rows
(entities), look up the corresponding input charge by entity.ID before calling
MapUsageBasedChargeFromDB; update the code that creates entitiesMapped (and
subsequently entitiesByID) to use that lookup, return an error if a DB row has
no matching input charge, and keep the final verification that the number of
mapped charges equals len(input.Charges). Ensure you touch the
MapUsageBasedChargeFromDB call site, the entitiesMapped construction, and the
lookup logic using entity.ID rather than the slice index.
🪄 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: 5ed6fb95-49a8-4cb2-9dcc-410193b76573
📒 Files selected for processing (5)
openmeter/billing/charges/charge.goopenmeter/billing/charges/service/service_test.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/charge.gotest/billing/suite.go
turip
force-pushed
the
feat/usage-based-charges
branch
from
9761ccb to
91086aa
Compare
(only get/create is working)
turip
force-pushed
the
feat/usage-based-charges
branch
from
91086aa to
14bf466
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openmeter/billing/charges/usagebased/adapter/charge.go (1)
122-153:⚠️ Potential issue | 🔴 CriticalMap DB rows to meta by
entity.ID, not byidx.
query.All(ctx)withIDIn(...)has no guaranteed positional match toinput.Charges, soinput.Charges[idx]can pair wrong meta with a row. Also, missing-ID details inerrsare currently hidden by the earlier length check.💡 Suggested fix
- entitiesMapped := make([]usagebased.Charge, 0, len(entities)) - for idx, entity := range entities { - charge, err := MapUsageBasedChargeFromDB(entity, input.Charges[idx], input.Expands) + metaByID := make(map[string]meta.Charge, len(input.Charges)) + for _, chargeMeta := range input.Charges { + metaByID[chargeMeta.ID] = chargeMeta + } + + entitiesMapped := make([]usagebased.Charge, 0, len(entities)) + for _, entity := range entities { + chargeMeta, ok := metaByID[entity.ID] + if !ok { + return nil, fmt.Errorf("charge meta not found for usage based charge [id=%s]", entity.ID) + } + + charge, err := MapUsageBasedChargeFromDB(entity, chargeMeta, input.Expands) if err != nil { return nil, err } entitiesMapped = append(entitiesMapped, charge) } @@ - if len(out) != len(input.Charges) { - return nil, fmt.Errorf("expected to fetch %d charges, got %d", len(input.Charges), len(out)) - } - if len(errs) > 0 { return nil, errors.Join(errs...) }#!/bin/bash # Verify index-based mapping and lack of explicit ordering in GetByIDs rg -n -C3 'MapUsageBasedChargeFromDB\(entity, input\.Charges\[idx\], input\.Expands\)' openmeter/billing/charges/usagebased/adapter/charge.go rg -n -C3 'IDIn\(' openmeter/billing/charges/usagebased/adapter/charge.go rg -n -C3 '\.Order\(' openmeter/billing/charges/usagebased/adapter/charge.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/adapter/charge.go` around lines 122 - 153, The loop currently pairs DB rows to input meta by index (MapUsageBasedChargeFromDB(entity, input.Charges[idx], ...)), which is unsafe because query.All(ctx) results are not guaranteed to preserve order; instead build a map of input charges by ID (e.g., inputByID from input.Charges keyed by charge.ID) and for each DB entity lookup the corresponding input meta by entity.ID and call MapUsageBasedChargeFromDB(entity, inputMeta, input.Expands); then reconstruct output in the original input order by iterating input.Charges and pulling mapped results from entitiesByID (or a map of mapped results keyed by ID), include missing-ID details in errs (use the charge.ID) and return errors.Join(errs...) if any remain (remove any length-check that hides these errs).
🧹 Nitpick comments (2)
openmeter/billing/charges/service/get.go (1)
97-113: Skip usage-based fetch when there are no usage-based metas.This branch currently calls
s.usageBasedService.GetByIDseven whenchargesByType[meta.ChargeTypeUsageBased]is empty, which adds avoidable DB work in a hot retrieval path.♻️ Suggested guard
- // Let's fetch usage based charges - usageBasedCharges, err := s.usageBasedService.GetByIDs(ctx, usagebased.GetByIDsInput{ - Namespace: input.Namespace, - Charges: lo.Map(chargesByType[meta.ChargeTypeUsageBased], func(chargeMeta charges.WithIndex[meta.Charge], _ int) meta.Charge { - return chargeMeta.Value - }), - Expands: input.Expands, - }) - if err != nil { - return nil, err - } - - for i, usageBasedCharge := range usageBasedCharges { - targetIndex := chargesByType[meta.ChargeTypeUsageBased][i].Index - out[targetIndex] = charges.NewCharge(usageBasedCharge) - nrFetched++ - } + // Let's fetch usage based charges + usageBasedMetas := chargesByType[meta.ChargeTypeUsageBased] + if len(usageBasedMetas) > 0 { + usageBasedCharges, err := s.usageBasedService.GetByIDs(ctx, usagebased.GetByIDsInput{ + Namespace: input.Namespace, + Charges: lo.Map(usageBasedMetas, func(chargeMeta charges.WithIndex[meta.Charge], _ int) meta.Charge { + return chargeMeta.Value + }), + Expands: input.Expands, + }) + if err != nil { + return nil, err + } + + for i, usageBasedCharge := range usageBasedCharges { + targetIndex := usageBasedMetas[i].Index + out[targetIndex] = charges.NewCharge(usageBasedCharge) + nrFetched++ + } + }As per coding guidelines, "Performance should be a priority in critical code paths... database operations ... should be vetted for potential performance bottlenecks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/get.go` around lines 97 - 113, The code always calls s.usageBasedService.GetByIDs even when there are no usage-based metas; add a guard that checks if len(chargesByType[meta.ChargeTypeUsageBased]) == 0 and skip the GetByIDs call and subsequent loop when empty to avoid unnecessary DB work; keep the existing logic inside the block (constructing GetByIDsInput, handling err, iterating usageBasedCharges and assigning out[targetIndex]) but wrap it in an if len(...) > 0 branch so s.usageBasedService.GetByIDs, usageBasedCharges and the for loop only run when there are usage-based charge metas.openmeter/billing/charges/service/create.go (1)
99-99: Comment text is a bit stale now.This now creates gathering lines for flat-fee and usage-based charges, so the comment could be updated to avoid confusion later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/create.go` at line 99, The inline comment "// Let's generate the gathering lines for the flat fees" is stale; update it to accurately describe what the following code does by replacing it with a concise comment stating that it generates gathering lines for both flat-fee and usage-based charges (or for all charge types) so future readers of the code around the gathering line generation in create.go understand that it handles usage-based charges as well.
🤖 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/base_test.go`:
- Around line 183-190: BaseSuite.SetupSuite is creating the service via
service.New(Config{...}) but no UsageBasedService is provided, which now fails
config validation; update the test suite wiring in BaseSuite.SetupSuite to
include a mock or real UsageBasedService implementation in the Config passed to
service.New (refer to the Config type and the UsageBasedService interface) so
service.New succeeds during test setup and the new usage-based intent tests
don't fail during initialization.
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 263-346: The test TestUsageBasedPartialCreditRealizations
currently only creates a usage-based charge and verifies gathering lines (via
Charges.Create, billing.BillingService.ListGatheringInvoices, and
mustGetChargeByID) but does not exercise invoicing or credit realization logic;
either rename the test to reflect "create/get and gathering line linkage" (e.g.,
TestUsageBasedCreateAndGatheringLine) or extend it to advance the clock, run
invoicing (invoke the service that generates invoices for
servicePeriod.From..To), apply/realize credits, then assert invoice totals and
credit realizations against usageBasedChargeID and the generated invoices;
update assertions to check realized credits/totals on the produced invoices and
any related credit records.
---
Duplicate comments:
In `@openmeter/billing/charges/usagebased/adapter/charge.go`:
- Around line 122-153: The loop currently pairs DB rows to input meta by index
(MapUsageBasedChargeFromDB(entity, input.Charges[idx], ...)), which is unsafe
because query.All(ctx) results are not guaranteed to preserve order; instead
build a map of input charges by ID (e.g., inputByID from input.Charges keyed by
charge.ID) and for each DB entity lookup the corresponding input meta by
entity.ID and call MapUsageBasedChargeFromDB(entity, inputMeta, input.Expands);
then reconstruct output in the original input order by iterating input.Charges
and pulling mapped results from entitiesByID (or a map of mapped results keyed
by ID), include missing-ID details in errs (use the charge.ID) and return
errors.Join(errs...) if any remain (remove any length-check that hides these
errs).
---
Nitpick comments:
In `@openmeter/billing/charges/service/create.go`:
- Line 99: The inline comment "// Let's generate the gathering lines for the
flat fees" is stale; update it to accurately describe what the following code
does by replacing it with a concise comment stating that it generates gathering
lines for both flat-fee and usage-based charges (or for all charge types) so
future readers of the code around the gathering line generation in create.go
understand that it handles usage-based charges as well.
In `@openmeter/billing/charges/service/get.go`:
- Around line 97-113: The code always calls s.usageBasedService.GetByIDs even
when there are no usage-based metas; add a guard that checks if
len(chargesByType[meta.ChargeTypeUsageBased]) == 0 and skip the GetByIDs call
and subsequent loop when empty to avoid unnecessary DB work; keep the existing
logic inside the block (constructing GetByIDsInput, handling err, iterating
usageBasedCharges and assigning out[targetIndex]) but wrap it in an if len(...)
> 0 branch so s.usageBasedService.GetByIDs, usageBasedCharges and the for loop
only run when there are usage-based charge metas.
🪄 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: 9e76768f-6861-421d-b431-0f9f7f78ea63
⛔ Files ignored due to path filters (55)
openmeter/ent/db/charge.gois excluded by!**/ent/db/**openmeter/ent/db/charge/charge.gois excluded by!**/ent/db/**openmeter/ent/db/charge/where.gois excluded by!**/ent/db/**openmeter/ent/db/charge_create.gois excluded by!**/ent/db/**openmeter/ent/db/charge_query.gois excluded by!**/ent/db/**openmeter/ent/db/charge_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebased_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage/chargeusagebasedruninvoicedusage.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruninvoicedusage_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment/chargeusagebasedrunpayment.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedrunpayment_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_delete.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_update.gois excluded by!**/ent/db/**openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/cursor.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.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/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (24)
openmeter/billing/charges/charge.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/get.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/service.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/adapter.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/adapter/mapper.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/create.goopenmeter/billing/charges/usagebased/service/get.goopenmeter/billing/charges/usagebased/service/service.goopenmeter/ent/schema/charges.goopenmeter/ent/schema/chargesusagebased.gotest/app/custominvoicing/invocing_test.gotest/billing/subscription_test.gotest/billing/suite.gotools/migrate/migrations/20260317164048_charge_usage_based.down.sqltools/migrate/migrations/20260317164048_charge_usage_based.up.sql
🚧 Files skipped from review as they are similar to previous changes (5)
- openmeter/billing/charges/usagebased/handler.go
- openmeter/billing/charges/usagebased/service/service.go
- openmeter/billing/charges/usagebased/service/get.go
- openmeter/billing/charges/usagebased/adapter/adapter.go
- openmeter/billing/charges/usagebased/adapter.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/billing/charges/service/base_test.go (1)
46-46:⚠️ Potential issue | 🟡 MinorPlease reset
UsageBasedTestHandlerin teardown.You initialize
UsageBasedTestHandlerin setup, butTeardownTestdoesn’t reset it. That can leak state across tests.Suggested fix
func (s *BaseSuite) TeardownTest() { s.FlatFeeTestHandler.Reset() s.CreditPurchaseTestHandler.Reset() + s.UsageBasedTestHandler.Reset() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/base_test.go` at line 46, TeardownTest currently doesn’t clear the test state set in setup: reset the UsageBasedTestHandler field in TeardownTest to avoid leaking state between tests by assigning nil (or a fresh zero value) to s.UsageBasedTestHandler; locate where setup calls newUsageBasedTestHandler() and add the reset in the TeardownTest method to mirror initialization and ensure tests are isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openmeter/billing/charges/service/base_test.go`:
- Line 46: TeardownTest currently doesn’t clear the test state set in setup:
reset the UsageBasedTestHandler field in TeardownTest to avoid leaking state
between tests by assigning nil (or a fresh zero value) to
s.UsageBasedTestHandler; locate where setup calls newUsageBasedTestHandler() and
add the reset in the TeardownTest method to mirror initialization and ensure
tests are isolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5490fe3-08d9-450f-9037-58177f0c6812
📒 Files selected for processing (2)
openmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/invoicable_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/charges/service/invoicable_test.go
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
Create database tables and basic infrastructure for usage based charges. The testcase only validates that we are passing a simple create /get cycle.
Notes for reviewer
Summary by CodeRabbit