Feature/pagination functionality #107
Conversation
WalkthroughThis update introduces pagination support across both client and server components. New methods for retrieving resources, resource templates, prompts, and tools by page are added to various client interfaces. A generic helper function is implemented to handle paginated requests uniformly. On the server side, pagination functionality is integrated by adding a configurable pagination limit and a generic pagination function to segment response data. Corresponding changes in the server tests ensure that pagination logic is exercised, including dynamic cursor generation and proper limit settings. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Nitpick comments (9)
server/server_test.go (1)
442-449: Validation of cursor-based resource listing
Adding a test for"List resources with cursor"properly verifies the new pagination logic. Consider adding additional tests for boundary cursor cases (e.g., an empty string cursor or a cursor beyond the last element).server/server.go (2)
168-173: Pagination limit setter
WithPaginationLimitcleanly adds a configurable page size. Consider validatinglimit > 0to avoid edge cases like zero or negative limits.
571-588: Sorted resources before pagination
Sorting byNamebefore callinglistByPaginationis consistent with the reflection-based approach. Consider clarifying behavior when multiple resources share the sameName.client/client.go (2)
112-113:sendRequestin the interface
Exposing an internal request method is useful for generic pagination logic. Ensure correct usage to avoid potential security or logging blind spots.
115-130: GenericlistByPagefunction
This function centralizes pagination logic on the client side. Consider adding stronger error-checking or logging if the server response does not conform to expected structures.client/sse.go (2)
461-484: Consider memory optimization for large result setsThe current implementation accumulates all results across pages into a single slice. For very large result sets across many pages, this could potentially lead to high memory usage.
Consider offering a streaming alternative or callback-based approach for cases where clients might need to process large datasets:
func (c *SSEMCPClient) ListResourceTemplates( ctx context.Context, request mcp.ListResourceTemplatesRequest, ) (*mcp.ListResourceTemplatesResult, error) { + // For large result sets, consider using ListResourceTemplatesStream result, err := c.ListResourceTemplatesByPage(ctx, request) if err != nil { return nil, err } for result.NextCursor != "" { select { case <-ctx.Done(): return nil, ctx.Err() default: request.Params.Cursor = result.NextCursor newPageRes, err := c.ListResourceTemplatesByPage(ctx, request) if err != nil { return nil, err } result.ResourceTemplates = append(result.ResourceTemplates, newPageRes.ResourceTemplates...) result.NextCursor = newPageRes.NextCursor } } return result, nil }
573-596: Consider adding documentation about pagination behaviorWhile the implementation is solid, adding a comment explaining the default behavior (fetching all pages) would be helpful for users of this API.
+// ListTools retrieves all tools by automatically handling pagination. +// It fetches each page sequentially and combines the results until all pages +// are retrieved or the context is cancelled. func (c *SSEMCPClient) ListTools( ctx context.Context, request mcp.ListToolsRequest, ) (*mcp.ListToolsResult, error) {client/stdio.go (2)
351-374: Consider extracting common pagination logicThe pagination logic is duplicated across multiple methods and client implementations. This could potentially be extracted to reduce code duplication.
Consider creating a utility function that handles the pagination logic generically:
// fetchAllPages fetches all pages for a given paginated request func fetchAllPages[T any, R any]( ctx context.Context, fetchPage func(context.Context, T) (*R, error), request T, getCursor func(*R) string, setCursor func(*T, string), appendResults func(*R, *R), ) (*R, error) { result, err := fetchPage(ctx, request) if err != nil { return nil, err } for getCursor(result) != "" { select { case <-ctx.Done(): return nil, ctx.Err() default: setCursor(&request, getCursor(result)) newPageRes, err := fetchPage(ctx, request) if err != nil { return nil, err } appendResults(result, newPageRes) // Update cursor for next iteration r := result setCursor((*T)(nil), getCursor(newPageRes)) } } return result, nil }This is a more advanced refactoring that could be considered in a future PR.
464-487: Consider adding documentation about pagination behaviorWhile the implementation is solid, adding a comment explaining the default behavior (fetching all pages) would be helpful for users of this API.
+// ListTools retrieves all tools by automatically handling pagination. +// It fetches each page sequentially and combines the results until all pages +// are retrieved or the context is cancelled. func (c *StdioMCPClient) ListTools( ctx context.Context, request mcp.ListToolsRequest, ) (*mcp.ListToolsResult, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/client.go(5 hunks)client/sse.go(3 hunks)client/stdio.go(3 hunks)server/server.go(8 hunks)server/server_test.go(3 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
server/server_test.go (2)
mcp/types.go (1)
JSONRPCMessage(75-75)server/server.go (1)
WithPaginationLimit(169-173)
server/server.go (3)
mcp/types.go (7)
Cursor(87-87)Resource(487-502)INVALID_PARAMS(225-225)ListResourcesResult(406-409)PaginatedResult(388-394)ResourceTemplate(506-523)ListResourceTemplatesResult(419-422)mcp/prompts.go (2)
Prompt(43-51)ListPromptsResult(13-16)mcp/tools.go (2)
Tool(69-78)ListToolsResult(19-22)
client/client.go (1)
mcp/types.go (6)
ListResourcesRequest(400-402)ListResourcesResult(406-409)ListResourceTemplatesRequest(413-415)ListResourceTemplatesResult(419-422)PaginatedRequest(379-386)Params(104-104)
client/stdio.go (2)
mcp/types.go (7)
ListResourcesRequest(400-402)ListResourcesResult(406-409)PaginatedRequest(379-386)Params(104-104)Cursor(87-87)ListResourceTemplatesRequest(413-415)ListResourceTemplatesResult(419-422)mcp/tools.go (2)
ListToolsRequest(13-15)ListToolsResult(19-22)
client/sse.go (3)
mcp/types.go (7)
ListResourcesRequest(400-402)ListResourcesResult(406-409)PaginatedRequest(379-386)Params(104-104)Cursor(87-87)ListResourceTemplatesRequest(413-415)ListResourceTemplatesResult(419-422)mcp/prompts.go (2)
ListPromptsRequest(7-9)ListPromptsResult(13-16)mcp/tools.go (2)
ListToolsRequest(13-15)ListToolsResult(19-22)
🔇 Additional comments (25)
server/server_test.go (3)
5-5: Use of "encoding/base64" import
Importing"encoding/base64"is appropriate for generating an encoded cursor.
434-434: Dynamic cursor generation
Defining a base64-encoded cursor for"My Resource"helps realistically test pagination. This approach is clear and maintainable.
1129-1129: Fixed pagination limit
WithPaginationLimit(2)is a good default for testing. If appropriate, consider adding tests for edge cases (e.g., zero limit, large limit) to ensure robust coverage.server/server.go (4)
6-6: New import "encoding/base64"
This is necessary for decoding/encoding the cursor. Looks good.
603-620: Resource templates pagination
The same reflection-based pagination is correctly applied to templates. Keep an eye on potential collisions in template names if they are not guaranteed to be unique.
703-717: Prompts pagination
Similarly, sorting prompts byNameand then paginating is consistent. If future prompt types contain noNamefield, ensure consistent handling.
770-778: Tools pagination
Pagination logic for tools looks aligned with the rest of the code. Watch out for collisions if multiple tools share the sameName.client/client.go (5)
6-7: New imports "encoding/json" and "fmt"
These imports are necessary for unmarshalling responses and constructing formatted errors.
23-28: ListResourcesByPage method
Introducing a manual paginated listing makes sense given the new server pagination. Ensure adequate client-side testing of boundary cursors.
35-41: ListResourceTemplatesByPage method
Provides consistent pagination for resource templates. Well aligned with the server.
61-66: ListPromptsByPage method
Good addition for prompt pagination on the client side. Matches the newly introduced server logic.
79-84: ListToolsByPage method
Keeps the client interface consistent, ensuring all resource types support pagination.client/sse.go (6)
413-423: Good implementation of pagination supportThe new
ListResourcesByPagemethod properly implements the pagination functionality by leveraging a generic helper. This allows clients to fetch resources one page at a time when needed.
425-448: Well-structured pagination handling with automatic cursor managementThis implementation efficiently handles multi-page results while properly managing context cancellation. The method correctly:
- Fetches initial page results
- Checks for additional pages via NextCursor
- Updates the cursor for subsequent requests
- Concatenates results across pages
- Handles context cancellation appropriately
450-459: Consistent pagination implementation for resource templatesThe implementation follows the same pattern as the resources pagination, maintaining consistency across the codebase.
514-523: Consistent pagination implementation for promptsThe implementation follows the same pattern as the other pagination methods, maintaining consistency across the codebase.
525-548: Good error handling and context awarenessThe implementation properly checks for errors at each step and respects context cancellation, allowing for clean termination of long-running pagination operations.
562-571: Consistent pagination implementation for toolsThe implementation follows the same pattern as the other pagination methods, maintaining consistency across the codebase.
client/stdio.go (7)
303-313: Good implementation of pagination supportThe new
ListResourcesByPagemethod properly implements the pagination functionality by leveraging a generic helper. Implementation is consistent with the SSEMCPClient version.
315-338: Well-structured pagination handling with automatic cursor managementThis implementation efficiently handles multi-page results while properly managing context cancellation, following the same pattern as in the SSEMCPClient implementation.
340-349: Consistent pagination implementation for resource templatesThe implementation follows the same pattern as the resources pagination, maintaining consistency across both client implementations.
405-414: Consistent pagination implementation for promptsThe implementation follows the same pattern as the other pagination methods, maintaining consistency across the codebase.
416-439: Good error handling and context awarenessThe implementation properly checks for errors at each step and respects context cancellation, allowing for clean termination of long-running pagination operations.
453-462: Consistent pagination implementation for toolsThe implementation follows the same pattern as the other pagination methods, maintaining consistency across the codebase.
315-487:✅ Verification successful
Verify implementation with usage tests
The pagination implementation appears correct, but it would be good to verify it works correctly with various test scenarios like empty results, single page results, and multi-page results.
Run the following script to check if there are tests covering the pagination functionality:
🏁 Script executed:
#!/bin/bash # Check for tests that verify pagination functionality echo "Checking for pagination tests in client implementations..." rg -A 3 -B 3 "Test.*Pagination|test.*pagination" --glob "**/*_test.go" echo "Checking for tests that might validate cursor handling..." rg -A 3 -B 3 "cursor|Cursor" --glob "**/*_test.go"Length of output: 2259
Pagination Implementation Verified by Tests
The existing tests in
server/server_test.go(specifically inTestMCPServer_HandlePagination) confirm that the pagination functionality works correctly for different scenarios (empty results, single page, and multi-page responses). No further changes are necessary.
|
@ezynda3 There's a race problem of server/stdio.go readNextLine function. |
* add transport layer interface * universal client * impl sse & stdio transport based on the original client * refactor old client to provide compibility * rename * remove old client types * add test for stdio transport * rename 'done' to 'closed', to distinguish with ctx.Done * add cancelSSEStream for better handling of close * fix connection leak when start timeout * avoid multiple starting * use atomic for closed to be more natural compared to started * fix leak of timer * Create sse_test.go * enforce test * add comment * sse: add custom header in start request * update comment * comment * cover #88 * cover #107 * fix demo sse server in race test
add pagination functionality: https://spec.modelcontextprotocol.io/specification/2025-03-26/server/utilities/pagination/
Summary by CodeRabbit