fix(api): use filters in listing customer entitlements #3390
Conversation
|
Warning Rate limit exceeded@GAlexIHU has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved offset/limit pagination from CustomerEntitlements V2 across OpenAPI/spec, generated server/client and JS validators; replaced former GetActiveEntitlementsOfCustomer usage with ListEntitlements-based flows that add an optional ActiveAt filter and adjusted handlers/connectors accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/entitlement/driver/entitlement.go (1)
256-261: You added Params to the request but don’t populate it—IncludeDeleted is ignored.Pass through query params from the HTTP layer.
return httptransport.NewHandlerWithArgs( @@ - return GetEntitlementsOfSubjectHandlerRequest{ + return GetEntitlementsOfSubjectHandlerRequest{ Namespace: ns, SubjectIdOrKey: params.SubjectIdOrKey, + Params: params.Params, }, nil
🧹 Nitpick comments (12)
openmeter/entitlement/connector.go (3)
46-48: Clarify ActiveAt semantics and its interaction with ExcludeInactive.ActiveAt is great, but please document precedence and boundary rules to avoid ambiguity with ExcludeInactive (e.g., does ActiveAt imply “active-only” and override ExcludeInactive? are intervals [start, end) or [start, end]? recommend UTC normalization).
Apply this diff to document intent:
type ListEntitlementsParams struct { @@ - ExcludeInactive bool - ActiveAt *time.Time + // When true, only include entitlements active at "now" (server time, normalized to UTC). + ExcludeInactive bool + // If set, filter entitlements active at this instant (normalized to UTC). + // Takes precedence over ExcludeInactive. Boundary semantics should be [start, end). + ActiveAt *time.Time
49-52: Use Go's deprecation convention for Limit/Offset.Replace vague comments with standard "Deprecated:" notes to surface in GoDoc and IDEs.
- // will be deprecated + // Deprecated: use Page (cursor-based pagination) instead. Limit int - // will be deprecated + // Deprecated: use Page (cursor-based pagination) instead. Offset int
74-75: Typo in comment.“featueres” → “features”.
-// For consistency, it is forbidden for entitlements to be created for featueres the keys of which could be mistaken for entitlement IDs. +// For consistency, it is forbidden for entitlements to be created for features the keys of which could be mistaken for entitlement IDs.api/client/go/client.gen.go (1)
43840-43916: Optional server-side BC: handle legacy offset/limit gracefully for one release.Consider mapping offset/limit -> page/pageSize (or returning 400 with an actionable error) and emitting a deprecation header to smooth consumer upgrades.
openmeter/entitlement/service/service.go (1)
123-131: Unbounded fetch risk (no page/limit).Leaving Page/Limit unset triggers a full scan/load via query.All(). If a customer accumulates many entitlements, this can be slow and memory‑heavy. Consider setting Page with a reasonable PageSize and iterating, or using Limit/Offset while they exist.
openmeter/entitlement/adapter/entitlement.go (1)
430-433: Tie DeletedAt filter to ActiveAt when provided.Currently, IncludeDeleted=false filters by DeletedAt relative to “now”, which conflicts with ActiveAt queries (historical lookups). Use ActiveAt as the DeletedAt reference when present to avoid dropping valid historical entitlements.
Suggested change (illustrative; adjust placement accordingly):
// near: now := clock.Now().UTC() deletedRef := now if params.ActiveAt != nil { deletedRef = params.ActiveAt.UTC() } // replace the current IncludeDeleted block to use deletedRef if !params.IncludeDeleted { query = query.Where( db_entitlement.Or( db_entitlement.DeletedAtGT(deletedRef), db_entitlement.DeletedAtIsNil(), ), ) }Also, consider validating that ExcludeInactive and ActiveAt aren’t combined (or define precedence), since applying both requires entitlements active both at
nowand atActiveAt.openmeter/entitlement/validators/customer/validator.go (3)
7-8: Avoid pulling in lo just for ToPtr; use a local variable pointer.This keeps deps light in validators.
-import ( +import ( "context" "fmt" - - "github.com/samber/lo" + "time" "github.com/openmeterio/openmeter/openmeter/customer" "github.com/openmeterio/openmeter/openmeter/entitlement" "github.com/openmeterio/openmeter/pkg/clock" "github.com/openmeterio/openmeter/pkg/models" ) @@ - ActiveAt: lo.ToPtr(clock.Now()), + ActiveAt: func(t time.Time) *time.Time { return &t }(clock.Now()), })Also applies to: 41-41
37-42: Only TotalCount is used—limit fetch to 1 item to reduce load. Also confirm ActiveAt semantics.
- Add Page {1,1} so backends can skip large scans.
- Verify that ActiveAt alone guarantees “active at now”; if not, also set ExcludeInactive = true.
import ( "context" "fmt" + "time" "github.com/openmeterio/openmeter/openmeter/customer" "github.com/openmeterio/openmeter/openmeter/entitlement" "github.com/openmeterio/openmeter/pkg/clock" "github.com/openmeterio/openmeter/pkg/models" + "github.com/openmeterio/openmeter/pkg/pagination" ) @@ - entitlements, err := v.entitlementRepo.ListEntitlements(ctx, entitlement.ListEntitlementsParams{ + entitlements, err := v.entitlementRepo.ListEntitlements(ctx, entitlement.ListEntitlementsParams{ CustomerIDs: []string{input.ID}, Namespaces: []string{input.Namespace}, - ActiveAt: lo.ToPtr(clock.Now()), + ActiveAt: func(t time.Time) *time.Time { return &t }(clock.Now()), + Page: pagination.Page{PageNumber: 1, PageSize: 1}, + // Optionally: + // ExcludeInactive: true, })
37-37: Nit: comment says “subject” but this checks the customer.- // Check for active entitlements for each subject + // Check for active entitlements for the customeropenmeter/entitlement/driver/entitlement.go (1)
11-12: Drop lo import if only used for ToPtr; use a local pointer.- "github.com/samber/lo" + // remove lo if unused after change @@ - ActiveAt: lo.ToPtr(clock.Now()), + ActiveAt: func(t time.Time) *time.Time { return &t }(clock.Now()),openmeter/entitlement/driver/v2/customer.go (2)
133-150: Use shared pagination defaults; also confirm ActiveAt=now is desired behavior.
- Prefer commonhttp.DefaultPage and DefaultPageSize to avoid drift.
- Verify that listing should include only currently-active entitlements; otherwise don’t force ActiveAt.
- Page: pagination.NewPage( - defaultx.WithDefault(p.Params.Page, 1), - defaultx.WithDefault(p.Params.PageSize, 100), - ), + Page: pagination.NewPage( + defaultx.WithDefault(p.Params.Page, commonhttp.DefaultPage), + defaultx.WithDefault(p.Params.PageSize, commonhttp.DefaultPageSize), + ), - ActiveAt: lo.ToPtr(clock.Now()), + ActiveAt: func(t time.Time) *time.Time { return &t }(clock.Now()),
145-149: Optional: validate OrderBy against supported values.Mirror v1 handler’s validation to fail fast on invalid order fields.
Would you like me to add the same slices/strcase-based check used in ListEntitlements (v1) to v2?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/api.gen.go(1 hunks)api/client/go/client.gen.go(1 hunks)api/client/javascript/src/client/schemas.ts(0 hunks)api/client/javascript/src/zod/index.ts(0 hunks)api/openapi.cloud.yaml(0 hunks)api/openapi.yaml(0 hunks)api/spec/src/entitlements/v2/customer.tsp(0 hunks)openmeter/entitlement/adapter/entitlement.go(1 hunks)openmeter/entitlement/connector.go(1 hunks)openmeter/entitlement/driver/entitlement.go(3 hunks)openmeter/entitlement/driver/v2/customer.go(3 hunks)openmeter/entitlement/repository.go(0 hunks)openmeter/entitlement/service/service.go(1 hunks)openmeter/entitlement/validators/customer/validator.go(2 hunks)
💤 Files with no reviewable changes (6)
- openmeter/entitlement/repository.go
- api/openapi.yaml
- api/openapi.cloud.yaml
- api/client/javascript/src/client/schemas.ts
- api/spec/src/entitlements/v2/customer.tsp
- api/client/javascript/src/zod/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
openmeter/entitlement/driver/entitlement.go (5)
api/api.gen.go (2)
ListSubjectEntitlementsParams(8553-8555)ListEntitlementsParams(7987-8031)pkg/models/errors.go (1)
NewGenericPreConditionFailedError(233-235)openmeter/entitlement/connector.go (1)
ListEntitlementsParams(32-53)openmeter/customer/defaults.go (1)
IncludeDeleted(4-4)pkg/clock/clock.go (1)
Now(14-21)
openmeter/entitlement/validators/customer/validator.go (2)
openmeter/entitlement/connector.go (1)
ListEntitlementsParams(32-53)pkg/clock/clock.go (1)
Now(14-21)
openmeter/entitlement/service/service.go (2)
api/api.gen.go (1)
ListEntitlementsParams(7987-8031)openmeter/entitlement/connector.go (1)
ListEntitlementsParams(32-53)
openmeter/entitlement/driver/v2/customer.go (5)
openmeter/entitlement/connector.go (2)
ListEntitlementsParams(32-53)ListEntitlementsOrderBy(12-12)openmeter/customer/customer.go (3)
GetCustomerInput(254-261)CustomerIDOrKey(137-140)Customer(18-30)pkg/models/errors.go (1)
NewGenericPreConditionFailedError(233-235)pkg/clock/clock.go (1)
Now(14-21)openmeter/entitlement/driver/v2/mapping.go (1)
ParserV2(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Lint
- GitHub Check: Code Generators
- GitHub Check: Artifacts / Container image
- GitHub Check: Build
- GitHub Check: Migration Checks
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
openmeter/entitlement/connector.go (1)
47-47: Verified — all repository ListEntitlementsParams literals are keyedScanned repo occurrences of ListEntitlementsParams; all composite literals use keyed fields (examples: openmeter/entitlement/driver/entitlement.go, openmeter/entitlement/service/service.go, openmeter/entitlement/balanceworker/recalculate.go); only an empty literal is at openmeter/entitlement/adapter/entitlement_test.go. No positional (unkeyed) literals found — adding ActiveAt does not break internal callers.
api/client/go/client.gen.go (3)
43840-43916: LGTM — regenerated Swagger embed OK; no stale offset/limit on V2 listConfirmed ListCustomerEntitlementsV2Params uses Page/PageSize (api/client/go/client.gen.go:7973–7991) and NewListCustomerEntitlementsV2Request is present (api/client/go/client.gen.go:22750–22752); rg found no offset/limit around the V2 list and api/openapi.yaml references Pagination.page/pageSize for this endpoint (LimitOffset.offset/limit remain elsewhere in the spec).
43840-43916: Breaking client surface (Offset/Limit removed): add migration notes and version bump — verification requiredRepo-wide search returned no matches for "offset"/"limit" — confirm whether docs/examples still reference these fields and where the generated change lives.
- Mark as breaking in CHANGELOG/release notes and bump version per semver.
- Add a MIGRATION snippet: map Offset/Limit → page/pageSize and state exact defaults (example: offset=0,limit=25 → page=1,pageSize=25; offset=25,limit=25 → page=2,pageSize=25).
- Update README/examples/snippets that reference offset/limit.
- Confirm the change location (api/client/go/client.gen.go lines 43840–43916) or provide the correct file(s); re-run a case-insensitive repo-wide search for "offset" and "limit" if unsure.
43840-43916: Ensure SDK pagination parity for listCustomerEntitlementsV2JS client omits offset/limit for this operation (api/client/javascript/src/client/schemas.ts:25076; customers.ts:402). Confirm the Go client and other generated SDKs likewise dropped offset/limit and expose page/pageSize (or the chosen cursor param) consistently — check api/client/go/client.gen.go:43840-43916.
api/api.gen.go (1)
20588-20895: Embedded swaggerSpec updated — action required: verify codegen & pagination params
- Server wrapper (api/api.gen.go): no offset/limit binding found — OK.
- Go client (api/client/go): no Offset/Limit params found — OK.
- JS client: offset/limit still present in generated validators/tests — see api/client/javascript/src/zod/index.ts (listCustomerEntitlementGrantsV2QueryParams ~17370; listEntitlementsV2Query ~18016–18024) and api/client/javascript/src/client/events.spec.ts (limit usage). Regenerate or reconcile JS client to match the new spec.
- OpenAPI specs: automated search errored and script could not confirm page/pageSize; manually inspect openapi*.yaml under api/ for the customers/*/entitlements path to verify page/pageSize vs offset/limit.
- Generator stamp: api/api.gen.go shows "Code generated by github.com/oapi-codegen/oapi-codegen/v2 version v2.5.0" — pin generator/version and regenerate all clients to keep diffs stable; avoid hand edits to generated files.
- Confirm runtime still ignores unknown query params (to avoid 400s for legacy clients).
openmeter/entitlement/driver/entitlement.go (2)
296-301: Confirm intent: always filter by ActiveAt=now.This will only return currently-active entitlements; previously behavior may have differed. If the endpoint should return all (optionally excluding inactive), consider using ExcludeInactive without forcing ActiveAt.
290-295: Potential nil deref on cust.ID; add a not-found guard.If resolveCustomerFromSubject ever returns (nil, nil), this panics.
- if cust != nil && cust.IsDeleted() { + if cust != nil && cust.IsDeleted() { return GetEntitlementsOfSubjectHandlerResponse{}, models.NewGenericPreConditionFailedError( fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cust.Namespace, cust.ID), ) } + if cust == nil { + return GetEntitlementsOfSubjectHandlerResponse{}, commonhttp.NewHTTPError(http.StatusNotFound, errors.New("customer not found")) + }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openmeter/entitlement/driver/v2/customer.go (1)
166-172: Guard against nil Customer in mapper to prevent panic.
e.Customercan be nil; dereferencinge.Customer.IDwill panic.Apply this diff:
- mapped, err := pagination.MapPagedResponseError(ents, func(e entitlement.Entitlement) (api.EntitlementV2, error) { - v2, err := ParserV2.ToAPIGenericV2(&e, e.Customer.ID, e.Customer.Key) + mapped, err := pagination.MapPagedResponseError(ents, func(e entitlement.Entitlement) (api.EntitlementV2, error) { + if e.Customer == nil { + return api.EntitlementV2{}, errors.New("entitlement item missing customer") + } + v2, err := ParserV2.ToAPIGenericV2(&e, e.Customer.ID, e.Customer.Key) if err != nil { return api.EntitlementV2{}, err } return lo.FromPtr(v2), nil })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openmeter/entitlement/adapter/entitlement.go(2 hunks)openmeter/entitlement/connector.go(1 hunks)openmeter/entitlement/driver/entitlement.go(3 hunks)openmeter/entitlement/driver/v2/customer.go(3 hunks)openmeter/entitlement/service/service.go(1 hunks)openmeter/entitlement/validators/customer/validator.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/entitlement/connector.go
🧰 Additional context used
🧬 Code graph analysis (4)
openmeter/entitlement/service/service.go (2)
openmeter/entitlement/connector.go (1)
ListEntitlementsParams(32-55)openmeter/customer/defaults.go (1)
IncludeDeleted(4-4)
openmeter/entitlement/driver/v2/customer.go (5)
openmeter/entitlement/connector.go (3)
ListEntitlementsParams(32-55)ListEntitlementsOrderBy(12-12)ListEntitlementsOrderByCreatedAt(15-15)api/api.gen.go (3)
ListEntitlementsParams(7987-8031)Entitlement(2571-2573)Customer(2188-2238)openmeter/customer/customer.go (3)
GetCustomerInput(254-261)CustomerIDOrKey(137-140)Customer(18-30)pkg/clock/clock.go (1)
Now(14-21)openmeter/entitlement/driver/v2/mapping.go (1)
ParserV2(21-21)
openmeter/entitlement/driver/entitlement.go (4)
api/api.gen.go (2)
ListSubjectEntitlementsParams(8553-8555)ListEntitlementsParams(7987-8031)pkg/models/errors.go (1)
NewGenericPreConditionFailedError(233-235)pkg/clock/clock.go (1)
Now(14-21)openmeter/entitlement/connector.go (1)
ListEntitlementsParams(32-55)
openmeter/entitlement/validators/customer/validator.go (2)
pkg/clock/clock.go (1)
Now(14-21)openmeter/entitlement/connector.go (1)
ListEntitlementsParams(32-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Code Generators
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
openmeter/entitlement/service/service.go (1)
123-137: Good fix: ActiveAt + IncludeDeleted(true) + IncludeDeletedAfter(at) preserves entitlements active at time ‘at’.This resolves the “deleted since then” exclusion and avoids over‑including items deleted before ‘at’. Return of
ents.Itemsfrom the paged response is correct for the non‑paginated path.Please confirm this same pattern (IncludeDeleted=true and IncludeDeletedAfter=ActiveAt) is consistently applied wherever ActiveAt filtering is used, to avoid subtle inconsistencies.
openmeter/entitlement/adapter/entitlement.go (2)
430-433: ActiveAt filter is correctly supported in ListEntitlements.Applying
EntitlementActiveAt(*params.ActiveAt)is the right place in the query pipeline.
474-476: TotalCount set for non‑paginated path.Populating
response.TotalCount = len(mapped)improves API consistency.openmeter/entitlement/validators/customer/validator.go (1)
37-46: Deletion precondition now honors ActiveAt and soft‑deletes.Using
ActiveAt: nowwithIncludeDeleted=trueandIncludeDeletedAfter=nowcorrectly blocks deletion only when the customer has entitlements active “now.”Also applies to: 51-51
openmeter/entitlement/driver/entitlement.go (2)
290-294: Precondition check for deleted customer is correct.Returning 412 via
NewGenericPreConditionFailedErroris appropriate.
314-315: Using ents.Items is correct for the non‑paginated list.Mapping over
ents.Itemsaligns with repository response shape.openmeter/entitlement/driver/v2/customer.go (2)
133-147: Param building looks good; defaults and ordering are wired correctly.
pagination.NewPage,OrderBywithstrcaseandOrderviaGetSortOrderare consistent with the API.
149-152: IncludeDeleted gating is correct and consistent with ActiveAt semantics.Pairing
IncludeDeleted=truewithIncludeDeletedAfter=nowavoids resurrecting historically deleted items.
GAlexIHU
force-pushed
the
galexi/fix/entitlement-api-cust-listing
branch
from
1cca274 to
59bd758
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/entitlement/connector.go (1)
51-55: Deprecate Limit/Offset on ListEntitlementsParams — mark Deprecated, enforce Page precedence, and migrate callersrg shows many active uses of Limit/Offset; the entitlement adapter already prefers Page when Page is non-zero, so removal must be staged and callers migrated.
- Add canonical Deprecated comments to Limit/Offset on the params struct (example patch remains valid).
- Enforce precedence at runtime: treat Page as canonical (if Page is non-zero ignore Limit/Offset) and add a runtime guard that logs/warns (or returns 400) when both Page and Limit/Offset are supplied.
- Migrate callers before removal: update drivers/handlers and internal callers to stop populating both fields and to use Page. Key locations to change: openmeter/entitlement/adapter/entitlement.go (fallback logic), openmeter/entitlement/driver/entitlement.go and openmeter/entitlement/driver/v2/entitlement.go (params construction), openmeter/subscription/entitlement/adapter.go, openmeter/entitlement/balanceworker/entitlementhandler.go, and affected tests (e.g. test/entitlement/regression/scenario_test.go).
Patch (apply to the params struct):
- Page pagination.Page - // will be deprecated - Limit int - // will be deprecated - Offset int + Page pagination.Page // Preferred pagination. + // Deprecated: use Page. + Limit int + // Deprecated: use Page. + Offset int
♻️ Duplicate comments (1)
openmeter/entitlement/driver/v2/customer.go (1)
166-172: Avoid panic: check e.Customer before dereference.Add a guard in the mapper.
- mapped, err := pagination.MapPagedResponseError(ents, func(e entitlement.Entitlement) (api.EntitlementV2, error) { - v2, err := ParserV2.ToAPIGenericV2(&e, e.Customer.ID, e.Customer.Key) + mapped, err := pagination.MapPagedResponseError(ents, func(e entitlement.Entitlement) (api.EntitlementV2, error) { + if e.Customer == nil { + return api.EntitlementV2{}, errors.New("entitlement item missing customer") + } + v2, err := ParserV2.ToAPIGenericV2(&e, e.Customer.ID, e.Customer.Key) if err != nil { return api.EntitlementV2{}, err } return lo.FromPtr(v2), nil })
🧹 Nitpick comments (7)
api/client/go/client.gen.go (1)
43840-43916: Consider go:embed for the swagger spec to reduce diff churn and leak-detector noise.Inlining multi-megabyte base64 strings bloats diffs and triggers scanners. If the generator supports it, prefer embedding the raw JSON/YAML with go:embed and decompress (if needed) at build or runtime.
Example:
//go:embed openapi.json var swaggerSpec []byteopenmeter/entitlement/connector.go (1)
34-44: Document filter precedence and AND/OR semantics across fieldsThis param surface mixes multiple overlapping selectors (IDs vs FeatureIDs/Keys/IDsOrKeys, CustomerIDs vs CustomerKeys, SubjectKeys). Please document:
- Whether items within a field are OR’d and fields across the struct are AND’d.
- Precedence when both ID- and key-based fields are set (e.g., does FeatureIDsOrKeys override FeatureIDs/FeatureKeys?).
Consider adding brief field-level comments to remove ambiguity. Example patch:
type ListEntitlementsParams struct { - IDs []string - Namespaces []string - SubjectKeys []string - CustomerIDs []string - CustomerKeys []string - FeatureIDs []string - FeatureKeys []string - FeatureIDsOrKeys []string + IDs []string // OR within; AND across fields. Exact entitlement IDs. + Namespaces []string // Limit to these namespaces. + SubjectKeys []string // Subject keys (non-customer subjects), if applicable. + CustomerIDs []string // Customer IDs; prefer IDs over keys where both are known. + CustomerKeys []string // Customer keys; don’t set together with CustomerIDs unless explicitly supported. + FeatureIDs []string // Feature IDs; see precedence with FeatureKeys/FeatureIDsOrKeys. + FeatureKeys []string // Feature keys. + FeatureIDsOrKeys []string // If set, treat as authoritative and ignore FeatureIDs/FeatureKeys. EntitlementTypes []EntitlementType OrderBy ListEntitlementsOrderBy Order sortx.OrderPlease confirm the intended precedence so downstream adapter/driver logic stays consistent. I can follow up with a Validate() helper to enforce it.
api/api.gen.go (2)
20588-20894: Codegen churn: pin generator versions to reduce large opaque diffs.This massive string diff is hard to review. Please pin codegen tool versions (OpenAPI generators for Go/JS/Zod) in CI and document the exact commands (e.g., CODEGEN.md or go:generate). This minimizes non-functional blob changes across PRs.
20588-20894: Backward compatibility and comms.Dropping offset/limit is a breaking API change for existing clients. Ensure:
- release notes call this out and provide migration (use activeAt/order/pagination guidance),
- server gracefully ignores unknown offset/limit if old clients still send them (no 400),
- add a short deprecation note in docs.
openmeter/entitlement/service/service.go (1)
145-148: Normalize the ‘at’ timestamp to UTC before querying.Prevents subtle timezone drift versus DB-comparison fields that are typically stored/queried in UTC.
- ActiveAt: &at, + ActiveAt: lo.ToPtr(at.UTC()),openmeter/entitlement/adapter/entitlement.go (1)
430-433: Use UTC for ActiveAt filter to match repository’s UTC usage elsewhere.Keeps comparisons consistent with other time filters using UTC.
- if params.ActiveAt != nil { - query = query.Where(EntitlementActiveAt(*params.ActiveAt)...) - } + if params.ActiveAt != nil { + query = query.Where(EntitlementActiveAt(params.ActiveAt.UTC())...) + }openmeter/entitlement/driver/v2/customer.go (1)
149-152: Redundant IncludeDeletedAfter=now when IncludeDeleted=true.
IncludeDeletedAfter=nowyields the same set as excluding deleted (i.e., only not‑deleted). If the intent is to truly “include deleted,” omit the time filter.if lo.FromPtr(p.Params.IncludeDeleted) { params.IncludeDeleted = true - params.IncludeDeletedAfter = now }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
api/api.gen.go(1 hunks)api/client/go/client.gen.go(1 hunks)api/client/javascript/src/client/schemas.ts(0 hunks)api/client/javascript/src/zod/index.ts(0 hunks)api/openapi.cloud.yaml(0 hunks)api/openapi.yaml(0 hunks)api/spec/src/entitlements/v2/customer.tsp(0 hunks)openmeter/entitlement/adapter/entitlement.go(2 hunks)openmeter/entitlement/connector.go(1 hunks)openmeter/entitlement/driver/entitlement.go(3 hunks)openmeter/entitlement/driver/v2/customer.go(3 hunks)openmeter/entitlement/repository.go(0 hunks)openmeter/entitlement/service/service.go(1 hunks)openmeter/entitlement/validators/customer/validator.go(2 hunks)
💤 Files with no reviewable changes (6)
- api/client/javascript/src/client/schemas.ts
- api/client/javascript/src/zod/index.ts
- openmeter/entitlement/repository.go
- api/openapi.yaml
- api/spec/src/entitlements/v2/customer.tsp
- api/openapi.cloud.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/entitlement/validators/customer/validator.go
- openmeter/entitlement/driver/entitlement.go
🧰 Additional context used
🧬 Code graph analysis (2)
openmeter/entitlement/service/service.go (1)
openmeter/entitlement/connector.go (1)
ListEntitlementsParams(33-56)
openmeter/entitlement/driver/v2/customer.go (6)
openmeter/entitlement/connector.go (3)
ListEntitlementsParams(33-56)ListEntitlementsOrderBy(13-13)ListEntitlementsOrderByCreatedAt(16-16)api/api.gen.go (3)
ListEntitlementsParams(7987-8031)Entitlement(2571-2573)Customer(2188-2238)openmeter/customer/customer.go (3)
GetCustomerInput(254-261)CustomerIDOrKey(137-140)Customer(18-30)pkg/models/errors.go (1)
NewGenericPreConditionFailedError(233-235)pkg/clock/clock.go (1)
Now(14-21)openmeter/entitlement/driver/v2/mapping.go (1)
ParserV2(21-21)
🪛 Gitleaks (8.27.2)
api/client/go/client.gen.go
[high] 43845-43846: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
api/client/go/client.gen.go (1)
43845-43846: ```shell
#!/bin/bash
set -euo pipefail
file="api/client/go/client.gen.go"if [ ! -f "$file" ]; then
echo "ERROR: file $file not found" >&2
exit 1
fipython3 - <<'PY'
import re,sys,codecs,base64,gzip
fn="api/client/go/client.gen.go"
text=open(fn, 'r', encoding='utf-8').read()
m = re.search(r'var\s+swaggerSpec\s*=\s*[]string\s*{\s*(.?)\s}', text, re.S)
if not m:
print("NO_BLOCK_FOUND")
sys.exit(0)
content = m.group(1)extract double-quoted strings
strings = re.findall(r'"((?:[^"\\]|\.)*)"', content, re.S)
if not strings:
print("NO_STRINGS_FOUND")
sys.exit(0)unescape C-style escapes
decoded_parts = []
for s in strings:
try:
decoded_parts.append(codecs.decode(s, 'unicode_escape'))
except Exception:
decoded_parts.append(s.replace('\\','\'))
b64 = ''.join(decoded_parts)
open('/tmp/swagger.b64','wb').write(b64.encode('ascii', 'ignore'))attempt base64 decode
try:
data = base64.b64decode(b64)
except Exception as e:
print("BASE64_DECODE_ERROR:"+str(e))
sys.exit(0)if gzip header, decompress
if data[:2]==b'\x1f\x8b':
try:
out = gzip.decompress(data)
open('/tmp/swagger.dec','wb').write(out)
except Exception as e:
print("GZIP_DECOMPRESS_ERROR:"+str(e))
open('/tmp/swagger.dec','wb').write(data)
else:
open('/tmp/swagger.dec','wb').write(data)
print("OK")
PYtarget="/tmp/swagger.dec"
if [ ! -s "$target" ]; then
echo "Decoded spec not found or empty at $target"
exit 0
fiecho "Scanning decoded spec for secret-like patterns..."
rg -nPI --max-columns=200 "(access|api|secret|token|auth)[^a-zA-Z0-9]{0,3}(key|token|secret)\s*[:=]\s*['"][A-Za-z0-9_-]{16,}|(?i)bearer\s+[A-Za-z0-9-.]{16,}|AIza[0-9A-Za-z-]{35}" "$target" || echo "No obvious secrets."echo "Check for removed params:"
rg -nP '"(offset|limit)"' "$target" || echo "No offset/limit params found."</blockquote></details> <details> <summary>openmeter/entitlement/service/service.go (1)</summary><blockquote> `140-150`: **Good fix: ActiveAt + IncludeDeleted(+After) avoids dropping past‑active entitlements.** Passing ActiveAt with IncludeDeleted=true and IncludeDeletedAfter=at is the right combo to return entitlements active at the reference time even if deleted later. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| cust, err := h.customerService.GetCustomer(ctx, customer.GetCustomerInput{ | ||
| CustomerIDOrKey: &customer.CustomerIDOrKey{ | ||
| Namespace: ns, | ||
| IDOrKey: p.CustomerIdOrKey, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return entitlement.ListEntitlementsParams{}, err | ||
| return ListCustomerEntitlementsHandlerRequest{}, err | ||
| } | ||
|
|
||
| if cus != nil && cus.IsDeleted() { | ||
| return entitlement.ListEntitlementsParams{}, models.NewGenericPreConditionFailedError( | ||
| fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cus.Namespace, cus.ID), | ||
| if cust != nil && cust.IsDeleted() { | ||
| return ListCustomerEntitlementsHandlerRequest{}, models.NewGenericPreConditionFailedError( | ||
| fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cust.Namespace, cust.ID), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Guard against nil customer after lookup to avoid panic later.
cust is dereferenced below; add a not-found guard.
cust, err := h.customerService.GetCustomer(ctx, customer.GetCustomerInput{
CustomerIDOrKey: &customer.CustomerIDOrKey{
Namespace: ns,
IDOrKey: p.CustomerIdOrKey,
},
})
if err != nil {
return ListCustomerEntitlementsHandlerRequest{}, err
}
+ if cust == nil {
+ return ListCustomerEntitlementsHandlerRequest{}, commonhttp.NewHTTPError(http.StatusNotFound, fmt.Errorf("customer not found [namespace=%s idOrKey=%s]", ns, p.CustomerIdOrKey))
+ }
if cust != nil && cust.IsDeleted() {
return ListCustomerEntitlementsHandlerRequest{}, models.NewGenericPreConditionFailedError(
fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cust.Namespace, cust.ID),
)
}📝 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.
| cust, err := h.customerService.GetCustomer(ctx, customer.GetCustomerInput{ | |
| CustomerIDOrKey: &customer.CustomerIDOrKey{ | |
| Namespace: ns, | |
| IDOrKey: p.CustomerIdOrKey, | |
| }, | |
| }) | |
| if err != nil { | |
| return entitlement.ListEntitlementsParams{}, err | |
| return ListCustomerEntitlementsHandlerRequest{}, err | |
| } | |
| if cus != nil && cus.IsDeleted() { | |
| return entitlement.ListEntitlementsParams{}, models.NewGenericPreConditionFailedError( | |
| fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cus.Namespace, cus.ID), | |
| if cust != nil && cust.IsDeleted() { | |
| return ListCustomerEntitlementsHandlerRequest{}, models.NewGenericPreConditionFailedError( | |
| fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cust.Namespace, cust.ID), | |
| ) | |
| } | |
| cust, err := h.customerService.GetCustomer(ctx, customer.GetCustomerInput{ | |
| CustomerIDOrKey: &customer.CustomerIDOrKey{ | |
| Namespace: ns, | |
| IDOrKey: p.CustomerIdOrKey, | |
| }, | |
| }) | |
| if err != nil { | |
| return ListCustomerEntitlementsHandlerRequest{}, err | |
| } | |
| if cust == nil { | |
| return ListCustomerEntitlementsHandlerRequest{}, commonhttp.NewHTTPError(http.StatusNotFound, fmt.Errorf("customer not found [namespace=%s idOrKey=%s]", ns, p.CustomerIdOrKey)) | |
| } | |
| if cust != nil && cust.IsDeleted() { | |
| return ListCustomerEntitlementsHandlerRequest{}, models.NewGenericPreConditionFailedError( | |
| fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cust.Namespace, cust.ID), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In openmeter/entitlement/driver/v2/customer.go around lines 118 to 131, add a
nil-check immediately after the GetCustomer call to guard against cust being
nil; if cust == nil return the empty ListCustomerEntitlementsHandlerRequest and
an appropriate not-found error (include contextual info like namespace and
requested id/key), so downstream dereferences won’t panic and callers get a
clear "customer not found" response.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
openmeter/entitlement/driver/entitlement.go (1)
265-269: Bug: IncludeDeleted (and future filters) are dropped — Params not propagated.The request builder doesn’t copy
params.Paramsinto the request, soid.Params.IncludeDeletedwill always be nil/false in the handler.Apply:
return GetEntitlementsOfSubjectHandlerRequest{ Namespace: ns, SubjectIdOrKey: params.SubjectIdOrKey, + Params: params.Params, }, nilopenmeter/entitlement/driver/v2/customer.go (3)
118-131: Guard against nil customer after lookup to avoid panic.You dereference
cust.IDlater; add a not-found guard first.cust, err := h.customerService.GetCustomer(ctx, customer.GetCustomerInput{ CustomerIDOrKey: &customer.CustomerIDOrKey{ Namespace: ns, IDOrKey: p.CustomerIdOrKey, }, }) if err != nil { return ListCustomerEntitlementsHandlerRequest{}, err } +if cust == nil { + return ListCustomerEntitlementsHandlerRequest{}, commonhttp.NewHTTPError( + http.StatusNotFound, + fmt.Errorf("customer not found [namespace=%s idOrKey=%s]", ns, p.CustomerIdOrKey), + ) +} if cust != nil && cust.IsDeleted() { return ListCustomerEntitlementsHandlerRequest{}, models.NewGenericPreConditionFailedError( fmt.Errorf("customer is deleted [namespace=%s customer.id=%s]", cust.Namespace, cust.ID), ) }
146-149: Fix unsafe pointer cast in OrderBy defaulting (compile-time error risk).Don’t cast
*api.EntitlementOrderByOrderingOrderByto*string. Default the typed value, then stringify and snake_case.- OrderBy: entitlement.ListEntitlementsOrderBy( - strcase.CamelToSnake(defaultx.WithDefault((*string)(p.Params.OrderBy), string(entitlement.ListEntitlementsOrderByCreatedAt))), - ), + // Default typed OrderBy, then convert to string and snake_case + OrderBy: func() entitlement.ListEntitlementsOrderBy { + ob := lo.FromPtrOr(p.Params.OrderBy, api.EntitlementOrderByOrderingOrderBy(entitlement.ListEntitlementsOrderByCreatedAt)) + return entitlement.ListEntitlementsOrderBy(strcase.CamelToSnake(string(ob))) + }(),
160-166: Guard against nil Customer in list items to avoid panics.
e.Customercan be nil; the mapper dereferences it.- mapped, err := pagination.MapPagedResponseError(ents, func(e entitlement.Entitlement) (api.EntitlementV2, error) { - v2, err := ParserV2.ToAPIGenericV2(&e, e.Customer.ID, e.Customer.Key) + mapped, err := pagination.MapPagedResponseError(ents, func(e entitlement.Entitlement) (api.EntitlementV2, error) { + if e.Customer == nil { + return api.EntitlementV2{}, errors.New("entitlement item missing customer") + } + v2, err := ParserV2.ToAPIGenericV2(&e, e.Customer.ID, e.Customer.Key) if err != nil { return api.EntitlementV2{}, err } return lo.FromPtr(v2), nil })
🧹 Nitpick comments (2)
openmeter/entitlement/driver/entitlement.go (1)
282-289: Verify IncludeDeleted semantics vs IncludeDeletedAfter.V2 flow sets
IncludeDeletedAfter: now, but this path only setsIncludeDeleted. If the connector expectsIncludeDeletedAfterto scope results, align semantics or confirm it’s unnecessary here.openmeter/entitlement/driver/v2/customer.go (1)
139-151: Double-check IncludeDeleted semantics; wiring likely incomplete.
IncludeDeletedAfter: nowwithout settingIncludeDeletedmay exclude all deleted items. If the API exposesincludeDeleted, propagate it and only setIncludeDeletedAfterwhen relevant.Possible adjustment:
ListParams: entitlement.ListEntitlementsParams{ CustomerIDs: []string{cust.ID}, Namespaces: []string{ns}, ActiveAt: lo.ToPtr(now), + IncludeDeleted: defaultIncludeDeleted(p.Params.IncludeDeleted), + // Optionally gate IncludeDeletedAfter behind IncludeDeleted: + // IncludeDeletedAfter: lo.Ternary(defaultIncludeDeleted(p.Params.IncludeDeleted), now, time.Time{}), Page: pagination.NewPage( defaultx.WithDefault(p.Params.Page, 1), defaultx.WithDefault(p.Params.PageSize, 100), ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openmeter/entitlement/driver/entitlement.go(3 hunks)openmeter/entitlement/driver/v2/customer.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
openmeter/entitlement/driver/entitlement.go (5)
api/api.gen.go (2)
ListSubjectEntitlementsParams(8553-8555)ListEntitlementsParams(7987-8031)pkg/models/errors.go (1)
NewGenericPreConditionFailedError(233-235)pkg/clock/clock.go (1)
Now(14-21)openmeter/entitlement/connector.go (1)
ListEntitlementsParams(33-56)openmeter/customer/defaults.go (1)
IncludeDeleted(4-4)
openmeter/entitlement/driver/v2/customer.go (4)
openmeter/entitlement/connector.go (3)
ListEntitlementsParams(33-56)ListEntitlementsOrderBy(13-13)ListEntitlementsOrderByCreatedAt(16-16)api/api.gen.go (3)
ListEntitlementsParams(7987-8031)Entitlement(2571-2573)Customer(2188-2238)openmeter/customer/customer.go (3)
GetCustomerInput(254-261)CustomerIDOrKey(137-140)Customer(18-30)pkg/clock/clock.go (1)
Now(14-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
openmeter/entitlement/driver/entitlement.go (1)
276-281: LGTM: Deleted-customer precondition.Returning a precondition error for deleted customers is correct and consistent.
| ents, err := h.connector.ListEntitlements(ctx, entitlement.ListEntitlementsParams{ | ||
| CustomerIDs: []string{cust.ID}, | ||
| Namespaces: []string{id.Namespace}, | ||
| ActiveAt: lo.ToPtr(now), | ||
| IncludeDeleted: lo.FromPtr(id.Params.IncludeDeleted), | ||
| }) |
There was a problem hiding this comment.
Guard against nil customer before using cust.ID.
cust can be nil from the resolver; dereferencing cust.ID would panic.
- ents, err := h.connector.ListEntitlements(ctx, entitlement.ListEntitlementsParams{
+ if cust == nil {
+ return GetEntitlementsOfSubjectHandlerResponse{}, commonhttp.NewHTTPError(http.StatusNotFound,
+ fmt.Errorf("customer not found [namespace=%s subject=%s]", id.Namespace, id.SubjectIdOrKey))
+ }
+ ents, err := h.connector.ListEntitlements(ctx, entitlement.ListEntitlementsParams{
CustomerIDs: []string{cust.ID},
Namespaces: []string{id.Namespace},
ActiveAt: lo.ToPtr(now),
IncludeDeleted: lo.FromPtr(id.Params.IncludeDeleted),
})🤖 Prompt for AI Agents
In openmeter/entitlement/driver/entitlement.go around lines 284 to 289, the code
dereferences cust.ID without checking whether cust is nil; guard against a nil
cust by checking if cust == nil before accessing cust.ID and handle that case
(e.g., return an error, skip this customer, or use an appropriate default) so
you never pass a nil dereference into ListEntitlements; ensure the chosen
behavior is consistent with the surrounding logic and tests and propagate or log
an informative error when cust is nil.
3 participants
Overview
The V2 API didn't honor all params which resulted in issues
Notes for reviewer
Summary by CodeRabbit
New Features
Refactor
Documentation