feat(rpc,entity): add context-aware filter lookup support - #111
Merged
Conversation
…-scoped state Introduce ContextLookupFunc and related interfaces to enable filters to resolve request-scoped state (e.g., per-request database handles) instead of process globals. New types: - ContextLookupFunc: context-aware variant of filter lookup closure - ContextFilter: optional interface for filters needing request context - ContextSearchableFilter: context-aware variant of SearchableFilter Changes: - Add contextLookupFuncRegistry to track context-aware lookups - Implement GetContextLookupFunc() to retrieve registered context lookups - Add AddCommandWithContext() for commands receiving request context - Refactor buildLookupFunc into resolveLookup() to support both context and non-context paths - Update EntityOperation and BulkActionInfo to include ContextLookupFunc field - Implement liftedFilter methods for context-aware option resolution - Update RPC converter and executor to prefer ContextLookupFunc over LookupFunc - Add hasLookup() helper to check for either lookup variant Filters implementing ContextFilter/ContextSearchableFilter can now access request context during option resolution, enabling dynamic filtering based on per-request state.
Contributor
|
Warning Review limit reached
More reviews will be available in 38 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR extends entity command generation with context-aware filter lookups and pagination support. New ChangesContext-aware Pagination and Entity Filtering
Dependency Updates
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Serve as rpc/serve
participant RPC as RPC Executor
participant Entity as Entity Handler
participant Filter as Filter
Client->>Serve: GET /api/v1/resource?limit=10&offset=0
Serve->>RPC: resolve operation
RPC->>RPC: check hasLookup(op)
alt ContextLookupFunc available
RPC->>Filter: ContextLookupFunc(ctx, flags)
else fallback
RPC->>Filter: LookupFunc(flags)
end
Filter-->>Entity: options
Entity->>Entity: ListPagedWithContext(ctx, opts)
Entity-->>Entity: PagedResult{Data, Page}
Serve->>Serve: detect Paged result
Serve->>Serve: set X-Total-Count header
Serve-->>Client: {data: [...], page: {limit, offset, total}}
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Contributor
Gavel summary
Totals: 1823 passed · 3 failed · 24 skipped · 1m36s Failing testsginkgo Executiongo: downloading github.com/charmbracelet/bubbletea v1.3.10 go: downloading github.com/charmbracelet/bubbletea v1.3.10 |
Introduce PagedResult type and pagination metadata to support list operations that return total counts and paging information. Add ListPaged and ListPagedWithContext methods to EntityBuilder for declaring paginated list handlers. Implement promoteListToEntityRoot to make the bare entity command runnable as its own list operation, exposed as GET /api/v1/<entity>. Update RPC converter and OpenAPI generator to handle paged responses with X-Total-Count, X-Page-Limit, and X-Page-Offset headers. Ensure only ListOpts filter flags are promoted to entity root, preventing global persistent flags from leaking into query parameters. BREAKING CHANGE: runEntityOp now extracts paged results via PageRows() interface method before formatting, changing how paged responses are serialized in RPC responses.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
entity.go (1)
785-805:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdmin list registration skips new context/paged list handlers.
Line 785 only checks
admin.List != nil, so admin entities configured withListWithContext,ListPaged, orListPagedWithContextalone won’t register a list operation. Mirror the main list switch here so admin behavior matches the new API surface.Suggested fix sketch
- if admin.List != nil { - adminInfo.Operations = append(adminInfo.Operations, EntityOperation{ - Verb: "list", - DataFunc: func(flagMap map[string]string, args []string) (any, error) { ... }, - ... - }) - } + if admin.ListPagedWithContext != nil || admin.ListPaged != nil || admin.ListWithContext != nil || admin.List != nil { + op := EntityOperation{ + Verb: "list", + LookupFunc: buildLookupFunc[ListOpts](admin.Filters), + ContextLookupFunc: buildLookupFuncWithContext[ListOpts](admin.Filters), + BindCompletions: buildFilterCompletionBinder[ListOpts](admin.Filters), + ResponseType: reflect.TypeOf((*T)(nil)).Elem(), + ResponseArray: true, + ResponseEntityID: true, + } + // same precedence as primary entity list: + // ListPagedWithContext > ListPaged > ListWithContext > List + // set op.ResponsePaged + DataFunc/ContextDataFunc accordingly + adminInfo.Operations = append(adminInfo.Operations, op) + }🤖 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 `@entity.go` around lines 785 - 805, The admin registration currently only checks admin.List when adding the "list" EntityOperation, so entities that implement ListWithContext, ListPaged, or ListPagedWithContext are skipped; update the block that appends to adminInfo.Operations to mirror the main list switch by checking all variants (admin.List, admin.ListWithContext, admin.ListPaged, admin.ListPagedWithContext), wiring the appropriate DataFunc (and preserving the existing use of resolveEntityOpts, withEntityIDs, and error handling) and keeping LookupFunc/ContextLookupFunc/BindCompletions/ResponseType/ResponseArray/ResponseEntityID consistent (use buildLookupFunc and buildLookupFuncWithContext as needed) so any of the four list handlers will register a "list" operation on adminInfo.
🧹 Nitpick comments (1)
rpc/entities_test.go (1)
113-168: ⚡ Quick winAdd a
application/json+clickypagination assertion.This test currently validates only
application/json. Adding oneclicky-jsonrequest/assertion would lock coverage for the structured paged envelope path and prevent format-specific regressions.🤖 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 `@rpc/entities_test.go` around lines 113 - 168, TestEntityPagedList_ResponseEnvelopeAndHeaders only exercises "application/json"; add a second request that uses the clicky structured paged envelope MIME to lock that code path. Duplicate the httptest request (create req2/rr2), set req2.Header.Set("Accept", "application/json+clicky"), call mux.ServeHTTP(rr2, req2), then assert rr2.Code == http.StatusOK, the same pagination headers ("X-Total-Count", "X-Page-Limit", "X-Page-Offset") and unmarshal rr2.Body into the same payload struct to assert payload.Data length and payload.Page fields (Limit/Offset/Total) match expected values; use variable names like req2 and rr2 to avoid clobbering the original test flow.
🤖 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 `@entity.go`:
- Around line 1484-1492: The code mutates the caller-owned flagMap by deleting
lookupFilterKeyParam and lookupQueryParam before calling buildOpts[T], which can
leak side-effects; to fix, make a shallow copy of flagMap (e.g., new map
variable) and perform the delete operations on that copy, then pass the copied
map into buildOpts[T] while preserving the original flagMap and keeping
searchKey/searchQuery extraction logic intact.
In `@rpc/serve.go`:
- Around line 752-758: The function shouldFormatPagedRows currently treats
formats "json", "yaml", and "yml" as non-formattable but omits the clicky JSON
variant; update shouldFormatPagedRows to also treat "clicky-json" (and therefore
requests with MIME "application/json+clicky") as false so responses use the
paged envelope (preserving data/page metadata) instead of being flattened by
PageRows().
---
Outside diff comments:
In `@entity.go`:
- Around line 785-805: The admin registration currently only checks admin.List
when adding the "list" EntityOperation, so entities that implement
ListWithContext, ListPaged, or ListPagedWithContext are skipped; update the
block that appends to adminInfo.Operations to mirror the main list switch by
checking all variants (admin.List, admin.ListWithContext, admin.ListPaged,
admin.ListPagedWithContext), wiring the appropriate DataFunc (and preserving the
existing use of resolveEntityOpts, withEntityIDs, and error handling) and
keeping
LookupFunc/ContextLookupFunc/BindCompletions/ResponseType/ResponseArray/ResponseEntityID
consistent (use buildLookupFunc and buildLookupFuncWithContext as needed) so any
of the four list handlers will register a "list" operation on adminInfo.
---
Nitpick comments:
In `@rpc/entities_test.go`:
- Around line 113-168: TestEntityPagedList_ResponseEnvelopeAndHeaders only
exercises "application/json"; add a second request that uses the clicky
structured paged envelope MIME to lock that code path. Duplicate the httptest
request (create req2/rr2), set req2.Header.Set("Accept",
"application/json+clicky"), call mux.ServeHTTP(rr2, req2), then assert rr2.Code
== http.StatusOK, the same pagination headers ("X-Total-Count", "X-Page-Limit",
"X-Page-Offset") and unmarshal rr2.Body into the same payload struct to assert
payload.Data length and payload.Page fields (Limit/Offset/Total) match expected
values; use variable names like req2 and rr2 to avoid clobbering the original
test flow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be00eb1d-3227-4b69-a92c-e76887a0338f
⛔ Files ignored due to path filters (2)
examples/go.sumis excluded by!**/*.sumexamples/uber_demo/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cobra_command.goentity.goentity_builder.goexamples/uber_demo/go.modpagination.goresponse_meta.gorpc/converter.gorpc/entities_test.gorpc/executor.gorpc/openapi.gorpc/serve.gorpc/types.go
Extend the fmt target to run 'go mod tidy' across all Go module directories (., valkey, examples, examples/uber_demo) in addition to code formatting. This ensures dependencies are kept in sync whenever code formatting is performed. Also clean up unused dependencies in examples/go.sum.
- resolveLookup: copy flagMap before stripping reserved lookup params so the caller-owned map is not mutated - shouldFormatPagedRows: preserve paged envelope for clicky-json responses so application/json+clicky keeps data/page metadata - lint e2e: set GOFLAGS=-mod=mod for fixture modules so package loading populates go.sum instead of failing on missing transitive entries
|
🎉 This PR is included in version 1.21.15 |
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
2 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.
What
ContextLookupFuncand related interfaces (ContextFilter,ContextSearchableFilter) to enable filters to resolve request-scoped statecontextLookupFuncRegistryto track context-aware lookupsGetContextLookupFunc()to retrieve registered context lookupsAddCommandWithContext()for commands receiving request contextbuildLookupFuncintoresolveLookup()to support both context and non-context pathsEntityOperationandBulkActionInfoto includeContextLookupFuncfieldContextLookupFuncoverLookupFuncWhy
Summary by CodeRabbit
New Features
X-Total-Count,X-Page-Limit,X-Page-Offset) for paginated list operations.Chores