Fix get-documents tool to return JSON instead of Python objects #33
Conversation
Fixes #16: The get-documents tool was returning Python object string representations instead of proper JSON-formatted data. Changes: - Fix documents.py to properly serialize DocumentsResults and Document objects - Add default values for offset (0) and limit (20) to fix None parameter issues - Improve JSON serializer to handle Meilisearch model objects recursively - Add comprehensive tests for JSON serialization behavior - Update existing test to handle async index creation timing The fix ensures that: - get-documents returns valid JSON instead of object representations - Document fields are properly accessible in the response - Empty indexes return valid JSON structure - Parameter validation prevents API errors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
WalkthroughThe changes update the document retrieval and serialization logic to ensure that the "get-documents" tool returns properly formatted JSON rather than Python object representations. The code now processes Meilisearch model objects into dictionaries, serializes them as JSON, and includes extensive new tests to verify correct JSON output and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DocumentManager
participant Meilisearch
Client->>Server: Call "get-documents" tool
Server->>DocumentManager: get_documents(index_uid, offset, limit, fields)
DocumentManager->>Meilisearch: Retrieve documents
Meilisearch-->>DocumentManager: DocumentsResults object
DocumentManager->>DocumentManager: Convert to dict, process results
DocumentManager-->>Server: Dict with results as list of dicts
Server->>Server: Serialize to JSON
Server-->>Client: JSON-formatted response
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/meilisearch_mcp/documents.py (1)
21-56: Well-implemented solution for parameter handling and JSON serialization.The code correctly:
- Excludes
Noneparameters to avoid API errors- Handles Meilisearch model objects by extracting data from their
__dict__attributes- Intelligently looks for private attributes that might contain the actual document data
Consider addressing the static analysis hint by removing the
elseafterreturnat line 55:return result_dict - else: - return result + return result🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 33-56: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
tests/test_mcp_client.py (1)
682-682: Remove redundant json imports.The
jsonmodule is already imported at the module level (line 12):-import json try: parsed_data = json.loads(json_part)Also applies to: 757-757
🧰 Tools
🪛 Ruff (0.11.9)
682-682: Redefinition of unused
jsonfrom line 12Remove definition:
json(F811)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/meilisearch_mcp/documents.py(1 hunks)src/meilisearch_mcp/server.py(2 hunks)tests/test_mcp_client.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/meilisearch_mcp/server.py (1)
src/meilisearch_mcp/documents.py (1)
get_documents(11-59)
tests/test_mcp_client.py (1)
src/meilisearch_mcp/server.py (2)
create_server(27-29)cleanup(525-528)
🪛 Pylint (3.3.7)
src/meilisearch_mcp/documents.py
[refactor] 33-56: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
tests/test_mcp_client.py
[refactor] 308-308: Too many public methods (25/20)
(R0904)
🪛 Ruff (0.11.9)
tests/test_mcp_client.py
374-374: Redefinition of unused asyncio from line 10
Remove definition: asyncio
(F811)
501-501: Local variable tasks_result is assigned to but never used
Remove assignment to unused variable tasks_result
(F841)
656-656: Redefinition of unused asyncio from line 10
Remove definition: asyncio
(F811)
682-682: Redefinition of unused json from line 12
Remove definition: json
(F811)
708-708: Redefinition of unused asyncio from line 10
Remove definition: asyncio
(F811)
738-738: Redefinition of unused asyncio from line 10
Remove definition: asyncio
(F811)
757-757: Redefinition of unused json from line 12
Remove definition: json
(F811)
🪛 GitHub Actions: Test and Lint
tests/test_mcp_client.py
[error] 377-377: Test failure in test_get_index_metrics_tool: AssertionError due to 'Index metrics:' not found in error message indicating index_not_found error from Meilisearch API.
[error] 450-450: Test failure in test_search_tool_with_index: AssertionError due to expected search results string not found; Meilisearch API returned index_not_found error.
[error] 477-477: Test failure in test_get_settings_tool: AssertionError due to 'Current settings:' not found in error message indicating index_not_found error from Meilisearch API.
🔇 Additional comments (3)
src/meilisearch_mcp/server.py (2)
21-23: LGTM! Clean solution for serializing Meilisearch model objects.The implementation correctly handles objects with
__dict__attributes, enabling proper JSON serialization of Meilisearch model objects. This aligns well with the corresponding changes indocuments.py.
324-336: Good fix for both the None parameter issue and JSON serialization.The implementation correctly:
- Provides sensible defaults for
offsetandlimitto prevent API errors (fixing issue #17)- Uses the enhanced
json_serializerto properly format the documents as JSON (fixing issue #16)The response format is consistent with other tools in the codebase.
tests/test_mcp_client.py (1)
637-691: Excellent test coverage for the JSON serialization fix.This test effectively verifies that:
- Python object representations (
<meilisearch.models.document.DocumentsResults object at...>) are not present- The response is valid JSON that can be parsed
- The structure contains expected fields
The test directly addresses issue #16.
🧰 Tools
🪛 Ruff (0.11.9)
656-656: Redefinition of unused
asynciofrom line 10Remove definition:
asyncio(F811)
682-682: Redefinition of unused
jsonfrom line 12Remove definition:
json(F811)
| import asyncio | ||
| await asyncio.sleep(0.5) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant asyncio imports.
The asyncio module is already imported at the module level (line 10). Remove these redundant imports:
-import asyncio
await asyncio.sleep(0.5)Also applies to: 656-657, 708-709, 738-739
🧰 Tools
🪛 Ruff (0.11.9)
374-374: Redefinition of unused asyncio from line 10
Remove definition: asyncio
(F811)
🤖 Prompt for AI Agents
In tests/test_mcp_client.py at lines 374-375 (and similarly at 656-657, 708-709,
738-739), remove the redundant local imports of the asyncio module since it is
already imported at the module level on line 10. Simply delete the lines
containing "import asyncio" in these locations to avoid unnecessary duplicate
imports.
| """Test get-task tool""" | ||
| # First get tasks to find a valid task UID | ||
| tasks_result = await self.simulate_tool_call(server, "get-tasks", {"limit": 1}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable.
The tasks_result variable is assigned but never used:
-# First get tasks to find a valid task UID
-tasks_result = await self.simulate_tool_call(server, "get-tasks", {"limit": 1})
+# Note: Using a fixed task UID for testing📝 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.
| """Test get-task tool""" | |
| # First get tasks to find a valid task UID | |
| tasks_result = await self.simulate_tool_call(server, "get-tasks", {"limit": 1}) | |
| """Test get-task tool""" | |
| # Note: Using a fixed task UID for testing |
🧰 Tools
🪛 Ruff (0.11.9)
501-501: Local variable tasks_result is assigned to but never used
Remove assignment to unused variable tasks_result
(F841)
🤖 Prompt for AI Agents
In tests/test_mcp_client.py around lines 499 to 501, the variable tasks_result
is assigned the result of simulate_tool_call but never used afterward. Remove
the assignment to tasks_result and directly await the simulate_tool_call without
storing its result to eliminate the unused variable.
| async def test_get_index_metrics_tool(self, server): | ||
| """Test get-index-metrics tool""" | ||
| # First create a test index | ||
| import time | ||
| test_index = f"test_metrics_{int(time.time() * 1000)}" | ||
| await self.simulate_tool_call(server, "create-index", {"uid": test_index}) | ||
|
|
||
| result = await self.simulate_tool_call(server, "get-index-metrics", { | ||
| "indexUid": test_index | ||
| }) | ||
|
|
||
| assert len(result) == 1 | ||
| assert result[0].type == "text" | ||
| assert "Index metrics:" in result[0].text | ||
|
|
There was a problem hiding this comment.
Add proper wait for index creation to fix test failures.
The pipeline failures indicate that the index is not ready when get-index-metrics is called. Add a wait after index creation:
await self.simulate_tool_call(server, "create-index", {"uid": test_index})
+
+# Wait for index to be ready
+import asyncio
+await asyncio.sleep(1.0)
result = await self.simulate_tool_call(server, "get-index-metrics", {📝 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.
| async def test_get_index_metrics_tool(self, server): | |
| """Test get-index-metrics tool""" | |
| # First create a test index | |
| import time | |
| test_index = f"test_metrics_{int(time.time() * 1000)}" | |
| await self.simulate_tool_call(server, "create-index", {"uid": test_index}) | |
| result = await self.simulate_tool_call(server, "get-index-metrics", { | |
| "indexUid": test_index | |
| }) | |
| assert len(result) == 1 | |
| assert result[0].type == "text" | |
| assert "Index metrics:" in result[0].text | |
| async def test_get_index_metrics_tool(self, server): | |
| """Test get-index-metrics tool""" | |
| # First create a test index | |
| import time | |
| test_index = f"test_metrics_{int(time.time() * 1000)}" | |
| await self.simulate_tool_call(server, "create-index", {"uid": test_index}) | |
| # Wait for index to be ready | |
| import asyncio | |
| await asyncio.sleep(1.0) | |
| result = await self.simulate_tool_call(server, "get-index-metrics", { | |
| "indexUid": test_index | |
| }) | |
| assert len(result) == 1 | |
| assert result[0].type == "text" | |
| assert "Index metrics:" in result[0].text |
🤖 Prompt for AI Agents
In tests/test_mcp_client.py around lines 350 to 364, the test calls
get-index-metrics immediately after creating the index, causing failures because
the index is not ready. Fix this by adding an appropriate wait or retry
mechanism after the create-index call to ensure the index is fully created
before calling get-index-metrics. This can be done by polling the index status
or adding a short delay before proceeding.
Summary
Fixes #16: The
get-documentstool was returning Python object string representations instead of proper JSON-formatted data, making it unusable for accessing document content.Changes Made
🔧 Core Fix
get_documents()to properly serializeDocumentsResultsand individualDocumentobjects to JSON-compatible dicts__dict__attributesoffset(0) andlimit(20) to preventNoneparameter API errors🧪 Comprehensive Testing
TestGetDocumentsJsonSerializationtest suite with 3 focused tests:test_get_documents_returns_json_not_object: Ensures no Python object representations in responsetest_get_documents_contains_expected_document_fields: Verifies actual document data is accessibletest_get_documents_empty_index_returns_proper_json: Tests edge case with empty indexesBefore vs After
Before (Issue #16):
After (Fixed):
Test Results
All new tests pass, demonstrating:
Related Issues
This PR also resolves parameter handling issues mentioned in #17 by providing sensible defaults for
offsetandlimitparameters.Closes #16
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests