Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an export feature to the Meilisearch Python client: a new Client.export() method, a config path for exports, code-sample additions, and integration tests exercising cross-instance exports with a secondary server fixture and helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Source as SourceMeili (API)
participant Target as TargetMeili (Remote API)
Client->>Source: POST /exports {url, apiKey?, payloadSize?, indexes?}
Note right of Source: Creates export task\nreturns TaskInfo
Source->>Client: 202 TaskInfo (task uid)
Client->>Source: GET /tasks/{uid} (poll)
Source->>Client: 200 TaskInfo (status: succeeded)
Note right of Source: Source pushes data to Target URL\n(or Target pulls depending on implementation)
Target->>Client: (visible via client2) Indexes created / documents synced
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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 (1)
tests/client/test_client_exports.py (1)
13-23: Consider using consistent property access.The test logic is correct and appropriately verifies the export functionality. However, there's an inconsistency: line 22 uses
index.get_primary_key()(method call) while line 21 usesindex2.primary_key(property access). Both work, but consistency would improve readability.Consider using the property consistently:
index2 = client2.get_index(index.uid) assert index2.uid == index.uid - assert index2.primary_key == index.get_primary_key() + assert index2.primary_key == index.primary_key assert index2.get_documents().total == index.get_documents().total
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.code-samples.meilisearch.yaml(1 hunks)docker-compose.yml(2 hunks)meilisearch/client.py(1 hunks)meilisearch/config.py(1 hunks)tests/client/test_client_exports.py(1 hunks)tests/common.py(1 hunks)tests/conftest.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
meilisearch/client.py (2)
meilisearch/models/task.py (1)
TaskInfo(77-106)meilisearch/_httprequests.py (1)
post(98-108)
tests/client/test_client_exports.py (3)
tests/conftest.py (2)
client(16-17)client2(21-22)meilisearch/client.py (5)
index(227-243)export(630-679)wait_for_task(801-828)get_index(185-204)get_indexes(128-159)meilisearch/index.py (2)
get_primary_key(145-153)get_documents(387-425)
🪛 GitHub Actions: Tests
tests/conftest.py
[error] 1-1: isort check failed: Imports are incorrectly sorted and/or formatted.
🔇 Additional comments (9)
meilisearch/config.py (1)
51-51: LGTM!The new
exportspath constant follows the established pattern and correctly maps to the "export" endpoint.docker-compose.yml (2)
9-15: LGTM!The integration of the second Meilisearch instance into the package service is correctly configured with the environment variable and dependency links.
27-34: LGTM!The second Meilisearch service configuration mirrors the primary instance appropriately for test purposes. The port mapping 7701:7700 allows external access for testing.
tests/common.py (1)
5-5: LGTM!The
BASE_URL_2constant follows the same pattern asBASE_URLand correctly defaults to port 7701, matching the docker-compose configuration.tests/conftest.py (3)
20-32: LGTM!The
client2fixture and_clear_indexeshelper function are well-structured and follow the existing patterns in the test suite.
35-44: LGTM!The conditional cleanup logic for the second client is appropriate—it only attempts cleanup when
MEILISEARCH_URL_2is configured, preventing errors when the second server isn't available.
63-70: LGTM!The
clear_all_tasksfixture correctly handles cleanup for both clients when the second server is configured.tests/client/test_client_exports.py (2)
7-10: LGTM!The skip marker appropriately gates these tests behind the
MEILISEARCH_URL_2environment variable, ensuring they only run when a second server is configured.
26-42: LGTM!The test correctly verifies that the export respects index filtering, ensuring only the specified index is exported to the second server.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/conftest.py (2)
20-23: Consider makingclient2explicitly conditional on second-instance configuration.Right now
client2is always constructed, even when a second Meilisearch instance is not configured; usage is guarded later byos.getenv("MEILISEARCH_URL_2"), but a misconfiguredBASE_URL_2could still yield confusing failures when a test directly depends onclient2. You could optionally make this fixture raise/skip with a clear message when the second instance is not configured (e.g., checkMEILISEARCH_URL_2orcommon.BASE_URL_2and callpytest.skip).
25-32: Handle potential pagination in_clear_indexesand reuse existingIndexinstances.
client.get_indexes()returns a dict with aresultslist ofIndexinstances built from the paginated/indexesendpoint; the HTTP layer uses Meilisearch’s default pagination (limit=20) when no parameters are provided.(python-sdk.meilisearch.com) If a test run ever generates more thanlimitindexes,_clear_indexeswill only delete the first page despite the “Deletes all the indexes” docstring.It would be more robust to either:
- Loop over pages using
offset/limituntilresultsis empty, or- At least request a higher
limitthat safely covers expected test indexes.Since
get_indexesalready returnsIndexobjects, you can also simplify the loop by callingindex.delete()directly instead of recreating them viaclient.index(index.uid).(python-sdk.meilisearch.com)Example of a more robust approach (for illustration):
-def _clear_indexes(meilisearch_client): - """Deletes all the indexes in the Meilisearch instance.""" - - indexes = meilisearch_client.get_indexes() - for index in indexes["results"]: - task = meilisearch_client.index(index.uid).delete() - meilisearch_client.wait_for_task(task.task_uid) +def _clear_indexes(meilisearch_client): + """Deletes all the indexes in the Meilisearch instance (handles pagination).""" + + offset, limit = 0, 1000 + while True: + page = meilisearch_client.get_indexes({"offset": offset, "limit": limit}) + results = page["results"] + if not results: + break + for index in results: + task = index.delete() + meilisearch_client.wait_for_task(task.task_uid) + offset += limitAlso applies to: 35-36, 40-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/conftest.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/conftest.py (3)
meilisearch/client.py (3)
index(227-243)wait_for_task(801-828)delete_tasks(782-799)meilisearch/_httprequests.py (1)
delete(142-147)meilisearch/index.py (2)
delete(88-105)wait_for_task(233-260)
🔇 Additional comments (2)
tests/conftest.py (2)
2-4: Stdlib import ofosis correctly placed.
osis now grouped with the other stdlib imports and used below forMEILISEARCH_URL_2checks; no issues from a style or functionality perspective.
63-70: Symmetric task cleanup forclient2looks good.Extending
clear_all_tasksto delete finished tasks onclient2whenMEILISEARCH_URL_2is set keeps task state consistent across both instances and matches the behavior introduced inclear_indexes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
meilisearch/client.py (1)
630-679:exportimplementation looks correct; consider tighteningindexesdoc wording.The method’s behavior (payload construction, field names, and return type) is consistent with other client methods and the Export API, and I don’t see functional issues here.
One small optional improvement: the docstring for
indexessays “A set of objects…”, but the type isMapping[str, Any]. To avoid confusion with Pythonset, you might rephrase to something like “A mapping/dict whose keys correspond to patterns matching the indexes you want to export (see Export API docs for structure).”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.code-samples.meilisearch.yaml(1 hunks)meilisearch/client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .code-samples.meilisearch.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
meilisearch/client.py (2)
meilisearch/models/task.py (1)
TaskInfo(77-106)meilisearch/_httprequests.py (1)
post(98-108)
|
@Strift not sure whether we should wait for meilisearch/meilisearch#6009 or let it go. Locally I was experiencing the same issue and it took quite of my time but here test cases are passing. |
|
Hi @jawad-khan and thanks for this PR 🙏 It's best to hold off until the issue is fixed. Otherwise, we would just be setting ourselves up to get reports in this repository that are unrelated to the SDK 😅 |
jawad-khan
force-pushed
the
jawad/export-api
branch
from
97c3633 to
10decc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.code-samples.meilisearch.yamldocker-compose.ymlmeilisearch/client.pymeilisearch/config.pytests/client/test_client_exports.pytests/common.pytests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (4)
- meilisearch/config.py
- .code-samples.meilisearch.yaml
- tests/common.py
- tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
meilisearch/client.py (2)
meilisearch/models/task.py (1)
TaskInfo(78-107)meilisearch/_httprequests.py (1)
post(98-108)
🔇 Additional comments (3)
docker-compose.yml (3)
634-683: LGTM! Clean implementation following established patterns.The
exportmethod is well-implemented and follows the same pattern as other async operations (create_dump,create_snapshot). The docstring is comprehensive, parameter naming is correct, and the payload construction properly handles optional fields with the correct camelCase API keys.
13-23: Export validation logic is correct.The test properly validates the export by comparing the UID, primary key, and document count between source and destination indexes. The assertions confirm that the export operation transferred the data correctly to the second Meilisearch instance.
26-29: This review comment cannot be addressed as written due to a critical file mismatch.The file header indicates
docker-compose.yml(lines 26-29) with a meilisearch2 service configuration, but the entire comment body discusses Python test fixtures intests/client/test_client_exports.py. These are unrelated files and content.Please clarify which file this review is intended for and provide the correct file path and code snippet so the review can be properly assessed.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/conftest.py (2)
20-23: Guardclient2fixture againstNoneURL to prevent potential session-scoped errors.The
client2fixture is session-scoped and will be instantiated even whenMEILISEARCH_URL_2is not set, resulting inmeilisearch.Client(None, ...). While the export tests are skipped, other fixtures likeclear_indexesandclear_all_tasksdepend onclient2and areautouse=True, meaning they run for every test.Consider adding a guard or making this fixture conditional:
🔎 Proposed fix
@fixture(scope="session") def client2(): + if not common.BASE_URL_2: + return None return meilisearch.Client(common.BASE_URL_2, common.MASTER_KEY)Then update the conditional checks to handle
None:- if os.getenv("MEILISEARCH_URL_2"): + if client2 is not None: _clear_indexes(client2)
275-280: Add guard forBASE_URL_2inenable_vector_searchfixture.The fixture patches
BASE_URL_2unconditionally. While currently safe (export tests using this fixture are skipped whenMEILISEARCH_URL_2is not set), this is fragile. If the fixture is used by a non-export test, it will fail when the second server is not configured.🔎 Proposed fix
requests.patch( f"{common.BASE_URL}/experimental-features", headers={"Authorization": f"Bearer {common.MASTER_KEY}"}, json={"vectorStoreSetting": True}, timeout=10, ) - requests.patch( - f"{common.BASE_URL_2}/experimental-features", - headers={"Authorization": f"Bearer {common.MASTER_KEY}"}, - json={"vectorStoreSetting": True}, - timeout=10, - ) + if common.BASE_URL_2: + requests.patch( + f"{common.BASE_URL_2}/experimental-features", + headers={"Authorization": f"Bearer {common.MASTER_KEY}"}, + json={"vectorStoreSetting": True}, + timeout=10, + )Apply the same pattern to the teardown section (lines 288-293).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.code-samples.meilisearch.yaml.github/workflows/tests.ymldocker-compose.ymlmeilisearch/client.pytests/client/test_client_exports.pytests/common.pytests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .code-samples.meilisearch.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
meilisearch/client.py (2)
meilisearch/models/task.py (1)
TaskInfo(78-107)meilisearch/_httprequests.py (1)
post(98-108)
tests/client/test_client_exports.py (1)
meilisearch/client.py (3)
index(231-247)export(634-683)wait_for_task(808-835)
🪛 Ruff (0.14.10)
tests/client/test_client_exports.py
15-15: Unused function argument: enable_vector_search
(ARG001)
30-30: Unused function argument: enable_vector_search
(ARG001)
🔇 Additional comments (5)
meilisearch/client.py (1)
634-683: LGTM!The
exportmethod is well-implemented and follows the existing patterns in the codebase (similar tocreate_dumpandcreate_snapshot). The payload construction correctly handles optional parameters, and the docstring is comprehensive.docker-compose.yml (1)
27-34: LGTM!The second Meilisearch service is properly configured with consistent settings (same image, same master key) as the primary instance. The port mapping (7701:7700) correctly exposes the second instance on a different host port.
tests/common.py (1)
5-5: LGTM!The
BASE_URL_2intentionally has no default value, which correctly gates the export tests and secondary server cleanup via environment variable checks elsewhere in the test suite.tests/client/test_client_exports.py (2)
14-26: LGTM!The test correctly validates the export functionality:
- Creates source index with documents
- Triggers export to the secondary server
- Waits for task completion
- Verifies the exported index matches (UID, primary key, document count)
The
enable_vector_searchfixture is correctly included for its side effects (enabling vector store on both servers), and thepylint: disable=unused-argumentcomment appropriately silences the linter.
48-60: Well-designed polling helper for eventual consistency.The
assert_exported_countfunction handles the async nature of document imports with appropriate retry logic (20 attempts × 1 second = 20 second max wait), providing clear failure messages when the timeout is exceeded.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/tests.yml
⏰ 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). (5)
- GitHub Check: integration-tests (3.12)
- GitHub Check: integration-tests (3.9)
- GitHub Check: integration-tests (3.11)
- GitHub Check: integration-tests (3.13)
- GitHub Check: integration-tests (3.10)
🔇 Additional comments (1)
.github/workflows/tests.yml (1)
38-39: LGTM! Environment variable correctly configured.The missing
MEILISEARCH_URL_2environment variable has been properly added and correctly points to the secondary Meilisearch instance. This ensures the export tests will run instead of being skipped.
jawad-khan
force-pushed
the
jawad/export-api
branch
from
9ea1c39 to
6392933
Compare
|
@Strift This PR is ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/client/test_client_exports.py (1)
48-60: Minor redundancy in final assertion.If the loop exhausts all attempts without returning,
doc_countfrom the last iteration (line 52) already holds the final value. The extraget_documents()call on line 59 is redundant.♻️ Suggested simplification
def assert_exported_count(index, expected_count): # Wait up to 20 seconds for documents to be imported max_attempts = 20 + doc_count = 0 for attempt in range(max_attempts): doc_count = index.get_documents().total if doc_count == expected_count: return if attempt < max_attempts - 1: time.sleep(1) # Final check with clear failure message - actual_count = index.get_documents().total - assert actual_count == expected_count + assert doc_count == expected_count, f"Expected {expected_count} documents, got {doc_count}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.code-samples.meilisearch.yamlmeilisearch/client.pymeilisearch/config.pytests/client/test_client_exports.pytests/common.pytests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (4)
- .code-samples.meilisearch.yaml
- tests/common.py
- tests/conftest.py
- meilisearch/client.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/client/test_client_exports.py (2)
meilisearch/client.py (5)
index(231-247)export(634-683)wait_for_task(808-835)get_index(189-208)get_indexes(131-163)meilisearch/index.py (2)
get_primary_key(168-176)get_documents(415-453)
🪛 Ruff (0.14.10)
tests/client/test_client_exports.py
15-15: Unused function argument: enable_vector_search
(ARG001)
30-30: Unused function argument: enable_vector_search
(ARG001)
🔇 Additional comments (4)
meilisearch/config.py (1)
52-52: LGTM!The new path constant correctly maps to the Meilisearch export API endpoint.
tests/client/test_client_exports.py (3)
1-11: LGTM!Good use of
pytest.mark.skipifto conditionally run export tests only when a second server is configured, avoiding test failures in environments without the dual-server setup.
14-26: LGTM!Test logic is correct. The
enable_vector_searchfixture is appropriately used for setup side effects—a common pytest pattern where the fixture's invocation matters, not its return value.
29-45: LGTM!Good coverage of the
indexesfilter functionality, correctly verifying that only the specified index is exported to the target server.
|
|
||
| from tests import common | ||
|
|
||
| pytestmark = pytest.mark.skipif( |
There was a problem hiding this comment.
I think these tests are being skipped since this ENV variable isnt present in the actions
There was a problem hiding this comment.
Yes, we need two meilisearch servers to check export status, I tried to change workflow but tests were taking too much time. Therefore I made them skip.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
meilisearch/client.py (1)
649-655: Consider adding validation for theurlparameter.Newer methods in this file (e.g.,
get_chat_workspace_settings,update_chat_workspace_settings) validate required string parameters with checks likeif not workspace_uid. For consistency and better error messages, you could add similar validation here:♻️ Optional improvement
def export( self, url: str, api_key: Optional[str] = None, payload_size: Optional[str] = None, indexes: Optional[Mapping[str, Any]] = None, ) -> TaskInfo: + if not url: + raise ValueError("url is required and cannot be empty")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meilisearch/client.py` around lines 649 - 655, The export method’s required url parameter lacks validation; add a precondition at the start of export to check that url is a non-empty string (same style as get_chat_workspace_settings/update_chat_workspace_settings), raise a ValueError with a clear message if not provided, and ensure any subsequent usage of url assumes it is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@meilisearch/client.py`:
- Around line 649-655: The export method’s required url parameter lacks
validation; add a precondition at the start of export to check that url is a
non-empty string (same style as
get_chat_workspace_settings/update_chat_workspace_settings), raise a ValueError
with a clear message if not provided, and ensure any subsequent usage of url
assumes it is valid.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8218ba82-779c-4102-8191-0229a9db9cd1
📒 Files selected for processing (2)
.code-samples.meilisearch.yamlmeilisearch/client.py
3 participants
Pull Request
Added support for the Export API as described in the official Meilisearch docs.
Related issue
Fixes # 1129
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Code Samples
Tests
Chores