From 31ef7adef5445c097ad8f7154539b9ab12caba55 Mon Sep 17 00:00:00 2001 From: Louis Dureuil Date: Mon, 3 Mar 2025 16:51:34 +0100 Subject: [PATCH] Don't use Deserr for ExternalDocumentId, instead convert to error afterward --- .../meilisearch/src/routes/indexes/similar.rs | 20 ++++++++----------- crates/meilisearch/src/search/mod.rs | 16 +++++++++------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/crates/meilisearch/src/routes/indexes/similar.rs b/crates/meilisearch/src/routes/indexes/similar.rs index fcd64291e..400be331b 100644 --- a/crates/meilisearch/src/routes/indexes/similar.rs +++ b/crates/meilisearch/src/routes/indexes/similar.rs @@ -5,7 +5,7 @@ use index_scheduler::IndexScheduler; use meilisearch_types::deserr::query_params::Param; use meilisearch_types::deserr::{DeserrJsonError, DeserrQueryParamError}; use meilisearch_types::error::deserr_codes::*; -use meilisearch_types::error::{ErrorCode as _, ResponseError}; +use meilisearch_types::error::ResponseError; use meilisearch_types::index_uid::IndexUid; use meilisearch_types::keys::actions; use meilisearch_types::serde_cs::vec::CS; @@ -111,7 +111,7 @@ pub async fn similar_get( ) -> Result { let index_uid = IndexUid::try_from(index_uid.into_inner())?; - let query = params.0.try_into()?; + let query = params.0.into(); let mut aggregate = SimilarAggregator::::from_query(&query); @@ -295,10 +295,8 @@ impl std::convert::TryFrom for RankingScoreThresholdGet { } } -impl TryFrom for SimilarQuery { - type Error = ResponseError; - - fn try_from( +impl From for SimilarQuery { + fn from( SimilarQueryGet { id, offset, @@ -311,7 +309,7 @@ impl TryFrom for SimilarQuery { embedder, ranking_score_threshold, }: SimilarQueryGet, - ) -> Result { + ) -> Self { let filter = match filter { Some(f) => match serde_json::from_str(&f) { Ok(v) => Some(v), @@ -320,10 +318,8 @@ impl TryFrom for SimilarQuery { None => None, }; - Ok(SimilarQuery { - id: id.0.try_into().map_err(|code: InvalidSimilarId| { - ResponseError::from_msg(code.to_string(), code.error_code()) - })?, + SimilarQuery { + id: serde_json::Value::String(id.0), offset: offset.0, limit: limit.0, filter, @@ -333,6 +329,6 @@ impl TryFrom for SimilarQuery { show_ranking_score: show_ranking_score.0, show_ranking_score_details: show_ranking_score_details.0, ranking_score_threshold: ranking_score_threshold.map(|x| x.0), - }) + } } } diff --git a/crates/meilisearch/src/search/mod.rs b/crates/meilisearch/src/search/mod.rs index 2091047fc..2fc96286d 100644 --- a/crates/meilisearch/src/search/mod.rs +++ b/crates/meilisearch/src/search/mod.rs @@ -635,7 +635,7 @@ impl SearchQueryWithIndex { pub struct SimilarQuery { #[deserr(error = DeserrJsonError)] #[schema(value_type = String)] - pub id: ExternalDocumentId, + pub id: serde_json::Value, #[deserr(default = DEFAULT_SEARCH_OFFSET(), error = DeserrJsonError)] pub offset: usize, #[deserr(default = DEFAULT_SEARCH_LIMIT(), error = DeserrJsonError)] @@ -657,8 +657,7 @@ pub struct SimilarQuery { pub ranking_score_threshold: Option, } -#[derive(Debug, Clone, PartialEq, Deserr)] -#[deserr(try_from(Value) = TryFrom::try_from -> InvalidSimilarId)] +#[derive(Debug, Clone, PartialEq)] pub struct ExternalDocumentId(String); impl AsRef for ExternalDocumentId { @@ -674,7 +673,7 @@ impl ExternalDocumentId { } impl TryFrom for ExternalDocumentId { - type Error = InvalidSimilarId; + type Error = milli::UserError; fn try_from(value: String) -> Result { serde_json::Value::String(value).try_into() @@ -682,10 +681,10 @@ impl TryFrom for ExternalDocumentId { } impl TryFrom for ExternalDocumentId { - type Error = InvalidSimilarId; + type Error = milli::UserError; fn try_from(value: Value) -> Result { - Ok(Self(milli::documents::validate_document_id_value(value).map_err(|_| InvalidSimilarId)?)) + Ok(Self(milli::documents::validate_document_id_value(value)?)) } } @@ -1597,6 +1596,11 @@ pub fn perform_similar( ranking_score_threshold, } = query; + let id: ExternalDocumentId = id.try_into().map_err(|error| { + let msg = format!("Invalid value at `.id`: {error}"); + ResponseError::from_msg(msg, Code::InvalidSimilarId) + })?; + // using let-else rather than `?` so that the borrow checker identifies we're always returning here, // preventing a use-after-move let Some(internal_id) = index.external_documents_ids().get(&rtxn, &id)? else {