210: Error handling r=MarinPostma a=MarinPostma

This pr implements the error handling for meilisearch.

Rather than grouping errors by types, this implementation groups them by scope, each scope enclosing errors from a scope further down, or new errors within this scope. This makes the tracking of the origins of errors easier , and error handling easier at the module level.

All errors that are eventually returned to the user implement the `Into<ResponseError>` trait. `ReponseError` in turn implements the `ErrorCode` trait from `meilisearch-error`.

Some new errors have been introduced with the new engine for which we haven't defined error codes yet. It has been decided with @gmourier that those would return the `internal-error` code until the correct error code is specified.


Co-authored-by: marin postma <postma.marin@protonmail.com>
This commit is contained in:
bors[bot] 2021-06-22 13:21:33 +00:00 committed by GitHub
commit 25af262e79
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
59 changed files with 849 additions and 1105 deletions

View file

@ -3,7 +3,7 @@ use std::io::{BufRead, BufReader, Write};
use std::path::Path;
use std::sync::Arc;
use anyhow::{bail, Context};
use anyhow::{Context, bail};
use heed::RoTxn;
use indexmap::IndexMap;
use milli::update::{IndexDocumentsMethod, UpdateFormat::JsonStream};
@ -11,6 +11,7 @@ use serde::{Deserialize, Serialize};
use crate::option::IndexerOpts;
use super::error::Result;
use super::{update_handler::UpdateHandler, Index, Settings, Unchecked};
#[derive(Serialize, Deserialize)]
@ -23,7 +24,7 @@ const META_FILE_NAME: &str = "meta.json";
const DATA_FILE_NAME: &str = "documents.jsonl";
impl Index {
pub fn dump(&self, path: impl AsRef<Path>) -> anyhow::Result<()> {
pub fn dump(&self, path: impl AsRef<Path>) -> Result<()> {
// acquire write txn make sure any ongoing write is finished before we start.
let txn = self.env.write_txn()?;
@ -33,7 +34,7 @@ impl Index {
Ok(())
}
fn dump_documents(&self, txn: &RoTxn, path: impl AsRef<Path>) -> anyhow::Result<()> {
fn dump_documents(&self, txn: &RoTxn, path: impl AsRef<Path>) -> Result<()> {
let document_file_path = path.as_ref().join(DATA_FILE_NAME);
let mut document_file = File::create(&document_file_path)?;
@ -60,7 +61,7 @@ impl Index {
Ok(())
}
fn dump_meta(&self, txn: &RoTxn, path: impl AsRef<Path>) -> anyhow::Result<()> {
fn dump_meta(&self, txn: &RoTxn, path: impl AsRef<Path>) -> Result<()> {
let meta_file_path = path.as_ref().join(META_FILE_NAME);
let mut meta_file = File::create(&meta_file_path)?;
@ -86,6 +87,7 @@ impl Index {
.as_ref()
.file_name()
.with_context(|| format!("invalid dump index: {}", src.as_ref().display()))?;
let dst_dir_path = dst.as_ref().join("indexes").join(dir_name);
create_dir_all(&dst_dir_path)?;

View file

@ -0,0 +1,52 @@
use std::error::Error;
use meilisearch_error::{Code, ErrorCode};
use serde_json::Value;
use crate::error::MilliError;
pub type Result<T> = std::result::Result<T, IndexError>;
#[derive(Debug, thiserror::Error)]
pub enum IndexError {
#[error("internal error: {0}")]
Internal(Box<dyn Error + Send + Sync + 'static>),
#[error("document with id {0} not found.")]
DocumentNotFound(String),
#[error("error with facet: {0}")]
Facet(#[from] FacetError),
#[error("{0}")]
Milli(#[from] milli::Error),
}
internal_error!(
IndexError: std::io::Error,
heed::Error,
fst::Error,
serde_json::Error
);
impl ErrorCode for IndexError {
fn error_code(&self) -> Code {
match self {
IndexError::Internal(_) => Code::Internal,
IndexError::DocumentNotFound(_) => Code::DocumentNotFound,
IndexError::Facet(e) => e.error_code(),
IndexError::Milli(e) => MilliError(e).error_code(),
}
}
}
#[derive(Debug, thiserror::Error)]
pub enum FacetError {
#[error("invalid facet expression, expected {}, found: {1}", .0.join(", "))]
InvalidExpression(&'static [&'static str], Value),
}
impl ErrorCode for FacetError {
fn error_code(&self) -> Code {
match self {
FacetError::InvalidExpression(_, _) => Code::Facet,
}
}
}

View file

@ -5,19 +5,24 @@ use std::ops::Deref;
use std::path::Path;
use std::sync::Arc;
use anyhow::{bail, Context};
use heed::{EnvOpenOptions, RoTxn};
use milli::obkv_to_json;
use serde::{de::Deserializer, Deserialize};
use serde_json::{Map, Value};
use crate::helpers::EnvSizer;
use error::Result;
pub use search::{SearchQuery, SearchResult, DEFAULT_SEARCH_LIMIT};
use serde::{de::Deserializer, Deserialize};
pub use updates::{Checked, Facets, Settings, Unchecked};
use self::error::IndexError;
pub mod error;
pub mod update_handler;
mod dump;
mod search;
pub mod update_handler;
mod updates;
pub type Document = Map<String, Value>;
@ -33,7 +38,7 @@ impl Deref for Index {
}
}
pub fn deserialize_some<'de, T, D>(deserializer: D) -> Result<Option<T>, D::Error>
pub fn deserialize_some<'de, T, D>(deserializer: D) -> std::result::Result<Option<T>, D::Error>
where
T: Deserialize<'de>,
D: Deserializer<'de>,
@ -42,7 +47,7 @@ where
}
impl Index {
pub fn open(path: impl AsRef<Path>, size: usize) -> anyhow::Result<Self> {
pub fn open(path: impl AsRef<Path>, size: usize) -> Result<Self> {
create_dir_all(&path)?;
let mut options = EnvOpenOptions::new();
options.map_size(size);
@ -50,12 +55,12 @@ impl Index {
Ok(Index(Arc::new(index)))
}
pub fn settings(&self) -> anyhow::Result<Settings<Checked>> {
pub fn settings(&self) -> Result<Settings<Checked>> {
let txn = self.read_txn()?;
self.settings_txn(&txn)
}
pub fn settings_txn(&self, txn: &RoTxn) -> anyhow::Result<Settings<Checked>> {
pub fn settings_txn(&self, txn: &RoTxn) -> Result<Settings<Checked>> {
let displayed_attributes = self
.displayed_fields(&txn)?
.map(|fields| fields.into_iter().map(String::from).collect());
@ -64,10 +69,7 @@ impl Index {
.searchable_fields(&txn)?
.map(|fields| fields.into_iter().map(String::from).collect());
let faceted_attributes = self
.faceted_fields(&txn)?
.into_iter()
.collect();
let faceted_attributes = self.faceted_fields(&txn)?.into_iter().collect();
let criteria = self
.criteria(&txn)?
@ -77,7 +79,7 @@ impl Index {
let stop_words = self
.stop_words(&txn)?
.map(|stop_words| -> anyhow::Result<BTreeSet<_>> {
.map(|stop_words| -> Result<BTreeSet<_>> {
Ok(stop_words.stream().into_strs()?.into_iter().collect())
})
.transpose()?
@ -114,7 +116,7 @@ impl Index {
offset: usize,
limit: usize,
attributes_to_retrieve: Option<Vec<S>>,
) -> anyhow::Result<Vec<Map<String, Value>>> {
) -> Result<Vec<Map<String, Value>>> {
let txn = self.read_txn()?;
let fields_ids_map = self.fields_ids_map(&txn)?;
@ -138,7 +140,7 @@ impl Index {
&self,
doc_id: String,
attributes_to_retrieve: Option<Vec<S>>,
) -> anyhow::Result<Map<String, Value>> {
) -> Result<Map<String, Value>> {
let txn = self.read_txn()?;
let fields_ids_map = self.fields_ids_map(&txn)?;
@ -149,18 +151,18 @@ impl Index {
let internal_id = self
.external_documents_ids(&txn)?
.get(doc_id.as_bytes())
.with_context(|| format!("Document with id {} not found", doc_id))?;
.ok_or_else(|| IndexError::DocumentNotFound(doc_id.clone()))?;
let document = self
.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) => Ok(obkv_to_json(&fields_to_display, &fields_ids_map, document)?),
None => bail!("Document with id {} not found", doc_id),
}
let document = obkv_to_json(&fields_to_display, &fields_ids_map, document)?;
Ok(document)
}
pub fn size(&self) -> u64 {
@ -172,7 +174,7 @@ impl Index {
txn: &heed::RoTxn,
attributes_to_retrieve: &Option<Vec<S>>,
fields_ids_map: &milli::FieldsIdsMap,
) -> anyhow::Result<Vec<u8>> {
) -> Result<Vec<u8>> {
let mut displayed_fields_ids = match self.displayed_fields_ids(&txn)? {
Some(ids) => ids.into_iter().collect::<Vec<_>>(),
None => fields_ids_map.iter().map(|(id, _)| id).collect(),

View file

@ -2,7 +2,6 @@ use std::borrow::Cow;
use std::collections::{BTreeMap, BTreeSet, HashSet};
use std::time::Instant;
use anyhow::bail;
use either::Either;
use heed::RoTxn;
use indexmap::IndexMap;
@ -11,6 +10,9 @@ use milli::{FilterCondition, FieldId, FieldsIdsMap, MatchingWords};
use serde::{Deserialize, Serialize};
use serde_json::Value;
use crate::index::error::FacetError;
use super::error::Result;
use super::Index;
pub type Document = IndexMap<String, Value>;
@ -71,7 +73,7 @@ struct FormatOptions {
}
impl Index {
pub fn perform_search(&self, query: SearchQuery) -> anyhow::Result<SearchResult> {
pub fn perform_search(&self, query: SearchQuery) -> Result<SearchResult> {
let before_search = Instant::now();
let rtxn = self.read_txn()?;
@ -96,7 +98,7 @@ impl Index {
candidates,
..
} = search.execute()?;
let mut documents = Vec::new();
let fields_ids_map = self.fields_ids_map(&rtxn).unwrap();
let displayed_ids = self
@ -158,7 +160,11 @@ impl Index {
let formatter =
Formatter::new(&stop_words, (String::from("<em>"), String::from("</em>")));
for (_id, obkv) in self.documents(&rtxn, documents_ids)? {
let mut documents = Vec::new();
let documents_iter = self.documents(&rtxn, documents_ids)?;
for (_id, obkv) in documents_iter {
let document = make_document(&to_retrieve_ids, &fields_ids_map, obkv)?;
let formatted = format_fields(
&fields_ids_map,
@ -167,6 +173,7 @@ impl Index {
&matching_words,
&formatted_options,
)?;
let hit = SearchHit {
document,
formatted,
@ -182,7 +189,9 @@ impl Index {
if fields.iter().all(|f| f != "*") {
facet_distribution.facets(fields);
}
Some(facet_distribution.candidates(candidates).execute()?)
let distribution = facet_distribution.candidates(candidates).execute()?;
Some(distribution)
}
None => None,
};
@ -326,7 +335,7 @@ fn make_document(
attributes_to_retrieve: &BTreeSet<FieldId>,
field_ids_map: &FieldsIdsMap,
obkv: obkv::KvReader,
) -> anyhow::Result<Document> {
) -> Result<Document> {
let mut document = Document::new();
for attr in attributes_to_retrieve {
if let Some(value) = obkv.get(*attr) {
@ -351,7 +360,7 @@ fn format_fields<A: AsRef<[u8]>>(
formatter: &Formatter<A>,
matching_words: &impl Matcher,
formatted_options: &BTreeMap<FieldId, FormatOptions>,
) -> anyhow::Result<Document> {
) -> Result<Document> {
let mut document = Document::new();
for (id, format) in formatted_options {
@ -514,15 +523,14 @@ impl<'a, A: AsRef<[u8]>> Formatter<'a, A> {
}
}
fn parse_filter(
facets: &Value,
index: &Index,
txn: &RoTxn,
) -> anyhow::Result<Option<FilterCondition>> {
fn parse_filter(facets: &Value, index: &Index, txn: &RoTxn) -> Result<Option<FilterCondition>> {
match facets {
Value::String(expr) => Ok(Some(FilterCondition::from_str(txn, index, expr)?)),
Value::String(expr) => {
let condition = FilterCondition::from_str(txn, index, expr)?;
Ok(Some(condition))
}
Value::Array(arr) => parse_filter_array(txn, index, arr),
v => bail!("Invalid facet expression, expected Array, found: {:?}", v),
v => Err(FacetError::InvalidExpression(&["Array"], v.clone()).into()),
}
}
@ -530,7 +538,7 @@ fn parse_filter_array(
txn: &RoTxn,
index: &Index,
arr: &[Value],
) -> anyhow::Result<Option<FilterCondition>> {
) -> Result<Option<FilterCondition>> {
let mut ands = Vec::new();
for value in arr {
match value {
@ -540,15 +548,18 @@ fn parse_filter_array(
for value in arr {
match value {
Value::String(s) => ors.push(s.clone()),
v => bail!("Invalid facet expression, expected String, found: {:?}", v),
v => {
return Err(FacetError::InvalidExpression(&["String"], v.clone()).into())
}
}
}
ands.push(Either::Left(ors));
}
v => bail!(
"Invalid facet expression, expected String or [String], found: {:?}",
v
),
v => {
return Err(
FacetError::InvalidExpression(&["String", "[String]"], v.clone()).into(),
)
}
}
}

View file

@ -1,7 +1,6 @@
use std::fs::File;
use crate::index::Index;
use anyhow::Result;
use grenad::CompressionType;
use milli::update::UpdateBuilder;
use rayon::ThreadPool;
@ -87,7 +86,7 @@ impl UpdateHandler {
match result {
Ok(result) => Ok(meta.process(result)),
Err(e) => Err(meta.fail(e.to_string())),
Err(e) => Err(meta.fail(e.into())),
}
}
}

View file

@ -1,4 +1,4 @@
use std::collections::{BTreeSet, BTreeMap, HashSet};
use std::collections::{BTreeMap, BTreeSet, HashSet};
use std::io;
use std::marker::PhantomData;
use std::num::NonZeroUsize;
@ -10,9 +10,13 @@ use serde::{Deserialize, Serialize, Serializer};
use crate::index_controller::UpdateResult;
use super::error::Result;
use super::{deserialize_some, Index};
fn serialize_with_wildcard<S>(field: &Option<Option<Vec<String>>>, s: S) -> Result<S::Ok, S::Error>
fn serialize_with_wildcard<S>(
field: &Option<Option<Vec<String>>>,
s: S,
) -> std::result::Result<S::Ok, S::Error>
where
S: Serializer,
{
@ -174,7 +178,7 @@ impl Index {
content: Option<impl io::Read>,
update_builder: UpdateBuilder,
primary_key: Option<&str>,
) -> anyhow::Result<UpdateResult> {
) -> Result<UpdateResult> {
let mut txn = self.write_txn()?;
let result = self.update_documents_txn(
&mut txn,
@ -196,13 +200,12 @@ impl Index {
content: Option<impl io::Read>,
update_builder: UpdateBuilder,
primary_key: Option<&str>,
) -> anyhow::Result<UpdateResult> {
) -> Result<UpdateResult> {
info!("performing document addition");
// 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(|_, _| ())?;
}
@ -228,18 +231,16 @@ impl Index {
Ok(UpdateResult::DocumentsAddition(addition))
}
pub fn clear_documents(&self, update_builder: UpdateBuilder) -> anyhow::Result<UpdateResult> {
pub fn clear_documents(&self, update_builder: UpdateBuilder) -> Result<UpdateResult> {
// We must use the write transaction of the update here.
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(e.into()),
}
let _count = builder.execute()?;
wtxn.commit()
.and(Ok(UpdateResult::Other))
.map_err(Into::into)
}
pub fn update_settings_txn<'a, 'b>(
@ -247,7 +248,7 @@ impl Index {
txn: &mut heed::RwTxn<'a, 'b>,
settings: &Settings<Checked>,
update_builder: UpdateBuilder,
) -> anyhow::Result<UpdateResult> {
) -> Result<UpdateResult> {
// We must use the write transaction of the update here.
let mut builder = update_builder.settings(txn, self);
@ -309,7 +310,7 @@ impl Index {
&self,
settings: &Settings<Checked>,
update_builder: UpdateBuilder,
) -> anyhow::Result<UpdateResult> {
) -> Result<UpdateResult> {
let mut txn = self.write_txn()?;
let result = self.update_settings_txn(&mut txn, settings, update_builder)?;
txn.commit()?;
@ -320,7 +321,7 @@ impl Index {
&self,
document_ids: &[String],
update_builder: UpdateBuilder,
) -> anyhow::Result<UpdateResult> {
) -> Result<UpdateResult> {
let mut txn = self.write_txn()?;
let mut builder = update_builder.delete_documents(&mut txn, self)?;
@ -329,13 +330,10 @@ impl Index {
builder.delete_external_id(id);
});
match builder.execute() {
Ok(deleted) => txn
.commit()
.and(Ok(UpdateResult::DocumentDeletion { deleted }))
.map_err(Into::into),
Err(e) => Err(e.into()),
}
let deleted = builder.execute()?;
txn.commit()
.and(Ok(UpdateResult::DocumentDeletion { deleted }))
.map_err(Into::into)
}
}