feat(api): improve API types#4292
Closed
tothandras wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR makes created/updated timestamps required across API spec models and aligns Go handler timestamp field representations from pointers to values. Additionally, credit transaction and grant filtering shifts from single optional values to slice-based collections to support multiple-value filtering. ChangesTimestamp Representation & Filter Refactoring
Sequence DiagramsequenceDiagram
participant Client
participant Handler as List Handler
participant Converter as Filter Converter
participant Service as Credit Service
participant Ledger as Ledger/Loaders
Client->>Handler: GET /credits/grants?filter[status]=active&filter[currency]=USD
activate Handler
Handler->>Converter: filterStringExactToStatuses(filter[status])
activate Converter
Converter-->>Handler: []meta.ChargeStatus{Active}
deactivate Converter
Handler->>Converter: filterStringExactToCurrencies(filter[currency])
activate Converter
Converter-->>Handler: []currencyx.Code{USD}
deactivate Converter
Handler->>Service: List(ctx, ListInput{Statuses: [Active], Currencies: [USD]})
activate Service
Service->>Ledger: creditTransactionLoaders([...types], ListInput{Statuses: [...], Currencies: [...]})
activate Ledger
Ledger-->>Service: []Loader (filtered by type & currency)
deactivate Ledger
Service-->>Handler: []CreditGrant (merged results)
deactivate Service
Handler-->>Client: 200 OK {data: [...grants with required created_at]}
deactivate Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes follow consistent patterns across many files (repetitive timestamp conversions reduce complexity), but the credit filtering refactoring is more intricate—it touches handlers, services, and loaders with slice-based semantics that require understanding the interaction across layers. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 3
🧹 Nitpick comments (7)
api/v3/handlers/llmcost/convert.go (1)
68-88: 💤 Low valueTiny nit:
CreatedAt/UpdatedAtcould move into the struct literalThey're the only fields not initialized inline — looks like a leftover from removing the old
if !p.CreatedAt.IsZero()guard. No bug here, just a quick cleanup opportunity.✨ Proposed cleanup
out := api.LLMCostPrice{ Id: p.ID, Provider: api.LLMCostProvider{ Id: providerID, Name: formatProviderName(providerID), }, Model: api.LLMCostModel{ Id: p.ModelID, Name: p.ModelName, }, Currency: p.Currency, Source: source, EffectiveFrom: p.EffectiveFrom, EffectiveTo: p.EffectiveTo, Pricing: domainPricingToAPI(p.Pricing), + CreatedAt: p.CreatedAt, + UpdatedAt: p.UpdatedAt, } - out.CreatedAt = p.CreatedAt - out.UpdatedAt = p.UpdatedAt - return out🤖 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 `@api/v3/handlers/llmcost/convert.go` around lines 68 - 88, Move the CreatedAt and UpdatedAt assignments into the api.LLMCostPrice struct literal so all fields are initialized inline; update the struct literal where out is constructed (referencing api.LLMCostPrice, api.LLMCostProvider, api.LLMCostModel, domainPricingToAPI and variable p) to include CreatedAt: p.CreatedAt and UpdatedAt: p.UpdatedAt, then remove the separate out.CreatedAt/out.UpdatedAt lines before the return.api/spec/packages/aip/src/customers/credits/operations.tsp (2)
164-170: ⚡ Quick winSame enum-erosion concern here, plus the
TODOon date filters.Two small things:
- Like
ListCreditGrantsParamsFilter, thetypeandcurrencyfields no longer surface allowed values in the spec — consider adding doc comments listing the valid options (funded,consumedfor type).- The
// TODO: add date filters for created_at and booked_atis great context — happy to open a tracking issue or sketch out the filter shape (likelyCommon.DateFieldFilteror similar) if that'd help land it in a follow-up.As per coding guidelines: "The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user."
🤖 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 `@api/spec/packages/aip/src/customers/credits/operations.tsp` around lines 164 - 170, The spec for Credits filter is losing allowed enum info and is missing date filters: update the comments for the type and currency fields in the Credits filter (same pattern used by ListCreditGrantsParamsFilter) to enumerate valid values (e.g., type: funded, consumed; currency: list allowed currency codes) so consumers see allowed options, and add filters for created_at and booked_at using the existing Common.DateFieldFilter (or the project’s DateFieldFilter shape) to match implementation; reference the type and currency fields and add created_at/booked_at entries using Common.DateFieldFilter to keep the API accurate and discoverable.
23-26: ⚡ Quick winHeads up: enum context is lost in the OpenAPI spec for
statusandcurrency.Switching
statusfromCreditGrantStatusandcurrencyfromShared.CurrencyCodeto a genericCommon.StringFieldFilterExactenables multi-value filtering (nice!), but the SDK/OpenAPI no longer surfaces the allowed enum values for these filter fields. API consumers will only see "string" inputs, and discover invalid values at runtime via theconvert.govalidation infilterStringExactToStatuses/filterStringExactToCurrencies.Consider adding a doc comment on each filter field that enumerates the accepted values (and same for
type/currencyinListCreditTransactionsParamsFilterat Lines 164-167), so the spec remains self-describing:📝 Suggested doc additions
model ListCreditGrantsParamsFilter { + /** + * Filter by credit grant status. Accepted values: `pending`, `active`, `voided`, `expired`. + */ `#suppress` "@openmeter/api-spec-aip/doc-decorator" "shared model" status?: Common.StringFieldFilterExact; + + /** + * Filter by currency code (ISO-4217 or custom currency code). + */ `#suppress` "@openmeter/api-spec-aip/doc-decorator" "shared model" currency?: Common.StringFieldFilterExact; }As per coding guidelines: "The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user."
🤖 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 `@api/spec/packages/aip/src/customers/credits/operations.tsp` around lines 23 - 26, The status and currency filter fields use Common.StringFieldFilterExact which loses enum context in the OpenAPI output; add doc comments above the status and currency fields (and similarly above type/currency in ListCreditTransactionsParamsFilter) listing the accepted values from CreditGrantStatus and Shared.CurrencyCode respectively so the generated spec surfaces allowed enum values; reference the conversion helpers filterStringExactToStatuses and filterStringExactToCurrencies in the comment if helpful for consumers.openmeter/ledger/customerbalance/transactions_test.go (1)
22-28: ⚡ Quick winTest still works — but coverage could grow with the new shape.
The test now exercises the slice-input path for the invalid case 👍. Since
creditTransactionLoadershas new branches (empty slice → all loaders; multiple values → ordered loaders; potential duplicates), it'd be lovely to add coverage:
- Empty
[]CreditTransactionTypereturns loaders for all known types increditTransactionLoaderOrder.- A slice with
[Funded, Consumed]returns 2 loaders in input order.- Duplicate types behavior (if you decide to dedupe per the loaders.go suggestion, this locks it in).
As per coding guidelines for
**/*_test.go: "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/ledger/customerbalance/transactions_test.go` around lines 22 - 28, Add tests to exercise the new branches of creditTransactionLoaders: (1) call creditTransactionLoaders with an empty []CreditTransactionType and assert it returns loaders for all known types in creditTransactionLoaderOrder (check length and that order matches creditTransactionLoaderOrder), (2) call it with []CreditTransactionType{Funded, Consumed} and assert it returns exactly two loaders in that input order, and (3) add a test for duplicate types (e.g. []CreditTransactionType{Funded, Funded}) asserting the expected behavior you choose (either deduped single loader or duplicated loaders) and adjust assertions to match that decision; use the service{} setup already in TestCreditTransactionLoaders_InvalidType and the existing require helpers for assertions.api/v3/handlers/customers/credits/convert.go (2)
495-495: 💤 Low valueUnused
context.Contextparameter — keep it intentional or drop it.All three new helpers take
_ context.Contextand never use it. If you anticipate needing a context soon (e.g., to validate currency codes against tenant-defined custom currencies), great — leave a brief comment explaining why it stays. Otherwise, dropping the parameter is cleaner per the repo's context-propagation guideline.As per coding guidelines for
openmeter/**/*.go: "Propagate caller's context through the full call path or remove unused context parameters." (the same spirit applies inapi/**/*.go).🤖 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 `@api/v3/handlers/customers/credits/convert.go` at line 495, The helper filterStringExactToTransactionTypes currently declares an unused context.Context parameter; either remove the unused parameter from this function (and the two sibling helpers that also take _ context.Context) to follow the repo's context-propagation guideline, or keep the parameter but add a one-line comment above each function explaining why the context is intentionally accepted (e.g., "context retained for future tenant currency validation") so reviewers know it's deliberate; update the function signatures for filterStringExactToTransactionTypes and the two related helpers accordingly.
493-589: ⚡ Quick winPlenty of overlap between the three filter helpers — a small generic could shrink this.
The three converters are nearly identical: pull
Eq/Ininto a[]string, rejectNe, then map each value via a per-type function. A tiny generic helper would remove ~60 lines of duplication while keeping the same per-field error messages.♻️ Sketch of a shared helper
func mapFilterStringExact[T any]( f *filters.FilterStringExact, field string, mapValue func(string) (T, error), ) ([]T, error) { fs, err := filters.FromAPIFilterStringExact(f) if err != nil { return nil, err } if fs == nil { return nil, nil } if fs.Ne != nil { return nil, models.NewGenericValidationError( fmt.Errorf("only eq and oeq operators are supported for %s", field), ) } var values []string if fs.Eq != nil { values = append(values, *fs.Eq) } if fs.In != nil { values = append(values, *fs.In...) } result := make([]T, 0, len(values)) for _, v := range values { mapped, err := mapValue(v) if err != nil { return nil, err } result = append(result, mapped) } return result, nil }Then each helper becomes a thin wrapper that just supplies the per-value mapping function.
🤖 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 `@api/v3/handlers/customers/credits/convert.go` around lines 493 - 589, Extract the common logic in filterStringExactToTransactionTypes, filterStringExactToStatuses, and filterStringExactToCurrencies into a generic helper (e.g. mapFilterStringExact[T]) that calls filters.FromAPIFilterStringExact, returns nil if fs==nil, rejects fs.Ne with the same per-field error text ("only eq and oeq operators are supported for <field>") and builds the []string from fs.Eq/fs.In, then maps each string via a provided mapValue func(string) (T, error); replace the three functions with thin wrappers that call mapFilterStringExact supplying the field name and a mapping function (for transaction types map to customerbalance.CreditTransactionType, for statuses call the provided convert func, for currencies map to currencyx.Code) so behavior and error messages remain identical.openmeter/ledger/customerbalance/transactions.go (1)
125-136: ⚡ Quick winConsider de-duplicating filter slices before loader/query fan-out.
A quick
lo.Uniqoninput.Types(and optionallyinput.Currencies) would avoid redundant loader calls/query work when clients pass duplicate filter values.💡 Suggested tweak
- loaders, err := s.creditTransactionLoaders(input.Types) + uniqueTypes := lo.Uniq(input.Types) + loaders, err := s.creditTransactionLoaders(uniqueTypes) if err != nil { return ListCreditTransactionsResult{}, err } loaderInput := creditTransactionLoaderInput{ Limit: input.Limit, After: input.After, Before: input.Before, CustomerID: input.CustomerID, AccountID: accountID, - Currencies: input.Currencies, + Currencies: lo.Uniq(input.Currencies), }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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/customerbalance/transactions.go` around lines 125 - 136, De-duplicate filter slices before fan-out: call a uniqueness operation on input.Types (and optionally input.Currencies) before passing them to creditTransactionLoaders and before populating creditTransactionLoaderInput so you don't create duplicate loaders or redundant queries; update the call site that constructs loaderInput (creditTransactionLoaderInput) and the call to s.creditTransactionLoaders to use the deduplicated slices (e.g., uniqueTypes and uniqueCurrencies) so loader creation and downstream queries only run once per distinct filter value.
🤖 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 `@api/v3/handlers/customers/credits/convert.go`:
- Around line 493-589: Replace plain errors and fmt.Errorf in
filterStringExactToTransactionTypes, filterStringExactToStatuses, and
filterStringExactToCurrencies with validation-typed errors so clients get 4xx
responses: for the unsupported operator checks in each function (currently
errors.New("only eq and oeq operators are supported for ...")) return a
models.NewGenericValidationError(...) (or an existing specific factory if one
exists) with the same message; in filterStringExactToTransactionTypes convert
the unsupported-type fmt.Errorf into a validation error (either call
models.NewGenericValidationError(fmt.Sprintf(...)) or add a specific factory
like newCreditTransactionTypeInvalid(value)); and ensure any errors returned
from the provided convert(...) in filterStringExactToStatuses are preserved or
mapped to a validation error type rather than a plain error. Use the function
names filterStringExactToTransactionTypes, filterStringExactToStatuses, and
filterStringExactToCurrencies to locate and update these error returns.
In `@openmeter/ledger/customerbalance/funded_loader.go`:
- Around line 21-33: The loader currently drops multi-value currency filters by
setting currency = nil when input.Currencies has length != 1; update
fundedCreditTransactionLoader.Load (and the analogous ledger loader method in
ledger_loader.go) to explicitly handle len(input.Currencies) > 1 by returning a
validation error instead of silently ignoring the filter: check
creditTransactionLoaderInput.Currencies (and ledger loader's input.Currencies)
and if len > 1 return a clear error (e.g., "multiple currencies not supported")
to the caller; keep the existing behavior of using &input.Currencies[0] when len
== 1 and nil when len == 0.
In `@openmeter/ledger/customerbalance/loaders.go`:
- Around line 53-63: The code currently returns (nil, txType.Validate()) when a
factory is missing, but txType.Validate() can be nil and mask the error; change
the missing-factory branch in the loader construction to return an explicit
non-nil error (e.g. fmt.Errorf("no creditTransactionLoader factory registered
for %v", txType)) instead of relying on txType.Validate(), and also deduplicate
input txTypes before creating loaders (use a seen map keyed by the
CreditTransactionType and only append one creditTransactionLoader per unique
type when building loaders) so the same loader is not created/run multiple
times.
---
Nitpick comments:
In `@api/spec/packages/aip/src/customers/credits/operations.tsp`:
- Around line 164-170: The spec for Credits filter is losing allowed enum info
and is missing date filters: update the comments for the type and currency
fields in the Credits filter (same pattern used by ListCreditGrantsParamsFilter)
to enumerate valid values (e.g., type: funded, consumed; currency: list allowed
currency codes) so consumers see allowed options, and add filters for created_at
and booked_at using the existing Common.DateFieldFilter (or the project’s
DateFieldFilter shape) to match implementation; reference the type and currency
fields and add created_at/booked_at entries using Common.DateFieldFilter to keep
the API accurate and discoverable.
- Around line 23-26: The status and currency filter fields use
Common.StringFieldFilterExact which loses enum context in the OpenAPI output;
add doc comments above the status and currency fields (and similarly above
type/currency in ListCreditTransactionsParamsFilter) listing the accepted values
from CreditGrantStatus and Shared.CurrencyCode respectively so the generated
spec surfaces allowed enum values; reference the conversion helpers
filterStringExactToStatuses and filterStringExactToCurrencies in the comment if
helpful for consumers.
In `@api/v3/handlers/customers/credits/convert.go`:
- Line 495: The helper filterStringExactToTransactionTypes currently declares an
unused context.Context parameter; either remove the unused parameter from this
function (and the two sibling helpers that also take _ context.Context) to
follow the repo's context-propagation guideline, or keep the parameter but add a
one-line comment above each function explaining why the context is intentionally
accepted (e.g., "context retained for future tenant currency validation") so
reviewers know it's deliberate; update the function signatures for
filterStringExactToTransactionTypes and the two related helpers accordingly.
- Around line 493-589: Extract the common logic in
filterStringExactToTransactionTypes, filterStringExactToStatuses, and
filterStringExactToCurrencies into a generic helper (e.g.
mapFilterStringExact[T]) that calls filters.FromAPIFilterStringExact, returns
nil if fs==nil, rejects fs.Ne with the same per-field error text ("only eq and
oeq operators are supported for <field>") and builds the []string from
fs.Eq/fs.In, then maps each string via a provided mapValue func(string) (T,
error); replace the three functions with thin wrappers that call
mapFilterStringExact supplying the field name and a mapping function (for
transaction types map to customerbalance.CreditTransactionType, for statuses
call the provided convert func, for currencies map to currencyx.Code) so
behavior and error messages remain identical.
In `@api/v3/handlers/llmcost/convert.go`:
- Around line 68-88: Move the CreatedAt and UpdatedAt assignments into the
api.LLMCostPrice struct literal so all fields are initialized inline; update the
struct literal where out is constructed (referencing api.LLMCostPrice,
api.LLMCostProvider, api.LLMCostModel, domainPricingToAPI and variable p) to
include CreatedAt: p.CreatedAt and UpdatedAt: p.UpdatedAt, then remove the
separate out.CreatedAt/out.UpdatedAt lines before the return.
In `@openmeter/ledger/customerbalance/transactions_test.go`:
- Around line 22-28: Add tests to exercise the new branches of
creditTransactionLoaders: (1) call creditTransactionLoaders with an empty
[]CreditTransactionType and assert it returns loaders for all known types in
creditTransactionLoaderOrder (check length and that order matches
creditTransactionLoaderOrder), (2) call it with []CreditTransactionType{Funded,
Consumed} and assert it returns exactly two loaders in that input order, and (3)
add a test for duplicate types (e.g. []CreditTransactionType{Funded, Funded})
asserting the expected behavior you choose (either deduped single loader or
duplicated loaders) and adjust assertions to match that decision; use the
service{} setup already in TestCreditTransactionLoaders_InvalidType and the
existing require helpers for assertions.
In `@openmeter/ledger/customerbalance/transactions.go`:
- Around line 125-136: De-duplicate filter slices before fan-out: call a
uniqueness operation on input.Types (and optionally input.Currencies) before
passing them to creditTransactionLoaders and before populating
creditTransactionLoaderInput so you don't create duplicate loaders or redundant
queries; update the call site that constructs loaderInput
(creditTransactionLoaderInput) and the call to s.creditTransactionLoaders to use
the deduplicated slices (e.g., uniqueTypes and uniqueCurrencies) so loader
creation and downstream queries only run once per distinct filter value.
🪄 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: c68ba0e1-2b22-47d2-9269-2c02c91ed249
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (32)
api/spec/packages/aip/src/currencies/cost-bases/cost-basis.tspapi/spec/packages/aip/src/currencies/currency.tspapi/spec/packages/aip/src/customers/charges/operations.tspapi/spec/packages/aip/src/customers/credits/operations.tspapi/spec/packages/aip/src/llmcost/prices.tspapi/spec/packages/aip/src/shared/resource.tspapi/v3/api.gen.goapi/v3/handlers/addons/convert.goapi/v3/handlers/apps/convert.goapi/v3/handlers/billingprofiles/convert.gen.goapi/v3/handlers/currencies/convert.goapi/v3/handlers/customers/charges/convert.goapi/v3/handlers/customers/convert.gen.goapi/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/list_grants.goapi/v3/handlers/customers/credits/list_transactions.goapi/v3/handlers/features/convert.goapi/v3/handlers/llmcost/convert.goapi/v3/handlers/meters/convert.gen.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goapi/v3/handlers/plans/planaddons/convert.goapi/v3/handlers/subscriptions/convert.goapi/v3/handlers/taxcodes/convert.gen.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.goopenmeter/ledger/customerbalance/funded_loader.goopenmeter/ledger/customerbalance/ledger_loader.goopenmeter/ledger/customerbalance/loaders.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/customerbalance/transactions_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/customers/credits/get_balance.go
Comment on lines
+493
to
+589
| // filterStringExactToTransactionTypes converts a StringFieldFilterExact to a slice of CreditTransactionType values. | ||
| // Uses FromAPIFilterStringExact for mapping; validates that only allowed values are provided. | ||
| func filterStringExactToTransactionTypes(_ context.Context, f *filters.FilterStringExact) ([]customerbalance.CreditTransactionType, error) { | ||
| fs, err := filters.FromAPIFilterStringExact(f) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if fs == nil { | ||
| return nil, nil | ||
| } | ||
| if fs.Ne != nil { | ||
| return nil, errors.New("only eq and oeq operators are supported for type") | ||
| } | ||
|
|
||
| var values []string | ||
| if fs.Eq != nil { | ||
| values = append(values, *fs.Eq) | ||
| } | ||
| if fs.In != nil { | ||
| values = append(values, *fs.In...) | ||
| } | ||
|
|
||
| result := make([]customerbalance.CreditTransactionType, 0, len(values)) | ||
| for _, v := range values { | ||
| switch api.BillingCreditTransactionType(v) { | ||
| case api.BillingCreditTransactionTypeFunded: | ||
| result = append(result, customerbalance.CreditTransactionTypeFunded) | ||
| case api.BillingCreditTransactionTypeConsumed: | ||
| result = append(result, customerbalance.CreditTransactionTypeConsumed) | ||
| default: | ||
| return nil, fmt.Errorf("unsupported credit transaction type: %s", v) | ||
| } | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| // filterStringExactToStatuses converts a StringFieldFilterExact to a slice of meta.ChargeStatus values. | ||
| // Uses FromAPIFilterStringExact for mapping; validates that only allowed values are provided. | ||
| func filterStringExactToStatuses(_ context.Context, f *filters.FilterStringExact, convert func(api.BillingCreditGrantStatus) (meta.ChargeStatus, error)) ([]meta.ChargeStatus, error) { | ||
| fs, err := filters.FromAPIFilterStringExact(f) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if fs == nil { | ||
| return nil, nil | ||
| } | ||
| if fs.Ne != nil { | ||
| return nil, errors.New("only eq and oeq operators are supported for status") | ||
| } | ||
|
|
||
| var values []string | ||
| if fs.Eq != nil { | ||
| values = append(values, *fs.Eq) | ||
| } | ||
| if fs.In != nil { | ||
| values = append(values, *fs.In...) | ||
| } | ||
|
|
||
| result := make([]meta.ChargeStatus, 0, len(values)) | ||
| for _, v := range values { | ||
| converted, err := convert(api.BillingCreditGrantStatus(v)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| result = append(result, converted) | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| // filterStringExactToCurrencies converts a StringFieldFilterExact to a slice of currencyx.Code values. | ||
| // Uses FromAPIFilterStringExact for mapping; validates that only allowed values are provided. | ||
| func filterStringExactToCurrencies(_ context.Context, f *filters.FilterStringExact) ([]currencyx.Code, error) { | ||
| fs, err := filters.FromAPIFilterStringExact(f) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if fs == nil { | ||
| return nil, nil | ||
| } | ||
| if fs.Ne != nil { | ||
| return nil, errors.New("only eq and oeq operators are supported for currency") | ||
| } | ||
|
|
||
| var codes []currencyx.Code | ||
| if fs.Eq != nil { | ||
| codes = append(codes, currencyx.Code(*fs.Eq)) | ||
| } | ||
| if fs.In != nil { | ||
| for _, v := range *fs.In { | ||
| codes = append(codes, currencyx.Code(v)) | ||
| } | ||
| } | ||
|
|
||
| return codes, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Use validation-typed errors so clients get 4xx instead of 5xx.
The three new helpers return plain errors.New(...) / fmt.Errorf(...) for what are clearly user-input validation problems (unsupported operator, unsupported transaction type, etc.). Other code in this same file uses models.NewGenericValidationError(...) and dedicated factories like newCreditGrantExternalSettlementStatusInvalid(...) for similar cases — without that, these end up surfaced as generic internal errors rather than ValidationIssue responses.
🛡️ Suggested error wrapping
- if fs.Ne != nil {
- return nil, errors.New("only eq and oeq operators are supported for type")
- }
+ if fs.Ne != nil {
+ return nil, models.NewGenericValidationError(errors.New("only eq and oeq operators are supported for type"))
+ }
@@
- default:
- return nil, fmt.Errorf("unsupported credit transaction type: %s", v)
+ default:
+ return nil, models.NewGenericValidationError(fmt.Errorf("unsupported credit transaction type: %s", v))(Apply analogously in filterStringExactToStatuses and filterStringExactToCurrencies.)
As per coding guidelines for api/**/*.go: "Follow HTTP handler patterns and ValidationIssue conventions as documented in the /api skill."
🤖 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 `@api/v3/handlers/customers/credits/convert.go` around lines 493 - 589, Replace
plain errors and fmt.Errorf in filterStringExactToTransactionTypes,
filterStringExactToStatuses, and filterStringExactToCurrencies with
validation-typed errors so clients get 4xx responses: for the unsupported
operator checks in each function (currently errors.New("only eq and oeq
operators are supported for ...")) return a
models.NewGenericValidationError(...) (or an existing specific factory if one
exists) with the same message; in filterStringExactToTransactionTypes convert
the unsupported-type fmt.Errorf into a validation error (either call
models.NewGenericValidationError(fmt.Sprintf(...)) or add a specific factory
like newCreditTransactionTypeInvalid(value)); and ensure any errors returned
from the provided convert(...) in filterStringExactToStatuses are preserved or
mapped to a validation error type rather than a plain error. Use the function
names filterStringExactToTransactionTypes, filterStringExactToStatuses, and
filterStringExactToCurrencies to locate and update these error returns.
Comment on lines
21
to
33
| func (l *fundedCreditTransactionLoader) Load(ctx context.Context, input creditTransactionLoaderInput) (creditTransactionLoaderResult, error) { | ||
| var currency *currencyx.Code | ||
| if len(input.Currencies) == 1 { | ||
| currency = &input.Currencies[0] | ||
| } | ||
|
|
||
| result, err := l.service.CreditPurchaseSvc.ListFundedCreditActivities(ctx, creditpurchase.ListFundedCreditActivitiesInput{ | ||
| Customer: input.CustomerID, | ||
| Limit: input.Limit, | ||
| After: toFundedCreditActivityCursor(input.After), | ||
| Before: toFundedCreditActivityCursor(input.Before), | ||
| Currency: input.Currency, | ||
| Currency: currency, | ||
| }) |
Contributor
There was a problem hiding this comment.
Multi-currency filters silently ignored — likely surprising behavior.
Now that the API filter accepts Common.StringFieldFilterExact (which can carry multiple values via In), filterStringExactToCurrencies can return a slice of length ≥ 2. But here, anything other than exactly one element results in currency = nil, meaning the currency filter is effectively dropped and the loader returns funded credit activities across all currencies. That'll feel pretty surprising to API users who think they're narrowing the result set.
Same issue exists in openmeter/ledger/customerbalance/ledger_loader.go (Lines 22-26) — flagging at the root cause here.
A few options to consider:
- Push
[]currencyx.CodethroughListFundedCreditActivitiesInput/ledger.ListTransactionsInputso multi-value filtering works end-to-end. - Or, if multi-currency isn't supported yet, return an explicit validation error from the handler when
len(currencies) > 1instead of silently ignoring it.
🛡️ Quick guard option (until backend supports multi-value)
- var currency *currencyx.Code
- if len(input.Currencies) == 1 {
- currency = &input.Currencies[0]
- }
+ if len(input.Currencies) > 1 {
+ return creditTransactionLoaderResult{}, fmt.Errorf("multi-currency filtering is not yet supported")
+ }
+ var currency *currencyx.Code
+ if len(input.Currencies) == 1 {
+ currency = &input.Currencies[0]
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (l *fundedCreditTransactionLoader) Load(ctx context.Context, input creditTransactionLoaderInput) (creditTransactionLoaderResult, error) { | |
| var currency *currencyx.Code | |
| if len(input.Currencies) == 1 { | |
| currency = &input.Currencies[0] | |
| } | |
| result, err := l.service.CreditPurchaseSvc.ListFundedCreditActivities(ctx, creditpurchase.ListFundedCreditActivitiesInput{ | |
| Customer: input.CustomerID, | |
| Limit: input.Limit, | |
| After: toFundedCreditActivityCursor(input.After), | |
| Before: toFundedCreditActivityCursor(input.Before), | |
| Currency: input.Currency, | |
| Currency: currency, | |
| }) | |
| func (l *fundedCreditTransactionLoader) Load(ctx context.Context, input creditTransactionLoaderInput) (creditTransactionLoaderResult, error) { | |
| if len(input.Currencies) > 1 { | |
| return creditTransactionLoaderResult{}, fmt.Errorf("multi-currency filtering is not yet supported") | |
| } | |
| var currency *currencyx.Code | |
| if len(input.Currencies) == 1 { | |
| currency = &input.Currencies[0] | |
| } | |
| result, err := l.service.CreditPurchaseSvc.ListFundedCreditActivities(ctx, creditpurchase.ListFundedCreditActivitiesInput{ | |
| Customer: input.CustomerID, | |
| Limit: input.Limit, | |
| After: toFundedCreditActivityCursor(input.After), | |
| Before: toFundedCreditActivityCursor(input.Before), | |
| Currency: currency, | |
| }) |
🤖 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/ledger/customerbalance/funded_loader.go` around lines 21 - 33, The
loader currently drops multi-value currency filters by setting currency = nil
when input.Currencies has length != 1; update fundedCreditTransactionLoader.Load
(and the analogous ledger loader method in ledger_loader.go) to explicitly
handle len(input.Currencies) > 1 by returning a validation error instead of
silently ignoring the filter: check creditTransactionLoaderInput.Currencies (and
ledger loader's input.Currencies) and if len > 1 return a clear error (e.g.,
"multiple currencies not supported") to the caller; keep the existing behavior
of using &input.Currencies[0] when len == 1 and nil when len == 0.
Comment on lines
+53
to
+63
| loaders := make([]creditTransactionLoader, 0, len(txTypes)) | ||
| for _, txType := range txTypes { | ||
| factory, ok := creditTransactionLoaderFactories[txType] | ||
| if !ok { | ||
| return nil, txType.Validate() | ||
| } | ||
|
|
||
| factory, ok := creditTransactionLoaderFactories[*txType] | ||
| if !ok { | ||
| return nil, txType.Validate() | ||
| loaders = append(loaders, factory(s)) | ||
| } | ||
|
|
||
| return []creditTransactionLoader{factory(s)}, nil | ||
| return loaders, nil |
Contributor
There was a problem hiding this comment.
Subtle: txType.Validate() could return nil and silently mask a missing factory.
If a new CreditTransactionType is added that passes Validate() but isn't yet registered in creditTransactionLoaderFactories, this branch returns (nil, nil). Callers will see "no loaders, no error" and silently produce empty results — a fun bug to debug later 🙃
A small belt-and-suspenders tweak:
🛡️ Suggested guard
factory, ok := creditTransactionLoaderFactories[txType]
if !ok {
- return nil, txType.Validate()
+ if err := txType.Validate(); err != nil {
+ return nil, err
+ }
+ return nil, fmt.Errorf("no loader registered for credit transaction type: %s", txType)
}Also: if the same txType appears twice in the input slice (possible from a multi-value filter), the corresponding loader will run twice. Worth either deduplicating here or documenting that callers must dedupe.
🤖 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/ledger/customerbalance/loaders.go` around lines 53 - 63, The code
currently returns (nil, txType.Validate()) when a factory is missing, but
txType.Validate() can be nil and mask the error; change the missing-factory
branch in the loader construction to return an explicit non-nil error (e.g.
fmt.Errorf("no creditTransactionLoader factory registered for %v", txType))
instead of relying on txType.Validate(), and also deduplicate input txTypes
before creating loaders (use a seen map keyed by the CreditTransactionType and
only append one creditTransactionLoader per unique type when building loaders)
so the same loader is not created/run multiple times.
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.
Summary by CodeRabbit
Bug Fixes
Documentation
real_time_usage.