From abdf642d68255d59108bff1e9cdb8886c044f010 Mon Sep 17 00:00:00 2001 From: marin postma Date: Thu, 17 Jun 2021 14:36:32 +0200 Subject: [PATCH] integrate milli errors --- Cargo.lock | 2 +- meilisearch-http/Cargo.toml | 2 +- meilisearch-http/src/error.rs | 42 ++++++++++++++++ meilisearch-http/src/index/dump.rs | 6 +-- meilisearch-http/src/index/error.rs | 5 ++ meilisearch-http/src/index/mod.rs | 48 ++++++------------ meilisearch-http/src/index/search.rs | 17 +++---- meilisearch-http/src/index/updates.rs | 50 ++++++------------- .../src/index_controller/index_actor/actor.rs | 12 ++--- .../src/index_controller/index_actor/error.rs | 5 +- .../src/index_controller/index_actor/mod.rs | 10 +--- .../src/index_controller/index_actor/store.rs | 3 +- meilisearch-http/tests/stats/mod.rs | 2 - 13 files changed, 101 insertions(+), 103 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 757a2a9e8..5cb6c28bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1708,7 +1708,7 @@ dependencies = [ [[package]] name = "milli" version = "0.4.0" -source = "git+https://github.com/meilisearch/milli.git?tag=v0.4.0#3bd4cf94cc60733393b94021fca77eb100bfe17a" +source = "git+https://github.com/meilisearch/milli.git?rev=70bee7d405711d5e6d24b62710e92671be5ac67a#70bee7d405711d5e6d24b62710e92671be5ac67a" dependencies = [ "bstr", "byteorder", diff --git a/meilisearch-http/Cargo.toml b/meilisearch-http/Cargo.toml index 70ab30d34..5ba376f08 100644 --- a/meilisearch-http/Cargo.toml +++ b/meilisearch-http/Cargo.toml @@ -51,7 +51,7 @@ main_error = "0.1.0" meilisearch-error = { path = "../meilisearch-error" } meilisearch-tokenizer = { git = "https://github.com/meilisearch/Tokenizer.git", tag = "v0.2.2" } memmap = "0.7.0" -milli = { git = "https://github.com/meilisearch/milli.git", tag = "v0.4.0" } +milli = { git = "https://github.com/meilisearch/milli.git", rev = "70bee7d405711d5e6d24b62710e92671be5ac67a" } mime = "0.3.16" once_cell = "1.5.2" oxidized-json-checker = "0.3.2" diff --git a/meilisearch-http/src/error.rs b/meilisearch-http/src/error.rs index 7468f0630..5bf41231f 100644 --- a/meilisearch-http/src/error.rs +++ b/meilisearch-http/src/error.rs @@ -7,6 +7,7 @@ use actix_web::body::Body; use actix_web::dev::BaseHttpResponseBuilder; use actix_web::http::StatusCode; use meilisearch_error::{Code, ErrorCode}; +use milli::UserError; use serde::ser::{Serialize, SerializeStruct, Serializer}; use crate::index_controller::error::IndexControllerError; @@ -139,3 +140,44 @@ macro_rules! internal_error { )* } } + +#[derive(Debug)] +pub struct MilliError<'a>(pub &'a milli::Error); + +impl Error for MilliError<'_> {} + +impl fmt::Display for MilliError<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.0.fmt(f) + } +} + +impl ErrorCode for MilliError<'_> { + fn error_code(&self) -> Code { + match self.0 { + milli::Error::InternalError(_) => Code::Internal, + milli::Error::IoError(_) => Code::Internal, + milli::Error::UserError(ref error) => { + match error { + // TODO: wait for spec for new error codes. + UserError::AttributeLimitReached + | UserError::Csv(_) + | UserError::SerdeJson(_) + | UserError::MaxDatabaseSizeReached + | UserError::InvalidCriterionName { .. } + | UserError::InvalidDocumentId { .. } + | UserError::InvalidStoreFile + | UserError::NoSpaceLeftOnDevice + | UserError::DocumentLimitReached => todo!(), + UserError::InvalidFilter(_) => Code::Filter, + UserError::InvalidFilterAttribute(_) => Code::Filter, + UserError::MissingDocumentId { .. } => Code::MissingDocumentId, + UserError::MissingPrimaryKey => Code::MissingPrimaryKey, + UserError::PrimaryKeyCannotBeChanged => Code::PrimaryKeyAlreadyPresent, + UserError::PrimaryKeyCannotBeReset => Code::PrimaryKeyAlreadyPresent, + UserError::UnknownInternalDocumentId { .. } => Code::DocumentNotFound, + } + }, + } + } +} diff --git a/meilisearch-http/src/index/dump.rs b/meilisearch-http/src/index/dump.rs index f66edd9d1..92513d10f 100644 --- a/meilisearch-http/src/index/dump.rs +++ b/meilisearch-http/src/index/dump.rs @@ -11,7 +11,7 @@ use serde::{Deserialize, Serialize}; use crate::option::IndexerOpts; -use super::error::{IndexError, Result}; +use super::error::Result; use super::{update_handler::UpdateHandler, Index, Settings, Unchecked}; #[derive(Serialize, Deserialize)] @@ -38,9 +38,7 @@ impl Index { let document_file_path = path.as_ref().join(DATA_FILE_NAME); let mut document_file = File::create(&document_file_path)?; - let documents = self - .all_documents(txn) - .map_err(|e| IndexError::Internal(e.into()))?; + let documents = self.all_documents(txn)?; let fields_ids_map = self.fields_ids_map(txn)?; // dump documents diff --git a/meilisearch-http/src/index/error.rs b/meilisearch-http/src/index/error.rs index 32f13a143..17b0ef2be 100644 --- a/meilisearch-http/src/index/error.rs +++ b/meilisearch-http/src/index/error.rs @@ -3,6 +3,8 @@ use std::error::Error; use meilisearch_error::{Code, ErrorCode}; use serde_json::Value; +use crate::error::MilliError; + pub type Result = std::result::Result; #[derive(Debug, thiserror::Error)] @@ -13,6 +15,8 @@ pub enum IndexError { DocumentNotFound(String), #[error("error with facet: {0}")] Facet(#[from] FacetError), + #[error("{0}")] + Milli(#[from] milli::Error), } internal_error!( @@ -29,6 +33,7 @@ impl ErrorCode for IndexError { IndexError::Internal(_) => Code::Internal, IndexError::DocumentNotFound(_) => Code::DocumentNotFound, IndexError::Facet(e) => e.error_code(), + IndexError::Milli(e) => MilliError(e).error_code(), } } } diff --git a/meilisearch-http/src/index/mod.rs b/meilisearch-http/src/index/mod.rs index 816629524..ca1518a2e 100644 --- a/meilisearch-http/src/index/mod.rs +++ b/meilisearch-http/src/index/mod.rs @@ -51,8 +51,7 @@ impl Index { create_dir_all(&path)?; let mut options = EnvOpenOptions::new(); options.map_size(size); - let index = - milli::Index::new(options, &path).map_err(|e| IndexError::Internal(e.into()))?; + let index = milli::Index::new(options, &path)?; Ok(Index(Arc::new(index))) } @@ -70,11 +69,7 @@ impl Index { .searchable_fields(&txn)? .map(|fields| fields.into_iter().map(String::from).collect()); - let faceted_attributes = self - .faceted_fields(&txn) - .map_err(|e| IndexError::Internal(Box::new(e)))? - .into_iter() - .collect(); + let faceted_attributes = self.faceted_fields(&txn)?.into_iter().collect(); let criteria = self .criteria(&txn)? @@ -83,8 +78,7 @@ impl Index { .collect(); let stop_words = self - .stop_words(&txn) - .map_err(|e| IndexError::Internal(e.into()))? + .stop_words(&txn)? .map(|stop_words| -> Result> { Ok(stop_words.stream().into_strs()?.into_iter().collect()) }) @@ -126,9 +120,8 @@ impl Index { let txn = self.read_txn()?; let fields_ids_map = self.fields_ids_map(&txn)?; - let fields_to_display = self - .fields_to_display(&txn, &attributes_to_retrieve, &fields_ids_map) - .map_err(|e| IndexError::Internal(e.into()))?; + let fields_to_display = + self.fields_to_display(&txn, &attributes_to_retrieve, &fields_ids_map)?; let iter = self.documents.range(&txn, &(..))?.skip(offset).take(limit); @@ -136,8 +129,7 @@ impl Index { for entry in iter { let (_id, obkv) = entry?; - let object = obkv_to_json(&fields_to_display, &fields_ids_map, obkv) - .map_err(|e| IndexError::Internal(e.into()))?; + let object = obkv_to_json(&fields_to_display, &fields_ids_map, obkv)?; documents.push(object); } @@ -153,31 +145,24 @@ impl Index { let fields_ids_map = self.fields_ids_map(&txn)?; - let fields_to_display = self - .fields_to_display(&txn, &attributes_to_retrieve, &fields_ids_map) - .map_err(|e| IndexError::Internal(e.into()))?; + let fields_to_display = + self.fields_to_display(&txn, &attributes_to_retrieve, &fields_ids_map)?; let internal_id = self - .external_documents_ids(&txn) - .map_err(|e| IndexError::Internal(e.into()))? + .external_documents_ids(&txn)? .get(doc_id.as_bytes()) .ok_or_else(|| IndexError::DocumentNotFound(doc_id.clone()))?; let document = self - .documents(&txn, std::iter::once(internal_id)) - .map_err(|e| IndexError::Internal(e.into()))? + .documents(&txn, std::iter::once(internal_id))? .into_iter() .next() - .map(|(_, d)| d); + .map(|(_, d)| d) + .ok_or(IndexError::DocumentNotFound(doc_id))?; - match document { - Some(document) => { - let document = obkv_to_json(&fields_to_display, &fields_ids_map, document) - .map_err(|e| IndexError::Internal(e.into()))?; - Ok(document) - } - None => Err(IndexError::DocumentNotFound(doc_id)), - } + let document = obkv_to_json(&fields_to_display, &fields_ids_map, document)?; + + Ok(document) } pub fn size(&self) -> u64 { @@ -190,8 +175,7 @@ impl Index { attributes_to_retrieve: &Option>, fields_ids_map: &milli::FieldsIdsMap, ) -> Result> { - let mut displayed_fields_ids = match self.displayed_fields_ids(&txn) - .map_err(|e| IndexError::Internal(Box::new(e)))? { + let mut displayed_fields_ids = match self.displayed_fields_ids(&txn)? { Some(ids) => ids.into_iter().collect::>(), None => fields_ids_map.iter().map(|(id, _)| id).collect(), }; diff --git a/meilisearch-http/src/index/search.rs b/meilisearch-http/src/index/search.rs index 3122f784e..c18a0bbb4 100644 --- a/meilisearch-http/src/index/search.rs +++ b/meilisearch-http/src/index/search.rs @@ -12,7 +12,7 @@ use serde_json::Value; use crate::index::error::FacetError; -use super::error::{IndexError, Result}; +use super::error::Result; use super::Index; pub type Document = IndexMap; @@ -97,9 +97,8 @@ impl Index { matching_words, candidates, .. - } = search - .execute() - .map_err(|e| IndexError::Internal(e.into()))?; + } = search.execute()?; + let fields_ids_map = self.fields_ids_map(&rtxn).unwrap(); let displayed_ids = self @@ -164,6 +163,8 @@ impl Index { let mut documents = Vec::new(); + let documents_iter = self.documents(&rtxn, documents_ids)?; + for (_id, obkv) in self.documents(&rtxn, documents_ids)? { let document = make_document(&to_retrieve_ids, &fields_ids_map, obkv)?; let formatted = format_fields( @@ -191,8 +192,7 @@ impl Index { } let distribution = facet_distribution .candidates(candidates) - .execute() - .map_err(|e| IndexError::Internal(e.into()))?; + .execute()?; Some(distribution) } @@ -528,8 +528,7 @@ impl<'a, A: AsRef<[u8]>> Formatter<'a, A> { fn parse_filter(facets: &Value, index: &Index, txn: &RoTxn) -> Result> { match facets { Value::String(expr) => { - let condition = FilterCondition::from_str(txn, index, expr) - .map_err(|e| IndexError::Internal(e.into()))?; + let condition = FilterCondition::from_str(txn, index, expr)?; Ok(Some(condition)) } Value::Array(arr) => parse_filter_array(txn, index, arr), @@ -566,7 +565,7 @@ fn parse_filter_array( } } - FilterCondition::from_array(txn, &index.0, ands).map_err(|e| IndexError::Internal(Box::new(e))) + Ok(FilterCondition::from_array(txn, &index.0, ands)?) } #[cfg(test)] diff --git a/meilisearch-http/src/index/updates.rs b/meilisearch-http/src/index/updates.rs index 84fc945d5..5d9c82a83 100644 --- a/meilisearch-http/src/index/updates.rs +++ b/meilisearch-http/src/index/updates.rs @@ -8,7 +8,6 @@ use log::info; use milli::update::{IndexDocumentsMethod, UpdateBuilder, UpdateFormat}; use serde::{Deserialize, Serialize, Serializer}; -use crate::index::error::IndexError; use crate::index_controller::UpdateResult; use super::error::Result; @@ -206,11 +205,9 @@ impl Index { // Set the primary key if not set already, ignore if already set. if let (None, Some(primary_key)) = (self.primary_key(txn)?, primary_key) { - let mut builder = UpdateBuilder::new(0) - .settings(txn, &self); + let mut builder = UpdateBuilder::new(0).settings(txn, &self); builder.set_primary_key(primary_key.to_string()); - builder.execute(|_, _| ()) - .map_err(|e| IndexError::Internal(Box::new(e)))?; + builder.execute(|_, _| ())?; } let mut builder = update_builder.index_documents(txn, self); @@ -222,15 +219,9 @@ impl Index { let gzipped = false; let addition = match content { - Some(content) if gzipped => builder - .execute(GzDecoder::new(content), indexing_callback) - .map_err(|e| IndexError::Internal(e.into()))?, - Some(content) => builder - .execute(content, indexing_callback) - .map_err(|e| IndexError::Internal(e.into()))?, - None => builder - .execute(std::io::empty(), indexing_callback) - .map_err(|e| IndexError::Internal(e.into()))?, + Some(content) if gzipped => builder.execute(GzDecoder::new(content), indexing_callback)?, + Some(content) => builder.execute(content, indexing_callback)?, + None => builder.execute(std::io::empty(), indexing_callback)?, }; info!("document addition done: {:?}", addition); @@ -243,13 +234,11 @@ impl Index { let mut wtxn = self.write_txn()?; let builder = update_builder.clear_documents(&mut wtxn, self); - match builder.execute() { - Ok(_count) => wtxn - .commit() - .and(Ok(UpdateResult::Other)) - .map_err(Into::into), - Err(e) => Err(IndexError::Internal(Box::new(e))), - } + let _count = builder.execute()?; + + wtxn.commit() + .and(Ok(UpdateResult::Other)) + .map_err(Into::into) } pub fn update_settings_txn<'a, 'b>( @@ -308,9 +297,7 @@ impl Index { } } - builder - .execute(|indexing_step, update_id| info!("update {}: {:?}", update_id, indexing_step)) - .map_err(|e| IndexError::Internal(e.into()))?; + builder.execute(|indexing_step, update_id| info!("update {}: {:?}", update_id, indexing_step))?; Ok(UpdateResult::Other) } @@ -332,22 +319,17 @@ impl Index { update_builder: UpdateBuilder, ) -> Result { let mut txn = self.write_txn()?; - let mut builder = update_builder - .delete_documents(&mut txn, self) - .map_err(|e| IndexError::Internal(e.into()))?; + let mut builder = update_builder.delete_documents(&mut txn, self)?; // We ignore unexisting document ids document_ids.iter().for_each(|id| { builder.delete_external_id(id); }); - match builder.execute() { - Ok(deleted) => txn - .commit() - .and(Ok(UpdateResult::DocumentDeletion { deleted })) - .map_err(Into::into), - Err(e) => Err(IndexError::Internal(Box::new(e))), - } + let deleted = builder.execute()?; + txn.commit() + .and(Ok(UpdateResult::DocumentDeletion { deleted })) + .map_err(Into::into) } } diff --git a/meilisearch-http/src/index_controller/index_actor/actor.rs b/meilisearch-http/src/index_controller/index_actor/actor.rs index 28c783eb2..f7b1a0981 100644 --- a/meilisearch-http/src/index_controller/index_actor/actor.rs +++ b/meilisearch-http/src/index_controller/index_actor/actor.rs @@ -268,9 +268,7 @@ impl IndexActor { } let mut builder = UpdateBuilder::new(0).settings(&mut txn, &index); builder.set_primary_key(primary_key); - builder - .execute(|_, _| ()) - .map_err(|e| IndexActorError::Internal(Box::new(e)))?; + builder.execute(|_, _| ())?; let meta = IndexMeta::new_txn(&index, &txn)?; txn.commit()?; Ok(meta) @@ -340,13 +338,9 @@ impl IndexActor { Ok(IndexStats { size: index.size(), - number_of_documents: index - .number_of_documents(&rtxn) - .map_err(|e| IndexActorError::Internal(Box::new(e)))?, + number_of_documents: index.number_of_documents(&rtxn)?, is_indexing: None, - fields_distribution: index - .fields_distribution(&rtxn) - .map_err(|e| IndexActorError::Internal(e.into()))?, + fields_distribution: index.fields_distribution(&rtxn)?, }) }) .await? diff --git a/meilisearch-http/src/index_controller/index_actor/error.rs b/meilisearch-http/src/index_controller/index_actor/error.rs index 7c7ad1607..37b43980c 100644 --- a/meilisearch-http/src/index_controller/index_actor/error.rs +++ b/meilisearch-http/src/index_controller/index_actor/error.rs @@ -1,6 +1,6 @@ use meilisearch_error::{Code, ErrorCode}; -use crate::index::error::IndexError; +use crate::{error::MilliError, index::error::IndexError}; pub type Result = std::result::Result; @@ -16,6 +16,8 @@ pub enum IndexActorError { ExistingPrimaryKey, #[error("Internal Index Error: {0}")] Internal(Box), + #[error("{0}")] + Milli(#[from] milli::Error), } macro_rules! internal_error { @@ -40,6 +42,7 @@ impl ErrorCode for IndexActorError { IndexActorError::UnexistingIndex => Code::IndexNotFound, IndexActorError::ExistingPrimaryKey => Code::PrimaryKeyAlreadyPresent, IndexActorError::Internal(_) => Code::Internal, + IndexActorError::Milli(e) => MilliError(e).error_code(), } } } diff --git a/meilisearch-http/src/index_controller/index_actor/mod.rs b/meilisearch-http/src/index_controller/index_actor/mod.rs index 65f196e41..4085dc0bd 100644 --- a/meilisearch-http/src/index_controller/index_actor/mod.rs +++ b/meilisearch-http/src/index_controller/index_actor/mod.rs @@ -17,8 +17,6 @@ use crate::index::{Checked, Document, Index, SearchQuery, SearchResult, Settings use crate::index_controller::{Failed, IndexStats, Processed, Processing}; use error::Result; -use self::error::IndexActorError; - use super::IndexSettings; mod actor; @@ -42,12 +40,8 @@ impl IndexMeta { } fn new_txn(index: &Index, txn: &heed::RoTxn) -> Result { - let created_at = index - .created_at(&txn) - .map_err(|e| IndexActorError::Internal(Box::new(e)))?; - let updated_at = index - .updated_at(&txn) - .map_err(|e| IndexActorError::Internal(Box::new(e)))?; + let created_at = index.created_at(&txn)?; + let updated_at = index.updated_at(&txn)?; let primary_key = index.primary_key(&txn)?.map(String::from); Ok(Self { created_at, diff --git a/meilisearch-http/src/index_controller/index_actor/store.rs b/meilisearch-http/src/index_controller/index_actor/store.rs index 4c2aed622..2cfda61b5 100644 --- a/meilisearch-http/src/index_controller/index_actor/store.rs +++ b/meilisearch-http/src/index_controller/index_actor/store.rs @@ -61,8 +61,7 @@ impl IndexStore for MapIndexStore { let mut builder = UpdateBuilder::new(0).settings(&mut txn, &index); builder.set_primary_key(primary_key); - builder.execute(|_, _| ()) - .map_err(|e| IndexActorError::Internal(Box::new(e)))?; + builder.execute(|_, _| ())?; txn.commit()?; } diff --git a/meilisearch-http/tests/stats/mod.rs b/meilisearch-http/tests/stats/mod.rs index 130849a64..b408823de 100644 --- a/meilisearch-http/tests/stats/mod.rs +++ b/meilisearch-http/tests/stats/mod.rs @@ -39,8 +39,6 @@ async fn stats() { assert_eq!(response["indexes"]["test"]["numberOfDocuments"], 0); assert!(response["indexes"]["test"]["isIndexing"] == false); - let last_update = response["lastUpdate"].as_str().unwrap(); - let documents = json!([ { "id": 1,