From eaaa4e4ee154ae8b4cda028acc92c2aacc60b05a Mon Sep 17 00:00:00 2001 From: Arnaud Gourlay Date: Wed, 7 May 2025 16:35:57 +0200 Subject: [PATCH] Validate IntegerIndexParams (#6494) * Validate IntegerIndexParams * focus --- lib/api/build.rs | 2 ++ lib/api/src/grpc/qdrant.rs | 3 +++ lib/api/src/grpc/validate.rs | 30 ++++++++++++++++++++++++++ lib/segment/src/data_types/index.rs | 28 ++++++++++++++++++++++++ lib/segment/src/types.rs | 26 ++++++++++++++++++++++ src/common/update.rs | 1 + tests/openapi/test_payload_indexing.py | 17 +++++++++++++++ 7 files changed, 107 insertions(+) diff --git a/lib/api/build.rs b/lib/api/build.rs index 08b554ff58..c952e8fa04 100644 --- a/lib/api/build.rs +++ b/lib/api/build.rs @@ -206,6 +206,8 @@ fn configure_validation(builder: Builder) -> Builder { ("UpdateBatchPoints.operations", "length(min = 1)"), ("CreateFieldIndexCollection.collection_name", "length(min = 1, max = 255)"), ("CreateFieldIndexCollection.field_name", "length(min = 1)"), + ("CreateFieldIndexCollection.field_index_params", ""), + ("PayloadIndexParams.index_params", ""), ("DeleteFieldIndexCollection.collection_name", "length(min = 1, max = 255)"), ("DeleteFieldIndexCollection.field_name", "length(min = 1)"), ("SearchPoints.collection_name", "length(min = 1, max = 255)"), diff --git a/lib/api/src/grpc/qdrant.rs b/lib/api/src/grpc/qdrant.rs index ba9a8a0b54..3626c5b256 100644 --- a/lib/api/src/grpc/qdrant.rs +++ b/lib/api/src/grpc/qdrant.rs @@ -865,6 +865,7 @@ pub struct UuidIndexParams { #[prost(bool, optional, tag = "2")] pub on_disk: ::core::option::Option, } +#[derive(validator::Validate)] #[derive(serde::Serialize)] #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] @@ -873,6 +874,7 @@ pub struct PayloadIndexParams { oneof = "payload_index_params::IndexParams", tags = "3, 2, 4, 5, 1, 6, 7, 8" )] + #[validate(nested)] pub index_params: ::core::option::Option, } /// Nested message and enum types in `PayloadIndexParams`. @@ -4389,6 +4391,7 @@ pub struct CreateFieldIndexCollection { pub field_type: ::core::option::Option, /// Payload index params. #[prost(message, optional, tag = "5")] + #[validate(nested)] pub field_index_params: ::core::option::Option, /// Write ordering guarantees #[prost(message, optional, tag = "6")] diff --git a/lib/api/src/grpc/validate.rs b/lib/api/src/grpc/validate.rs index e68b4ba3d7..8babedcc4c 100644 --- a/lib/api/src/grpc/validate.rs +++ b/lib/api/src/grpc/validate.rs @@ -2,6 +2,7 @@ use std::borrow::Cow; use std::collections::HashMap; use common::validation::{validate_range_generic, validate_shard_different_peers}; +use segment::data_types::index::validate_integer_index_params; use validator::{Validate, ValidationError, ValidationErrors}; use super::qdrant as grpc; @@ -383,6 +384,35 @@ pub fn validate_timestamp(ts: &prost_wkt_types::Timestamp) -> Result<(), Validat Ok(()) } +impl Validate for super::qdrant::payload_index_params::IndexParams { + fn validate(&self) -> Result<(), ValidationErrors> { + match self { + grpc::payload_index_params::IndexParams::KeywordIndexParams(_) => Ok(()), + grpc::payload_index_params::IndexParams::IntegerIndexParams(integer_index_params) => { + integer_index_params.validate() + } + grpc::payload_index_params::IndexParams::FloatIndexParams(_) => Ok(()), + grpc::payload_index_params::IndexParams::GeoIndexParams(_) => Ok(()), + grpc::payload_index_params::IndexParams::TextIndexParams(_) => Ok(()), + grpc::payload_index_params::IndexParams::BoolIndexParams(_) => Ok(()), + grpc::payload_index_params::IndexParams::DatetimeIndexParams(_) => Ok(()), + grpc::payload_index_params::IndexParams::UuidIndexParams(_) => Ok(()), + } + } +} + +impl Validate for super::qdrant::IntegerIndexParams { + fn validate(&self) -> Result<(), ValidationErrors> { + let super::qdrant::IntegerIndexParams { + lookup, + range, + is_principal: _, + on_disk: _, + } = &self; + validate_integer_index_params(lookup, range) + } +} + #[cfg(test)] mod tests { use validator::Validate; diff --git a/lib/segment/src/data_types/index.rs b/lib/segment/src/data_types/index.rs index 375fbfe295..e7137ed98c 100644 --- a/lib/segment/src/data_types/index.rs +++ b/lib/segment/src/data_types/index.rs @@ -1,5 +1,6 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use validator::{Validate, ValidationError, ValidationErrors}; // Keyword @@ -55,6 +56,33 @@ pub struct IntegerIndexParams { pub on_disk: Option, } +impl Validate for IntegerIndexParams { + fn validate(&self) -> Result<(), ValidationErrors> { + let IntegerIndexParams { + r#type: _, + lookup, + range, + is_principal: _, + on_disk: _, + } = &self; + validate_integer_index_params(lookup, range) + } +} + +pub fn validate_integer_index_params( + lookup: &Option, + range: &Option, +) -> Result<(), ValidationErrors> { + if lookup == &Some(false) && range == &Some(false) { + let mut errors = ValidationErrors::new(); + let error = + ValidationError::new("the 'lookup' and 'range' capabilities can't be both disabled"); + errors.add("lookup", error); + return Err(errors); + } + Ok(()) +} + // UUID #[derive(Default, Debug, Deserialize, Serialize, JsonSchema, Clone, Copy, PartialEq, Hash, Eq)] diff --git a/lib/segment/src/types.rs b/lib/segment/src/types.rs index 11e10d554e..7ff2137ae2 100644 --- a/lib/segment/src/types.rs +++ b/lib/segment/src/types.rs @@ -1774,6 +1774,21 @@ impl PayloadSchemaParams { } } +impl Validate for PayloadSchemaParams { + fn validate(&self) -> Result<(), ValidationErrors> { + match self { + PayloadSchemaParams::Keyword(_) => Ok(()), + PayloadSchemaParams::Integer(integer_index_params) => integer_index_params.validate(), + PayloadSchemaParams::Float(_) => Ok(()), + PayloadSchemaParams::Geo(_) => Ok(()), + PayloadSchemaParams::Text(_) => Ok(()), + PayloadSchemaParams::Bool(_) => Ok(()), + PayloadSchemaParams::Datetime(_) => Ok(()), + PayloadSchemaParams::Uuid(_) => Ok(()), + } + } +} + #[derive(Debug, Deserialize, Serialize, JsonSchema, Clone, PartialEq, Hash, Eq)] #[serde(untagged, rename_all = "snake_case")] pub enum PayloadFieldSchema { @@ -1781,6 +1796,17 @@ pub enum PayloadFieldSchema { FieldParams(PayloadSchemaParams), } +impl Validate for PayloadFieldSchema { + fn validate(&self) -> Result<(), ValidationErrors> { + match self { + PayloadFieldSchema::FieldType(_) => Ok(()), // nothing to validate + PayloadFieldSchema::FieldParams(payload_schema_params) => { + payload_schema_params.validate() + } + } + } +} + impl Display for PayloadFieldSchema { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { match self { diff --git a/src/common/update.rs b/src/common/update.rs index 1a5124a789..548f931c26 100644 --- a/src/common/update.rs +++ b/src/common/update.rs @@ -224,6 +224,7 @@ pub struct DeleteVectorsOperation { pub struct CreateFieldIndex { pub field_name: PayloadKeyType, #[serde(alias = "field_type")] + #[validate(nested)] pub field_schema: Option, } diff --git a/tests/openapi/test_payload_indexing.py b/tests/openapi/test_payload_indexing.py index c9bd5bd822..e9fd23fe1c 100644 --- a/tests/openapi/test_payload_indexing.py +++ b/tests/openapi/test_payload_indexing.py @@ -10,6 +10,23 @@ def setup(collection_name): yield drop_collection(collection_name=collection_name) +def test_payload_indexing_validation(collection_name): + response = request_with_validation( + api='/collections/{collection_name}/index', + method="PUT", + path_params={'collection_name': collection_name}, + query_params={'wait': 'true'}, + body={ + "field_name": "test_payload", + "field_schema": { + "type": "integer", + "lookup": False, + "range": False, + } + } + ) + assert response.status_code == 422 + assert "Validation error: the 'lookup' and 'range' capabilities can't be both disabled" in response.json()["status"]["error"] def test_payload_indexing_operations(collection_name): # create payload