Conversation
WalkthroughA new "List Contexts" feature was introduced, enabling the retrieval of all context IDs for an organization via a new API endpoint. This involved backend storage updates, a new permission, protocol buffer changes, OpenAPI documentation, and a new handler method. Logging and permission management were also updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Auth
participant Storage
Client->>API: GET /v1/context
API->>Auth: permissionCheck("context.list")
Auth-->>API: Permission granted/denied
alt Permission granted
API->>Storage: ListContexts(orgID)
Storage-->>API: [contextID1, contextID2, ...]
API-->>Client: ListContextsResponse(contextIds)
else Permission denied
API-->>Client: Error (permission denied)
end
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🤖 API structural change detected: Added (1)
|
|
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}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
apps/api/api/context.go(1 hunks)apps/api/auth/middleware.go(2 hunks)apps/api/go.mod(1 hunks)apps/api/openapi/assets/openapi.yaml(3 hunks)apps/api/storage/buntdb/db.go(1 hunks)apps/api/storage/interface.go(1 hunks)apps/api/storage/keys.go(1 hunks)apps/api/storage/redis/db.go(1 hunks)proto/brease/context/v1/models.proto(1 hunks)proto/brease/context/v1/service.proto(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/api/storage/redis/db.go (2)
apps/api/storage/buntdb/db.go (1)
Container(51-55)apps/api/storage/keys.go (2)
ContextKey(11-13)SplitContextKey(15-18)
apps/api/storage/buntdb/db.go (2)
apps/api/storage/redis/db.go (1)
Container(58-62)apps/api/storage/keys.go (1)
ContextKey(11-13)
apps/api/api/context.go (2)
apps/api/api/handler.go (1)
BreaseHandler(19-27)apps/api/auth/middleware.go (1)
PermissionListContext(39-39)
🪛 Buf (1.55.1)
proto/brease/context/v1/service.proto
12-12: RPC request type "ListContextsReqeust" should be named "ListContextsRequest" or "ContextServiceListContextsRequest".
(RPC_REQUEST_STANDARD_NAME)
⏰ 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). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
apps/api/storage/interface.go (1)
21-21: LGTM! Well-designed interface method.The new
ListContextsmethod follows consistent patterns with other interface methods, properly uses context for cancellation, and has an appropriate signature for returning context IDs scoped to an owner.apps/api/go.mod (1)
8-10: LGTM! Necessary dependency updates for new protocol buffer definitions.The version updates for the generated protocol buffer and connectrpc dependencies are appropriate and required to support the new
ListContextsRPC method.proto/brease/context/v1/service.proto (1)
11-16: LGTM! Well-designed RPC with appropriate HTTP binding.The RPC definition follows good practices with a descriptive comment, appropriate GET method for listing, and clean URL path that fits the REST API design.
proto/brease/context/v1/models.proto (1)
135-137: LGTM: Response message structure is appropriate.The
ListContextsResponsemessage correctly defines a repeated string field for context IDs.apps/api/api/context.go (2)
10-14: LGTM: Proper permission checking and error handling.The permission check using
auth.PermissionListContextand error handling are implemented correctly.
17-22: No action required: Context IDs are already sanitizedThe Redis
ListContextsimplementation inapps/api/storage/redis/db.gousesstorage.SplitContextKeyto strip the"org:123:context:"prefix and return only the raw IDs. Sinceb.db.ListContextsalready returns clean IDs, there’s no risk of leaking internal key formats to API consumers.apps/api/openapi/assets/openapi.yaml (3)
79-97: LGTM: Well-structured OpenAPI endpoint definition.The new GET
/v1/contextendpoint is properly defined with appropriate HTTP method, description, and response schema. The endpoint structure follows existing API patterns.
543-549: LGTM: Correct response schema definition.The
ListContextsResponseschema correctly definescontextIdsas an array of strings, matching the protobuf definition and using appropriate JSON naming conventions.
374-374: LGTM: Helpful description addition.The description "Updates the schema definition of the context" clarifies the purpose of the POST endpoint.
apps/api/auth/middleware.go (3)
39-39: LGTM: Consistent permission naming.The new
PermissionListContext = "context.list"follows the established naming pattern and clearly represents the operation.
43-50: LGTM: Proper integration with permissions system.The new permission is correctly added to
allPermissionsslice, maintaining the complete list of available permissions.
260-260: LGTM: Enhanced debug logging.Adding permissions to the debug log will improve troubleshooting capabilities for authentication issues.
| func (b *Container) ListContexts(ctx context.Context, ownerID string) ([]string, error) { | ||
| var contexts []string | ||
| err := b.db.View(func(tx *buntdb.Tx) error { | ||
| return tx.AscendKeys(storage.ContextKey(ownerID, "*"), func(key, val string) bool { | ||
| contexts = append(contexts, key) | ||
| return true | ||
| }) | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return contexts, nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if full keys or context IDs should be returned.
The implementation correctly queries for contexts using a wildcard pattern, but it returns the full storage keys (e.g., "org:123:context:abc") rather than just the context IDs ("abc").
Based on the ContextKey function from apps/api/storage/keys.go (lines 11-13), keys are formatted as "org:<orgID>:context:<contextID>". The API response expects just the context IDs.
🏁 Script executed:
#!/bin/bash
# Description: Check how context keys are processed in the API layer and Redis implementation
# Expected: Find if keys are parsed to extract context IDs or used as-is
# Search for usage of ListContexts method
rg -A 10 -B 5 "ListContexts.*ctx.*ownerID"
# Check if there's a helper function to extract context ID from full key
rg -A 5 "SplitContextKey\|ParseContextKey\|ExtractContextID"Length of output: 2803
Extract and return only context IDs in BuntDB’s ListContexts
The Redis implementation in apps/api/storage/redis/db.go uses storage.SplitContextKey to strip the prefix and return only the context IDs. The BuntDB version currently returns full keys (e.g. "org:123:context:abc"), which doesn’t match the API contract.
Please update the BuntDB ListContexts method in apps/api/storage/buntdb/db.go:
- File: apps/api/storage/buntdb/db.go
- Method:
func (b *Container) ListContexts(...)
Change the AscendKeys callback to split the key and append only the contextID:
- err := b.db.View(func(tx *buntdb.Tx) error {
- return tx.AscendKeys(storage.ContextKey(ownerID, "*"), func(key, val string) bool {
- contexts = append(contexts, key)
- return true
- })
- })
+ err := b.db.View(func(tx *buntdb.Tx) error {
+ return tx.AscendKeys(storage.ContextKey(ownerID, "*"), func(key, val string) bool {
+ _, ctxID := storage.SplitContextKey(key)
+ contexts = append(contexts, ctxID)
+ return true
+ })
+ })This will align BuntDB’s behavior with the Redis backend and the API’s expected output.
📝 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.
| func (b *Container) ListContexts(ctx context.Context, ownerID string) ([]string, error) { | |
| var contexts []string | |
| err := b.db.View(func(tx *buntdb.Tx) error { | |
| return tx.AscendKeys(storage.ContextKey(ownerID, "*"), func(key, val string) bool { | |
| contexts = append(contexts, key) | |
| return true | |
| }) | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return contexts, nil | |
| } | |
| func (b *Container) ListContexts(ctx context.Context, ownerID string) ([]string, error) { | |
| var contexts []string | |
| err := b.db.View(func(tx *buntdb.Tx) error { | |
| return tx.AscendKeys(storage.ContextKey(ownerID, "*"), func(key, val string) bool { | |
| _, ctxID := storage.SplitContextKey(key) | |
| contexts = append(contexts, ctxID) | |
| return true | |
| }) | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return contexts, nil | |
| } |
🤖 Prompt for AI Agents
In apps/api/storage/buntdb/db.go around lines 353 to 365, the ListContexts
method currently appends full keys to the contexts slice, which includes
prefixes. Modify the AscendKeys callback to use storage.SplitContextKey on each
key and append only the extracted contextID to the contexts slice. This change
will ensure the method returns only context IDs, matching the Redis
implementation and API expectations.
| func SplitContextKey(key string) (orgID, contextID string) { | ||
| parts := strings.Split(key, ":") | ||
| return parts[2], parts[3] | ||
| } |
There was a problem hiding this comment.
Add bounds checking to prevent runtime panics.
The function assumes the key has at least 4 colon-separated parts but doesn't validate this, which could cause an index out of bounds panic with malformed keys.
-func SplitContextKey(key string) (orgID, contextID string) {
+func SplitContextKey(key string) (orgID, contextID string, err error) {
parts := strings.Split(key, ":")
+ if len(parts) < 4 {
+ return "", "", fmt.Errorf("invalid context key format: %s", key)
+ }
return parts[2], parts[3]
}Alternatively, if you prefer to keep the current signature and assume valid input, add a comment documenting this assumption and consider adding validation in the calling code.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/storage/keys.go around lines 15 to 18, the SplitContextKey function
accesses parts[2] and parts[3] without checking if the split result has enough
elements, risking a runtime panic. Fix this by adding a check on the length of
parts before accessing these indices; if the length is insufficient, return
empty strings or an error as appropriate. Alternatively, document the assumption
that the input key must have at least 4 colon-separated parts and ensure
validation occurs before calling this function.
| func (r *Container) ListContexts(ctx context.Context, ownerID string) ([]string, error) { | ||
| ck := storage.ContextKey(ownerID, "*") | ||
| list, err := r.db.Keys(ctx, ck).Result() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // slice up key | ||
| res := make([]string, len(list)) | ||
| for i, k := range list { | ||
| _, ctxID := storage.SplitContextKey(k) | ||
| res[i] = ctxID | ||
| } | ||
| return res, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address performance and error handling concerns.
This implementation has several issues:
-
Performance concern: The Redis
KEYScommand blocks the server and can be slow with large datasets. Consider usingSCANfor production environments. -
Missing error handling: The
SplitContextKeyfunction can panic with malformed keys, but no error handling is present.
Consider this safer implementation:
func (r *Container) ListContexts(ctx context.Context, ownerID string) ([]string, error) {
ck := storage.ContextKey(ownerID, "*")
- list, err := r.db.Keys(ctx, ck).Result()
+ // Use SCAN instead of KEYS for better performance
+ var list []string
+ iter := r.db.Scan(ctx, 0, ck, 0).Iterator()
+ for iter.Next(ctx) {
+ list = append(list, iter.Val())
+ }
+ if err := iter.Err(); err != nil {
+ return nil, err
+ }
- if err != nil {
- return nil, err
- }
- // slice up key
res := make([]string, len(list))
for i, k := range list {
- _, ctxID := storage.SplitContextKey(k)
+ _, ctxID, err := storage.SplitContextKey(k)
+ if err != nil {
+ r.logger.Warn("Invalid context key format", zap.String("key", k))
+ continue
+ }
res[i] = ctxID
}
return res, nil
}Note: This assumes the SplitContextKey function is updated to return an error as suggested in the previous review.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/storage/redis/db.go around lines 372 to 385, replace the use of the
Redis KEYS command with the SCAN command to avoid blocking the server on large
datasets. Implement iteration over the SCAN cursor to collect keys matching the
pattern. Additionally, update the code to handle errors returned by the
SplitContextKey function instead of assuming it always succeeds, skipping or
logging malformed keys as appropriate. This will improve performance and
robustness in production environments.
| message ListContextsReqeust { | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix typo in message name.
The message name ListContextsReqeust has a typo - it should be ListContextsRequest (missing 't' in "Request").
-message ListContextsReqeust {
+message ListContextsRequest {This typo will propagate through all generated code and needs to be fixed before merging.
📝 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.
| message ListContextsReqeust { | |
| } | |
| message ListContextsRequest { | |
| } |
🤖 Prompt for AI Agents
In proto/brease/context/v1/models.proto around lines 131 to 133, the message
name `ListContextsReqeust` contains a typo and should be corrected to
`ListContextsRequest`. Rename the message to fix the spelling error to prevent
the typo from propagating into all generated code.
|
|
||
| service ContextService { | ||
| // Returns a list of the contexts within the organization | ||
| rpc ListContexts(ListContextsReqeust) returns (ListContextsResponse) { |
There was a problem hiding this comment.
Fix typo in request message name.
The request message name has a typo: ListContextsReqeust should be ListContextsRequest.
- rpc ListContexts(ListContextsReqeust) returns (ListContextsResponse) {
+ rpc ListContexts(ListContextsRequest) returns (ListContextsResponse) {This typo will cause compilation errors and breaks naming conventions.
📝 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.
| rpc ListContexts(ListContextsReqeust) returns (ListContextsResponse) { | |
| rpc ListContexts(ListContextsRequest) returns (ListContextsResponse) { |
🧰 Tools
🪛 Buf (1.55.1)
12-12: RPC request type "ListContextsReqeust" should be named "ListContextsRequest" or "ContextServiceListContextsRequest".
(RPC_REQUEST_STANDARD_NAME)
🤖 Prompt for AI Agents
In proto/brease/context/v1/service.proto at line 12, fix the typo in the RPC
request message name by changing `ListContextsReqeust` to `ListContextsRequest`
to ensure correct compilation and adherence to naming conventions.
1 participant
We'd love to see more contributions
Read how you can contribute to the project.
Summary by CodeRabbit
New Features
Documentation