Add Or Update Networks #1097
Add Or Update Networks #1097
Conversation
Copilot changes for typo Co-authored-by: Copilot <[email protected]>
WalkthroughThis update introduces experimental support for remote federated search requests in the Meilisearch client. It adds methods to manage remote networks, extends multi-search functionality to accept federation options, and provides new tests and fixtures to validate network federation and configuration. Constants and documentation are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Client as Meilisearch Client
participant API as Meilisearch API
Test->>Client: add_or_update_networks(networks_dict)
Client->>API: PATCH /network with networks_dict
API-->>Client: Updated networks info
Client-->>Test: Updated networks info
Test->>Client: multi_search([{"federationOptions": {...}}])
Client->>API: POST /multi-search with federationOptions
API-->>Client: Federated search results
Client-->>Test: Federated search results
Assessment against linked issues
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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: 1
♻️ Duplicate comments (1)
meilisearch/client.py (1)
783-796: Typo in the docstring.There is a typo in the word "containeing" in the docstring - it should be "containing".
🧹 Nitpick comments (2)
tests/client/test_client_network.py (1)
7-12: Testing approach could be more comprehensiveWhile the basic test for
get_all_networksconfirms the response type, consider adding more specific assertions about the expected structure of the response.def test_get_all_networks(client): """Tests get all network in MS""" response = client.get_all_networks() assert isinstance(response, dict) + assert "self" in response + assert "remotes" in response + assert isinstance(response["remotes"], dict)tests/client/test_client_multi_search_meilisearch.py (1)
80-111: Well-structured test for network federation functionality.This test correctly validates the integration between multi-search and the new network federation feature. It properly:
- Uses the enable_network_options fixture
- Sets up network configurations
- Performs a federated search query
- Verifies the expected response structure and metadata
Consider improving these assertions:
- assert len(response["hits"]) >= 0 + assert len(response["hits"]) > 0 # Ensure we actually got resultsThe current assertion
len(response["hits"]) >= 0will always pass since a list length can't be negative.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
meilisearch/client.py(2 hunks)meilisearch/config.py(1 hunks)tests/client/test_client_multi_search_meilisearch.py(2 hunks)tests/client/test_client_network.py(1 hunks)tests/common.py(1 hunks)tests/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/client/test_client_network.py (2)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (2)
get_all_networks(783-796)add_or_update_networks(761-781)
tests/client/test_client_multi_search_meilisearch.py (2)
tests/conftest.py (2)
client(15-16)index_with_documents(121-128)meilisearch/client.py (2)
add_or_update_networks(761-781)multi_search(226-255)
meilisearch/client.py (1)
meilisearch/_httprequests.py (2)
patch(99-107)get(84-85)
tests/conftest.py (1)
meilisearch/_httprequests.py (1)
patch(99-107)
🔇 Additional comments (7)
meilisearch/config.py (1)
48-48: LGTM: Network path constant addedThe new network path constant follows the existing pattern and will enable the federated search functionality.
tests/conftest.py (1)
279-293: LGTM: Network options fixture properly implementedThe new fixture follows the established pattern of other experimental feature fixtures, correctly enabling the network feature before tests and disabling it afterward.
tests/common.py (1)
17-18: LGTM: Remote Meilisearch instance constants addedThese constants will be used as identifiers for remote Meilisearch instances in the network-related tests.
meilisearch/client.py (3)
237-238: Great addition to the documentation!This documentation update clearly explains that queries can now include remote options in federationOptions, which is essential for users who want to use the new network federation functionality.
761-782: Well-implemented method for managing remote networks.The
add_or_update_networksmethod is correctly implemented with appropriate type annotations, error handling, and follows the Meilisearch API design patterns. It correctly uses the HTTP PATCH method to update network configurations.
783-796: Well-implemented network retrieval method.The
get_all_networksmethod is correctly implemented with proper return type annotations and follows the Meilisearch API design patterns. It correctly uses the HTTP GET method to retrieve network configurations.tests/client/test_client_multi_search_meilisearch.py (1)
4-4: Appropriate imports added.The additional imports from tests.common are correctly added to support the new network-related test.
| @pytest.mark.usefixtures("enable_network_options") | ||
| def test_add_or_update_networks(client): | ||
| """Tests upsert network remote instance.""" | ||
| body = { | ||
| "self": REMOTE_MS_1, | ||
| "remotes": { | ||
| REMOTE_MS_1: {"url": "http://localhost:7700", "searchApiKey": "xxxxxxxxxxxxxx"}, | ||
| REMOTE_MS_2: {"url": "http://localhost:7720", "searchApiKey": "xxxxxxxxxxxxxxx"}, | ||
| }, | ||
| } | ||
| response = client.add_or_update_networks(body=body) | ||
|
|
||
| assert isinstance(response, dict) | ||
| assert response["self"] == REMOTE_MS_1 | ||
| assert len(response["remotes"]) >= 2 | ||
| assert REMOTE_MS_2 in response["remotes"] | ||
| assert REMOTE_MS_1 in response["remotes"] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding edge case tests
The happy path test looks good, but consider adding tests for edge cases such as:
- Empty remotes dictionary
- Invalid URLs or API keys
- Non-existent remote instances
Also, ensure that the test URLs don't point to actual production services. The current localhost URLs look good, but it's worth verifying these are test environments only.
🏁 Script executed:
#!/bin/bash
# Check if there are any other URLs used in tests
rg --ignore-case "http" tests/ | grep -v "localhost" | grep -v "127.0.0.1"Length of output: 1884
Enhance test_add_or_update_networks with edge‐case coverage
While the happy-path for upserting networks looks solid, please add tests for these scenarios in tests/client/test_client_network.py:
- Empty
remotesdictionary (what should the response be? error vs. no-ops) - Malformed URLs (e.g.
htp://…) or missing/invalid API keys selfvalue not included in theremotespayload- Non-existent remote instance identifiers
Note: this test already uses only localhost endpoints, so there’s no risk of hitting production services here.
🤖 Prompt for AI Agents
In tests/client/test_client_network.py around lines 14 to 30, the current test
only covers the happy path for adding or updating networks. To improve coverage,
add separate test cases for edge scenarios: an empty remotes dictionary to
verify behavior (error or no-op), malformed URLs or invalid/missing API keys to
check validation, a self value not present in the remotes dictionary to test
consistency, and non-existent remote instance identifiers to confirm error
handling. Ensure these tests use localhost URLs or mocks to avoid external
calls.
Pull Request
Related issue
Fixes #1082
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Bug Fixes
Tests