Add headers in embedder#757
Conversation
📝 WalkthroughWalkthroughAdded an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/batches.rs (1)
240-257: Test mock response uses removed fields and lacks new required fields.The mock response includes
enqueuedAt,indexUid, andtaskUidswhich were removed fromBatch, and is missing required fieldsprogress,stats, andduration. The test needs to be updated to match the newBatchstructure.
🤖 Fix all issues with AI agents
In `@src/batches.rs`:
- Around line 15-20: The tests fail because Batch now has non-optional fields
progress, stats, and duration but mock responses omit them; fix by making these
fields optional on the Batch struct (change types to Option<BatchProgress>,
Option<BatchStats>, Option<String>) and add #[serde(default)] to each field so
deserialization succeeds with missing keys, then update any code that reads
Batch::progress, Batch::stats, or Batch::duration to handle None (or provide
sensible defaults) or alternatively add the missing fields to all mock responses
in tests to match the new non-optional types.
- Around line 168-173: The BatchWriteChannelCongestion struct is missing serde
renaming for camelCase; update its derive to include serde(rename_all =
"camelCase") (or add per-field serde(rename = "...")) so blocking_attempts ->
blockingAttempts and blocking_ratio -> blockingRatio during (de)serialization;
modify the BatchWriteChannelCongestion declaration to include the serde
attribute alongside Deserialize to fix the JSON field mapping.
- Around line 145-184: Add serde attribute to use camelCase renaming on the
structs that map to the Meilisearch API: add #[serde(rename_all = "camelCase")]
to BatchProgressStep and BatchInternalDatabaseSizes so their snake_case Rust
fields (e.g., current_step, external_documents_id) deserialize from API
camelCase keys (currentStep, externalDocumentsId); update the struct definitions
(BatchProgressStep, BatchInternalDatabaseSizes) to include this attribute above
each #[derive(...)].
- Around line 214-223: The serde rename attributes for the Batch task enum
variants are incorrect and cause deserialization failures; update the rename
strings for SettingsUpdate, TaskCancellation, and UpgradeDatabase to match
Meilisearch API values by changing SettingsUpdate's attribute from
"settingUpdate" to "settingsUpdate", TaskCancellation's attribute from
"taskCancellation" to "taskCancelation" (single 'l'), and UpgradeDatabase's
attribute from "updateDatabase" to "upgradeDatabase" so the enum variants
SettingsUpdate, TaskCancellation, and UpgradeDatabase deserialize correctly.
- Around line 152-166: BatchStats currently declares status: Status and types:
Type but the API returns maps for stats.status and stats.types; change the
BatchStats struct to use HashMap<Status, i32> for status and HashMap<Type, i32>
for types (preserve serde renames if any), and update the Status and Type enums
to derive Eq and Hash (in addition to existing derives like
Deserialize/Debug/Clone) so they can be used as HashMap keys; ensure HashMap is
imported where BatchStats is defined.
🧹 Nitpick comments (3)
src/settings.rs (1)
230-250: Avoid panicking in a public builder.Consider returning a
Result(or a no-op) for non-Rest sources to prevent runtime panics in consumer apps.src/batches.rs (2)
65-96: New query fields are inaccessible—no builder methods provided.The new filtering fields (
uids,batch_uids,index_uids,statuses,types,reverse, and the datetime filters) are private with no correspondingwith_*builder methods. Users cannot leverage these filtering capabilities.Consider adding builder methods similar to
with_limitandwith_from:♻️ Example builder methods
#[must_use] pub fn with_uids(&mut self, uids: impl IntoIterator<Item = i64>) -> &mut Self { self.uids = uids.into_iter().collect(); self } #[must_use] pub fn with_statuses(&mut self, statuses: impl IntoIterator<Item = Status>) -> &mut Self { self.statuses = statuses.into_iter().collect(); self } #[must_use] pub fn with_reverse(&mut self, reverse: bool) -> &mut Self { self.reverse = reverse; self } // ... similar for other fields
67-69: Inconsistent UID types:i64vsu32.
uidsandbatch_uidsusei64, whilefrom(line 82) andBatch.uid(line 14) useu32. Consider usingu32consistently for UID fields to match theBatchstruct and avoid potential conversion issues.
valkrypton
force-pushed
the
add/headers-in-embedder
branch
from
f10108e to
def69e4
Compare
|
@Strift can someone review this? |
Strift
left a comment
Strift
left a comment
There was a problem hiding this comment.
@valkrypton can you please add tests?
valkrypton
force-pushed
the
add/headers-in-embedder
branch
from
def69e4 to
ee42bc2
Compare
Tests added |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/settings.rs (1)
230-244: LetNonemean “no custom headers.”The REST embedder docs describe
headersas additional headers, and say Meilisearch already addsContent-Typeplus bearer auth whenapiKeyis supplied. Materializing those defaults here makeswith_headers(None)mean something different from “unset this optional field” and hardcodes server-side defaults into the SDK. I’d keepself.headers = headershere, and add a separate convenience if you want an explicit “fill default headers” helper. (meilisearch.dev)♻️ Simpler builder behavior
- self.headers = if let Some(headers) = headers { - Some(headers) - } else if let Some(api_key) = self.api_key.clone() { - Some(HashMap::from([ - (String::from("Authorization"), format!("Bearer {}", api_key)), - ("Content-Type".to_string(), "application/json".to_string()), - ])) - } else { - Some(HashMap::from([( - "Content-Type".to_string(), - "application/json".to_string(), - )])) - }; + self.headers = headers;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings.rs` around lines 230 - 244, The with_headers method currently materializes default REST headers when headers is None, making with_headers(None) mean something other than “no custom headers”; update the method (with_headers) to simply set self.headers = headers (leave self.source, self.api_key checks out) so None represents no custom headers, and remove the embedded logic that injects Authorization/Content-Type defaults for EmbedderSource::Rest; if you still want a helper to populate server-side defaults, add a separate method (e.g., fill_default_headers) instead of baking it into with_headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/settings.rs`:
- Around line 190-196: The private headers field in the Embedder struct prevents
external construction/inspection; make it public by changing the field
declaration for headers (the Option<HashMap<String, String>> field currently
annotated with #[serde(skip_serializing_if = "Option::is_none")]) to be pub so
it matches the other pub fields and allows external code to use Embedder { …,
..Embedder::default() } and inspect/modify returned values; keep the existing
serde attribute and type unchanged.
---
Nitpick comments:
In `@src/settings.rs`:
- Around line 230-244: The with_headers method currently materializes default
REST headers when headers is None, making with_headers(None) mean something
other than “no custom headers”; update the method (with_headers) to simply set
self.headers = headers (leave self.source, self.api_key checks out) so None
represents no custom headers, and remove the embedded logic that injects
Authorization/Content-Type defaults for EmbedderSource::Rest; if you still want
a helper to populate server-side defaults, add a separate method (e.g.,
fill_default_headers) instead of baking it into with_headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ffc5f0e-13e4-436f-b071-5763d1b34358
📒 Files selected for processing (2)
src/search.rssrc/settings.rs
✅ Files skipped from review due to trivial changes (1)
- src/search.rs
| /// Headers is a JSON object whose keys represent the name of additional headers to send in | ||
| /// requests to embedders, and whose values represent the value of these additional headers. | ||
| /// | ||
| /// This field is optional when using the rest embedder. | ||
| /// This field is incompatible with all other embedders. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| headers: Option<HashMap<String, String>>, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the file size and structure around the headers field
wc -l src/settings.rsRepository: meilisearch/meilisearch-rust
Length of output: 92
🏁 Script executed:
# Read the Embedder struct definition around lines 190-196
sed -n '180,260p' src/settings.rs | cat -nRepository: meilisearch/meilisearch-rust
Length of output: 3983
🏁 Script executed:
# Search for examples that construct Embedder using struct literals (Lines 1209-1213, 1252-1256)
sed -n '1200,1220p' src/settings.rs | cat -n
sed -n '1245,1265p' src/settings.rs | cat -nRepository: meilisearch/meilisearch-rust
Length of output: 2560
🏁 Script executed:
# Search for any pub visibility modifier on the Embedder struct and fields
rg -A 20 'pub struct Embedder' src/settings.rs | head -40Repository: meilisearch/meilisearch-rust
Length of output: 1338
🏁 Script executed:
# Check if there are any builder methods or constructor patterns
rg -n 'impl Embedder|fn new|with_headers' src/settings.rs | head -20Repository: meilisearch/meilisearch-rust
Length of output: 659
Make headers field public to maintain API consistency.
Embedder is publicly constructed via struct literals in the documented examples at lines 1209–1213 and 1252–1256 using Embedder { …, ..Embedder::default() }. The private headers field breaks this pattern—external code cannot construct instances following the documented examples. All other fields in the struct are pub, so this field should be as well to maintain consistency and allow users to inspect or modify headers on returned values.
Minimal fix
- headers: Option<HashMap<String, String>>,
+ pub headers: Option<HashMap<String, String>>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Headers is a JSON object whose keys represent the name of additional headers to send in | |
| /// requests to embedders, and whose values represent the value of these additional headers. | |
| /// | |
| /// This field is optional when using the rest embedder. | |
| /// This field is incompatible with all other embedders. | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| headers: Option<HashMap<String, String>>, | |
| /// Headers is a JSON object whose keys represent the name of additional headers to send in | |
| /// requests to embedders, and whose values represent the value of these additional headers. | |
| /// | |
| /// This field is optional when using the rest embedder. | |
| /// This field is incompatible with all other embedders. | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| pub headers: Option<HashMap<String, String>>, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/settings.rs` around lines 190 - 196, The private headers field in the
Embedder struct prevents external construction/inspection; make it public by
changing the field declaration for headers (the Option<HashMap<String, String>>
field currently annotated with #[serde(skip_serializing_if =
"Option::is_none")]) to be pub so it matches the other pub fields and allows
external code to use Embedder { …, ..Embedder::default() } and inspect/modify
returned values; keep the existing serde attribute and type unchanged.
2 participants
Pull Request
Related issue
Fixes #720.
What does this PR do?
headersfield to embedder settings.with_headersfunction on theEmbedderstruct to add headers. Headers should only be added with Rest embedder.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