add export module for the /export route#763
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThis pull request implements the Changes
Sequence DiagramsequenceDiagram
participant User
participant SDK as SDK Client
participant HTTP as HTTP Client
participant ExportSvc as Export Service
participant TargetMS as Target Meilisearch
User->>SDK: ExportQuery::new()
User->>SDK: with_payload_size("50MiB")
User->>SDK: with_indexes(...)
User->>SDK: execute()
SDK->>HTTP: POST /export with config
HTTP->>ExportSvc: Export request (serialized payload)
ExportSvc->>TargetMS: Connect & initiate export
TargetMS-->>ExportSvc: Export progress
ExportSvc-->>HTTP: TaskInfo response
HTTP-->>SDK: TaskInfo result
SDK-->>User: Result<TaskInfo, Error>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 Fix all issues with AI agents
In `@src/export.rs`:
- Around line 195-207: The execute method of the export request currently
expects a 200 status; update the HTTP status check in the request invocation
inside Export::execute (the call to self.client.http_client.request with
Method::Post { query: (), body: self }) to expect 202 instead of 200 so the POST
/export async task response is accepted; ensure only the numeric status code is
changed from 200 to 202 to match other async endpoints.
🧹 Nitpick comments (4)
meilisearch-test-macro/src/lib.rs (3)
64-66: Update error message to include ExportClient.The error message lists valid parameter types but doesn't include the newly added
ExportClient.Suggested fix
// TODO: throw this error while pointing to the specific token ty => panic!( - "#[meilisearch_test] can only receive Client, Index or String as parameters but received {ty:?}" + "#[meilisearch_test] can only receive Client, ExportClient, Index or String as parameters but received {ty:?}" ),
70-72: Update error message to include ExportClient.Same issue here - the error message should include
ExportClientas a valid parameter type.Suggested fix
// TODO: throw this error while pointing to the specific token // Used `self` as a parameter FnArg::Receiver(_) => panic!( - "#[meilisearch_test] can only receive Client, Index or String as parameters" + "#[meilisearch_test] can only receive Client, ExportClient, Index or String as parameters" ),
104-114: Potential variable shadowing when bothClientandExportClientare requested.When a test uses both
ClientandExportClientparameters,meilisearch_api_keyis declared twice (lines 97 and 109), which causes shadowing. While Rust allows this and it works correctly in this case (both use the same env var), it may trigger compiler warnings or be confusing.Consider reusing the existing variable or using a distinct name.
Option 1: Conditionally declare api_key only if not already declared
if use_export_client { outer_block.push(parse_quote!( let meilisearch_export_url = option_env!("MEILISEARCH_EXPORT_URL").unwrap_or("http://localhost:7700"); )); + // Only declare meilisearch_api_key if use_client is false (to avoid shadowing) + } + if use_export_client && !use_client { outer_block.push(parse_quote!( let meilisearch_api_key = option_env!("MEILISEARCH_API_KEY").unwrap_or("masterKey"); )); + } + if use_export_client { outer_block.push(parse_quote!( let export_client = ExportClient(Client::new(meilisearch_export_url, Some(meilisearch_api_key)).unwrap()); )); }src/export.rs (1)
46-56: Consider addingskip_serializing_ifforpayload_size.The
indexesfield has#[serde(skip_serializing_if = "Option::is_none")]butpayload_sizedoesn't. This meansnullwill be serialized whenpayload_sizeisNone. While this may be acceptable, it's inconsistent with theindexesfield handling.Suggested fix for consistency
#[derive(Debug, Serialize, Clone)] #[serde(rename_all = "camelCase")] pub struct ExportQuery<'a, Http: HttpClient> { #[serde(skip_serializing)] pub client: &'a Client<Http>, pub url: String, pub api_key: String, + #[serde(skip_serializing_if = "Option::is_none")] pub payload_size: Option<String>, #[serde(skip_serializing_if = "Option::is_none")] pub indexes: Option<HashMap<String, ExportQueryIndexOptions>>, }
aznszn
force-pushed
the
feat/add-export-route
branch
from
01d135e to
b817170
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/export.rs`:
- Around line 18-22: Doc examples in src/export.rs incorrectly read the export
server URL from MEILISEARCH_URL instead of MEILISEARCH_EXPORT_URL; update the
doc comment blocks that define the example env vars so the export URL variable
uses MEILISEARCH_EXPORT_URL (and keep MEILISEARCH_EXPORT_API_KEY mapped
appropriately to MEILISEARCH_API_KEY) in both places where the examples appear
so the docs consistently reference the export-specific env var.
🧹 Nitpick comments (2)
meilisearch-test-macro/src/lib.rs (1)
104-113: Allow an export-specific API key env var fallback.Using only
MEILISEARCH_API_KEYprevents tests against a target with a different key.♻️ Suggested tweak
- outer_block.push(parse_quote!( - let meilisearch_export_api_key = option_env!("MEILISEARCH_API_KEY").unwrap_or("masterKey"); - )); + outer_block.push(parse_quote!( + let meilisearch_export_api_key = option_env!("MEILISEARCH_EXPORT_API_KEY") + .or(option_env!("MEILISEARCH_API_KEY")) + .unwrap_or("masterKey"); + ));docker-compose.yml (1)
29-43: Pin the Meilisearch enterprise image tag for reproducible runs.The
latesttag can cause unexpected breaking changes in CI. The current stable version isv1.35.0.♻️ Suggested fix
- image: getmeili/meilisearch-enterprise:latest + image: getmeili/meilisearch-enterprise:${MEILISEARCH_IMAGE_TAG:-v1.35.0}Apply to both the
meilisearchandmeilisearch_exportservices.
aznszn
force-pushed
the
feat/add-export-route
branch
from
b817170 to
3024fd3
Compare
aznszn
force-pushed
the
feat/add-export-route
branch
from
3024fd3 to
8358863
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/export.rs`:
- Around line 171-172: Update the documentation example to use the
export-specific env var name: replace option_env!("MEILISEARCH_API_KEY") with
option_env!("MEILISEARCH_EXPORT_API_KEY") in the doc comment so the example
aligns with other examples that use MEILISEARCH_EXPORT_API_KEY.
🧹 Nitpick comments (1)
docker-compose.yml (1)
29-43: Pin the Meilisearch image tags instead oflatestfor repeatable builds.Replace
getmeili/meilisearch-enterprise:latestwith a specific version tag (e.g.,v1.32.1) in both services. Thelatesttag can cause non-deterministic CI runs when new releases are published.
aznszn
force-pushed
the
feat/add-export-route
branch
from
8358863 to
152cb76
Compare
|
@curquiza could you have a look at this? |
1 participant
Pull Request
Related issue
closes #758
What does this PR do?
exportmoduleExportQuerystruct to represent/exportroute requestsExportQueryIndexOptionsto represent theindexesfield in export request bodymeilisearch_exportservice in Docker to represent a server being exported toExportClient(which is a wrapper for a Client configured to connect to themeilisearch_exportservice) in themeilisearch_testmacroPR checklist
Please check if your PR fulfills the following requirements:
AI was not used to generate code, tests, or docs. I did use Claude for debugging though
Summary by CodeRabbit