Added support for conversational search #1133
Conversation
WalkthroughAdded HTTP streaming support via a new HttpRequests.post_stream method and four Client chat-related methods: create_chat_completion (streaming), get_chat_workspaces, get_chat_workspace_settings, and update_chat_workspace_settings. Added unit tests covering streaming, JSON errors, HTTP error propagation, and workspace endpoints. Changes
Sequence Diagram(s)Streaming Chat Completion FlowsequenceDiagram
participant User
participant Client
participant HttpRequests
participant MeiliAPI
rect rgba(200,230,255,0.4)
User->>Client: create_chat_completion(workspace_uid, messages, stream=True)
end
Client->>HttpRequests: post_stream(path="/api/chats/{uid}/completions", body=...)
HttpRequests->>MeiliAPI: POST /api/chats/{uid}/completions (stream=True)
note right of MeiliAPI: Streams lines prefixed with "data:"
MeiliAPI-->>HttpRequests: streaming lines (data: {...}\n, ..., data: "[DONE]\n")
HttpRequests-->>Client: requests.Response (stream)
loop each streamed line
Client->>Client: read line, strip "data:", parse JSON (or raise)
Client-->>User: yield parsed JSON chunk
end
Client->>HttpRequests: close() response
Retrieve / Update Chat Workspaces FlowsequenceDiagram
participant User
participant Client
participant HttpRequests
participant MeiliAPI
User->>Client: get_chat_workspaces(offset?, limit?)
Client->>HttpRequests: get("/api/chats", params)
HttpRequests->>MeiliAPI: GET /api/chats?offset=...&limit=...
MeiliAPI-->>HttpRequests: JSON response
HttpRequests-->>Client: parsed JSON
Client-->>User: return workspaces
User->>Client: update_chat_workspace_settings(uid, settings)
Client->>HttpRequests: patch("/api/chats/{uid}/settings", body)
HttpRequests->>MeiliAPI: PATCH /api/chats/{uid}/settings
MeiliAPI-->>HttpRequests: JSON response
HttpRequests-->>Client: parsed JSON
Client-->>User: return updated settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
🧹 Nitpick comments (3)
meilisearch/_httprequests.py (1)
5-5: Remove unused import.The
Iteratorimport is not used in this file and should be removed as flagged by static analysis.-from typing import Any, Callable, Iterator, List, Mapping, Optional, Sequence, Tuple, Type, Union +from typing import Any, Callable, List, Mapping, Optional, Sequence, Tuple, Type, Unionmeilisearch/client.py (1)
798-869: Excellent streaming chat completion implementation with minor exception chaining issue.The
create_chat_completionmethod is well-designed with proper parameter validation, streaming response handling, JSON parsing, and resource cleanup. The implementation correctly follows the Server-Sent Events (SSE) protocol pattern with "data: " prefixed lines and "[DONE]" completion signal.However, there's a minor issue with exception chaining on line 866 as flagged by static analysis:
- raise MeilisearchCommunicationError(f"Failed to parse chat chunk: {e}") + raise MeilisearchCommunicationError(f"Failed to parse chat chunk: {e}") from eThis preserves the original exception context for better debugging.
tests/client/test_chat_completions.py (1)
3-3: Remove unused imports.Several imports are flagged as unused by static analysis and should be removed to keep the code clean:
-import json from unittest.mock import patch import pytest import requests -import meilisearch from meilisearch.errors import MeilisearchApiError, MeilisearchCommunicationError -from tests import BASE_URL, MASTER_KEYThe
json,meilisearch,BASE_URL, andMASTER_KEYimports are not used in this test file.Also applies to: 9-9, 11-11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
meilisearch/_httprequests.py(2 hunks)meilisearch/client.py(2 hunks)tests/client/test_chat_completions.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/client/test_chat_completions.py (3)
meilisearch/errors.py (2)
MeilisearchApiError(28-51)MeilisearchCommunicationError(54-58)tests/conftest.py (1)
client(15-16)meilisearch/client.py (2)
create_chat_completion(798-868)get_chat_workspaces(870-901)
meilisearch/client.py (3)
meilisearch/_httprequests.py (2)
post_stream(149-213)get(95-96)meilisearch/errors.py (3)
MeilisearchApiError(28-51)MeilisearchCommunicationError(54-58)MeilisearchError(17-25)tests/client/test_chat_completions.py (2)
iter_lines(24-27)close(29-31)
meilisearch/_httprequests.py (2)
tests/client/test_chat_completions.py (1)
raise_for_status(33-36)meilisearch/errors.py (3)
MeilisearchTimeoutError(61-65)MeilisearchCommunicationError(54-58)MeilisearchApiError(28-51)
🪛 Ruff (0.12.2)
tests/client/test_chat_completions.py
3-3: json imported but unused
Remove unused import: json
(F401)
9-9: meilisearch imported but unused
Remove unused import: meilisearch
(F401)
11-11: tests.BASE_URL imported but unused
Remove unused import
(F401)
11-11: tests.MASTER_KEY imported but unused
Remove unused import
(F401)
meilisearch/client.py
16-16: meilisearch.errors.MeilisearchApiError imported but unused
Remove unused import: meilisearch.errors.MeilisearchApiError
(F401)
866-866: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
meilisearch/_httprequests.py
5-5: typing.Iterator imported but unused
Remove unused import: typing.Iterator
(F401)
🔇 Additional comments (4)
meilisearch/_httprequests.py (1)
149-214: Streaming POST method is safely implementedThe
post_streammethod correctly handles body serialization, streaming, and error mapping. Sinceresponseis always assigned beforeraise_for_status()is called, theHTTPErrorhandler cannot encounter an undefinedresponsevariable. No further changes needed—approving these updates.meilisearch/client.py (2)
16-16: Import is correctly used.The static analysis tool incorrectly flags
MeilisearchApiErroras unused. This import is actually used in the docstrings for exception documentation and the error handling logic relies on it being available. The import should remain.
870-902: Clean and straightforward workspace listing implementation.The
get_chat_workspacesmethod follows established patterns in the codebase for pagination handling and query string construction. The implementation is clean and correctly handles optional parameters.tests/client/test_chat_completions.py (1)
14-140: Excellent comprehensive test coverage.The test suite provides thorough coverage of the new chat completion functionality with well-designed test cases:
- MockStreamingResponse: Well-implemented mock that accurately simulates the streaming response behavior
- Happy path testing: Verifies correct streaming, JSON parsing, and response handling
- Error conditions: Tests parameter validation, JSON decode errors, and HTTP error propagation
- Edge cases: Covers both methods with and without optional parameters
- Resource cleanup: Verifies proper response closure
The test structure follows established patterns and uses appropriate mocking strategies. The assertions are specific and verify both the HTTP calls and the returned data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
meilisearch/_httprequests.py (1)
5-5: Remove unused import.The
Iteratortype is imported but not used in this file. Based on the static analysis hint, this import should be removed.-from typing import Any, Callable, Iterator, List, Mapping, Optional, Sequence, Tuple, Type, Union +from typing import Any, Callable, List, Mapping, Optional, Sequence, Tuple, Type, Unionmeilisearch/client.py (1)
798-869: Solid streaming implementation with minor exception chaining issue.The
create_chat_completionmethod is well-implemented with proper SSE parsing, error handling, and resource cleanup. However, there's a minor issue with exception chaining.Line 866 should use proper exception chaining:
except json.JSONDecodeError as e: - raise MeilisearchCommunicationError(f"Failed to parse chat chunk: {e}") + raise MeilisearchCommunicationError(f"Failed to parse chat chunk: {e}") from eThis preserves the original exception context for better debugging.
tests/client/test_chat_completions.py (1)
3-3: Remove unused imports.Several imports are not used in this test file:
json(line 3) - not used anywhere in the filemeilisearch(line 9) - the client fixture provides the client instanceBASE_URLandMASTER_KEY(line 11) - not used in these tests-import json from unittest.mock import patch import pytest import requests -import meilisearch from meilisearch.errors import MeilisearchApiError, MeilisearchCommunicationError -from tests import BASE_URL, MASTER_KEYAlso applies to: 9-9, 11-11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
meilisearch/_httprequests.py(2 hunks)meilisearch/client.py(2 hunks)tests/client/test_chat_completions.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
meilisearch/_httprequests.py (2)
tests/client/test_chat_completions.py (1)
raise_for_status(33-36)meilisearch/errors.py (3)
MeilisearchTimeoutError(61-65)MeilisearchCommunicationError(54-58)MeilisearchApiError(28-51)
tests/client/test_chat_completions.py (4)
meilisearch/_httprequests.py (1)
patch(110-118)meilisearch/errors.py (2)
MeilisearchApiError(28-51)MeilisearchCommunicationError(54-58)tests/conftest.py (1)
client(15-16)meilisearch/client.py (3)
create_chat_completion(798-868)get_chat_workspaces(870-901)update_chat_workspace_settings(924-946)
🪛 Ruff (0.12.2)
meilisearch/client.py
16-16: meilisearch.errors.MeilisearchApiError imported but unused
Remove unused import: meilisearch.errors.MeilisearchApiError
(F401)
866-866: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
meilisearch/_httprequests.py
5-5: typing.Iterator imported but unused
Remove unused import: typing.Iterator
(F401)
tests/client/test_chat_completions.py
3-3: json imported but unused
Remove unused import: json
(F401)
9-9: meilisearch imported but unused
Remove unused import: meilisearch
(F401)
11-11: tests.BASE_URL imported but unused
Remove unused import
(F401)
11-11: tests.MASTER_KEY imported but unused
Remove unused import
(F401)
🔇 Additional comments (13)
meilisearch/_httprequests.py (1)
149-214: Good streaming implementation with one error handling issue.The
post_streammethod follows established patterns and correctly implements streaming POST requests. However, there's an issue with the HTTPError exception handling.In Line 203, the HTTPError exception handler references
responsewhich may not be defined if the exception occurs before the response assignment. This could cause aNameError:except requests.exceptions.HTTPError as err: - raise MeilisearchApiError(str(err), response) from err + # For HTTPError, we need to extract response from the exception + # since response might not be in scope if error occurs during assignment + http_response = getattr(err, 'response', None) + if http_response is not None: + raise MeilisearchApiError(str(err), http_response) from err + else: + raise MeilisearchCommunicationError(str(err)) from errLikely an incorrect or invalid review comment.
meilisearch/client.py (5)
11-11: Iterator import is correctly used.The
Iteratortype is used in the return type annotation of thecreate_chat_completionmethod (Line 804), so this import is necessary despite what static analysis suggests.
16-16: MeilisearchApiError import is correctly used.The
MeilisearchApiErroris used in the docstring documentation (Line 826) and is part of the public API contract, so this import is necessary despite the static analysis hint.
870-902: LGTM!The
get_chat_workspacesmethod is cleanly implemented with proper optional parameter handling and clear query string construction.
903-922: LGTM!The
get_chat_workspace_settingsmethod is straightforward and correctly implemented.
924-947: LGTM!The
update_chat_workspace_settingsmethod correctly uses the PATCH method for updating workspace settings.tests/client/test_chat_completions.py (7)
14-37: Excellent mock implementation for streaming tests.The
MockStreamingResponseclass properly simulates therequests.Responseinterface needed for testing streaming functionality, including proper error simulation and closure tracking.
39-67: Comprehensive test for streaming functionality.This test thoroughly validates the streaming chat completion implementation, including HTTP call verification, chunk parsing, and proper resource cleanup.
69-75: LGTM!This test correctly verifies that the method enforces streaming mode and raises the appropriate error when disabled.
77-89: LGTM!This test properly validates error handling for malformed JSON in the streaming response, ensuring robust error reporting.
91-100: LGTM!This test ensures that HTTP errors from the underlying
post_streammethod are properly propagated to the caller.
102-122: LGTM!This test properly validates the
get_chat_workspacesfunctionality with realistic mock data and correct HTTP call verification.
124-144: LGTM!This test correctly validates the
update_chat_workspace_settingsfunctionality, ensuring proper PATCH request handling and response processing.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
meilisearch/client.py (3)
855-855: Consider URL path validation for security.The endpoint construction directly interpolates
workspace_uidwithout validation, which could potentially allow path traversal if the UID contains malicious characters.Consider adding basic validation or URL encoding:
-endpoint = f"chats/{workspace_uid}/chat/completions" +# Basic validation to ensure workspace_uid doesn't contain path separators +if "/" in workspace_uid or "\\" in workspace_uid: + raise ValueError("Invalid workspace_uid: must not contain path separators") +endpoint = f"chats/{workspace_uid}/chat/completions"Alternatively, use URL encoding if special characters are expected:
+from urllib.parse import quote -endpoint = f"chats/{workspace_uid}/chat/completions" +endpoint = f"chats/{quote(workspace_uid, safe='')}/chat/completions"
905-911: Consider using urllib.parse.urlencode for query string construction.Manual query string construction works but using
urllib.parse.urlencodewould be more consistent with other methods in the codebase and handle edge cases better.+from urllib import parse def get_chat_workspaces( self, *, offset: Optional[int] = None, limit: Optional[int] = None, ) -> Dict[str, Any]: # ... docstring ... - q = [] - if offset is not None: - q.append(f"offset={offset}") - if limit is not None: - q.append(f"limit={limit}") - path = "chats" + ("?" + "&".join(q) if q else "") + params = {} + if offset is not None: + params["offset"] = offset + if limit is not None: + params["limit"] = limit + path = "chats" + ("?" + parse.urlencode(params) if params else "") return self.http.get(path)Note: The
parseimport is already available at the top of the file.
880-885: Consider adding input validation for pagination parameters.The method should validate that offset and limit are non-negative when provided.
def get_chat_workspaces( self, *, offset: Optional[int] = None, limit: Optional[int] = None, ) -> Dict[str, Any]: + if offset is not None and offset < 0: + raise ValueError("offset must be non-negative") + if limit is not None and limit < 1: + raise ValueError("limit must be positive")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
meilisearch/client.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
meilisearch/client.py (3)
meilisearch/_httprequests.py (2)
post_stream(149-213)get(95-96)meilisearch/errors.py (3)
MeilisearchApiError(28-51)MeilisearchCommunicationError(54-58)MeilisearchError(17-25)tests/client/test_chat_completions.py (2)
iter_lines(21-23)close(25-27)
🪛 Ruff (0.12.2)
meilisearch/client.py
27-27: meilisearch.errors.MeilisearchApiError imported but unused
Remove unused import: meilisearch.errors.MeilisearchApiError
(F401)
🔇 Additional comments (2)
meilisearch/client.py (2)
844-846: LGTM!The validation for streaming requirement is appropriate since non-streaming mode is not currently supported by the API.
27-27: MeilisearchApiError import is used and should be retained.Search results show multiple references to
MeilisearchApiErrorthroughoutmeilisearch/client.py, so the import is required and must not be removed. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
meilisearch/client.py (2)
844-855: Add validation for messages/model/stream for robustness (matches prior feedback)Validate non-empty messages with required keys/types, non-empty model, and boolean stream to fail fast on bad inputs.
Apply this diff:
if not stream: # The API currently only supports streaming responses: raise ValueError("Non-streaming chat completions are not supported. Use stream=True.") # Basic security validation (only what's needed) if not workspace_uid: raise ValueError("workspace_uid is required and cannot be empty") if "/" in workspace_uid or "\\" in workspace_uid: raise ValueError("Invalid workspace_uid: must not contain path separators") + # Validate input payload + if not isinstance(messages, list) or len(messages) == 0: + raise ValueError("messages must be a non-empty list") + for i, msg in enumerate(messages): + if not isinstance(msg, Mapping) or "role" not in msg or "content" not in msg: + raise ValueError(f"messages[{i}] must be a mapping with 'role' and 'content' keys") + if not isinstance(msg["role"], str) or not isinstance(msg["content"], str): + raise ValueError(f"messages[{i}]['role'] and ['content'] must be strings") + if not isinstance(model, str) or not model.strip(): + raise ValueError("model name is required and cannot be empty") + if not isinstance(stream, bool): + raise TypeError("stream must be a boolean") + payload = {"model": model, "messages": messages, "stream": True}
969-979: Type-check settings mapping in addition to non-emptyRuntime type check helps catch accidental non-mapping inputs before serialization.
Apply this diff:
if "/" in workspace_uid or "\\" in workspace_uid: raise ValueError("Invalid workspace_uid: must not contain path separators") - if not settings: - raise ValueError("settings cannot be empty") + if not isinstance(settings, Mapping): + raise TypeError("settings must be a mapping/dict") + if not settings: + raise ValueError("settings cannot be empty") return self.http.patch(f"chats/{workspace_uid}/settings", body=settings)
🧹 Nitpick comments (4)
meilisearch/client.py (4)
27-27: Remove unused MeilisearchApiError import (Ruff F401 / Pylint W0611)Static analysis flags this as unused. It’s only referenced in docstrings, so the import isn’t required.
Apply this diff:
-from meilisearch.errors import MeilisearchApiError, MeilisearchCommunicationError, MeilisearchError +from meilisearch.errors import MeilisearchCommunicationError, MeilisearchError
868-880: Harden streaming decode path against UnicodeDecodeErrorIf the server emits invalid UTF-8,
.decode()will raise and skip your nice JSON error handling. Catch and wrap as communication error.Apply this diff:
- line = raw_line.decode("utf-8") + try: + line = raw_line.decode("utf-8") + except UnicodeDecodeError as e: + raise MeilisearchCommunicationError( + f"Failed to decode chat chunk as UTF-8: {e}" + ) from e if line.startswith("data: "):
909-915: Validate offset/limit types and boundsAvoid sending invalid pagination params to the API by validating integers and ranges.
Apply this diff:
- params = {} - if offset is not None: - params["offset"] = offset - if limit is not None: - params["limit"] = limit + params: Dict[str, Any] = {} + if offset is not None: + if not isinstance(offset, int) or offset < 0: + raise ValueError("offset must be a non-negative integer") + params["offset"] = offset + if limit is not None: + if not isinstance(limit, int) or limit <= 0: + raise ValueError("limit must be a positive integer") + params["limit"] = limit path = "chats" + ("?" + parse.urlencode(params) if params else "") return self.http.get(path)
857-858: Consider adding a config.paths entry for 'chats' for consistencyMost endpoints use self.config.paths.*; these hard-coded "chats" strings could be centralized similarly for consistency and easier future changes.
Also applies to: 914-915, 943-944, 978-979
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
meilisearch/client.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
meilisearch/client.py (3)
meilisearch/_httprequests.py (4)
HttpRequests(19-227)post_stream(149-213)get(95-96)patch(110-118)meilisearch/errors.py (3)
MeilisearchApiError(28-51)MeilisearchCommunicationError(54-58)MeilisearchError(17-25)tests/client/test_chat_completions.py (2)
iter_lines(21-23)close(25-27)
🪛 Ruff (0.12.2)
meilisearch/client.py
27-27: meilisearch.errors.MeilisearchApiError imported but unused
Remove unused import: meilisearch.errors.MeilisearchApiError
(F401)
🪛 GitHub Actions: Tests
meilisearch/client.py
[warning] 27-27: Pylint warning: Unused import 'MeilisearchApiError' from meilisearch.errors (W0611).
🔇 Additional comments (3)
meilisearch/client.py (3)
11-22: Typing imports update looks goodBrings in Iterator for the streaming method; all imported symbols appear used. No concerns.
937-944: LGTM: consistent workspace_uid validation and path usageValidation mirrors create_chat_completion and prevents path traversal. Implementation is clean.
813-826: No change required:modelparameter is mandatory
According to the official Meilisearch Chat Completions API docs, themodelfield is marked as Required in every request and cannot be omitted in favor of a workspace default. The existing signature (model: str = "gpt-3.5-turbo") is correct and should remain unchanged.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
meilisearch/client.py (7)
27-31: Fix lints: remove unused MeilisearchApiError importMeilisearchApiError isn’t used in this file (only referenced in docstrings). This triggers Ruff F401 and Pylint W0611. Drop the unused import and the trailing per-line disable (it currently doesn’t suppress the warning on the multiline import anyway).
Apply:
-from meilisearch.errors import ( - MeilisearchApiError, - MeilisearchCommunicationError, - MeilisearchError, -) # pylint: disable=unused-import +from meilisearch.errors import ( + MeilisearchCommunicationError, + MeilisearchError, +)
848-857: Strengthen input validation for messages/model/streamYou already validate workspace_uid. Extend validation to catch common user errors earlier and avoid sending malformed payloads.
Apply:
# Basic security validation (only what's needed) if not workspace_uid: raise ValueError("workspace_uid is required and cannot be empty") if "/" in workspace_uid or "\\" in workspace_uid: raise ValueError("Invalid workspace_uid: must not contain path separators") + # Validate messages + if not isinstance(messages, list) or not messages: + raise ValueError("messages must be a non-empty list of dicts with 'role' and 'content'") + for i, m in enumerate(messages): + if not isinstance(m, Mapping) or "role" not in m or "content" not in m: + raise ValueError(f"messages[{i}] must be a mapping with 'role' and 'content' keys") + + # Validate model + if not isinstance(model, str) or not model.strip(): + raise ValueError("model name is required and cannot be empty") + + # Validate stream flag type + if not isinstance(stream, bool): + raise ValueError("stream must be a boolean")
861-862: URL-encode the workspace_uid when building the pathEven with slash/backslash checks, encoding the path segment is safer and avoids surprises with special characters like spaces, “?”, or “%”.
Apply:
- endpoint = f"chats/{workspace_uid}/chat/completions" + encoded_uid = parse.quote(workspace_uid, safe="") + endpoint = f"chats/{encoded_uid}/chat/completions"
867-874: Leverage decode_unicode in iter_lines to simplify decodingrequests.Response.iter_lines can decode for you. This reduces branching and byte/string juggling.
Apply:
- # Iterate over the streaming response lines - for raw_line in response.iter_lines(): - if raw_line is None or raw_line == b"": - continue - - line = raw_line.decode("utf-8") + # Iterate over the streaming response lines + for line in response.iter_lines(decode_unicode=True): + if not line: + continue
888-920: Optional: validate pagination inputs (offset/limit)Guardrails on pagination parameters avoid server-side 400s and make misuse fail fast.
Apply:
params = {} - if offset is not None: + if offset is not None: + if not isinstance(offset, int) or offset < 0: + raise ValueError("offset must be a non-negative integer") params["offset"] = offset - if limit is not None: + if limit is not None: + if not isinstance(limit, int) or limit <= 0: + raise ValueError("limit must be a positive integer") params["limit"] = limit path = "chats" + ("?" + parse.urlencode(params) if params else "") return self.http.get(path)
941-947: URL-encode workspace_uid for settings GET pathConsistent with chat completions, encode the UID to avoid malformed URLs on special characters.
Apply:
- return self.http.get(f"chats/{workspace_uid}/settings") + encoded_uid = parse.quote(workspace_uid, safe="") + return self.http.get(f"chats/{encoded_uid}/settings")
973-983: URL-encode workspace_uid for settings PATCH pathSame rationale as above; improves robustness.
Apply:
- return self.http.patch(f"chats/{workspace_uid}/settings", body=settings) + encoded_uid = parse.quote(workspace_uid, safe="") + return self.http.patch(f"chats/{encoded_uid}/settings", body=settings)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
meilisearch/client.py(2 hunks)tests/client/test_chat_completions.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/client/test_chat_completions.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
meilisearch/client.py (3)
meilisearch/_httprequests.py (4)
HttpRequests(19-227)post_stream(149-213)get(95-96)patch(110-118)meilisearch/errors.py (3)
MeilisearchApiError(28-51)MeilisearchCommunicationError(54-58)MeilisearchError(17-25)tests/client/test_chat_completions.py (2)
iter_lines(21-23)close(25-27)
🪛 Ruff (0.12.2)
meilisearch/client.py
28-28: meilisearch.errors.MeilisearchApiError imported but unused
Remove unused import: meilisearch.errors.MeilisearchApiError
(F401)
🪛 GitHub Actions: Tests
meilisearch/client.py
[warning] 27-27: Pylint: Unused import 'MeilisearchApiError' from meilisearch.errors (W0611).
🔇 Additional comments (1)
meilisearch/client.py (1)
11-22: LGTM: typing imports (including Iterator) are appropriateThe additions are consistent with the new streaming API surface.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
meilisearch/client.py (1)
851-866: Strengthen input validation and safely encode workspace_uid in path
- Add minimal validation for model and messages for better DX and earlier failures.
- Quote workspace_uid to prevent path confusion with reserved characters (e.g., ?, #, %), not just slashes.
if not stream: # The API currently only supports streaming responses: raise ValueError("Non-streaming chat completions are not supported. Use stream=True.") # Basic security validation (only what's needed) if not workspace_uid: raise ValueError("workspace_uid is required and cannot be empty") if "/" in workspace_uid or "\\" in workspace_uid: raise ValueError("Invalid workspace_uid: must not contain path separators") + # Basic payload validation + if not isinstance(model, str) or not model.strip(): + raise ValueError("model is required and cannot be empty") + if not messages or not isinstance(messages, Sequence): + raise ValueError("messages must be a non-empty sequence") + + # Optionally validate minimal schema of each message + for i, m in enumerate(messages): + if not isinstance(m, Mapping): + raise ValueError(f"messages[{i}] must be a mapping") + if "role" not in m or "content" not in m: + raise ValueError(f"messages[{i}] must contain 'role' and 'content'") + if not isinstance(m["role"], str) or not isinstance(m["content"], str): + raise ValueError(f"messages[{i}]['role'|'content'] must be strings") + payload = {"model": model, "messages": messages, "stream": True} # Construct the URL for the chat completions route. - endpoint = f"chats/{workspace_uid}/chat/completions" + sanitized_uid = parse.quote(workspace_uid, safe="") + endpoint = f"chats/{sanitized_uid}/chat/completions" # Initiate the HTTP POST request in streaming mode. response = self.http.post_stream(endpoint, body=payload)If you prefer to avoid repeating validation/quoting across methods, consider a small helper:
# Place inside Client (e.g., near other @staticmethods) @staticmethod def _sanitize_workspace_uid(uid: str) -> str: if not uid: raise ValueError("workspace_uid is required and cannot be empty") if "/" in uid or "\\" in uid: raise ValueError("Invalid workspace_uid: must not contain path separators") return parse.quote(uid, safe="")
🧹 Nitpick comments (6)
meilisearch/client.py (6)
27-31: Nit: Drop the noqa; aliasing approach is fine
- The class-level alias for MeilisearchApiError is acceptable for docstring references.
- The import is actually used (via the alias and direct references), so the inline
# noqa: F401is likely unnecessary.Apply this tiny cleanup:
-from meilisearch.errors import ( # noqa: F401 +from meilisearch.errors import ( MeilisearchApiError, MeilisearchCommunicationError, MeilisearchError, )Also applies to: 46-48
816-822: Broaden messages type for forward-compatibilityConsider allowing additional keys and collection types to match evolving chat payloads without forcing casts for users.
- messages: List[Dict[str, str]], + messages: Sequence[Mapping[str, Any]],
881-887: Include a small data preview on JSON parse errors for faster debuggingAugment the error message with a short preview of the problematic payload.
- except json.JSONDecodeError as e: - raise MeilisearchCommunicationError( - f"Failed to parse chat chunk: {e}" - ) from e + except json.JSONDecodeError as e: + raw_preview = data[:200] + raise MeilisearchCommunicationError( + f"Failed to parse chat chunk: {e}. data preview='{raw_preview}'" + ) from e
916-922: Validate offset/limit to catch bad inputs earlyGuard against negative or invalid values before building the request.
- params = {} + params = {} + if offset is not None and (not isinstance(offset, int) or offset < 0): + raise ValueError("offset must be a non-negative integer") + if limit is not None and (not isinstance(limit, int) or limit <= 0): + raise ValueError("limit must be a positive integer") if offset is not None: params["offset"] = offset if limit is not None: params["limit"] = limit path = "chats" + ("?" + parse.urlencode(params) if params else "") return self.http.get(path)
944-951: URL-encode workspace_uid to avoid path issuesSafely encode the UID to avoid accidental query/fragment injection or path corruption.
# Basic security validation (only what's needed) if not workspace_uid: raise ValueError("workspace_uid is required and cannot be empty") if "/" in workspace_uid or "\\" in workspace_uid: raise ValueError("Invalid workspace_uid: must not contain path separators") - return self.http.get(f"chats/{workspace_uid}/settings") + sanitized_uid = parse.quote(workspace_uid, safe="") + return self.http.get(f"chats/{sanitized_uid}/settings")
976-985: Same hardening here: URL-encode workspace_uid before building the path# Basic security validation (only what's needed) if not workspace_uid: raise ValueError("workspace_uid is required and cannot be empty") if "/" in workspace_uid or "\\" in workspace_uid: raise ValueError("Invalid workspace_uid: must not contain path separators") + sanitized_uid = parse.quote(workspace_uid, safe="") if not settings: raise ValueError("settings cannot be empty") - return self.http.patch(f"chats/{workspace_uid}/settings", body=settings) + return self.http.patch(f"chats/{sanitized_uid}/settings", body=settings)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
meilisearch/client.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
meilisearch/client.py (3)
meilisearch/_httprequests.py (4)
HttpRequests(19-227)post_stream(149-213)get(95-96)patch(110-118)meilisearch/errors.py (3)
MeilisearchApiError(28-51)MeilisearchCommunicationError(54-58)MeilisearchError(17-25)tests/client/test_chat_completions.py (2)
iter_lines(21-23)close(25-27)
🔇 Additional comments (1)
meilisearch/client.py (1)
11-22: Typing imports look appropriate for streaming and new endpointsAdding Iterator, Mapping, MutableMapping, Optional, Sequence, etc., aligns with the new APIs’ signatures and usage.
3 participants
Pull Request
Adds first-class support for the experimental Chat Completions API
API reference: https://www.meilisearch.com/docs/reference/api/chats
Related issue
#1130
What does this PR do?
New methods are:
Summary by CodeRabbit
New Features
Bug Fixes
Tests