From 1f75caae8849c1e5c2b990922ca9fe3ed85094ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Lecrenier?= Date: Wed, 26 Oct 2022 12:57:29 +0200 Subject: [PATCH] Fix a few index swap bugs. 1. Details of the indexSwap task 2. Query tasks with type=indexUid 3. Synchronous error message for multiple index not found --- dump/src/lib.rs | 6 ++-- index-scheduler/src/autobatcher.rs | 5 ++- index-scheduler/src/batch.rs | 4 +-- index-scheduler/src/insta_snapshot.rs | 2 +- index-scheduler/src/lib.rs | 10 ++++-- .../swap_indexes/first_swap_processed.snap | 2 +- .../swap_indexes/second_swap_processed.snap | 4 +-- .../third_empty_swap_processed.snap | 6 ++-- index-scheduler/src/utils.rs | 6 ++-- meilisearch-http/src/error.rs | 5 +++ meilisearch-http/src/routes/swap_indexes.rs | 33 +++++++++++++++---- meilisearch-http/src/routes/tasks.rs | 8 +++-- meilisearch-types/src/tasks.rs | 19 +++++++---- 13 files changed, 75 insertions(+), 35 deletions(-) diff --git a/dump/src/lib.rs b/dump/src/lib.rs index 567043a57..21b4497df 100644 --- a/dump/src/lib.rs +++ b/dump/src/lib.rs @@ -5,7 +5,7 @@ use meilisearch_types::error::ResponseError; use meilisearch_types::keys::Key; use meilisearch_types::milli::update::IndexDocumentsMethod; use meilisearch_types::settings::Unchecked; -use meilisearch_types::tasks::{Details, KindWithContent, Status, Task, TaskId}; +use meilisearch_types::tasks::{Details, IndexSwap, KindWithContent, Status, Task, TaskId}; use meilisearch_types::InstanceUid; use roaring::RoaringBitmap; use serde::{Deserialize, Serialize}; @@ -113,9 +113,7 @@ pub enum KindDump { IndexUpdate { primary_key: Option, }, - IndexSwap { - swaps: Vec<(String, String)>, - }, + IndexSwap { swaps: Vec }, TaskCancelation { query: String, tasks: RoaringBitmap, diff --git a/index-scheduler/src/autobatcher.rs b/index-scheduler/src/autobatcher.rs index 7b849efb0..d1ed691c6 100644 --- a/index-scheduler/src/autobatcher.rs +++ b/index-scheduler/src/autobatcher.rs @@ -433,6 +433,7 @@ pub fn autobatch( #[cfg(test)] mod tests { + use meilisearch_types::tasks::IndexSwap; use uuid::Uuid; use super::*; @@ -492,7 +493,9 @@ mod tests { } fn idx_swap() -> KindWithContent { - KindWithContent::IndexSwap { swaps: vec![(String::from("doggo"), String::from("catto"))] } + KindWithContent::IndexSwap { + swaps: vec![IndexSwap { indexes: (String::from("doggo"), String::from("catto")) }], + } } #[test] diff --git a/index-scheduler/src/batch.rs b/index-scheduler/src/batch.rs index 69c14e8ea..2972778c2 100644 --- a/index-scheduler/src/batch.rs +++ b/index-scheduler/src/batch.rs @@ -836,8 +836,8 @@ impl IndexScheduler { } else { unreachable!() }; - for (lhs, rhs) in swaps { - self.apply_index_swap(&mut wtxn, task.uid, lhs, rhs)?; + for swap in swaps { + self.apply_index_swap(&mut wtxn, task.uid, &swap.indexes.0, &swap.indexes.1)?; } wtxn.commit()?; task.status = Status::Succeeded; diff --git a/index-scheduler/src/insta_snapshot.rs b/index-scheduler/src/insta_snapshot.rs index dd5b6bb8b..50846c555 100644 --- a/index-scheduler/src/insta_snapshot.rs +++ b/index-scheduler/src/insta_snapshot.rs @@ -194,7 +194,7 @@ fn snapshot_details(d: &Details) -> String { format!("{{ dump_uid: {dump_uid:?} }}") }, Details::IndexSwap { swaps } => { - format!("{{ indexes: {swaps:?} }}") + format!("{{ swaps: {swaps:?} }}") } } } diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs index e0703f9f2..0d9c767d3 100644 --- a/index-scheduler/src/lib.rs +++ b/index-scheduler/src/lib.rs @@ -971,6 +971,7 @@ mod tests { use meilisearch_types::milli::update::IndexDocumentsMethod::{ ReplaceDocuments, UpdateDocuments, }; + use meilisearch_types::tasks::IndexSwap; use meilisearch_types::VERSION_FILE_NAME; use tempfile::TempDir; use time::Duration; @@ -1543,7 +1544,10 @@ mod tests { index_scheduler .register(KindWithContent::IndexSwap { - swaps: vec![("a".to_owned(), "b".to_owned()), ("c".to_owned(), "d".to_owned())], + swaps: vec![ + IndexSwap { indexes: ("a".to_owned(), "b".to_owned()) }, + IndexSwap { indexes: ("c".to_owned(), "d".to_owned()) }, + ], }) .unwrap(); index_scheduler.assert_internally_consistent(); @@ -1553,7 +1557,9 @@ mod tests { snapshot!(snapshot_index_scheduler(&index_scheduler), name: "first_swap_processed"); index_scheduler - .register(KindWithContent::IndexSwap { swaps: vec![("a".to_owned(), "c".to_owned())] }) + .register(KindWithContent::IndexSwap { + swaps: vec![IndexSwap { indexes: ("a".to_owned(), "c".to_owned()) }], + }) .unwrap(); index_scheduler.assert_internally_consistent(); diff --git a/index-scheduler/src/snapshots/lib.rs/swap_indexes/first_swap_processed.snap b/index-scheduler/src/snapshots/lib.rs/swap_indexes/first_swap_processed.snap index 6744367c3..7fafc3db5 100644 --- a/index-scheduler/src/snapshots/lib.rs/swap_indexes/first_swap_processed.snap +++ b/index-scheduler/src/snapshots/lib.rs/swap_indexes/first_swap_processed.snap @@ -10,7 +10,7 @@ source: index-scheduler/src/lib.rs 1 {uid: 1, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "a", primary_key: Some("id") }} 2 {uid: 2, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "d", primary_key: Some("id") }} 3 {uid: 3, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "c", primary_key: Some("id") }} -4 {uid: 4, status: succeeded, details: { indexes: [("a", "b"), ("c", "d")] }, kind: IndexSwap { swaps: [("a", "b"), ("c", "d")] }} +4 {uid: 4, status: succeeded, details: { swaps: [IndexSwap { indexes: ("a", "b") }, IndexSwap { indexes: ("c", "d") }] }, kind: IndexSwap { swaps: [IndexSwap { indexes: ("a", "b") }, IndexSwap { indexes: ("c", "d") }] }} ---------------------------------------------------------------------- ### Status: enqueued [] diff --git a/index-scheduler/src/snapshots/lib.rs/swap_indexes/second_swap_processed.snap b/index-scheduler/src/snapshots/lib.rs/swap_indexes/second_swap_processed.snap index 7cb38dbbd..d820e04e6 100644 --- a/index-scheduler/src/snapshots/lib.rs/swap_indexes/second_swap_processed.snap +++ b/index-scheduler/src/snapshots/lib.rs/swap_indexes/second_swap_processed.snap @@ -10,8 +10,8 @@ source: index-scheduler/src/lib.rs 1 {uid: 1, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "c", primary_key: Some("id") }} 2 {uid: 2, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "d", primary_key: Some("id") }} 3 {uid: 3, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "a", primary_key: Some("id") }} -4 {uid: 4, status: succeeded, details: { indexes: [("c", "b"), ("a", "d")] }, kind: IndexSwap { swaps: [("c", "b"), ("a", "d")] }} -5 {uid: 5, status: succeeded, details: { indexes: [("a", "c")] }, kind: IndexSwap { swaps: [("a", "c")] }} +4 {uid: 4, status: succeeded, details: { swaps: [IndexSwap { indexes: ("c", "b") }, IndexSwap { indexes: ("a", "d") }] }, kind: IndexSwap { swaps: [IndexSwap { indexes: ("c", "b") }, IndexSwap { indexes: ("a", "d") }] }} +5 {uid: 5, status: succeeded, details: { swaps: [IndexSwap { indexes: ("a", "c") }] }, kind: IndexSwap { swaps: [IndexSwap { indexes: ("a", "c") }] }} ---------------------------------------------------------------------- ### Status: enqueued [] diff --git a/index-scheduler/src/snapshots/lib.rs/swap_indexes/third_empty_swap_processed.snap b/index-scheduler/src/snapshots/lib.rs/swap_indexes/third_empty_swap_processed.snap index 9d5cc2e6f..26bd1b0d3 100644 --- a/index-scheduler/src/snapshots/lib.rs/swap_indexes/third_empty_swap_processed.snap +++ b/index-scheduler/src/snapshots/lib.rs/swap_indexes/third_empty_swap_processed.snap @@ -10,9 +10,9 @@ source: index-scheduler/src/lib.rs 1 {uid: 1, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "c", primary_key: Some("id") }} 2 {uid: 2, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "d", primary_key: Some("id") }} 3 {uid: 3, status: succeeded, details: { primary_key: Some("id") }, kind: IndexCreation { index_uid: "a", primary_key: Some("id") }} -4 {uid: 4, status: succeeded, details: { indexes: [("c", "b"), ("a", "d")] }, kind: IndexSwap { swaps: [("c", "b"), ("a", "d")] }} -5 {uid: 5, status: succeeded, details: { indexes: [("a", "c")] }, kind: IndexSwap { swaps: [("a", "c")] }} -6 {uid: 6, status: succeeded, details: { indexes: [] }, kind: IndexSwap { swaps: [] }} +4 {uid: 4, status: succeeded, details: { swaps: [IndexSwap { indexes: ("c", "b") }, IndexSwap { indexes: ("a", "d") }] }, kind: IndexSwap { swaps: [IndexSwap { indexes: ("c", "b") }, IndexSwap { indexes: ("a", "d") }] }} +5 {uid: 5, status: succeeded, details: { swaps: [IndexSwap { indexes: ("a", "c") }] }, kind: IndexSwap { swaps: [IndexSwap { indexes: ("a", "c") }] }} +6 {uid: 6, status: succeeded, details: { swaps: [] }, kind: IndexSwap { swaps: [] }} ---------------------------------------------------------------------- ### Status: enqueued [] diff --git a/index-scheduler/src/utils.rs b/index-scheduler/src/utils.rs index 60889396d..992e60fbb 100644 --- a/index-scheduler/src/utils.rs +++ b/index-scheduler/src/utils.rs @@ -5,7 +5,7 @@ use std::ops::Bound; use meilisearch_types::heed::types::{DecodeIgnore, OwnedType}; use meilisearch_types::heed::{Database, RoTxn, RwTxn}; use meilisearch_types::milli::{CboRoaringBitmapCodec, BEU32}; -use meilisearch_types::tasks::{Details, Kind, KindWithContent, Status}; +use meilisearch_types::tasks::{Details, IndexSwap, Kind, KindWithContent, Status}; use roaring::{MultiOps, RoaringBitmap}; use time::OffsetDateTime; @@ -244,7 +244,7 @@ pub fn swap_index_uid_in_task(task: &mut Task, swap: (&str, &str)) { K::IndexCreation { index_uid, .. } => index_uids.push(index_uid), K::IndexUpdate { index_uid, .. } => index_uids.push(index_uid), K::IndexSwap { swaps } => { - for (lhs, rhs) in swaps.iter_mut() { + for IndexSwap { indexes: (lhs, rhs) } in swaps.iter_mut() { if lhs == swap.0 || lhs == swap.1 { index_uids.push(lhs); } @@ -259,7 +259,7 @@ pub fn swap_index_uid_in_task(task: &mut Task, swap: (&str, &str)) { | K::SnapshotCreation => (), }; if let Some(Details::IndexSwap { swaps }) = &mut task.details { - for (lhs, rhs) in swaps.iter_mut() { + for IndexSwap { indexes: (lhs, rhs) } in swaps.iter_mut() { if lhs == swap.0 || lhs == swap.1 { index_uids.push(lhs); } diff --git a/meilisearch-http/src/error.rs b/meilisearch-http/src/error.rs index 7350aa4b4..b0f29f9fd 100644 --- a/meilisearch-http/src/error.rs +++ b/meilisearch-http/src/error.rs @@ -38,6 +38,10 @@ pub enum MeilisearchHttpError { .0.iter().map(|s| format!("`{}`", s)).collect::>().join(", ") )] SwapDuplicateIndexesFound(Vec), + #[error("Two indexes must be given for each swap. The list `{:?}` contains {} indexes.", + .0, .0.len() + )] + SwapIndexPayloadWrongLength(Vec), #[error(transparent)] IndexUid(#[from] IndexUidFormatError), #[error(transparent)] @@ -70,6 +74,7 @@ impl ErrorCode for MeilisearchHttpError { MeilisearchHttpError::IndexesNotFound(_) => Code::IndexNotFound, MeilisearchHttpError::SwapDuplicateIndexFound(_) => Code::DuplicateIndexFound, MeilisearchHttpError::SwapDuplicateIndexesFound(_) => Code::DuplicateIndexFound, + MeilisearchHttpError::SwapIndexPayloadWrongLength(_) => Code::BadRequest, MeilisearchHttpError::IndexUid(e) => e.error_code(), MeilisearchHttpError::SerdeJson(_) => Code::Internal, MeilisearchHttpError::HeedError(_) => Code::Internal, diff --git a/meilisearch-http/src/routes/swap_indexes.rs b/meilisearch-http/src/routes/swap_indexes.rs index 86d66910b..08536591c 100644 --- a/meilisearch-http/src/routes/swap_indexes.rs +++ b/meilisearch-http/src/routes/swap_indexes.rs @@ -4,7 +4,7 @@ use actix_web::web::Data; use actix_web::{web, HttpResponse}; use index_scheduler::IndexScheduler; use meilisearch_types::error::ResponseError; -use meilisearch_types::tasks::KindWithContent; +use meilisearch_types::tasks::{IndexSwap, KindWithContent}; use serde::Deserialize; use crate::error::MeilisearchHttpError; @@ -16,11 +16,10 @@ use crate::routes::tasks::TaskView; pub fn configure(cfg: &mut web::ServiceConfig) { cfg.service(web::resource("").route(web::post().to(SeqHandler(swap_indexes)))); } - #[derive(Deserialize, Debug, Clone, PartialEq, Eq)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct SwapIndexesPayload { - indexes: (String, String), + indexes: Vec, } pub async fn swap_indexes( @@ -33,23 +32,43 @@ pub async fn swap_indexes( let mut indexes_set = HashSet::::default(); let mut unknown_indexes = HashSet::new(); let mut duplicate_indexes = HashSet::new(); - for SwapIndexesPayload { indexes: (lhs, rhs) } in params.into_inner().into_iter() { + for SwapIndexesPayload { indexes } in params.into_inner().into_iter() { + let (lhs, rhs) = match indexes.as_slice() { + [lhs, rhs] => (lhs, rhs), + _ => { + return Err(MeilisearchHttpError::SwapIndexPayloadWrongLength(indexes).into()); + } + }; if !search_rules.is_index_authorized(&lhs) { unknown_indexes.insert(lhs.clone()); } if !search_rules.is_index_authorized(&rhs) { unknown_indexes.insert(rhs.clone()); } + match index_scheduler.index(&lhs) { + Ok(_) => (), + Err(index_scheduler::Error::IndexNotFound(_)) => { + unknown_indexes.insert(lhs.clone()); + } + Err(e) => return Err(e.into()), + } + match index_scheduler.index(&rhs) { + Ok(_) => (), + Err(index_scheduler::Error::IndexNotFound(_)) => { + unknown_indexes.insert(rhs.clone()); + } + Err(e) => return Err(e.into()), + } - swaps.push((lhs.clone(), rhs.clone())); + swaps.push(IndexSwap { indexes: (lhs.clone(), rhs.clone()) }); let is_unique_index_lhs = indexes_set.insert(lhs.clone()); if !is_unique_index_lhs { - duplicate_indexes.insert(lhs); + duplicate_indexes.insert(lhs.clone()); } let is_unique_index_rhs = indexes_set.insert(rhs.clone()); if !is_unique_index_rhs { - duplicate_indexes.insert(rhs); + duplicate_indexes.insert(rhs.clone()); } } if !duplicate_indexes.is_empty() { diff --git a/meilisearch-http/src/routes/tasks.rs b/meilisearch-http/src/routes/tasks.rs index e10779012..41a499c20 100644 --- a/meilisearch-http/src/routes/tasks.rs +++ b/meilisearch-http/src/routes/tasks.rs @@ -5,7 +5,9 @@ use meilisearch_types::error::ResponseError; use meilisearch_types::index_uid::IndexUid; use meilisearch_types::settings::{Settings, Unchecked}; use meilisearch_types::star_or::StarOr; -use meilisearch_types::tasks::{serialize_duration, Details, Kind, KindWithContent, Status, Task}; +use meilisearch_types::tasks::{ + serialize_duration, Details, IndexSwap, Kind, KindWithContent, Status, Task, +}; use serde::{Deserialize, Serialize}; use serde_cs::vec::CS; use serde_json::json; @@ -103,7 +105,7 @@ pub struct DetailsView { #[serde(flatten)] pub settings: Option>>, #[serde(skip_serializing_if = "Option::is_none")] - pub indexes: Option>, + pub swaps: Option>, } impl From
for DetailsView { @@ -151,7 +153,7 @@ impl From
for DetailsView { DetailsView { dump_uid: Some(dump_uid), ..DetailsView::default() } } Details::IndexSwap { swaps } => { - DetailsView { indexes: Some(swaps), ..Default::default() } + DetailsView { swaps: Some(swaps), ..Default::default() } } } } diff --git a/meilisearch-types/src/tasks.rs b/meilisearch-types/src/tasks.rs index 2e070caa5..6fbb1bd09 100644 --- a/meilisearch-types/src/tasks.rs +++ b/meilisearch-types/src/tasks.rs @@ -116,7 +116,7 @@ pub enum KindWithContent { primary_key: Option, }, IndexSwap { - swaps: Vec<(String, String)>, + swaps: Vec, }, TaskCancelation { query: String, @@ -134,6 +134,12 @@ pub enum KindWithContent { SnapshotCreation, } +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct IndexSwap { + pub indexes: (String, String), +} + impl KindWithContent { pub fn as_kind(&self) -> Kind { match self { @@ -169,9 +175,9 @@ impl KindWithContent { | IndexDeletion { index_uid } => vec![index_uid], IndexSwap { swaps } => { let mut indexes = HashSet::<&str>::default(); - for (lhs, rhs) in swaps { - indexes.insert(lhs.as_str()); - indexes.insert(rhs.as_str()); + for swap in swaps { + indexes.insert(swap.indexes.0.as_str()); + indexes.insert(swap.indexes.1.as_str()); } indexes.into_iter().collect() } @@ -383,6 +389,8 @@ impl FromStr for Kind { Ok(Kind::IndexCreation) } else if kind.eq_ignore_ascii_case("indexUpdate") { Ok(Kind::IndexUpdate) + } else if kind.eq_ignore_ascii_case("indexSwap") { + Ok(Kind::IndexSwap) } else if kind.eq_ignore_ascii_case("indexDeletion") { Ok(Kind::IndexDeletion) } else if kind.eq_ignore_ascii_case("documentAdditionOrUpdate") { @@ -429,8 +437,7 @@ pub enum Details { TaskCancelation { matched_tasks: u64, canceled_tasks: Option, original_query: String }, TaskDeletion { matched_tasks: u64, deleted_tasks: Option, original_query: String }, Dump { dump_uid: String }, - // TODO: Lo: Revisit this variant once we have decided on what the POST payload of swapping indexes should be - IndexSwap { swaps: Vec<(String, String)> }, + IndexSwap { swaps: Vec }, } /// Serialize a `time::Duration` as a best effort ISO 8601 while waiting for