From 8306c2b89cd662ae13e8003f73581f0c1bc6d70f Mon Sep 17 00:00:00 2001 From: Tamo Date: Wed, 17 Sep 2025 12:14:11 +0200 Subject: [PATCH] review and fix all error codes --- crates/meilisearch-types/src/error.rs | 13 ++++++++++++- crates/milli/src/error.rs | 2 ++ crates/milli/src/lib.rs | 2 +- crates/milli/src/search/facet/filter.rs | 10 ++-------- .../index_documents/extract/extract_geo_points.rs | 5 ++--- .../milli/src/update/index_documents/extract/mod.rs | 2 -- 6 files changed, 19 insertions(+), 15 deletions(-) diff --git a/crates/meilisearch-types/src/error.rs b/crates/meilisearch-types/src/error.rs index db1e15060..d45580274 100644 --- a/crates/meilisearch-types/src/error.rs +++ b/crates/meilisearch-types/src/error.rs @@ -5,6 +5,7 @@ use actix_web::{self as aweb, HttpResponseBuilder}; use aweb::http::header; use aweb::rt::task::JoinError; use convert_case::Casing; +use milli::cellulite; use milli::heed::{Error as HeedError, MdbError}; use serde::{Deserialize, Serialize}; use utoipa::ToSchema; @@ -239,6 +240,7 @@ InconsistentDocumentChangeHeaders , InvalidRequest , BAD_REQU InvalidDocumentFilter , InvalidRequest , BAD_REQUEST ; InvalidDocumentSort , InvalidRequest , BAD_REQUEST ; InvalidDocumentGeoField , InvalidRequest , BAD_REQUEST ; +InvalidDocumentGeojsonField , InvalidRequest , BAD_REQUEST ; InvalidHeaderValue , InvalidRequest , BAD_REQUEST ; InvalidVectorDimensions , InvalidRequest , BAD_REQUEST ; InvalidVectorsType , InvalidRequest , BAD_REQUEST ; @@ -527,7 +529,16 @@ impl ErrorCode for milli::Error { | UserError::DocumentEditionCompilationError(_) => { Code::EditDocumentsByFunctionError } - UserError::CelluliteError(_) => Code::Internal, + UserError::CelluliteError(err) => match err { + cellulite::Error::BuildCanceled + | cellulite::Error::VersionMismatchOnBuild(_) + | cellulite::Error::DatabaseDoesntExists + | cellulite::Error::Heed(_) + | cellulite::Error::InvalidGeometry(_) + | cellulite::Error::InternalDocIdMissing(_, _) + | cellulite::Error::CannotConvertLineToCell(_, _, _) => Code::Internal, + cellulite::Error::InvalidGeoJson(_) => Code::InvalidDocumentGeojsonField, + }, } } } diff --git a/crates/milli/src/error.rs b/crates/milli/src/error.rs index be4f98922..2a42c37c0 100644 --- a/crates/milli/src/error.rs +++ b/crates/milli/src/error.rs @@ -121,6 +121,8 @@ pub enum FieldIdMapMissingEntry { pub enum UserError { #[error(transparent)] CelluliteError(#[from] cellulite::Error), + #[error("Malformed geojson: {0}")] + MalformedGeojson(serde_json::Error), #[error("A document cannot contain more than 65,535 fields.")] AttributeLimitReached, #[error(transparent)] diff --git a/crates/milli/src/lib.rs b/crates/milli/src/lib.rs index 9def99b84..6e4d833f7 100644 --- a/crates/milli/src/lib.rs +++ b/crates/milli/src/lib.rs @@ -54,7 +54,7 @@ pub use search::new::{ }; use serde_json::Value; pub use thread_pool_no_abort::{PanicCatched, ThreadPoolNoAbort, ThreadPoolNoAbortBuilder}; -pub use {arroy, charabia as tokenizer, hannoy, heed, rhai}; +pub use {arroy, cellulite, charabia as tokenizer, hannoy, heed, rhai}; pub use self::asc_desc::{AscDesc, AscDescError, Member, SortError}; pub use self::attribute_patterns::{AttributePatterns, PatternMatch}; diff --git a/crates/milli/src/search/facet/filter.rs b/crates/milli/src/search/facet/filter.rs index a99ee92da..a234ecb16 100644 --- a/crates/milli/src/search/facet/filter.rs +++ b/crates/milli/src/search/facet/filter.rs @@ -870,10 +870,7 @@ impl<'a> Filter<'a> { Vec::new(), ); - let result = index - .cellulite - .in_shape(rtxn, &polygon) - .map_err(InternalError::CelluliteError)?; // TODO: error code in invalid + let result = index.cellulite.in_shape(rtxn, &polygon)?; r2 = Some(RoaringBitmap::from_iter(result)); // TODO: Remove once we update roaring in meilisearch } @@ -922,10 +919,7 @@ impl<'a> Filter<'a> { } let polygon = geo_types::Polygon::new(geo_types::LineString(coords), Vec::new()); - let result = index - .cellulite - .in_shape(rtxn, &polygon) - .map_err(InternalError::CelluliteError)?; // TODO: update error code + let result = index.cellulite.in_shape(rtxn, &polygon)?; let result = roaring::RoaringBitmap::from_iter(result); // TODO: Remove once we update roaring in meilisearch diff --git a/crates/milli/src/update/index_documents/extract/extract_geo_points.rs b/crates/milli/src/update/index_documents/extract/extract_geo_points.rs index 2ad994af0..c2b8b270e 100644 --- a/crates/milli/src/update/index_documents/extract/extract_geo_points.rs +++ b/crates/milli/src/update/index_documents/extract/extract_geo_points.rs @@ -10,7 +10,7 @@ use crate::error::GeoError; use crate::update::del_add::{DelAdd, KvReaderDelAdd, KvWriterDelAdd}; use crate::update::index_documents::extract_finite_float_from_value; use crate::update::settings::{InnerIndexSettings, InnerIndexSettingsDiff}; -use crate::{DocumentId, FieldId, InternalError, Result}; +use crate::{DocumentId, FieldId, InternalError, Result, UserError}; /// Extracts the geographical coordinates contained in each document under the `_geo` field. /// @@ -174,9 +174,8 @@ fn extract_geojson_field( match settings.geojson_fid { Some(fid) if settings.filterable_attributes_rules.iter().any(|rule| rule.has_geojson()) => { let value = obkv.get(fid).map(KvReaderDelAdd::from_slice).and_then(|r| r.get(deladd)); - // TODO: That's a user error, not an internal error Ok(value - .map(|v| serde_json::from_slice(v).map_err(InternalError::SerdeJson)) + .map(|v| GeoJson::from_reader(v).map_err(UserError::MalformedGeojson)) .transpose()?) } _ => Ok(None), diff --git a/crates/milli/src/update/index_documents/extract/mod.rs b/crates/milli/src/update/index_documents/extract/mod.rs index 886801696..c272a2dff 100644 --- a/crates/milli/src/update/index_documents/extract/mod.rs +++ b/crates/milli/src/update/index_documents/extract/mod.rs @@ -244,9 +244,7 @@ fn send_original_documents_data( let original_documents_chunk = original_documents_chunk.and_then(|c| unsafe { as_cloneable_grenad(&c) })?; - tracing::warn!("Do we have a geojson"); if settings_diff.reindex_geojson() { - tracing::warn!("Yes we do"); let documents_chunk_cloned = original_documents_chunk.clone(); let lmdb_writer_sx_cloned = lmdb_writer_sx.clone(); let settings_diff = settings_diff.clone();