Add support for export api #1171
Conversation
|
Warning Rate limit exceeded@jawad-khan has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an exports API path and Client.export(...), provisions a second Meilisearch instance and client (client2) for tests, adds end-to-end export tests against the secondary instance, and adds a code sample demonstrating an export request. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test runner
participant Client as Client.export()
participant Source as Meilisearch_A
participant Target as Meilisearch_B
Note over Test,Client: Initiate export from tests to secondary server
Test->>Client: call export(url, api_key, payload_size, indexes)
activate Client
Client->>Client: build payload (url + optional fields)
Client->>Source: POST /exports (payload)
activate Source
Source-->>Client: TaskInfo (task created)
deactivate Source
Client-->>Test: return TaskInfo
deactivate Client
Note right of Test: Poll task until completion
Test->>Target: Query indexes / index details
activate Target
Target-->>Test: index metadata & document counts
deactivate Target
Note over Test,Target: Verify UID, primary key, document count match source
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
2 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
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.