From 1d1aef9d5465e6d567952eabb2521c0d1b5b342a Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Wed, 19 Aug 2026 10:49:09 +0000 Subject: [PATCH] fix(vector): a HASH index searched with no condition expression must be refused `search_vectors` validated the conditions against the index search schema only when the caller supplied a non-empty `SearchConditionExpression`. An index that declares a HASH element, searched with no expression at all, therefore skipped validation entirely and reached the backend with `hash_key: None`. That contradicted two promises the contract makes. The comment at the resolution site said the index having a HASH element guarantees the search supplied it, and `VectorSearch::hash_key` in `crates/storage/src/lib.rs` tells backend authors that `Some` is always populated when the index declares one, so they may treat it as a mandatory predicate rather than a hint. A backend written against that doc would have served an unscoped search where a scoped one was promised, returning neighbours from every partition instead of the one the caller asked for. The guard is removed, so validation runs unconditionally. With no conditions the membership and type checks iterate nothing and the HASH-presence check does the work, and an index with no search schema still accepts an absent expression. Rather than leave the invariant resting on a comment, the validate-and-resolve step moves into `resolve_search_scope`, which validates, splits the conditions into the partition scope and the inline filters, and asserts that a declared HASH element always yields a populated `hash_key`. A future change that reintroduces a conditional guard fails that assertion instead of silently serving an unscoped search. Unreachable from the wire today: every in-tree backend refuses to create a vector index, so no integration test can construct one. It becomes reachable with the SQLite backend in #244, which is why this is worth fixing before that lands rather than after. Verification: 3 tests on `resolve_search_scope` plus 2 on the underlying validator. Proven discriminating by reintroducing the pre-fix guard, which fails `hash_index_searched_with_no_conditions_is_refused` in release builds, where the `debug_assert` is compiled out, so it is a logic failure rather than the assertion firing; both converse controls keep passing. fmt and `clippy --all-targets -- -D warnings` exit 0, 941 workspace tests, 0 filtered. Reported-by: robinnsc --- .../core/src/expression/search_condition.rs | 27 +++ crates/engine/src/search_vectors.rs | 170 ++++++++++++++---- 2 files changed, 165 insertions(+), 32 deletions(-) diff --git a/crates/core/src/expression/search_condition.rs b/crates/core/src/expression/search_condition.rs index 11bce142..9dfba565 100644 --- a/crates/core/src/expression/search_condition.rs +++ b/crates/core/src/expression/search_condition.rs @@ -532,4 +532,31 @@ mod tests { .contains("does not match type in search schema") ); } + + /// An absent `SearchConditionExpression` against an index that declares a + /// HASH element is itself a validation failure, so the caller may run this + /// unconditionally rather than guarding on a non-empty condition list. + /// Guarding it would let such a search reach a backend with no partition + /// scope, while `VectorSearch::hash_key` promises backends that `Some` is a + /// mandatory predicate whenever the index declares a HASH element. + #[test] + fn schema_requires_partition_key_when_no_conditions_supplied() { + assert!( + err(validate_conditions_against_search_schema( + &[], + Some(&schema()), + &defs() + )) + .contains("SearchConditionExpression must have all HASH attributes") + ); + } + + /// Converse control: with no search schema there is no HASH element to + /// require, so an absent expression stays valid. Without this the test + /// above would also pass for a function that rejected every empty input. + #[test] + fn no_schema_and_no_conditions_is_valid() { + validate_conditions_against_search_schema(&[], None, &defs()).unwrap(); + validate_conditions_against_search_schema(&[], Some(&[]), &defs()).unwrap(); + } } diff --git a/crates/engine/src/search_vectors.rs b/crates/engine/src/search_vectors.rs index c8acb7b3..25dc804b 100644 --- a/crates/engine/src/search_vectors.rs +++ b/crates/engine/src/search_vectors.rs @@ -180,38 +180,11 @@ pub async fn handle_search_vectors( ))); } - if !conditions.is_empty() { - validate_conditions_against_search_schema( - &conditions, - vector_index.search_schema.as_deref(), - &table.attribute_definitions, - )?; - } - - // The index's HASH element scopes the search to one partition; the - // remaining conditions narrow within it. Declaring a HASH element is - // optional, but when the index has one the service requires the search to - // supply it, so validation upstream guarantees it is present here. - let hash_attr = vector_index - .search_schema - .as_deref() - .unwrap_or_default() - .iter() - .find(|e| e.element_type == SearchSchemaElementType::Hash) - .map(|e| e.attribute_name.as_str()); - - let hash_key: Option<(&str, &AttributeValue)> = hash_attr.and_then(|name| { - conditions - .iter() - .find(|c| c.attribute_name == name) - .map(|c| (c.attribute_name.as_str(), &c.value)) - }); - - let filters: Vec<(&str, &AttributeValue)> = conditions - .iter() - .filter(|c| Some(c.attribute_name.as_str()) != hash_attr) - .map(|c| (c.attribute_name.as_str(), &c.value)) - .collect(); + let (hash_key, filters) = resolve_search_scope( + &conditions, + vector_index.search_schema.as_deref(), + &table.attribute_definitions, + )?; let search_output = vector_search .search_vectors(extenddb_storage::VectorSearch { @@ -379,9 +352,142 @@ fn search_request_bytes(dimensions: u32, returned_non_vector_bytes: usize) -> f6 .max(MIN_SEARCH_BYTES) } +/// Resolved search scope: the partition-scoping HASH equality, if the index +/// declares one, and the remaining inline-filter equalities. +type SearchScope<'a> = ( + Option<(&'a str, &'a AttributeValue)>, + Vec<(&'a str, &'a AttributeValue)>, +); + +/// Validate a search's conditions against the index search schema, then split +/// them into the partition scope and the remaining inline filters. +/// +/// The index's HASH element scopes the search to one partition; the remaining +/// conditions narrow within it. Declaring a HASH element is optional, but when +/// the index has one the service requires the search to supply it. +/// +/// Validation runs unconditionally, including when the caller supplied no +/// `SearchConditionExpression` at all. That is the whole point of doing it here +/// rather than at the call site behind a non-empty check: an index declaring a +/// HASH element and a search with no expression is a validation failure, and +/// skipping the check for empty conditions would return `None` for `hash_key` +/// and hand the backend an unscoped search. `VectorSearch::hash_key` promises +/// backend authors that `Some` is a mandatory predicate whenever the index +/// declares a HASH element, so that promise has to hold on every path. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` when the conditions reference an +/// attribute outside the search schema, omit a declared HASH attribute, or carry +/// a value whose type disagrees with the table's attribute definitions. +fn resolve_search_scope<'a>( + conditions: &'a [extenddb_core::expression::SearchCondition], + search_schema: Option<&'a [extenddb_core::types::SearchSchemaElement]>, + attribute_definitions: &[extenddb_core::types::AttributeDefinition], +) -> Result, DynamoDbError> { + validate_conditions_against_search_schema(conditions, search_schema, attribute_definitions)?; + + let hash_attr = search_schema + .unwrap_or_default() + .iter() + .find(|e| e.element_type == SearchSchemaElementType::Hash) + .map(|e| e.attribute_name.as_str()); + + let hash_key: Option<(&str, &AttributeValue)> = hash_attr.and_then(|name| { + conditions + .iter() + .find(|c| c.attribute_name == name) + .map(|c| (c.attribute_name.as_str(), &c.value)) + }); + + // The validation above is what makes this hold; assert it so a future change + // that reintroduces a conditional guard fails here rather than silently + // serving an unscoped search. + debug_assert!( + hash_attr.is_none() || hash_key.is_some(), + "index declares a HASH element but the resolved scope has no hash_key" + ); + + let filters: Vec<(&str, &AttributeValue)> = conditions + .iter() + .filter(|c| Some(c.attribute_name.as_str()) != hash_attr) + .map(|c| (c.attribute_name.as_str(), &c.value)) + .collect(); + + Ok((hash_key, filters)) +} + #[cfg(test)] mod tests { use super::*; + use extenddb_core::expression::SearchCondition; + use extenddb_core::types::{ + AttributeDefinition, ScalarAttributeType, SearchSchemaElement, SearchSchemaElementType, + }; + + fn hash_schema() -> Vec { + vec![SearchSchemaElement { + attribute_name: "Country".to_owned(), + element_type: SearchSchemaElementType::Hash, + }] + } + + fn country_defs() -> Vec { + vec![AttributeDefinition { + attribute_name: "Country".to_owned(), + attribute_type: ScalarAttributeType::S, + }] + } + + fn country_cond(v: &str) -> Vec { + vec![SearchCondition { + attribute_name: "Country".to_owned(), + value: AttributeValue::S(v.to_owned()), + }] + } + + /// The defect this guards: an index declaring a HASH element, searched with + /// no `SearchConditionExpression` at all, must be refused. Resolving the + /// scope without validating first would yield `hash_key: None` and hand the + /// backend an unscoped search, contradicting the `VectorSearch::hash_key` + /// contract that backends may treat `Some` as a mandatory predicate. + #[test] + fn hash_index_searched_with_no_conditions_is_refused() { + let schema = hash_schema(); + let err = resolve_search_scope(&[], Some(&schema), &country_defs()).unwrap_err(); + let DynamoDbError::ValidationException(msg) = err else { + panic!("expected ValidationException"); + }; + assert!( + msg.contains("SearchConditionExpression must have all HASH attributes"), + "got: {msg}" + ); + } + + /// Converse control: an index with no search schema has no scope to require, + /// so no conditions is valid and the resolved scope is empty. Without this, + /// the test above would also pass for a function that refused every search. + #[test] + fn index_without_search_schema_allows_no_conditions() { + let (hash_key, filters) = resolve_search_scope(&[], None, &country_defs()).unwrap(); + assert!(hash_key.is_none()); + assert!(filters.is_empty()); + } + + /// The scope is populated when supplied, and the HASH attribute is not also + /// repeated as an inline filter. + #[test] + fn hash_condition_becomes_the_scope_and_not_a_filter() { + let conds = country_cond("USA"); + let schema = hash_schema(); + let (hash_key, filters) = + resolve_search_scope(&conds, Some(&schema), &country_defs()).unwrap(); + assert_eq!(hash_key.map(|(n, _)| n), Some("Country")); + assert!( + filters.is_empty(), + "HASH attribute must not be repeated as an inline filter, got {filters:?}" + ); + } #[test] fn parse_search_vector_ok() {