Merge #1848

1848: Error format and Definition r=MarinPostma a=ManyTheFish



Co-authored-by: many <maxime@meilisearch.com>

meilisearch-lib/src/document_formats.rs

@@ -25,13 +25,15 @@ impl fmt::Display for PayloadType {
 
 #[derive(thiserror::Error, Debug)]
 pub enum DocumentFormatError {
-    #[error("Internal error: {0}")]
+    #[error("Internal error!: {0}")]
     Internal(Box<dyn std::error::Error + Send + Sync + 'static>),
-    #[error("{0}. The {1} payload provided is malformed.")]
+    #[error("The `{1}` payload provided is malformed. `{0}`.")]
     MalformedPayload(
         Box<dyn std::error::Error + Send + Sync + 'static>,
         PayloadType,
     ),
+    #[error("The `{0}` payload must contain at least one document.")]
+    EmptyPayload(PayloadType),
 }
 
 impl From<(PayloadType, milli::documents::Error)> for DocumentFormatError {
@@ -48,6 +50,7 @@ impl ErrorCode for DocumentFormatError {
         match self {
             DocumentFormatError::Internal(_) => Code::Internal,
             DocumentFormatError::MalformedPayload(_, _) => Code::MalformedPayload,
+            DocumentFormatError::EmptyPayload(_) => Code::MalformedPayload,
         }
     }
 }
@@ -57,10 +60,14 @@ internal_error!(DocumentFormatError: io::Error);
 /// reads csv from input and write an obkv batch to writer.
 pub fn read_csv(input: impl Read, writer: impl Write + Seek) -> Result<()> {
     let writer = BufWriter::new(writer);
-    DocumentBatchBuilder::from_csv(input, writer)
-        .map_err(|e| (PayloadType::Csv, e))?
-        .finish()
-        .map_err(|e| (PayloadType::Csv, e))?;
+    let builder =
+        DocumentBatchBuilder::from_csv(input, writer).map_err(|e| (PayloadType::Csv, e))?;
+
+    if builder.len() == 0 {
+        return Err(DocumentFormatError::EmptyPayload(PayloadType::Csv));
+    }
+
+    builder.finish().map_err(|e| (PayloadType::Csv, e))?;
 
     Ok(())
 }
@@ -80,6 +87,10 @@ pub fn read_ndjson(input: impl Read, writer: impl Write + Seek) -> Result<()> {
         buf.clear();
     }
 
+    if builder.len() == 0 {
+        return Err(DocumentFormatError::EmptyPayload(PayloadType::Ndjson));
+    }
+
     builder.finish().map_err(|e| (PayloadType::Ndjson, e))?;
 
     Ok(())
@@ -92,6 +103,11 @@ pub fn read_json(input: impl Read, writer: impl Write + Seek) -> Result<()> {
     builder
         .extend_from_json(input)
         .map_err(|e| (PayloadType::Json, e))?;
+
+    if builder.len() == 0 {
+        return Err(DocumentFormatError::EmptyPayload(PayloadType::Json));
+    }
+
     builder.finish().map_err(|e| (PayloadType::Json, e))?;
 
     Ok(())

meilisearch-lib/src/error.rs

@@ -37,19 +37,17 @@ impl ErrorCode for MilliError<'_> {
                     // TODO: wait for spec for new error codes.
                     UserError::SerdeJson(_)
                     | UserError::MaxDatabaseSizeReached
-                    | UserError::InvalidDocumentId { .. }
                     | UserError::InvalidStoreFile
                     | UserError::NoSpaceLeftOnDevice
-                    | UserError::DocumentLimitReached => Code::Internal,
+                    | UserError::DocumentLimitReached
+                    | UserError::UnknownInternalDocumentId { .. } => Code::Internal,
                     UserError::AttributeLimitReached => Code::MaxFieldsLimitExceeded,
                     UserError::InvalidFilter(_) => Code::Filter,
-                    UserError::InvalidFilterAttribute(_) => Code::Filter,
                     UserError::MissingDocumentId { .. } => Code::MissingDocumentId,
+                    UserError::InvalidDocumentId { .. } => Code::InvalidDocumentId,
                     UserError::MissingPrimaryKey => Code::MissingPrimaryKey,
-                    UserError::PrimaryKeyCannotBeChanged => Code::PrimaryKeyAlreadyPresent,
-                    UserError::PrimaryKeyCannotBeReset => Code::PrimaryKeyAlreadyPresent,
+                    UserError::PrimaryKeyCannotBeChanged(_) => Code::PrimaryKeyAlreadyPresent,
                     UserError::SortRankingRuleMissing => Code::Sort,
-                    UserError::UnknownInternalDocumentId { .. } => Code::DocumentNotFound,
                     UserError::InvalidFacetsDistribution { .. } => Code::BadRequest,
                     UserError::InvalidSortableAttribute { .. } => Code::Sort,
                     UserError::CriterionError(_) => Code::InvalidRankingRule,

meilisearch-lib/src/index/error.rs

@@ -11,14 +11,12 @@ pub type Result<T> = std::result::Result<T, IndexError>;
 pub enum IndexError {
     #[error("Internal error: {0}")]
     Internal(Box<dyn Error + Send + Sync + 'static>),
-    #[error("Document with id {0} not found.")]
+    #[error("Document `{0}` not found.")]
     DocumentNotFound(String),
     #[error("{0}")]
     Facet(#[from] FacetError),
     #[error("{0}")]
     Milli(#[from] milli::Error),
-    #[error("A primary key is already present. It's impossible to update it")]
-    ExistingPrimaryKey,
 }
 
 internal_error!(
@@ -35,21 +33,20 @@ impl ErrorCode for IndexError {
             IndexError::DocumentNotFound(_) => Code::DocumentNotFound,
             IndexError::Facet(e) => e.error_code(),
             IndexError::Milli(e) => MilliError(e).error_code(),
-            IndexError::ExistingPrimaryKey => Code::PrimaryKeyAlreadyPresent,
         }
     }
 }
 
 #[derive(Debug, thiserror::Error)]
 pub enum FacetError {
-    #[error("Invalid facet expression, expected {}, found: {1}", .0.join(", "))]
+    #[error("Invalid syntax for the filter parameter: `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,
+            FacetError::InvalidExpression(_, _) => Code::Filter,
         }
     }
 }

meilisearch-lib/src/index/updates.rs

@@ -11,7 +11,7 @@ use uuid::Uuid;
 use crate::index_controller::updates::status::{Failed, Processed, Processing, UpdateResult};
 use crate::Update;
 
-use super::error::{IndexError, Result};
+use super::error::Result;
 use super::index::{Index, IndexMeta};
 
 fn serialize_with_wildcard<S>(
@@ -222,9 +222,6 @@ impl Index {
         match primary_key {
             Some(primary_key) => {
                 let mut txn = self.write_txn()?;
-                if self.primary_key(&txn)?.is_some() {
-                    return Err(IndexError::ExistingPrimaryKey);
-                }
                 let mut builder = UpdateBuilder::new(0).settings(&mut txn, self);
                 builder.set_primary_key(primary_key);
                 builder.execute(|_, _| ())?;

meilisearch-lib/src/index_controller/dump_actor/error.rs

@@ -9,7 +9,7 @@ pub type Result<T> = std::result::Result<T, DumpActorError>;
 pub enum DumpActorError {
     #[error("Another dump is already in progress")]
     DumpAlreadyRunning,
-    #[error("Dump `{0}` not found")]
+    #[error("Dump `{0}` not found.")]
     DumpDoesNotExist(String),
     #[error("Internal error: {0}")]
     Internal(Box<dyn std::error::Error + Send + Sync + 'static>),

meilisearch-lib/src/index_controller/index_resolver/error.rs

@@ -3,6 +3,7 @@ use std::fmt;
 use meilisearch_error::{Code, ErrorCode};
 use tokio::sync::mpsc::error::SendError as MpscSendError;
 use tokio::sync::oneshot::error::RecvError as OneshotRecvError;
+use uuid::Uuid;
 
 use crate::{error::MilliError, index::error::IndexError};
 
@@ -12,17 +13,19 @@ pub type Result<T> = std::result::Result<T, IndexResolverError>;
 pub enum IndexResolverError {
     #[error("{0}")]
     IndexError(#[from] IndexError),
-    #[error("Index already exists")]
-    IndexAlreadyExists,
-    #[error("Index {0} not found")]
+    #[error("Index `{0}` already exists.")]
+    IndexAlreadyExists(String),
+    #[error("Index `{0}` not found.")]
     UnexistingIndex(String),
     #[error("A primary key is already present. It's impossible to update it")]
     ExistingPrimaryKey,
-    #[error("Internal Error: {0}")]
+    #[error("Internal Error: `{0}`")]
     Internal(Box<dyn std::error::Error + Send + Sync + 'static>),
+    #[error("Internal Error: Index uuid `{0}` is already assigned.")]
+    UuidAlreadyExists(Uuid),
     #[error("{0}")]
     Milli(#[from] milli::Error),
-    #[error("Index must have a valid uid; Index uid can be of type integer or string only composed of alphanumeric characters, hyphens (-) and underscores (_).")]
+    #[error("`{0}` is not a valid index uid. Index uid can be an integer or a string containing only alphanumeric characters, hyphens (-) and underscores (_).")]
     BadlyFormatted(String),
 }
 
@@ -53,10 +56,11 @@ impl ErrorCode for IndexResolverError {
     fn error_code(&self) -> Code {
         match self {
             IndexResolverError::IndexError(e) => e.error_code(),
-            IndexResolverError::IndexAlreadyExists => Code::IndexAlreadyExists,
+            IndexResolverError::IndexAlreadyExists(_) => Code::IndexAlreadyExists,
             IndexResolverError::UnexistingIndex(_) => Code::IndexNotFound,
             IndexResolverError::ExistingPrimaryKey => Code::PrimaryKeyAlreadyPresent,
             IndexResolverError::Internal(_) => Code::Internal,
+            IndexResolverError::UuidAlreadyExists(_) => Code::Internal,
             IndexResolverError::Milli(e) => MilliError(e).error_code(),
             IndexResolverError::BadlyFormatted(_) => Code::InvalidIndexUid,
         }

meilisearch-lib/src/index_controller/index_resolver/index_store.rs

@@ -64,7 +64,7 @@ impl IndexStore for MapIndexStore {
         }
         let path = self.path.join(format!("{}", uuid));
         if path.exists() {
-            return Err(IndexResolverError::IndexAlreadyExists);
+            return Err(IndexResolverError::UuidAlreadyExists(uuid));
         }
 
         let index_size = self.index_size;

meilisearch-lib/src/index_controller/index_resolver/uuid_store.rs

@@ -100,7 +100,7 @@ impl HeedUuidStore {
         let mut txn = env.write_txn()?;
 
         if db.get(&txn, &name)?.is_some() {
-            return Err(IndexResolverError::IndexAlreadyExists);
+            return Err(IndexResolverError::IndexAlreadyExists(name));
         }
 
         db.put(&mut txn, &name, uuid.as_bytes())?;

meilisearch-lib/src/index_controller/snapshot.rs

@@ -222,10 +222,11 @@ mod test {
         setup();
 
         let mut uuid_store = MockUuidStore::new();
-        uuid_store
-            .expect_snapshot()
-            .once()
-            .returning(move |_| Box::pin(err(IndexResolverError::IndexAlreadyExists)));
+        uuid_store.expect_snapshot().once().returning(move |_| {
+            Box::pin(err(IndexResolverError::IndexAlreadyExists(
+                "test".to_string(),
+            )))
+        });
 
         let mut index_store = MockIndexStore::new();
         index_store.expect_get().never();
@@ -264,9 +265,9 @@ mod test {
         let mut indexes = uuids.clone().into_iter().map(|uuid| {
             let mocker = Mocker::default();
             // index returns random error
-            mocker
-                .when("snapshot")
-                .then(|_: &Path| -> IndexResult<()> { Err(IndexError::ExistingPrimaryKey) });
+            mocker.when("snapshot").then(|_: &Path| -> IndexResult<()> {
+                Err(IndexError::DocumentNotFound("1".to_string()))
+            });
             mocker.when("uuid").then(move |_: ()| uuid);
             Index::faux(mocker)
         });

meilisearch-lib/src/index_controller/updates/error.rs

@@ -14,7 +14,7 @@ pub type Result<T> = std::result::Result<T, UpdateLoopError>;
 #[derive(Debug, thiserror::Error)]
 #[allow(clippy::large_enum_variant)]
 pub enum UpdateLoopError {
-    #[error("Update {0} not found.")]
+    #[error("Task `{0}` not found.")]
     UnexistingUpdate(u64),
     #[error("Internal error: {0}")]
     Internal(Box<dyn Error + Send + Sync + 'static>),
@@ -24,9 +24,8 @@ pub enum UpdateLoopError {
     FatalUpdateStoreError,
     #[error("{0}")]
     DocumentFormatError(#[from] DocumentFormatError),
-    // TODO: The reference to actix has to go.
-    #[error("{0}")]
-    PayloadError(#[from] actix_web::error::PayloadError),
+    #[error("The provided payload reached the size limit.")]
+    PayloadTooLarge,
     #[error("A {0} payload is missing.")]
     MissingPayload(DocumentAdditionFormat),
     #[error("{0}")]
@@ -48,6 +47,15 @@ impl From<tokio::sync::oneshot::error::RecvError> for UpdateLoopError {
     }
 }
 
+impl From<actix_web::error::PayloadError> for UpdateLoopError {
+    fn from(other: actix_web::error::PayloadError) -> Self {
+        match other {
+            actix_web::error::PayloadError::Overflow => Self::PayloadTooLarge,
+            _ => Self::Internal(Box::new(other)),
+        }
+    }
+}
+
 internal_error!(
     UpdateLoopError: heed::Error,
     std::io::Error,
@@ -59,14 +67,11 @@ internal_error!(
 impl ErrorCode for UpdateLoopError {
     fn error_code(&self) -> Code {
         match self {
-            Self::UnexistingUpdate(_) => Code::NotFound,
+            Self::UnexistingUpdate(_) => Code::TaskNotFound,
             Self::Internal(_) => Code::Internal,
             Self::FatalUpdateStoreError => Code::Internal,
             Self::DocumentFormatError(error) => error.error_code(),
-            Self::PayloadError(error) => match error {
-                actix_web::error::PayloadError::Overflow => Code::PayloadTooLarge,
-                _ => Code::Internal,
-            },
+            Self::PayloadTooLarge => Code::PayloadTooLarge,
             Self::MissingPayload(_) => Code::MissingPayload,
             Self::IndexError(e) => e.error_code(),
         }

meilisearch-lib/src/index_controller/updates/store/mod.rs

@@ -731,7 +731,7 @@ mod test {
                 mocker
                     .when::<Processing, std::result::Result<Processed, Failed>>("handle_update")
                     .once()
-                    .then(|update| Err(update.fail(IndexError::ExistingPrimaryKey)));
+                    .then(|update| Err(update.fail(IndexError::DocumentNotFound("1".to_string()))));
 
                 Box::pin(ok(Some(Index::faux(mocker))))
             });