From 2808be9d45816d477f7fcee305f67cf9da4f4060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Lecrenier?= Date: Mon, 24 Oct 2022 14:49:39 +0200 Subject: [PATCH] Fix the /swap-indexes route API 1. payload 2. error messages 3. auth errors --- meilisearch-http/src/routes/indexes_swap.rs | 70 ---------- meilisearch-http/src/routes/mod.rs | 4 +- meilisearch-http/src/routes/swap_indexes.rs | 131 +++++++++++++++++++ meilisearch-http/tests/auth/authorization.rs | 2 +- meilisearch-types/src/error.rs | 5 + meilisearch-types/src/keys.rs | 1 + 6 files changed, 140 insertions(+), 73 deletions(-) delete mode 100644 meilisearch-http/src/routes/indexes_swap.rs create mode 100644 meilisearch-http/src/routes/swap_indexes.rs diff --git a/meilisearch-http/src/routes/indexes_swap.rs b/meilisearch-http/src/routes/indexes_swap.rs deleted file mode 100644 index f55949619..000000000 --- a/meilisearch-http/src/routes/indexes_swap.rs +++ /dev/null @@ -1,70 +0,0 @@ -use std::collections::HashSet; - -use actix_web::web::Data; -use actix_web::{web, HttpResponse}; -use index_scheduler::IndexScheduler; -use meilisearch_types::error::{Code, ResponseError}; -use meilisearch_types::tasks::KindWithContent; -use serde::Deserialize; - -use crate::extractors::authentication::policies::*; -use crate::extractors::authentication::GuardedData; -use crate::extractors::sequential_extractor::SeqHandler; -use crate::routes::tasks::TaskView; - -pub fn configure(cfg: &mut web::ServiceConfig) { - cfg.service(web::resource("").route(web::post().to(SeqHandler(indexes_swap)))); -} - -// TODO: Lo: revisit this struct once we have decided on what the payload should be -#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct IndexesSwapPayload { - indexes: (String, String), -} - -pub async fn indexes_swap( - index_scheduler: GuardedData, Data>, - params: web::Json>, -) -> Result { - let search_rules = &index_scheduler.filters().search_rules; - - // TODO: Lo: error when the params are empty - // TODO: Lo: error when the same index appears more than once - // TODO: Lo: error when not authorized to swap - - let mut swaps = vec![]; - let mut indexes_set = HashSet::::default(); - for IndexesSwapPayload { indexes: (lhs, rhs) } in params.into_inner().into_iter() { - if !search_rules.is_index_authorized(&lhs) || !search_rules.is_index_authorized(&rhs) { - return Err(ResponseError::from_msg( - "TODO: error message when we swap with an index were not allowed to access" - .to_owned(), - Code::BadRequest, - )); - } - swaps.push((lhs.clone(), rhs.clone())); - // TODO: Lo: should this check be here or within the index scheduler? - let is_unique_index_lhs = indexes_set.insert(lhs); - if !is_unique_index_lhs { - return Err(ResponseError::from_msg( - "TODO: error message when same index is in more than one swap".to_owned(), - Code::BadRequest, - )); - } - let is_unique_index_rhs = indexes_set.insert(rhs); - if !is_unique_index_rhs { - return Err(ResponseError::from_msg( - "TODO: error message when same index is in more than one swap".to_owned(), - Code::BadRequest, - )); - } - } - - let task = KindWithContent::IndexSwap { swaps }; - - let task = index_scheduler.register(task)?; - let task_view = TaskView::from_task(&task); - - Ok(HttpResponse::Accepted().json(task_view)) -} diff --git a/meilisearch-http/src/routes/mod.rs b/meilisearch-http/src/routes/mod.rs index ca569f903..fb6462f84 100644 --- a/meilisearch-http/src/routes/mod.rs +++ b/meilisearch-http/src/routes/mod.rs @@ -20,7 +20,7 @@ use crate::extractors::authentication::GuardedData; mod api_key; mod dump; pub mod indexes; -mod indexes_swap; +mod swap_indexes; mod tasks; pub fn configure(cfg: &mut web::ServiceConfig) { @@ -31,7 +31,7 @@ pub fn configure(cfg: &mut web::ServiceConfig) { .service(web::resource("/stats").route(web::get().to(get_stats))) .service(web::resource("/version").route(web::get().to(get_version))) .service(web::scope("/indexes").configure(indexes::configure)) - .service(web::scope("indexes-swap").configure(indexes_swap::configure)); + .service(web::scope("swap-indexes").configure(swap_indexes::configure)); } /// Extracts the raw values from the `StarOr` types and diff --git a/meilisearch-http/src/routes/swap_indexes.rs b/meilisearch-http/src/routes/swap_indexes.rs new file mode 100644 index 000000000..5c484cd91 --- /dev/null +++ b/meilisearch-http/src/routes/swap_indexes.rs @@ -0,0 +1,131 @@ +use std::collections::HashSet; + +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 serde::Deserialize; + +use self::errors::{DuplicateSwappedIndexError, IndexesNotFoundError}; +use crate::extractors::authentication::policies::*; +use crate::extractors::authentication::GuardedData; +use crate::extractors::sequential_extractor::SeqHandler; +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 { + swap: (String, String), +} + +pub async fn swap_indexes( + index_scheduler: GuardedData, Data>, + params: web::Json>, +) -> Result { + let search_rules = &index_scheduler.filters().search_rules; + + let mut swaps = vec![]; + let mut indexes_set = HashSet::::default(); + let mut unknown_indexes = HashSet::new(); + let mut duplicate_indexes = HashSet::new(); + for SwapIndexesPayload { swap: (lhs, rhs) } in params.into_inner().into_iter() { + if !search_rules.is_index_authorized(&lhs) { + unknown_indexes.insert(lhs.clone()); + } + if !search_rules.is_index_authorized(&rhs) { + unknown_indexes.insert(rhs.clone()); + } + + swaps.push((lhs.clone(), rhs.clone())); + + let is_unique_index_lhs = indexes_set.insert(lhs.clone()); + if !is_unique_index_lhs { + duplicate_indexes.insert(lhs); + } + let is_unique_index_rhs = indexes_set.insert(rhs.clone()); + if !is_unique_index_rhs { + duplicate_indexes.insert(rhs); + } + } + if !duplicate_indexes.is_empty() { + return Err(DuplicateSwappedIndexError { + indexes: duplicate_indexes.into_iter().collect(), + } + .into()); + } + if !unknown_indexes.is_empty() { + return Err(IndexesNotFoundError { indexes: unknown_indexes.into_iter().collect() }.into()); + } + + let task = KindWithContent::IndexSwap { swaps }; + + let task = index_scheduler.register(task)?; + let task_view = TaskView::from_task(&task); + + Ok(HttpResponse::Accepted().json(task_view)) +} + +pub mod errors { + use std::fmt::Display; + + use meilisearch_types::error::{Code, ErrorCode}; + + #[derive(Debug)] + pub struct IndexesNotFoundError { + pub indexes: Vec, + } + impl Display for IndexesNotFoundError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if self.indexes.len() == 1 { + write!(f, "Index `{}` not found,", self.indexes[0])?; + } else { + write!(f, "Indexes `{}`", self.indexes[0])?; + for index in self.indexes.iter().skip(1) { + write!(f, ", `{}`", index)?; + } + write!(f, "not found.")?; + } + Ok(()) + } + } + impl std::error::Error for IndexesNotFoundError {} + impl ErrorCode for IndexesNotFoundError { + fn error_code(&self) -> Code { + Code::IndexNotFound + } + } + #[derive(Debug)] + pub struct DuplicateSwappedIndexError { + pub indexes: Vec, + } + impl Display for DuplicateSwappedIndexError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if self.indexes.len() == 1 { + write!(f, "Indexes must be declared only once during a swap. `{}` was specified several times.", self.indexes[0])?; + } else { + write!( + f, + "Indexes must be declared only once during a swap. `{}`", + self.indexes[0] + )?; + for index in self.indexes.iter().skip(1) { + write!(f, ", `{}`", index)?; + } + write!(f, "were specified several times.")?; + } + + Ok(()) + } + } + impl std::error::Error for DuplicateSwappedIndexError {} + impl ErrorCode for DuplicateSwappedIndexError { + fn error_code(&self) -> Code { + Code::DuplicateIndexFound + } + } +} diff --git a/meilisearch-http/tests/auth/authorization.rs b/meilisearch-http/tests/auth/authorization.rs index da58cad34..aab351b7c 100644 --- a/meilisearch-http/tests/auth/authorization.rs +++ b/meilisearch-http/tests/auth/authorization.rs @@ -26,7 +26,7 @@ pub static AUTHORIZATIONS: Lazy hashset!{"indexes.delete", "indexes.*", "*"}, ("POST", "/indexes") => hashset!{"indexes.create", "indexes.*", "*"}, ("GET", "/indexes") => hashset!{"indexes.get", "indexes.*", "*"}, - // ("POST", "/indexes-swap") => hashset!{"indexes.swap", "indexes.*", "*"}, // TODO: uncomment and fix this test + ("POST", "/swap-indexes") => hashset!{"indexes.swap", "indexes.*", "*"}, ("GET", "/indexes/products/settings") => hashset!{"settings.get", "settings.*", "*"}, ("GET", "/indexes/products/settings/displayed-attributes") => hashset!{"settings.get", "settings.*", "*"}, ("GET", "/indexes/products/settings/distinct-attribute") => hashset!{"settings.get", "settings.*", "*"}, diff --git a/meilisearch-types/src/error.rs b/meilisearch-types/src/error.rs index d36875192..330a6f082 100644 --- a/meilisearch-types/src/error.rs +++ b/meilisearch-types/src/error.rs @@ -120,6 +120,8 @@ pub enum Code { InvalidIndexUid, InvalidMinWordLengthForTypo, + DuplicateIndexFound, + // invalid state error InvalidState, MissingPrimaryKey, @@ -298,6 +300,9 @@ impl Code { InvalidMinWordLengthForTypo => { ErrCode::invalid("invalid_min_word_length_for_typo", StatusCode::BAD_REQUEST) } + DuplicateIndexFound => { + ErrCode::invalid("duplicate_index_found", StatusCode::BAD_REQUEST) + } } } diff --git a/meilisearch-types/src/keys.rs b/meilisearch-types/src/keys.rs index bf6096428..89073c3ad 100644 --- a/meilisearch-types/src/keys.rs +++ b/meilisearch-types/src/keys.rs @@ -270,6 +270,7 @@ impl Action { INDEXES_GET => Some(Self::IndexesGet), INDEXES_UPDATE => Some(Self::IndexesUpdate), INDEXES_DELETE => Some(Self::IndexesDelete), + INDEXES_SWAP => Some(Self::IndexesSwap), TASKS_ALL => Some(Self::TasksAll), TASKS_CANCEL => Some(Self::TasksCancel), TASKS_DELETE => Some(Self::TasksDelete),