feat(entitlement): add grant balances to v2 entitlement value api#3922
Merged
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 Entitlement V2: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as "Client"
participant Router as "Router\n(openmeter/server)"
participant Handler as "CustomerHandler\nV2 Handler"
participant Service as "EntitlementService"
participant Metered as "Metered Connector\n(GetValue/GetEntitlementBalance)"
participant Mapper as "APIMapping\nMapEntitlementValueToAPIV2"
Client->>Router: GET /api/.../entitlement/value (V2)
Router->>Handler: invoke GetCustomerEntitlementValueV2
Handler->>Service: Fetch entitlement value for customer (with Time)
Service->>Metered: GetValue / GetEntitlementBalance (includes GrantBalances)
Metered-->>Service: EntitlementValue (with GrantBalances)
Service-->>Handler: domain EntitlementValue (with GrantBalances)
Handler->>Mapper: MapEntitlementValueToAPIV2(domain value)
Mapper-->>Handler: EntitlementValueV2 (includes grantBalances)
Handler-->>Client: 200 OK + EntitlementValueV2 JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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 (1)
openmeter/customer/httpdriver/customer.go (1)
430-481: Please extract the shared V1/V2 customer-resolution flow to avoid drift.Line 432 onward and Line 550 onward repeat almost the same request/customer lookup and deleted-check flow as V1. Pulling that into small helpers would make these endpoints easier to maintain and less likely to diverge.
♻️ Suggested extraction sketch
+func (h *handler) buildEntitlementValueRequest( + ctx context.Context, + params GetCustomerEntitlementValueParams, +) (GetCustomerEntitlementValueRequest, error) { + ns, err := h.resolveNamespace(ctx) + if err != nil { + return GetCustomerEntitlementValueRequest{}, err + } + + cus, err := h.service.GetCustomer(ctx, customer.GetCustomerInput{ + CustomerIDOrKey: &customer.CustomerIDOrKey{ + IDOrKey: params.CustomerIDOrKey, + Namespace: ns, + }, + }) + if err != nil { + return GetCustomerEntitlementValueRequest{}, err + } + if cus != nil && cus.IsDeleted() { + return GetCustomerEntitlementValueRequest{}, + models.NewGenericPreConditionFailedError( + fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cus.Namespace, cus.ID), + ) + } + + return GetCustomerEntitlementValueRequest{ + FeatureKey: params.FeatureKey, + CustomerID: cus.GetID(), + }, nil +} + +func (h *handler) buildCustomerAccessRequest( + ctx context.Context, + params GetCustomerAccessParams, +) (GetCustomerAccessRequest, error) { + ns, err := h.resolveNamespace(ctx) + if err != nil { + return GetCustomerAccessRequest{}, err + } + return GetCustomerAccessRequest{ + CustomerIDOrKey: &customer.CustomerIDOrKey{ + Namespace: ns, + IDOrKey: params.CustomerIDOrKey, + }, + }, nil +}As per coding guidelines,
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Also applies to: 548-596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/customer/httpdriver/customer.go` around lines 430 - 481, The customer resolution, lookup and deleted-check logic repeated in GetCustomerEntitlementValueV2 (and the V1 handler) should be extracted into a small helper to avoid drift: create a helper function (e.g. resolveAndValidateCustomer(ctx context.Context, idOrKey string) (customer.CustomerID, error)) that calls h.resolveNamespace, calls h.service.GetCustomer with customer.GetCustomerInput/customer.CustomerIDOrKey, checks cus != nil && cus.IsDeleted() and returns models.NewGenericPreConditionFailedError when deleted, and returns the canonical customer ID on success; then replace the inline blocks in GetCustomerEntitlementValueV2 (and the V1 handler) to call this helper and use its returned CustomerID for entitlementService.GetEntitlementValue to keep behavior identical and reduce duplication.
🤖 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/spec/src/entitlements/v2/customer.tsp`:
- Around line 235-236: Replace the floating-point types used for monetary/credit
fields with the shared precise numeric type: update the type of balance, usage,
overage, totalAvailableGrantAmount, and grantBalances from float64 to
Shared.Numeric in the customer model/type definitions (e.g., where the fields
balance?: float64; appears). Ensure the TypeSpec file imports/uses the Shared
namespace so Shared.Numeric is resolvable and adjust any array/map annotations
for grantBalances to use Shared.Numeric elements as needed.
In `@openmeter/customer/httpdriver/apimapping.go`:
- Around line 17-25: The JSON tag on the HasAccess field in EntitlementValueV2
currently uses `omitempty` which drops false and violates the API contract;
update the struct definition for EntitlementValueV2 by removing `omitempty` from
the `HasAccess` json tag so the boolean is always serialized (leave other fields
unchanged) and run tests to verify serialization matches the contract.
In `@openmeter/server/router/entitlement.go`:
- Around line 223-227: GetCustomerEntitlementValueV2 currently ignores the
incoming params (so query time filters like the `time` param are dropped);
update Router.GetCustomerEntitlementValueV2 to populate and pass the received
`params` into customerdriver.GetCustomerEntitlementValueParams (e.g., attach the
relevant fields from the `params` argument such as time/start/end or the entire
params struct) when calling
a.customerHandler.GetCustomerEntitlementValueV2().With(...).ServeHTTP so the
handler receives and honors historical/time-based lookup parameters.
---
Nitpick comments:
In `@openmeter/customer/httpdriver/customer.go`:
- Around line 430-481: The customer resolution, lookup and deleted-check logic
repeated in GetCustomerEntitlementValueV2 (and the V1 handler) should be
extracted into a small helper to avoid drift: create a helper function (e.g.
resolveAndValidateCustomer(ctx context.Context, idOrKey string)
(customer.CustomerID, error)) that calls h.resolveNamespace, calls
h.service.GetCustomer with customer.GetCustomerInput/customer.CustomerIDOrKey,
checks cus != nil && cus.IsDeleted() and returns
models.NewGenericPreConditionFailedError when deleted, and returns the canonical
customer ID on success; then replace the inline blocks in
GetCustomerEntitlementValueV2 (and the V1 handler) to call this helper and use
its returned CustomerID for entitlementService.GetEntitlementValue to keep
behavior identical and reduce duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0af068b1-88b1-4f97-b85d-0ba29a6fb8b9
⛔ Files ignored due to path filters (8)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/python/openmeter/_generated/aio/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/__init__.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (10)
api/api.gen.goapi/spec/src/entitlements/v2/customer.tspe2e/entitlement_parity_test.goe2e/multisubject_test.goopenmeter/customer/httpdriver/apimapping.goopenmeter/customer/httpdriver/customer.goopenmeter/customer/httpdriver/handler.goopenmeter/entitlement/metered/balance.goopenmeter/entitlement/metered/connector.goopenmeter/server/router/entitlement.go
Comment on lines
+235
to
+236
| balance?: float64; | ||
|
|
Contributor
There was a problem hiding this comment.
Use Shared.Numeric for these financial fields instead of float64.
balance, usage, overage, totalAvailableGrantAmount, and grantBalances values are monetary/credit quantities; using float64 here risks precision drift across generated clients.
💡 Proposed TypeSpec adjustment
- balance?: float64;
+ balance?: Shared.Numeric;
- usage?: float64;
+ usage?: Shared.Numeric;
- overage?: float64;
+ overage?: Shared.Numeric;
- totalAvailableGrantAmount?: float64;
+ totalAvailableGrantAmount?: Shared.Numeric;
- grantBalances?: Record<float64>;
+ grantBalances?: Record<Shared.Numeric>;Based on learnings: In TypeSpec API specs, prefer using Shared.Numeric for financial values (e.g., exchange rates, amounts, prices) instead of native numeric types like decimal128 or float64.
Also applies to: 242-243, 249-250, 256-257, 270-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/spec/src/entitlements/v2/customer.tsp` around lines 235 - 236, Replace
the floating-point types used for monetary/credit fields with the shared precise
numeric type: update the type of balance, usage, overage,
totalAvailableGrantAmount, and grantBalances from float64 to Shared.Numeric in
the customer model/type definitions (e.g., where the fields balance?: float64;
appears). Ensure the TypeSpec file imports/uses the Shared namespace so
Shared.Numeric is resolvable and adjust any array/map annotations for
grantBalances to use Shared.Numeric elements as needed.
Comment on lines
+17
to
+25
| type EntitlementValueV2 struct { | ||
| HasAccess bool `json:"hasAccess,omitempty"` | ||
| Balance *float64 `json:"balance,omitempty"` | ||
| Config *string `json:"config,omitempty"` | ||
| Overage *float64 `json:"overage,omitempty"` | ||
| TotalAvailableGrantAmount *float64 `json:"totalAvailableGrantAmount,omitempty"` | ||
| Usage *float64 `json:"usage,omitempty"` | ||
| GrantBalances *map[string]float64 `json:"grantBalances,omitempty"` | ||
| } |
Contributor
There was a problem hiding this comment.
hasAccess is required but can be omitted due omitempty.
The generated API contract expects hasAccess to always be present; with omitempty, false gets dropped from JSON.
💡 Proposed fix
type EntitlementValueV2 struct {
- HasAccess bool `json:"hasAccess,omitempty"`
+ HasAccess bool `json:"hasAccess"`
Balance *float64 `json:"balance,omitempty"`
Config *string `json:"config,omitempty"`
Overage *float64 `json:"overage,omitempty"`
TotalAvailableGrantAmount *float64 `json:"totalAvailableGrantAmount,omitempty"`
Usage *float64 `json:"usage,omitempty"`
GrantBalances *map[string]float64 `json:"grantBalances,omitempty"`
}📝 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
| type EntitlementValueV2 struct { | |
| HasAccess bool `json:"hasAccess,omitempty"` | |
| Balance *float64 `json:"balance,omitempty"` | |
| Config *string `json:"config,omitempty"` | |
| Overage *float64 `json:"overage,omitempty"` | |
| TotalAvailableGrantAmount *float64 `json:"totalAvailableGrantAmount,omitempty"` | |
| Usage *float64 `json:"usage,omitempty"` | |
| GrantBalances *map[string]float64 `json:"grantBalances,omitempty"` | |
| } | |
| type EntitlementValueV2 struct { | |
| HasAccess bool `json:"hasAccess"` | |
| Balance *float64 `json:"balance,omitempty"` | |
| Config *string `json:"config,omitempty"` | |
| Overage *float64 `json:"overage,omitempty"` | |
| TotalAvailableGrantAmount *float64 `json:"totalAvailableGrantAmount,omitempty"` | |
| Usage *float64 `json:"usage,omitempty"` | |
| GrantBalances *map[string]float64 `json:"grantBalances,omitempty"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/customer/httpdriver/apimapping.go` around lines 17 - 25, The JSON
tag on the HasAccess field in EntitlementValueV2 currently uses `omitempty`
which drops false and violates the API contract; update the struct definition
for EntitlementValueV2 by removing `omitempty` from the `HasAccess` json tag so
the boolean is always serialized (leave other fields unchanged) and run tests to
verify serialization matches the contract.
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 `@openmeter/entitlement/metered/balance.go`:
- Around line 25-31: The EntitlementBalance struct's GrantBalances map must be
initialized on every return path so responses never omit/null that field; modify
the reset flow in the EntitlementBalance creation/update in reset.go (the code
that constructs or returns an EntitlementBalance after reset) to ensure
GrantBalances is set to an empty map (map[string]float64{}) when no grants
exist, and similarly add initialization wherever EntitlementBalance is
constructed or returned (look for functions/methods that return
EntitlementBalance or modify its fields, including the reset handler in reset.go
and any factory/constructor for EntitlementBalance) so GrantBalances is always
non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0f85e82-3e0c-452d-8253-a3c2a83b2930
📒 Files selected for processing (2)
openmeter/entitlement/metered/balance.goopenmeter/entitlement/metered/connector.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/entitlement/metered/connector.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/customer/httpdriver/customer.go (1)
437-491: Consider extracting shared customer-read flow used by v1/v2 handlers.The v1/v2 entitlement-value and access handlers are almost identical; pulling customer resolution + validation into a helper would reduce drift and make future fixes safer.
As per coding guidelines, “In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.”
Also applies to: 558-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/customer/httpdriver/customer.go` around lines 437 - 491, The customer-resolution and validation logic in GetCustomerEntitlementValueV2 (resolveNamespace, h.service.GetCustomer call, deleted check and building GetEntitlementValueInput) is duplicated across v1/v2 handlers; extract it into a single helper (e.g., handler.resolveAndValidateCustomerForEntitlement or loadCustomerForEntitlement) that returns the namespace, customer ID wrapper (customer.CustomerIDOrKey/GetEntitlementValueInput) and any error, reuse it from GetCustomerEntitlementValueV2 and the corresponding v1 handler (the near-duplicate block around lines 558-606) and update calls to use the helper before calling h.entitlementService.GetEntitlementValue; keep existing error types and precondition check (models.NewGenericPreConditionFailedError) unchanged.
🤖 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/customer/httpdriver/customer.go`:
- Around line 407-410: The code dereferences the variable cus when building
GetEntitlementValueInput (and at the other uses around the same handler), but
cus can be nil if the lookup returned (nil, nil); add a nil-check immediately
after the customer lookup that returns an appropriate error/HTTP 404 (or the
handler's not-found response) instead of proceeding, and apply the same guard
before any other dereference sites referenced (the occurrences around the
GetEntitlementValueInput construction and the uses at the other two locations).
Ensure the check is placed before any use of cus.GetID() or cus fields so
GetEntitlementValueInput and other code never dereference a nil pointer.
---
Nitpick comments:
In `@openmeter/customer/httpdriver/customer.go`:
- Around line 437-491: The customer-resolution and validation logic in
GetCustomerEntitlementValueV2 (resolveNamespace, h.service.GetCustomer call,
deleted check and building GetEntitlementValueInput) is duplicated across v1/v2
handlers; extract it into a single helper (e.g.,
handler.resolveAndValidateCustomerForEntitlement or loadCustomerForEntitlement)
that returns the namespace, customer ID wrapper
(customer.CustomerIDOrKey/GetEntitlementValueInput) and any error, reuse it from
GetCustomerEntitlementValueV2 and the corresponding v1 handler (the
near-duplicate block around lines 558-606) and update calls to use the helper
before calling h.entitlementService.GetEntitlementValue; keep existing error
types and precondition check (models.NewGenericPreConditionFailedError)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1148714-ba59-44eb-bb39-be34b977bcd0
📒 Files selected for processing (4)
openmeter/customer/httpdriver/customer.goopenmeter/entitlement/metered/balance.goopenmeter/entitlement/metered/reset.goopenmeter/server/router/entitlement.go
Comment on lines
+407
to
+410
| GetEntitlementValueInput: customer.GetEntitlementValueInput{ | ||
| FeatureKey: params.FeatureKey, | ||
| CustomerID: cus.GetID(), | ||
| }, |
Contributor
There was a problem hiding this comment.
Guard against nil cus before dereferencing.
Line 410, Line 465, and Line 588 dereference cus without a cus == nil check. If lookup returns (nil, nil) on not-found, this can panic.
💡 Suggested guard pattern
@@
- return GetCustomerEntitlementValueRequest{
+ if cus == nil {
+ return GetCustomerEntitlementValueRequest{}, fmt.Errorf("customer not found")
+ }
+
+ return GetCustomerEntitlementValueRequest{
GetEntitlementValueInput: customer.GetEntitlementValueInput{
FeatureKey: params.FeatureKey,
CustomerID: cus.GetID(),
},
At: defaultx.WithDefault(params.Time, clock.Now()),
}, nil
@@
- access, err := h.entitlementService.GetAccess(ctx, cus.Namespace, cus.ID)
+ if cus == nil {
+ return GetCustomerAccessV2Response{}, fmt.Errorf("customer not found")
+ }
+
+ access, err := h.entitlementService.GetAccess(ctx, cus.Namespace, cus.ID)Also applies to: 462-466, 588-589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/customer/httpdriver/customer.go` around lines 407 - 410, The code
dereferences the variable cus when building GetEntitlementValueInput (and at the
other uses around the same handler), but cus can be nil if the lookup returned
(nil, nil); add a nil-check immediately after the customer lookup that returns
an appropriate error/HTTP 404 (or the handler's not-found response) instead of
proceeding, and apply the same guard before any other dereference sites
referenced (the occurrences around the GetEntitlementValueInput construction and
the uses at the other two locations). Ensure the check is placed before any use
of cus.GetID() or cus fields so GetEntitlementValueInput and other code never
dereference a nil pointer.
GAlexIHU
force-pushed
the
feat/grant-balances-on-entitlement-value
branch
from
9ae4109 to
6140c1e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openmeter/customer/httpdriver/customer.go (1)
399-412:⚠️ Potential issue | 🔴 CriticalGuard
cus == nilbefore dereferencing in the new/updated GET handlers.Line 409, Line 465, and Line 588 can panic when lookup returns
(nil, nil).💡 Suggested fix pattern
@@ - if cus != nil && cus.IsDeleted() { + if cus == nil { + return GetCustomerEntitlementValueRequest{}, fmt.Errorf("failed to get customer") + } + if cus.IsDeleted() { return GetCustomerEntitlementValueRequest{}, models.NewGenericPreConditionFailedError( fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cus.Namespace, cus.ID), ) } @@ - if cus != nil && cus.IsDeleted() { + if cus == nil { + return GetCustomerEntitlementValueRequest{}, fmt.Errorf("failed to get customer") + } + if cus.IsDeleted() { return GetCustomerEntitlementValueRequest{}, models.NewGenericPreConditionFailedError( fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cus.Namespace, cus.ID), ) } @@ - if cus != nil && cus.IsDeleted() { + if cus == nil { + return GetCustomerAccessV2Response{}, fmt.Errorf("failed to get customer") + } + if cus.IsDeleted() { return GetCustomerAccessV2Response{}, models.NewGenericPreConditionFailedError( fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cus.Namespace, cus.ID), ) }Also applies to: 455-468, 581-589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/customer/httpdriver/customer.go` around lines 399 - 412, Guard against cus being nil before any dereference in the GET handlers: check "if cus == nil" before calling cus.IsDeleted() or accessing cus.Namespace, cus.ID, or cus.GetID(), and if nil return an appropriate not-found error (e.g., models.NewGenericNotFoundError) along with an empty GetCustomerEntitlementValueRequest; keep the existing return path that builds GetCustomerEntitlementValueRequest (using customer.GetEntitlementValueInput, params.FeatureKey, defaultx.WithDefault(params.Time, clock.Now())) only after the nil check passes.
🧹 Nitpick comments (1)
openmeter/customer/httpdriver/customer.go (1)
437-491: Recommended: extract shared customer-resolution logic used by v1/v2 handlers.
GetCustomerEntitlementValuevsGetCustomerEntitlementValueV2andGetCustomerAccessvsGetCustomerAccessV2are mostly duplicated; a shared helper would reduce drift and prevent bug duplication.As per coding guidelines "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
Also applies to: 558-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/customer/httpdriver/customer.go` around lines 437 - 491, The handler contains duplicated customer-resolution logic across GetCustomerEntitlementValueV2 (and similar blocks in GetCustomerEntitlementValue, GetCustomerAccess, GetCustomerAccessV2); extract that into a shared helper (e.g., resolveCustomerForHandler or buildCustomerRequest) that performs h.resolveNamespace(ctx), calls h.service.GetCustomer with customer.GetCustomerInput / customer.CustomerIDOrKey, checks cus != nil && cus.IsDeleted() and returns models.NewGenericPreConditionFailedError, and computes defaults like At via defaultx.WithDefault(params.Time, clock.Now()); then replace the duplicated request-construction code in the request factory functions (which currently build GetCustomerEntitlementValueRequest and similar) to call this helper and return its result so all handlers reuse the same customer resolution path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/multisubject_test.go`:
- Around line 263-267: The test currently compares floating sums with
assert.Equal which is brittle; change the final assertion to a tolerant float
comparison (e.g., use testify's assert.InDelta) comparing *ent.Balance and
grantBalancesSum with a small delta (for example 1e-6) so tiny rounding
differences won't fail the test—replace the assert.Equal(t, *ent.Balance,
grantBalancesSum) line with an assert.InDelta(t, *ent.Balance, grantBalancesSum,
1e-6) (or equivalent) while keeping the sum computation using grantBalancesSum
and ent.GrantBalances as-is.
In `@openmeter/server/router/entitlement.go`:
- Around line 137-139: The route is calling
customerHandler.GetCustomerAccessV2() which returns the v2 schema (including
GrantBalances) and breaks the existing v1 contract; replace the call with
customerHandler.GetCustomerAccess().With(customerdriver.GetCustomerAccessParams{
CustomerIDOrKey: customerIdOrKey }).ServeHTTP(w, r) so the v1 CustomerAccess
handler is used, or if you intentionally want v2, update the route/operation
spec to reflect the new response schema instead of swapping handlers silently.
---
Duplicate comments:
In `@openmeter/customer/httpdriver/customer.go`:
- Around line 399-412: Guard against cus being nil before any dereference in the
GET handlers: check "if cus == nil" before calling cus.IsDeleted() or accessing
cus.Namespace, cus.ID, or cus.GetID(), and if nil return an appropriate
not-found error (e.g., models.NewGenericNotFoundError) along with an empty
GetCustomerEntitlementValueRequest; keep the existing return path that builds
GetCustomerEntitlementValueRequest (using customer.GetEntitlementValueInput,
params.FeatureKey, defaultx.WithDefault(params.Time, clock.Now())) only after
the nil check passes.
---
Nitpick comments:
In `@openmeter/customer/httpdriver/customer.go`:
- Around line 437-491: The handler contains duplicated customer-resolution logic
across GetCustomerEntitlementValueV2 (and similar blocks in
GetCustomerEntitlementValue, GetCustomerAccess, GetCustomerAccessV2); extract
that into a shared helper (e.g., resolveCustomerForHandler or
buildCustomerRequest) that performs h.resolveNamespace(ctx), calls
h.service.GetCustomer with customer.GetCustomerInput / customer.CustomerIDOrKey,
checks cus != nil && cus.IsDeleted() and returns
models.NewGenericPreConditionFailedError, and computes defaults like At via
defaultx.WithDefault(params.Time, clock.Now()); then replace the duplicated
request-construction code in the request factory functions (which currently
build GetCustomerEntitlementValueRequest and similar) to call this helper and
return its result so all handlers reuse the same customer resolution path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29dc2af4-ce12-483e-99dc-1c0641b53c11
⛔ Files ignored due to path filters (8)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/python/openmeter/_generated/aio/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/__init__.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (11)
api/api.gen.goapi/spec/src/entitlements/v2/customer.tspe2e/entitlement_parity_test.goe2e/multisubject_test.goopenmeter/customer/httpdriver/apimapping.goopenmeter/customer/httpdriver/customer.goopenmeter/customer/httpdriver/handler.goopenmeter/entitlement/metered/balance.goopenmeter/entitlement/metered/connector.goopenmeter/entitlement/metered/reset.goopenmeter/server/router/entitlement.go
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/entitlement_parity_test.go
- openmeter/customer/httpdriver/handler.go
- openmeter/entitlement/metered/balance.go
- openmeter/customer/httpdriver/apimapping.go
- api/spec/src/entitlements/v2/customer.tsp
Comment on lines
+263
to
+267
| var grantBalancesSum float64 | ||
| for _, grantBalance := range *ent.GrantBalances { | ||
| grantBalancesSum += grantBalance | ||
| } | ||
| assert.Equal(t, *ent.Balance, grantBalancesSum) |
Contributor
There was a problem hiding this comment.
Use tolerant float comparison for sum parity.
This exact equality can get flaky with fractional balances; better to assert within a tiny delta.
💡 Suggested tweak
- assert.Equal(t, *ent.Balance, grantBalancesSum)
+ assert.InDelta(t, *ent.Balance, grantBalancesSum, 1e-9)📝 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
| var grantBalancesSum float64 | |
| for _, grantBalance := range *ent.GrantBalances { | |
| grantBalancesSum += grantBalance | |
| } | |
| assert.Equal(t, *ent.Balance, grantBalancesSum) | |
| var grantBalancesSum float64 | |
| for _, grantBalance := range *ent.GrantBalances { | |
| grantBalancesSum += grantBalance | |
| } | |
| assert.InDelta(t, *ent.Balance, grantBalancesSum, 1e-9) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/multisubject_test.go` around lines 263 - 267, The test currently compares
floating sums with assert.Equal which is brittle; change the final assertion to
a tolerant float comparison (e.g., use testify's assert.InDelta) comparing
*ent.Balance and grantBalancesSum with a small delta (for example 1e-6) so tiny
rounding differences won't fail the test—replace the assert.Equal(t,
*ent.Balance, grantBalancesSum) line with an assert.InDelta(t, *ent.Balance,
grantBalancesSum, 1e-6) (or equivalent) while keeping the sum computation using
grantBalancesSum and ent.GrantBalances as-is.
Comment on lines
+137
to
139
| a.customerHandler.GetCustomerAccessV2().With(customerdriver.GetCustomerAccessParams{ | ||
| CustomerIDOrKey: customerIdOrKey, | ||
| }).ServeHTTP(w, r) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for GetCustomerAccess in generated API files and identify v1 vs v2 contracts
find . -name "*.go" -type f | head -20Repository: openmeterio/openmeter
Length of output: 509
🏁 Script executed:
# Search for GetCustomerAccess definitions and their signatures
rg -n 'GetCustomerAccess' --type go | grep -E '(api\.gen|httpdriver|entitlement)' | head -30Repository: openmeterio/openmeter
Length of output: 3481
🏁 Script executed:
# Look for the actual response types/structures
rg -n 'type.*CustomerAccess|type.*EntitlementValue' --type go -C 2 | head -50Repository: openmeterio/openmeter
Length of output: 3152
🏁 Script executed:
# Find CustomerAccessV2 type definition
rg -n 'type CustomerAccessV2' --type go -A 10Repository: openmeterio/openmeter
Length of output: 876
🏁 Script executed:
# Check the OpenAPI/generated route for GetCustomerAccess in api.gen.go
rg -n 'GetCustomerAccess' openmeter/customer/httpdriver/customer.go -C 5 | head -40Repository: openmeterio/openmeter
Length of output: 1593
🏁 Script executed:
# Check if there's an OpenAPI spec defining the original GetCustomerAccess contract
find . -name "*.yaml" -o -name "*.yml" -o -name "*.json" | xargs grep -l "GetCustomerAccess\|get.*customer.*access" 2>/dev/null | head -5Repository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
# Find api.CustomerAccess type definition
rg -n 'type CustomerAccess struct' --type go -A 15 api/Repository: openmeterio/openmeter
Length of output: 2083
🏁 Script executed:
# Compare the two structures side by side
echo "=== CustomerAccessV2 ===" && \
rg -n 'type CustomerAccessV2 struct' --type go -A 5 openmeter/customer/httpdriver/apimapping.go && \
echo "" && \
echo "=== api.CustomerAccess ===" && \
rg -n 'type CustomerAccess struct' --type go -A 15 api/types/Repository: openmeterio/openmeter
Length of output: 340
🏁 Script executed:
# Check EntitlementValue vs EntitlementValueV2 type definitions
rg -n 'type EntitlementValue' openmeter/customer/httpdriver/ --type go -A 10Repository: openmeterio/openmeter
Length of output: 1189
🏁 Script executed:
# Also check api.EntitlementValue
rg -n 'type EntitlementValue struct' api/api.gen.go -A 10Repository: openmeterio/openmeter
Length of output: 971
🏁 Script executed:
# Let's see the full EntitlementValue structure in api.gen.go
rg -n 'type EntitlementValue struct' api/api.gen.go -A 25Repository: openmeterio/openmeter
Length of output: 2052
🏁 Script executed:
# Also check the router to confirm the wiring is indeed swapped
sed -n '130,145p' openmeter/server/router/entitlement.goRepository: openmeterio/openmeter
Length of output: 588
Keep GetCustomerAccess handler here, not GetCustomerAccessV2.
The endpoint is wired to call GetCustomerAccessV2(), which returns a different response schema. The v2 version adds a GrantBalances field to the entitlement values that doesn't exist in v1. Clients expecting the v1 CustomerAccess schema will get the v2 one instead—that's a silent contract change even though JSON unmarshaling is forgiving about extra fields.
Either swap back to the v1 handler, or update the route spec/operation definition alongside this change so the API contract stays clear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/server/router/entitlement.go` around lines 137 - 139, The route is
calling customerHandler.GetCustomerAccessV2() which returns the v2 schema
(including GrantBalances) and breaks the existing v1 contract; replace the call
with
customerHandler.GetCustomerAccess().With(customerdriver.GetCustomerAccessParams{
CustomerIDOrKey: customerIdOrKey }).ServeHTTP(w, r) so the v1 CustomerAccess
handler is used, or if you intentionally want v2, update the route/operation
spec to reflect the new response schema instead of swapping handlers silently.
tothandras
previously approved these changes
Mar 5, 2026
GAlexIHU
force-pushed
the
feat/grant-balances-on-entitlement-value
branch
from
6140c1e to
5d7a8fb
Compare
gergely-kurucz-konghq
approved these changes
Mar 11, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
openmeter/customer/httpdriver/apimapping.go (1)
17-25:⚠️ Potential issue | 🟠 Major
HasAccessshould not useomitempty.The JSON tag on
HasAccessincludesomitempty, which will drop the field when it'sfalse. SincehasAccessis required in the API contract (not optional), it should always be serialized.💡 Proposed fix
type EntitlementValueV2 struct { - HasAccess bool `json:"hasAccess,omitempty"` + HasAccess bool `json:"hasAccess"` Balance *float64 `json:"balance,omitempty"` Config *string `json:"config,omitempty"` Overage *float64 `json:"overage,omitempty"` TotalAvailableGrantAmount *float64 `json:"totalAvailableGrantAmount,omitempty"` Usage *float64 `json:"usage,omitempty"` GrantBalances *map[string]float64 `json:"grantBalances,omitempty"` },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/customer/httpdriver/apimapping.go` around lines 17 - 25, The JSON tag for the field HasAccess in type EntitlementValueV2 should not include "omitempty" because hasAccess is required by the API contract; update the struct field tag on HasAccess to `json:"hasAccess"` (remove ",omitempty") so the boolean is always serialized, and then run/adjust any serialization tests or callers that assume the field is present.openmeter/customer/httpdriver/customer.go (2)
445-465:⚠️ Potential issue | 🟠 MajorAdd nil guard before dereferencing
cus.If
GetCustomerreturns(nil, nil)for a not-found case, line 465 (cus.GetID()) will panic. The same pattern exists in the v1 handler, so this might be a pre-existing issue, but worth addressing in the new code.💡 Suggested guard
if err != nil { return GetCustomerEntitlementValueRequest{}, err } + + if cus == nil { + return GetCustomerEntitlementValueRequest{}, commonhttp.NewHTTPError(http.StatusNotFound, fmt.Errorf("customer not found")) + } if cus != nil && cus.IsDeleted() {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/customer/httpdriver/customer.go` around lines 445 - 465, The code dereferences cus (returned from h.service.GetCustomer) without checking for nil, which can panic if GetCustomer returns (nil, nil); add a nil guard after the GetCustomer call and before using cus (e.g., before cus.IsDeleted() and before building GetCustomerEntitlementValueRequest) to return an appropriate not-found or precondition error when cus == nil; update the logic around the cus variable in the handler that builds GetCustomerEntitlementValueRequest so all uses (cus.IsDeleted(), cus.GetID(), cus.Namespace, cus.ID) are only accessed after confirming cus != nil.
574-588:⚠️ Potential issue | 🟠 MajorSame nil guard needed here for
cus.Line 588 dereferences
cus.Namespacewithout a nil check. Apply the same guard pattern as suggested for GetCustomerEntitlementValueV2.💡 Suggested guard
if err != nil { return GetCustomerAccessV2Response{}, err } + + if cus == nil { + return GetCustomerAccessV2Response{}, commonhttp.NewHTTPError(http.StatusNotFound, fmt.Errorf("customer not found")) + } if cus != nil && cus.IsDeleted() {,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/customer/httpdriver/customer.go` around lines 574 - 588, Add a nil-check for cus before dereferencing cus.Namespace/cus.ID when calling h.entitlementService.GetAccess: after the call to h.service.GetCustomer (the variable cus returned there) return early with the same not-found/precondition-failed style used in GetCustomerEntitlementValueV2 if cus == nil, so you never call h.entitlementService.GetAccess(ctx, cus.Namespace, cus.ID) on a nil pointer.
🧹 Nitpick comments (1)
openmeter/server/router/entitlement.go (1)
134-140: Section header may be misleading.The route comment says
/api/v2/customers/{customerId}/accessbut it's under the "Customer Entitlement APIs (V1)" section header. If this is actually a v2 endpoint, it might be cleaner to move it under the V2 section (after line 156) for clarity. Otherwise, LGTM on the handler swap since the route is v2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/server/router/entitlement.go` around lines 134 - 140, The comment/header for the GetCustomerAccess route is inconsistent: the route comment shows a v2 path but the function is placed under the "Customer Entitlement APIs (V1)" section. Fix by either moving the GetCustomerAccess function (Router.GetCustomerAccess) and its route comment into the V2 section where other v2 handlers live, or if this is intended to be V1, change the route comment to the correct v1 path; ensure the section header and the comment for GetCustomerAccess (CustomerIDOrKey route) match the intended API version.
🤖 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/customer/httpdriver/apimapping.go`:
- Around line 17-25: The JSON tag for the field HasAccess in type
EntitlementValueV2 should not include "omitempty" because hasAccess is required
by the API contract; update the struct field tag on HasAccess to
`json:"hasAccess"` (remove ",omitempty") so the boolean is always serialized,
and then run/adjust any serialization tests or callers that assume the field is
present.
In `@openmeter/customer/httpdriver/customer.go`:
- Around line 445-465: The code dereferences cus (returned from
h.service.GetCustomer) without checking for nil, which can panic if GetCustomer
returns (nil, nil); add a nil guard after the GetCustomer call and before using
cus (e.g., before cus.IsDeleted() and before building
GetCustomerEntitlementValueRequest) to return an appropriate not-found or
precondition error when cus == nil; update the logic around the cus variable in
the handler that builds GetCustomerEntitlementValueRequest so all uses
(cus.IsDeleted(), cus.GetID(), cus.Namespace, cus.ID) are only accessed after
confirming cus != nil.
- Around line 574-588: Add a nil-check for cus before dereferencing
cus.Namespace/cus.ID when calling h.entitlementService.GetAccess: after the call
to h.service.GetCustomer (the variable cus returned there) return early with the
same not-found/precondition-failed style used in GetCustomerEntitlementValueV2
if cus == nil, so you never call h.entitlementService.GetAccess(ctx,
cus.Namespace, cus.ID) on a nil pointer.
---
Nitpick comments:
In `@openmeter/server/router/entitlement.go`:
- Around line 134-140: The comment/header for the GetCustomerAccess route is
inconsistent: the route comment shows a v2 path but the function is placed under
the "Customer Entitlement APIs (V1)" section. Fix by either moving the
GetCustomerAccess function (Router.GetCustomerAccess) and its route comment into
the V2 section where other v2 handlers live, or if this is intended to be V1,
change the route comment to the correct v1 path; ensure the section header and
the comment for GetCustomerAccess (CustomerIDOrKey route) match the intended API
version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 220920ea-4cc7-47c0-bb2e-c6f9ddc027f2
⛔ Files ignored due to path filters (8)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/python/openmeter/_generated/aio/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/__init__.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/client/python/openmeter/_generated/operations/_operations.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (11)
api/api.gen.goapi/spec/src/entitlements/v2/customer.tspe2e/entitlement_parity_test.goe2e/multisubject_test.goopenmeter/customer/httpdriver/apimapping.goopenmeter/customer/httpdriver/customer.goopenmeter/customer/httpdriver/handler.goopenmeter/entitlement/metered/balance.goopenmeter/entitlement/metered/connector.goopenmeter/entitlement/metered/reset.goopenmeter/server/router/entitlement.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/entitlement/metered/balance.go
- e2e/entitlement_parity_test.go
- openmeter/entitlement/metered/connector.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
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.
Overview
Adds grant level balances to V2 entitlement value APIs
Summary by CodeRabbit
New Features
Tests