feat(api): add charges list api http handler#4103
Merged
Conversation
mark-vass-konghq
added
the
release-note/feature
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
b4a50fa to
3ed7395
Compare
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 a new charges handler and ListCustomerCharges endpoint, conversion utilities mapping billing charge domain models into v3 API union types, wires a ChargeService through router/server/CLI, and adds tests including a NoopChargeService and route assertions. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler
participant ChargeService
participant Converter
Client->>Handler: GET /api/v3/.../customers/{id}/charges
Handler->>Handler: parse & validate query params
Handler->>Handler: resolve namespace
Handler->>Handler: parse status filter & compute complement
Handler->>ChargeService: ListCharges(ctx, filters, pagination)
ChargeService-->>Handler: charges + total count
loop per charge
Handler->>Converter: convertChargeToAPI(charge)
Converter-->>Handler: api.BillingCharge
end
Handler-->>Client: 200 JSON PagePaginationResponse[api.BillingCharge]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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 |
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
3ed7395 to
7cc600d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openmeter/server/server_test.go (1)
603-613: Please cover at least one non-empty charges page.Between the
501case and the empty-pageNoopChargeService, these tests only prove the routing. None of the new flat-fee / usage-based conversion logic gets exercised, so response-shape bugs can slip through pretty easily.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: 790-809, 838-843
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/server/server_test.go` around lines 603 - 613, Add at least one test case that returns a non-empty charges page to exercise the conversion logic (flat-fee and usage-based) rather than only routing; create a testRequest to GET "/api/v3/openmeter/customers/01ARZ3NDEKTSV4RRFFQ69G5FAV/charges" but configure the test server to use a real/mock ChargeService (not NoopChargeService) that returns a page with at least one Charge containing both flat-fee and usage-based entries, then assert the testResponse body and status (instead of only expecting http.StatusNotImplemented) to validate the response shape and conversion behavior handled by the charges handler (the code that reads testRequest/testResponse and invokes the charges endpoint).api/v3/handlers/charges/list.go (1)
138-160: Consider deriving statuses fromValues()to stay in sync with the enum.The hardcoded list mirrors
meta.ChargeStatusvalues, but if a new status is added to the enum later, this list could get out of sync silently. When that happens, the complement calculation would be incorrect - users could see charges with unexpected statuses.You could derive this dynamically from the
Values()method:♻️ Suggested refactor to use Values()
// chargeStatusComplement returns all valid statuses not in the given set. func chargeStatusComplement(include []meta.ChargeStatus) []meta.ChargeStatus { - all := []meta.ChargeStatus{ - meta.ChargeStatusCreated, - meta.ChargeStatusActive, - meta.ChargeStatusFinal, - meta.ChargeStatusDeleted, - } + // Derive all valid statuses from the enum's Values() method + // to stay in sync if new statuses are added. + allStrings := meta.ChargeStatus("").Values() + all := make([]meta.ChargeStatus, len(allStrings)) + for i, s := range allStrings { + all[i] = meta.ChargeStatus(s) + } includeSet := make(map[meta.ChargeStatus]bool, len(include)) for _, s := range include { includeSet[s] = true } var complement []meta.ChargeStatus for _, s := range all { if !includeSet[s] { complement = append(complement, s) } } return complement }Alternatively, at minimum a comment noting the coupling would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/charges/list.go` around lines 138 - 160, The hardcoded list in chargeStatusComplement is coupled to the enum; replace the manual all := [...] slice with a dynamic slice derived from the enum's Values() helper (e.g. call meta.ChargeStatus.Values() or the package's equivalent that returns []meta.ChargeStatus) so new statuses are automatically included, then keep the existing includeSet/complement logic unchanged; if no Values() exists, add a clear comment explaining the coupling and a TODO to switch to the enum Values() when available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/charges/convert.go`:
- Around line 39-49: The handler currently lets meta.ChargeTypeCreditPurchase
through so convertChargeToAPI crashes; either fix the handler by populating the
ListChargesInput.ChargeTypes to exclude meta.ChargeTypeCreditPurchase when
building the query (so the adapter never returns credit-purchase rows), or make
convertChargeToAPI skip credit purchases instead of erroring by detecting
meta.ChargeTypeCreditPurchase (the switch case using
charge.AsCreditPurchaseCharge()) and returning a skip signal (e.g., nil/zero
value or a boolean) that the caller will ignore; update the caller code to
handle that skip signal if you choose the converter-change route.
---
Nitpick comments:
In `@api/v3/handlers/charges/list.go`:
- Around line 138-160: The hardcoded list in chargeStatusComplement is coupled
to the enum; replace the manual all := [...] slice with a dynamic slice derived
from the enum's Values() helper (e.g. call meta.ChargeStatus.Values() or the
package's equivalent that returns []meta.ChargeStatus) so new statuses are
automatically included, then keep the existing includeSet/complement logic
unchanged; if no Values() exists, add a clear comment explaining the coupling
and a TODO to switch to the enum Values() when available.
In `@openmeter/server/server_test.go`:
- Around line 603-613: Add at least one test case that returns a non-empty
charges page to exercise the conversion logic (flat-fee and usage-based) rather
than only routing; create a testRequest to GET
"/api/v3/openmeter/customers/01ARZ3NDEKTSV4RRFFQ69G5FAV/charges" but configure
the test server to use a real/mock ChargeService (not NoopChargeService) that
returns a page with at least one Charge containing both flat-fee and usage-based
entries, then assert the testResponse body and status (instead of only expecting
http.StatusNotImplemented) to validate the response shape and conversion
behavior handled by the charges handler (the code that reads
testRequest/testResponse and invokes the charges endpoint).
🪄 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: e596a720-200c-4d29-a227-79924d220e0e
📒 Files selected for processing (9)
api/v3/handlers/charges/convert.goapi/v3/handlers/charges/handler.goapi/v3/handlers/charges/list.goapi/v3/server/routes.goapi/v3/server/server.gocmd/server/main.goopenmeter/server/router/router.goopenmeter/server/server.goopenmeter/server/server_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
api/v3/handlers/charges/convert.go (2)
220-226: Consider returning nil when metadata is empty.The function always returns a non-nil
*api.Labels, even whensourceis empty. If the API expectslabelsto be omitted when there are none (based onomitemptyin the JSON tag), this could result in an empty object{}in the response instead of the field being absent.✨ Optional fix
func convertMetadataToLabels(source models.Metadata) *api.Labels { + if len(source) == 0 { + return nil + } labels := make(api.Labels) for k, v := range source { labels[k] = v } return &labels }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/charges/convert.go` around lines 220 - 226, The convertMetadataToLabels function always returns a non-nil *api.Labels even for empty input; change it to return nil when the incoming models.Metadata is nil or has zero entries so the JSON omitempty can omit the field. Locate convertMetadataToLabels and add a guard that checks len(source) == 0 (or source == nil) and returns nil; otherwise build and return the populated *api.Labels as before. Ensure callers that receive the result can handle a nil *api.Labels appropriately.
162-174: TODO: Implement proper totals aggregation.Just flagging this for visibility—the
convertUsageBasedChargeTotalsfunction returns zero values for all totals. The comment indicates this is intentional for now, but make sure to track this as a follow-up.Would you like me to open an issue to track this implementation?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/charges/convert.go` around lines 162 - 174, The convertUsageBasedChargeTotals function currently returns hard-coded zero totals; replace the TODO by aggregating actual totals from the usagebased.Charge (e.g., iterate ch.Realizations or equivalent field on usagebased.Charge), summing numeric fields into an api.BillingTotals value and returning that inside api.BillingChargeTotals.Booked. Ensure you parse/accumulate amounts using a precise decimal type (not float64) and format the final summed values as strings to match api.BillingTotals (Amount, ChargesTotal, CreditsTotal, DiscountsTotal, TaxesExclusiveTotal); update any helper functions or add a small aggregator inside convertUsageBasedChargeTotals to perform the sums safely.api/v3/handlers/charges/list.go (1)
138-160: Consider moving the status list to a canonical method in the meta package.The status list in
chargeStatusComplementmirrors the constants defined inmeta.ChargeStatus(inmeta/charge.go). While theValues()method returns string representations, it'd be cleaner to add a dedicatedStatuses()orAll()method to themetapackage that returns[]meta.ChargeStatusdirectly. Then the handler can call that instead of maintaining a separate list.This prevents accidental drift if someone adds a new status to the constants but forgets to update the handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/charges/list.go` around lines 138 - 160, The handler's chargeStatusComplement currently hardcodes the list of statuses; add a canonical accessor in the meta package (e.g., a function like meta.AllChargeStatuses() or meta.ChargeStatusValues() that returns []meta.ChargeStatus) and update chargeStatusComplement to call that instead of the local all slice; modify or add the new function in the meta package near the ChargeStatus definitions and replace the inline list in chargeStatusComplement with a call to the new meta method so the source of truth for statuses lives in meta.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v3/handlers/charges/convert.go`:
- Around line 220-226: The convertMetadataToLabels function always returns a
non-nil *api.Labels even for empty input; change it to return nil when the
incoming models.Metadata is nil or has zero entries so the JSON omitempty can
omit the field. Locate convertMetadataToLabels and add a guard that checks
len(source) == 0 (or source == nil) and returns nil; otherwise build and return
the populated *api.Labels as before. Ensure callers that receive the result can
handle a nil *api.Labels appropriately.
- Around line 162-174: The convertUsageBasedChargeTotals function currently
returns hard-coded zero totals; replace the TODO by aggregating actual totals
from the usagebased.Charge (e.g., iterate ch.Realizations or equivalent field on
usagebased.Charge), summing numeric fields into an api.BillingTotals value and
returning that inside api.BillingChargeTotals.Booked. Ensure you
parse/accumulate amounts using a precise decimal type (not float64) and format
the final summed values as strings to match api.BillingTotals (Amount,
ChargesTotal, CreditsTotal, DiscountsTotal, TaxesExclusiveTotal); update any
helper functions or add a small aggregator inside convertUsageBasedChargeTotals
to perform the sums safely.
In `@api/v3/handlers/charges/list.go`:
- Around line 138-160: The handler's chargeStatusComplement currently hardcodes
the list of statuses; add a canonical accessor in the meta package (e.g., a
function like meta.AllChargeStatuses() or meta.ChargeStatusValues() that returns
[]meta.ChargeStatus) and update chargeStatusComplement to call that instead of
the local all slice; modify or add the new function in the meta package near the
ChargeStatus definitions and replace the inline list in chargeStatusComplement
with a call to the new meta method so the source of truth for statuses lives in
meta.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87aed2e1-1c3a-4273-a5bc-d3c63c1704d2
📒 Files selected for processing (9)
api/v3/handlers/charges/convert.goapi/v3/handlers/charges/handler.goapi/v3/handlers/charges/list.goapi/v3/server/routes.goapi/v3/server/server.gocmd/server/main.goopenmeter/server/router/router.goopenmeter/server/server.goopenmeter/server/server_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- openmeter/server/server.go
- openmeter/server/router/router.go
- cmd/server/main.go
- api/v3/server/routes.go
- api/v3/handlers/charges/handler.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/charges/list.go`:
- Around line 124-138: parseChargeStatusFilter currently treats an empty oeq
string as a single empty part which triggers validation errors; add an early
guard in parseChargeStatusFilter to treat an empty or all-whitespace oeq as no
filters and return an empty []meta.ChargeStatus and nil error. Check
strings.TrimSpace(oeq) at the top of the function and return the empty slice
immediately before calling strings.Split or creating meta.ChargeStatus values so
validation isn't invoked on an empty string.
🪄 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: 9ced6ed0-3532-40a5-88a2-2f0688db442d
📒 Files selected for processing (2)
api/v3/handlers/charges/convert.goapi/v3/handlers/charges/list.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/v3/handlers/charges/list.go (1)
84-89: Consider deduplicatingexpandvalues before appending.If clients send repeated
expandvalues,req.Expandscan contain duplicates. Tiny thing, but this endpoint is on a DB-backed list path, so trimming redundant flags helps keep downstream work predictable.♻️ Suggested tweak
// Parse expand if args.Params.Expand != nil { for _, exp := range *args.Params.Expand { if exp == api.BillingChargesExpandRealTimeUsage { - req.Expands = append(req.Expands, meta.ExpandRealizations) + req.Expands = append(req.Expands, meta.ExpandRealizations) + break // prevent duplicate realizations expand } } }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 `@api/v3/handlers/charges/list.go` around lines 84 - 89, Deduplicate expand values before appending to req.Expands: when handling args.Params.Expand in the handler (the loop that checks for api.BillingChargesExpandRealTimeUsage), track already-added expands (e.g., a local set keyed by meta.ExpandRealizations or existing entries in req.Expands) and only append meta.ExpandRealizations if it isn't already present; update the logic around args.Params.Expand, api.BillingChargesExpandRealTimeUsage, req.Expands and meta.ExpandRealizations to perform this existence check to avoid duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v3/handlers/charges/list.go`:
- Around line 84-89: Deduplicate expand values before appending to req.Expands:
when handling args.Params.Expand in the handler (the loop that checks for
api.BillingChargesExpandRealTimeUsage), track already-added expands (e.g., a
local set keyed by meta.ExpandRealizations or existing entries in req.Expands)
and only append meta.ExpandRealizations if it isn't already present; update the
logic around args.Params.Expand, api.BillingChargesExpandRealTimeUsage,
req.Expands and meta.ExpandRealizations to perform this existence check to avoid
duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67b92011-7dd3-42cf-b7db-1ffec6ac629d
📒 Files selected for processing (1)
api/v3/handlers/charges/list.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/v3/handlers/charges/convert.go (1)
141-172: Placeholder values for Price and Totals.The
Priceis hardcoded to"0"andconvertUsageBasedChargeTotalsreturns zeroed totals. Both have TODO comments acknowledging they need proper implementation. If this is intentional for an MVP/incremental release, that's fine - just wanted to call it out in case you'd like help mapping theproductcatalog.Pricetype or aggregating realization runs.Would you like me to help draft the implementation for either of these TODOs, or open tracking issues for them?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/charges/convert.go` around lines 141 - 172, Replace the placeholder apiUB.Price = api.CurrencyAmount{Amount: "0"} with a proper mapping from the productcatalog.Price value on the usage-based intent (e.g., implement and call a helper like mapProductPriceToCurrencyAmount(productcatalog.Price) to populate api.CurrencyAmount fields). Implement convertUsageBasedChargeTotals(usagebased.Charge) to aggregate booked and realtime totals by iterating the realization runs on the provided usagebased.Charge (sum amounts into Booked and Realtime api.BillingTotals fields: Amount, ChargesTotal, CreditsTotal, DiscountsTotal, TaxesExclusiveTotal), and return those totals in api.BillingChargeTotals; use clear helper names (e.g., sumRealizationsToBillingTotals) and reference the usagebased.Charge’s realizations/intent fields when computing the sums. Ensure the new helpers are used where apiUB.Price and convertUsageBasedChargeTotals are currently invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v3/handlers/charges/convert.go`:
- Around line 141-172: Replace the placeholder apiUB.Price =
api.CurrencyAmount{Amount: "0"} with a proper mapping from the
productcatalog.Price value on the usage-based intent (e.g., implement and call a
helper like mapProductPriceToCurrencyAmount(productcatalog.Price) to populate
api.CurrencyAmount fields). Implement
convertUsageBasedChargeTotals(usagebased.Charge) to aggregate booked and
realtime totals by iterating the realization runs on the provided
usagebased.Charge (sum amounts into Booked and Realtime api.BillingTotals
fields: Amount, ChargesTotal, CreditsTotal, DiscountsTotal,
TaxesExclusiveTotal), and return those totals in api.BillingChargeTotals; use
clear helper names (e.g., sumRealizationsToBillingTotals) and reference the
usagebased.Charge’s realizations/intent fields when computing the sums. Ensure
the new helpers are used where apiUB.Price and convertUsageBasedChargeTotals are
currently invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af2095e1-c589-4e63-909b-13bff9db29cd
📒 Files selected for processing (3)
api/v3/handlers/charges/convert.gen.goapi/v3/handlers/charges/convert.goapi/v3/handlers/charges/list.go
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
d05e387 to
c1399de
Compare
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
c1399de to
af1aebf
Compare
chrisgacsal
reviewed
Apr 9, 2026
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
ea2dad0 to
e287e32
Compare
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
e287e32 to
c3a6151
Compare
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
ed06e24 to
61db4f2
Compare
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
8ca98c9 to
9e9f0f3
Compare
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
9e9f0f3 to
2304a43
Compare
turip
reviewed
Apr 16, 2026
turip
reviewed
Apr 16, 2026
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
2304a43 to
b68e1db
Compare
turip
previously approved these changes
Apr 16, 2026
mark-vass-konghq
force-pushed
the
feat/add-charges-list-api-http-handler
branch
from
b68e1db to
c765084
Compare
turip
approved these changes
Apr 16, 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
3 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.
@coderabbitai
Summary by CodeRabbit