From 59f64a5256b8f56347385da90638181668919956 Mon Sep 17 00:00:00 2001 From: ManyTheFish Date: Tue, 20 Jun 2023 17:04:59 +0200 Subject: [PATCH] Return an error when an attribute is not searchable --- meilisearch-types/src/error.rs | 3 + meilisearch/tests/search/errors.rs | 24 ++++++++ .../tests/search/restrict_searchable.rs | 16 ------ milli/src/error.rs | 10 ++++ milli/src/search/new/mod.rs | 55 ++++++++++++++++++- 5 files changed, 89 insertions(+), 19 deletions(-) diff --git a/meilisearch-types/src/error.rs b/meilisearch-types/src/error.rs index 8ce7102cb..8bfd15810 100644 --- a/meilisearch-types/src/error.rs +++ b/meilisearch-types/src/error.rs @@ -333,6 +333,9 @@ impl ErrorCode for milli::Error { UserError::SortRankingRuleMissing => Code::InvalidSearchSort, UserError::InvalidFacetsDistribution { .. } => Code::InvalidSearchFacets, UserError::InvalidSortableAttribute { .. } => Code::InvalidSearchSort, + UserError::InvalidSearchableAttribute { .. } => { + Code::InvalidAttributesToSearchOn + } UserError::CriterionError(_) => Code::InvalidSettingsRankingRules, UserError::InvalidGeoField { .. } => Code::InvalidDocumentGeoField, UserError::SortError(_) => Code::InvalidSearchSort, diff --git a/meilisearch/tests/search/errors.rs b/meilisearch/tests/search/errors.rs index f314e8800..72c2c5b90 100644 --- a/meilisearch/tests/search/errors.rs +++ b/meilisearch/tests/search/errors.rs @@ -963,3 +963,27 @@ async fn sort_unset_ranking_rule() { ) .await; } + +#[actix_rt::test] +async fn search_on_unknown_field() { + let server = Server::new().await; + let index = server.index("test"); + let documents = DOCUMENTS.clone(); + index.add_documents(documents, None).await; + index.wait_task(0).await; + + index + .search( + json!({"q": "Captain Marvel", "attributesToSearchOn": ["unknown"]}), + |response, code| { + assert_eq!(400, code, "{}", response); + assert_eq!(response, json!({ + "message": "Attribute `unknown` is not searchable. Available searchable attributes are: `id, title`.", + "code": "invalid_attributes_to_search_on", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#invalid_attributes_to_search_on" + })); + }, + ) + .await; +} diff --git a/meilisearch/tests/search/restrict_searchable.rs b/meilisearch/tests/search/restrict_searchable.rs index 8805d097d..9379b6558 100644 --- a/meilisearch/tests/search/restrict_searchable.rs +++ b/meilisearch/tests/search/restrict_searchable.rs @@ -75,22 +75,6 @@ async fn simple_search_on_title_matching_strategy_all() { .await; } -#[actix_rt::test] -async fn simple_search_on_unknown_field() { - let server = Server::new().await; - let index = index_with_documents(&server, &SIMPLE_SEARCH_DOCUMENTS).await; - // simple search on unknown field shouldn't return any document. - index - .search( - json!({"q": "Captain Marvel", "attributesToSearchOn": ["unknown"]}), - |response, code| { - assert_eq!(200, code, "{}", response); - assert_eq!(response["hits"].as_array().unwrap().len(), 0); - }, - ) - .await; -} - #[actix_rt::test] async fn simple_search_on_no_field() { let server = Server::new().await; diff --git a/milli/src/error.rs b/milli/src/error.rs index 8d55eabbd..a3c843254 100644 --- a/milli/src/error.rs +++ b/milli/src/error.rs @@ -124,6 +124,16 @@ only composed of alphanumeric characters (a-z A-Z 0-9), hyphens (-) and undersco } )] InvalidSortableAttribute { field: String, valid_fields: BTreeSet }, + #[error("Attribute `{}` is not searchable. Available searchable attributes are: `{}{}`.", + .field, + .valid_fields.iter().map(AsRef::as_ref).collect::>().join(", "), + .hidden_fields.then_some(", <..hidden-attributes>").unwrap_or(""), + )] + InvalidSearchableAttribute { + field: String, + valid_fields: BTreeSet, + hidden_fields: bool, + }, #[error("{}", HeedError::BadOpenOptions)] InvalidLmdbOpenOptions, #[error("You must specify where `sort` is listed in the rankingRules setting to use the sort parameter at search time.")] diff --git a/milli/src/search/new/mod.rs b/milli/src/search/new/mod.rs index 015940945..3accd8528 100644 --- a/milli/src/search/new/mod.rs +++ b/milli/src/search/new/mod.rs @@ -20,7 +20,7 @@ mod sort; #[cfg(test)] mod tests; -use std::collections::HashSet; +use std::collections::{BTreeSet, HashSet}; use bucket_sort::{bucket_sort, BucketSortOutput}; use charabia::TokenizerBuilder; @@ -44,6 +44,7 @@ use self::geo_sort::GeoSort; pub use self::geo_sort::Strategy as GeoSortStrategy; use self::graph_based_ranking_rule::Words; use self::interner::Interned; +use crate::error::FieldIdMapMissingEntry; use crate::score_details::{ScoreDetails, ScoringStrategy}; use crate::search::new::distinct::apply_distinct_rule; use crate::{AscDesc, DocumentId, Filter, Index, Member, Result, TermsMatchingStrategy, UserError}; @@ -76,8 +77,56 @@ impl<'ctx> SearchContext<'ctx> { pub fn searchable_attributes(&mut self, searchable_attributes: &'ctx [String]) -> Result<()> { let fids_map = self.index.fields_ids_map(self.txn)?; - let restricted_fids = - searchable_attributes.iter().filter_map(|name| fids_map.id(name)).collect(); + let searchable_names = self.index.searchable_fields(self.txn)?; + + let mut restricted_fids = Vec::new(); + for field_name in searchable_attributes { + let searchable_contains_name = + searchable_names.as_ref().map(|sn| sn.iter().any(|name| name == field_name)); + let fid = match (fids_map.id(field_name), searchable_contains_name) { + // The Field id exist and the field is searchable + (Some(fid), Some(true)) | (Some(fid), None) => fid, + // The field is searchable but the Field id doesn't exist => Internal Error + (None, Some(true)) => { + return Err(FieldIdMapMissingEntry::FieldName { + field_name: field_name.to_string(), + process: "search", + } + .into()) + } + // The field is not searchable => User error + _otherwise => { + let mut valid_fields: BTreeSet<_> = + fids_map.names().map(String::from).collect(); + + // Filter by the searchable names + if let Some(sn) = searchable_names { + let searchable_names = sn.iter().map(|s| s.to_string()).collect(); + valid_fields = &valid_fields & &searchable_names; + } + + let searchable_count = valid_fields.len(); + + // Remove hidden fields + if let Some(dn) = self.index.displayed_fields(self.txn)? { + let displayable_names = dn.iter().map(|s| s.to_string()).collect(); + valid_fields = &valid_fields & &displayable_names; + } + + let hidden_fields = searchable_count > valid_fields.len(); + let field = field_name.to_string(); + return Err(UserError::InvalidSearchableAttribute { + field, + valid_fields, + hidden_fields, + } + .into()); + } + }; + + restricted_fids.push(fid); + } + self.restricted_fids = Some(restricted_fids); Ok(())