From ee39309aaeb1a595e9489351aac41223b61a1d79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Renault?= Date: Tue, 11 Jun 2024 16:03:39 -0400 Subject: [PATCH] Improve errors and introduce a new InvalidSearchDistinct error code --- meilisearch-types/src/error.rs | 4 +++- meilisearch/src/routes/indexes/search.rs | 3 +-- meilisearch/src/search.rs | 6 ++---- milli/src/error.rs | 11 +++++++++++ milli/src/search/mod.rs | 16 +++++++++++----- 5 files changed, 28 insertions(+), 12 deletions(-) diff --git a/meilisearch-types/src/error.rs b/meilisearch-types/src/error.rs index 150c56b9d..1d91887e7 100644 --- a/meilisearch-types/src/error.rs +++ b/meilisearch-types/src/error.rs @@ -270,13 +270,14 @@ InvalidSimilarShowRankingScore , InvalidRequest , BAD_REQUEST ; InvalidSearchShowRankingScoreDetails , InvalidRequest , BAD_REQUEST ; InvalidSimilarShowRankingScoreDetails , InvalidRequest , BAD_REQUEST ; InvalidSearchSort , InvalidRequest , BAD_REQUEST ; +InvalidSearchDistinct , InvalidRequest , BAD_REQUEST ; InvalidSettingsDisplayedAttributes , InvalidRequest , BAD_REQUEST ; InvalidSettingsDistinctAttribute , InvalidRequest , BAD_REQUEST ; InvalidSettingsProximityPrecision , InvalidRequest , BAD_REQUEST ; InvalidSettingsFaceting , InvalidRequest , BAD_REQUEST ; InvalidSettingsFilterableAttributes , InvalidRequest , BAD_REQUEST ; InvalidSettingsPagination , InvalidRequest , BAD_REQUEST ; -InvalidSettingsSearchCutoffMs , InvalidRequest , BAD_REQUEST ; +InvalidSettingsSearchCutoffMs , InvalidRequest , BAD_REQUEST ; InvalidSettingsEmbedders , InvalidRequest , BAD_REQUEST ; InvalidSettingsRankingRules , InvalidRequest , BAD_REQUEST ; InvalidSettingsSearchableAttributes , InvalidRequest , BAD_REQUEST ; @@ -381,6 +382,7 @@ impl ErrorCode for milli::Error { Code::IndexPrimaryKeyMultipleCandidatesFound } UserError::PrimaryKeyCannotBeChanged(_) => Code::IndexPrimaryKeyAlreadyExists, + UserError::InvalidDistinctAttribute { .. } => Code::InvalidSearchDistinct, UserError::SortRankingRuleMissing => Code::InvalidSearchSort, UserError::InvalidFacetsDistribution { .. } => Code::InvalidSearchFacets, UserError::InvalidSortableAttribute { .. } => Code::InvalidSearchSort, diff --git a/meilisearch/src/routes/indexes/search.rs b/meilisearch/src/routes/indexes/search.rs index 6ea6802d9..cf179a234 100644 --- a/meilisearch/src/routes/indexes/search.rs +++ b/meilisearch/src/routes/indexes/search.rs @@ -61,8 +61,7 @@ pub struct SearchQueryGet { filter: Option, #[deserr(default, error = DeserrQueryParamError)] sort: Option, - // TODO change the InvalidSearchSort to InvalidSearchDistinct error - #[deserr(default, error = DeserrQueryParamError)] + #[deserr(default, error = DeserrQueryParamError)] distinct: Option, #[deserr(default, error = DeserrQueryParamError)] show_matches_position: Param, diff --git a/meilisearch/src/search.rs b/meilisearch/src/search.rs index edc3feb5d..522577cde 100644 --- a/meilisearch/src/search.rs +++ b/meilisearch/src/search.rs @@ -75,8 +75,7 @@ pub struct SearchQuery { pub filter: Option, #[deserr(default, error = DeserrJsonError)] pub sort: Option>, - // TODO Change the error to InvalidSearchDistinct - #[deserr(default, error = DeserrJsonError)] + #[deserr(default, error = DeserrJsonError)] pub distinct: Option, #[deserr(default, error = DeserrJsonError)] pub facets: Option>, @@ -393,8 +392,7 @@ pub struct SearchQueryWithIndex { pub filter: Option, #[deserr(default, error = DeserrJsonError)] pub sort: Option>, - // TODO change error to InvalidSearchDistinct - #[deserr(default, error = DeserrJsonError)] + #[deserr(default, error = DeserrJsonError)] pub distinct: Option, #[deserr(default, error = DeserrJsonError)] pub facets: Option>, diff --git a/milli/src/error.rs b/milli/src/error.rs index 83754afe4..7420ce667 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -134,6 +134,17 @@ only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and undersco } )] InvalidSortableAttribute { field: String, valid_fields: BTreeSet, hidden_fields: bool }, + #[error("Attribute `{}` is not filterable and thus, cannot be used as distinct attribute. {}", + .field, + match .valid_fields.is_empty() { + true => "This index does not have configured filterable attributes.".to_string(), + false => format!("Available filterable attributes are: `{}{}`.", + valid_fields.iter().map(AsRef::as_ref).collect::>().join(", "), + .hidden_fields.then_some(", <..hidden-attributes>").unwrap_or(""), + ), + } + )] + InvalidDistinctAttribute { field: String, valid_fields: BTreeSet, hidden_fields: bool }, #[error("Attribute `{}` is not facet-searchable. {}", .field, match .valid_fields.is_empty() { diff --git a/milli/src/search/mod.rs b/milli/src/search/mod.rs index d937875da..922b72d04 100644 --- a/milli/src/search/mod.rs +++ b/milli/src/search/mod.rs @@ -11,8 +11,8 @@ use self::new::{execute_vector_search, PartialSearchResult}; use crate::score_details::{ScoreDetails, ScoringStrategy}; use crate::vector::Embedder; use crate::{ - execute_search, filtered_universe, AscDesc, DefaultSearchLogger, DocumentId, Index, Result, - SearchContext, TimeBudget, + execute_search, filtered_universe, AscDesc, DefaultSearchLogger, DocumentId, Error, Index, + Result, SearchContext, TimeBudget, UserError, }; // Building these factories is not free. @@ -177,9 +177,15 @@ impl<'a> Search<'a> { } if let Some(distinct) = &self.distinct { - if !ctx.index.filterable_fields(ctx.txn)?.contains(distinct) { - // TODO return a real error message - panic!("Distinct search field is not a filterable attribute"); + let filterable_fields = ctx.index.filterable_fields(ctx.txn)?; + if !filterable_fields.contains(distinct) { + let (valid_fields, hidden_fields) = + ctx.index.remove_hidden_fields(ctx.txn, filterable_fields)?; + return Err(Error::UserError(UserError::InvalidDistinctAttribute { + field: distinct.clone(), + valid_fields, + hidden_fields, + })); } }