diff --git a/crates/core/src/expression/key_condition.rs b/crates/core/src/expression/key_condition.rs index 4e306512d..edaa1f8e5 100755 --- a/crates/core/src/expression/key_condition.rs +++ b/crates/core/src/expression/key_condition.rs @@ -17,6 +17,9 @@ use super::ast::{CompareOp, Expr, PathElement}; use super::parser_common; use super::tokenizer::Token; use crate::error::DynamoDbError; +use crate::types::{ + KeySchemaElement, KeyType, hash_key_elements, is_multipart_key_schema, range_key_elements, +}; /// Parsed key condition: partition key value expression(s) and optional sort key condition. /// @@ -80,8 +83,8 @@ impl KeyCondition { if let Some(ref_name) = name.strip_prefix('#') { names.get(ref_name).map(String::as_str).ok_or_else(|| { DynamoDbError::ValidationException(format!( - "An expression attribute name used in the document path \ - is not defined; attribute name: #{ref_name}" + "Invalid KeyConditionExpression: An expression attribute name used in \ + the document path is not defined; attribute name: #{ref_name}" )) })? } else { @@ -115,91 +118,348 @@ impl KeyCondition { Ok(()) } - /// Reclassify conditions for multi-part key schemas. + /// Classify and validate the conditions of a multi-attribute key schema. /// - /// Given the full set of HASH attribute names, moves any equality conditions - /// on HASH attributes from `sk_condition` into `extra_pk_conditions`. - /// The remaining non-HASH equality or range condition stays as `sk_condition`. + /// A multi-attribute key schema has more than one HASH attribute, more + /// than one RANGE attribute, or both. `hash_attrs` and `range_attrs` are + /// the index's HASH and RANGE attribute names in key-schema order. The + /// parser cannot tell HASH from RANGE equalities, and clause order in the + /// expression is free, so every clause is assigned here by name. /// - /// Call this after `resolve_pk_sk()` for multi-part key queries. + /// The rules the service applies: + /// + /// - Each key attribute may carry one condition; a second one is refused + /// as `KeyConditionExpressions must only contain one condition per key`. + /// - Every HASH attribute needs an equality condition. Every one without + /// it is named, in key-schema order, in `HASH key attributes ... must + /// have equality conditions specified in the query`. + /// - Conditions on RANGE attributes must cover a leading run of them in + /// key-schema order. Every RANGE attribute before the last one that has + /// a condition needs an equality; only that last one may use `<`, `<=`, + /// `>`, `>=`, `BETWEEN` or `begins_with`. `h = :h AND b = :b` on a schema + /// whose RANGE attributes are `a, b` is refused, and so is + /// `h = :h AND a = :a AND b > :b AND c = :c`. + /// + /// On return the HASH conditions are in key-schema order (`pk_path` and + /// `extra_pk_conditions`, matching the composite partition key the write + /// path builds). When a RANGE attribute carries a non-equality condition, + /// that condition is `sk_condition` and the equalities before it are + /// `extra_sk_conditions`; otherwise `sk_condition` is the equality on the + /// first RANGE attribute and the rest follow in `extra_sk_conditions`. + /// Either way `extra_sk_conditions` is in key-schema order. A storage + /// backend maps each condition to its column by attribute name. + /// + /// Call this instead of [`KeyCondition::resolve_pk_sk`] when the key + /// schema is multi-attribute. A schema with one HASH and at most one + /// RANGE attribute is left unchanged. + /// + /// # Errors + /// + /// Returns `ValidationException` when a `#name` reference cannot be + /// resolved, a HASH attribute has no equality condition, the RANGE + /// conditions skip an attribute or put a non-equality before the last + /// one, or a non-equality names a HASH attribute, or a non-key attribute + /// while a RANGE attribute also has a condition. pub fn resolve_multipart( &mut self, hash_attrs: &[&str], + range_attrs: &[&str], names: &std::collections::HashMap, ) -> Result<(), DynamoDbError> { - if hash_attrs.len() <= 1 { + self.resolve_multipart_with(hash_attrs, range_attrs, names, |_, _| Ok(())) + } + + /// [`Self::resolve_multipart`] with a check on every clause's value, + /// called with the clause's path and value after the structural rules + /// (one condition per key, condition count, HASH coverage, RANGE gap) and + /// before the non-key rule. That is where the service reports a value of + /// the wrong type (measured 2026-10-21: a wrong-typed value loses to a + /// duplicate, a count overflow, a missing HASH attribute and a RANGE gap, + /// and wins over a condition on a non-key attribute). + /// + /// # Errors + /// + /// As [`Self::resolve_multipart`], plus whatever `value_check` returns. + pub fn resolve_multipart_with( + &mut self, + hash_attrs: &[&str], + range_attrs: &[&str], + names: &std::collections::HashMap, + mut value_check: F, + ) -> Result<(), DynamoDbError> + where + F: FnMut(&[PathElement], &Expr) -> Result<(), DynamoDbError>, + { + if hash_attrs.len() <= 1 && range_attrs.len() <= 1 { return Ok(()); } - // Collect all equality conditions (pk + extra_pk + sk if Eq) + // Every equality clause, whichever field the parser put it in. let mut all_eq: Vec<(Vec, Expr)> = Vec::new(); all_eq.push((self.pk_path.clone(), self.pk_value.clone())); all_eq.append(&mut self.extra_pk_conditions); + all_eq.append(&mut self.extra_sk_conditions); - let mut sk_non_eq: Option = None; - + // The parser allows at most one non-equality clause. + let mut non_eq: Option = None; if let Some(sk) = self.sk_condition.take() { match sk { SortKeyCondition::Compare { path, op: CompareOp::Eq, value, - } => { - all_eq.push((path, value)); - } - other => { - sk_non_eq = Some(other); - } + } => all_eq.push((path, value)), + other => non_eq = Some(other), } } - // Classify each equality condition as HASH or RANGE. - // Index by position in hash_attrs so we can reorder to key schema order. - let mut pk_by_position: Vec<(usize, Vec, Expr)> = Vec::new(); - let mut sk_eqs: Vec<(Vec, Expr)> = Vec::new(); - + let mut hash_eqs: Vec<(usize, Vec, Expr)> = Vec::new(); + let mut range_eqs: Vec<(usize, Vec, Expr)> = Vec::new(); + let mut other_eqs: Vec<(Vec, Expr)> = Vec::new(); for (path, value) in all_eq { let attr_name = resolve_path_attr(&path, names)?; - if let Some(pos) = hash_attrs.iter().position(|a| *a == attr_name.as_str()) { - pk_by_position.push((pos, path, value)); + if let Some(pos) = hash_attrs.iter().position(|a| *a == attr_name) { + hash_eqs.push((pos, path, value)); + } else if let Some(pos) = range_attrs.iter().position(|a| *a == attr_name) { + range_eqs.push((pos, path, value)); } else { - sk_eqs.push((path, value)); + other_eqs.push((path, value)); } } - // Sort PK conditions by key schema order to ensure composite PK - // concatenation matches the write path regardless of expression order. - pk_by_position.sort_by_key(|(pos, _, _)| *pos); + // The non-equality clause's attribute: its RANGE position when it + // names a RANGE attribute, its HASH position when it names a HASH + // attribute, neither when it names a non-key attribute. + let non_eq_attr = match &non_eq { + Some(cond) => Some(resolve_path_attr(cond.path(), names)?), + None => None, + }; + let non_eq_pos = non_eq_attr + .as_deref() + .and_then(|a| range_attrs.iter().position(|r| *r == a)); + let non_eq_hash_pos = non_eq_attr + .as_deref() + .and_then(|a| hash_attrs.iter().position(|h| *h == a)); + + // One condition per key attribute. A second condition on one HASH or + // RANGE attribute, equality or not, is refused before any coverage + // check (measured on the service, 2026-10-21). + hash_eqs.sort_by_key(|(pos, _, _)| *pos); + range_eqs.sort_by_key(|(pos, _, _)| *pos); + let duplicated = hash_eqs.windows(2).any(|w| w[0].0 == w[1].0) + || range_eqs.windows(2).any(|w| w[0].0 == w[1].0) + || non_eq_pos.is_some_and(|p| range_eqs.iter().any(|(q, _, _)| *q == p)) + || non_eq_hash_pos.is_some_and(|p| hash_eqs.iter().any(|(q, _, _)| *q == p)); + if duplicated { + return Err(DynamoDbError::ValidationException( + "Invalid KeyConditionExpression: KeyConditionExpressions must only contain \ + one condition per key" + .to_string(), + )); + } + + // More conditions than key attributes (measured 2026-10-21: the count + // is reported ahead of which attribute is missing). + let condition_count = + hash_eqs.len() + range_eqs.len() + other_eqs.len() + usize::from(non_eq.is_some()); + let key_attribute_count = hash_attrs.len() + range_attrs.len(); + if condition_count > key_attribute_count { + return Err(DynamoDbError::ValidationException(format!( + "The number of query conditions ({condition_count}) exceeds the number of key \ + attributes defined in the schema ({key_attribute_count})" + ))); + } - // Assign first PK condition as primary, rest as extra - let mut pk_iter = pk_by_position - .into_iter() - .map(|(_, path, value)| (path, value)); - if let Some((first_path, first_value)) = pk_iter.next() { + // Every HASH attribute needs an equality condition. On a + // multi-attribute key schema the service names every HASH attribute + // without one, in key-schema order (measured 2026-10-21); the + // single-attribute `Query condition missed key schema element` + // wording belongs to the single-attribute path in the engine. + let missing_hash: Vec<&str> = hash_attrs + .iter() + .enumerate() + .filter(|(pos, _)| !hash_eqs.iter().any(|(p, _, _)| p == pos)) + .map(|(_, attr)| *attr) + .collect(); + if !missing_hash.is_empty() { + return Err(DynamoDbError::ValidationException(format!( + "HASH key attributes {} must have equality conditions specified in the query", + missing_hash.join(", ") + ))); + } + + // RANGE conditions must cover a leading run of the RANGE attributes, + // with an equality on every one before the last. + let last_pos = range_eqs.iter().map(|(p, _, _)| *p).chain(non_eq_pos).max(); + if let Some(last_pos) = last_pos { + let without_eq: Vec<&str> = range_attrs[..last_pos] + .iter() + .enumerate() + .filter(|(pos, _)| !range_eqs.iter().any(|(p, _, _)| p == pos)) + .map(|(_, attr)| *attr) + .collect(); + if !without_eq.is_empty() { + return Err(DynamoDbError::ValidationException(format!( + "RANGE key attributes {} must have equality conditions specified in the \ + query because a condition is present on key attribute {}", + without_eq.join(", "), + range_attrs[last_pos] + ))); + } + } + + for (_, path, value) in hash_eqs.iter().chain(range_eqs.iter()) { + value_check(path, value)?; + } + for (path, value) in &other_eqs { + value_check(path, value)?; + } + match &non_eq { + Some(SortKeyCondition::Compare { path, value, .. }) => value_check(path, value)?, + Some(SortKeyCondition::Between { path, low, high }) => { + value_check(path, low)?; + value_check(path, high)?; + } + Some(SortKeyCondition::BeginsWith { path, prefix }) => value_check(path, prefix)?, + None => {} + } + + // A condition on an attribute outside the index key schema. The + // service refuses it by naming the first RANGE attribute that has no + // condition (measured 2026-10-21: `h = :h AND z = :z` on RANGE + // `a, b, c, d` is `Query condition missed key schema element: a`; + // with `a` given, `b`). The count check above has already refused + // the case where every key attribute has a condition as well. + let non_key_non_eq = non_eq.is_some() && non_eq_pos.is_none() && non_eq_hash_pos.is_none(); + if !other_eqs.is_empty() || non_key_non_eq { + let missing_range = range_attrs + .iter() + .enumerate() + .find(|(pos, _)| { + !range_eqs.iter().any(|(p, _, _)| p == pos) && non_eq_pos != Some(*pos) + }) + .map(|(_, attr)| *attr) + .unwrap_or(hash_attrs[0]); + return Err(DynamoDbError::ValidationException(format!( + "Query condition missed key schema element: {missing_range}" + ))); + } + + let mut hash_iter = hash_eqs.into_iter().map(|(_, path, value)| (path, value)); + if let Some((first_path, first_value)) = hash_iter.next() { self.pk_path = first_path; self.pk_value = first_value; - self.extra_pk_conditions = pk_iter.collect(); + self.extra_pk_conditions = hash_iter.collect(); } - // Assign SK conditions: non-eq condition takes priority as primary SK, - // otherwise first equality RANGE condition is primary, rest are extra. - if let Some(non_eq) = sk_non_eq { - self.sk_condition = Some(non_eq); - self.extra_sk_conditions = sk_eqs; - } else if let Some((path, value)) = sk_eqs.first().cloned() { + let mut range_eqs: Vec<(Vec, Expr)> = range_eqs + .into_iter() + .map(|(_, path, value)| (path, value)) + .collect(); + + if let Some(cond) = non_eq { + self.sk_condition = Some(cond); + } else if range_eqs.is_empty() { + self.sk_condition = None; + } else { + let (path, value) = range_eqs.remove(0); self.sk_condition = Some(SortKeyCondition::Compare { path, op: CompareOp::Eq, value, }); - self.extra_sk_conditions = sk_eqs.into_iter().skip(1).collect(); - } else { - self.sk_condition = None; - self.extra_sk_conditions = Vec::new(); } + self.extra_sk_conditions = range_eqs; Ok(()) } + + /// Assign every clause to its key attribute under `key_schema`, the key + /// schema of the table or index the query reads. + /// + /// A multi-attribute key schema goes through + /// [`Self::resolve_multipart_with`], which classifies every clause by name, + /// applies the service's rules for such a schema and calls `value_check` + /// where the service reports a value of the wrong type. On any other key + /// schema `value_check` runs on every clause first, which is where the + /// service reports it for such a schema (measured 2026-10-21), and then + /// [`Self::resolve_pk_sk`] picks the partition key clause. + /// + /// The engine reads a `Query` key condition through this, and so does + /// authorization when it finds the partition key value for + /// `dynamodb:LeadingKeys`, so both read the same clause as the partition + /// key. On a multi-attribute key schema `pk_path` then names the first + /// HASH attribute. + /// + /// # Errors + /// + /// As [`Self::resolve_multipart_with`] on a multi-attribute key schema and + /// as [`Self::resolve_pk_sk`] on any other, plus whatever `value_check` + /// returns. + pub fn resolve_key_schema_with( + &mut self, + key_schema: &[KeySchemaElement], + names: &std::collections::HashMap, + mut value_check: F, + ) -> Result<(), DynamoDbError> + where + F: FnMut(&[PathElement], &Expr) -> Result<(), DynamoDbError>, + { + if is_multipart_key_schema(key_schema) { + let hash_attrs: Vec<&str> = hash_key_elements(key_schema) + .iter() + .map(|ks| ks.attribute_name.as_str()) + .collect(); + let range_attrs: Vec<&str> = range_key_elements(key_schema) + .iter() + .map(|ks| ks.attribute_name.as_str()) + .collect(); + return self.resolve_multipart_with(&hash_attrs, &range_attrs, names, value_check); + } + self.for_each_clause(&mut value_check)?; + match key_schema.iter().find(|ks| ks.key_type == KeyType::Hash) { + Some(hash) => self.resolve_pk_sk(&hash.attribute_name, names), + None => Ok(()), + } + } + + /// [`Self::resolve_key_schema_with`] without a value check. + /// + /// # Errors + /// + /// As [`Self::resolve_key_schema_with`]. + pub fn resolve_key_schema( + &mut self, + key_schema: &[KeySchemaElement], + names: &std::collections::HashMap, + ) -> Result<(), DynamoDbError> { + self.resolve_key_schema_with(key_schema, names, |_, _| Ok(())) + } + + /// Call `f` with the path and value of every clause: the partition key + /// clause, the extra HASH and RANGE equalities, and each operand of the + /// sort key condition. + fn for_each_clause(&self, f: &mut F) -> Result<(), DynamoDbError> + where + F: FnMut(&[PathElement], &Expr) -> Result<(), DynamoDbError>, + { + f(&self.pk_path, &self.pk_value)?; + for (path, value) in &self.extra_pk_conditions { + f(path, value)?; + } + for (path, value) in &self.extra_sk_conditions { + f(path, value)?; + } + match &self.sk_condition { + Some(SortKeyCondition::Compare { path, value, .. }) => f(path, value), + Some(SortKeyCondition::Between { path, low, high }) => { + f(path, low)?; + f(path, high) + } + Some(SortKeyCondition::BeginsWith { path, prefix }) => f(path, prefix), + None => Ok(()), + } + } } /// Resolve a path's top-level attribute name, handling `#name` references. @@ -212,8 +472,8 @@ fn resolve_path_attr( if let Some(ref_name) = name.strip_prefix('#') { names.get(ref_name).cloned().ok_or_else(|| { DynamoDbError::ValidationException(format!( - "An expression attribute name used in the document path \ - is not defined; attribute name: #{ref_name}" + "Invalid KeyConditionExpression: An expression attribute name used in \ + the document path is not defined; attribute name: #{ref_name}" )) }) } else { @@ -248,6 +508,18 @@ pub enum SortKeyCondition { }, } +impl SortKeyCondition { + /// The attribute path the condition applies to. + #[must_use] + pub fn path(&self) -> &[PathElement] { + match self { + Self::Compare { path, .. } + | Self::Between { path, .. } + | Self::BeginsWith { path, .. } => path, + } + } +} + /// Parse a `KeyConditionExpression` token stream. /// /// Extracts the partition key equality and optional sort key condition. @@ -899,7 +1171,8 @@ mod tests { let tokens = tokenize("pk1 = :v1 AND pk2 = :v2 AND sk1 = :v3").unwrap(); let mut kc = parse_key_condition(&tokens).unwrap(); let names = HashMap::new(); - kc.resolve_multipart(&["pk1", "pk2"], &names).unwrap(); + kc.resolve_multipart(&["pk1", "pk2"], &["sk1"], &names) + .unwrap(); // After resolve_multipart, pk_path + extra_pk should have pk1 and pk2 let pk_count = 1 + kc.extra_pk_conditions.len(); @@ -913,7 +1186,8 @@ mod tests { let tokens = tokenize("pk1 = :v1 AND pk2 = :v2 AND sk1 > :v3").unwrap(); let mut kc = parse_key_condition(&tokens).unwrap(); let names = HashMap::new(); - kc.resolve_multipart(&["pk1", "pk2"], &names).unwrap(); + kc.resolve_multipart(&["pk1", "pk2"], &["sk1"], &names) + .unwrap(); assert_eq!(1 + kc.extra_pk_conditions.len(), 2); match &kc.sk_condition { @@ -930,7 +1204,8 @@ mod tests { let tokens = tokenize("pk2 = :v2 AND pk1 = :v1 AND sk = :v3").unwrap(); let mut kc = parse_key_condition(&tokens).unwrap(); let names = HashMap::new(); - kc.resolve_multipart(&["pk1", "pk2"], &names).unwrap(); + kc.resolve_multipart(&["pk1", "pk2"], &["sk"], &names) + .unwrap(); // After resolve_multipart, pk_path should be pk1 (first in schema) assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk1".to_owned())]); @@ -948,7 +1223,8 @@ mod tests { let tokens = tokenize("pk1 = :v1 AND pk2 = :v2 AND sk1 = :v3 AND sk2 = :v4").unwrap(); let mut kc = parse_key_condition(&tokens).unwrap(); let names = HashMap::new(); - kc.resolve_multipart(&["pk1", "pk2"], &names).unwrap(); + kc.resolve_multipart(&["pk1", "pk2"], &["sk1", "sk2"], &names) + .unwrap(); // 2 PK conditions assert_eq!(1 + kc.extra_pk_conditions.len(), 2); @@ -1133,4 +1409,497 @@ mod tests { ); } } + + /// The index key schema the single-HASH cases use: HASH `h`, then RANGE + /// `a`, `b`, `c`, `d`. + const ONE_HASH: [&str; 1] = ["h"]; + const FOUR_RANGE: [&str; 4] = ["a", "b", "c", "d"]; + + /// Parse `expr` and classify it against `h` + `a, b, c, d`. + fn classify_one_hash(expr: &str) -> Result { + classify_with_names(expr, &HashMap::new()) + } + + fn classify_with_names( + expr: &str, + names: &HashMap, + ) -> Result { + let tokens = tokenize(expr).unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&ONE_HASH, &FOUR_RANGE, names)?; + Ok(kc) + } + + /// The attribute names of `extra_sk_conditions`, in order. + fn extra_sk_names(kc: &KeyCondition) -> Vec { + kc.extra_sk_conditions + .iter() + .map(|(path, _)| match path.first() { + Some(PathElement::Attribute(name)) => name.clone(), + other => panic!("unexpected path {other:?}"), + }) + .collect() + } + + /// The sort-key condition's attribute and a label for its operator. + fn sk_shape(kc: &KeyCondition) -> Option<(String, &'static str)> { + let cond = kc.sk_condition.as_ref()?; + let name = match cond.path().first() { + Some(PathElement::Attribute(name)) => name.clone(), + other => panic!("unexpected path {other:?}"), + }; + let op = match cond { + SortKeyCondition::Compare { op, .. } => match op { + CompareOp::Eq => "=", + CompareOp::Ne => "<>", + CompareOp::Lt => "<", + CompareOp::Le => "<=", + CompareOp::Gt => ">", + CompareOp::Ge => ">=", + }, + SortKeyCondition::Between { .. } => "BETWEEN", + SortKeyCondition::BeginsWith { .. } => "begins_with", + }; + Some((name, op)) + } + + fn assert_range_gap(expr: &str, without_eq: &str, present: &str) { + let err = classify_one_hash(expr).unwrap_err(); + let want = format!( + "RANGE key attributes {without_eq} must have equality conditions specified in the \ + query because a condition is present on key attribute {present}" + ); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}, want {want}" + ); + } + + #[test] + fn one_hash_multi_range_hash_only_condition_is_accepted() { + let kc = classify_one_hash("h = :h").unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("h".to_owned())]); + assert!(kc.extra_pk_conditions.is_empty()); + assert!(kc.sk_condition.is_none()); + assert!(kc.extra_sk_conditions.is_empty()); + } + + #[test] + fn one_hash_multi_range_equality_prefixes_are_accepted() { + // Equalities on a leading run of the RANGE attributes, up to all four. + for (expr, extra) in [ + ("h = :h AND a = :a", vec![]), + ("h = :h AND a = :a AND b = :b", vec!["b"]), + ("h = :h AND a = :a AND b = :b AND c = :c", vec!["b", "c"]), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d", + vec!["b", "c", "d"], + ), + ] { + let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); + assert_eq!( + kc.pk_path, + vec![PathElement::Attribute("h".to_owned())], + "{expr}" + ); + assert!(kc.extra_pk_conditions.is_empty(), "{expr}"); + assert_eq!(sk_shape(&kc), Some(("a".to_owned(), "=")), "{expr}"); + assert_eq!(extra_sk_names(&kc), extra, "{expr}"); + } + } + + #[test] + fn one_hash_multi_range_clause_order_is_free() { + // The defect: the first clause was taken as the partition key, so + // `b = :b AND h = :h AND a = :a` was refused as missing `h`. + for expr in [ + "b = :b AND h = :h AND a = :a", + "a = :a AND b = :b AND h = :h", + "b = :b AND a = :a AND h = :h", + ] { + let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); + assert_eq!( + kc.pk_path, + vec![PathElement::Attribute("h".to_owned())], + "{expr}" + ); + assert_eq!(sk_shape(&kc), Some(("a".to_owned(), "=")), "{expr}"); + assert_eq!(extra_sk_names(&kc), vec!["b"], "{expr}"); + } + } + + #[test] + fn one_hash_multi_range_operator_on_the_last_given_attribute_is_accepted() { + for (expr, sk, extra) in [ + ("h = :h AND a > :a", ("a", ">"), vec![]), + ("h = :h AND a = :a AND b < :b", ("b", "<"), vec!["a"]), + ( + "h = :h AND a = :a AND b = :b AND c > :c", + ("c", ">"), + vec!["a", "b"], + ), + ( + "h = :h AND a = :a AND begins_with(b, :b)", + ("b", "begins_with"), + vec!["a"], + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND begins_with(d, :d)", + ("d", "begins_with"), + vec!["a", "b", "c"], + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d BETWEEN :x AND :y", + ("d", "BETWEEN"), + vec!["a", "b", "c"], + ), + ( + "c > :c AND b = :b AND h = :h AND a = :a", + ("c", ">"), + vec!["a", "b"], + ), + ] { + let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); + assert_eq!( + kc.pk_path, + vec![PathElement::Attribute("h".to_owned())], + "{expr}" + ); + assert_eq!(sk_shape(&kc), Some((sk.0.to_owned(), sk.1)), "{expr}"); + assert_eq!(extra_sk_names(&kc), extra, "{expr}"); + } + } + + #[test] + fn one_hash_multi_range_gap_is_refused() { + // A condition on a RANGE attribute needs an equality on every RANGE + // attribute before it. The defect accepted `h = :h AND b = :b` and + // returned no items. + assert_range_gap("h = :h AND b = :b", "a", "b"); + assert_range_gap("h = :h AND a = :a AND c = :c", "b", "c"); + assert_range_gap("h = :h AND a = :a AND b = :b AND d > :d", "c", "d"); + } + + #[test] + fn one_hash_multi_range_operator_before_a_later_condition_is_refused() { + assert_range_gap("h = :h AND a = :a AND b > :b AND c = :c", "b", "c"); + assert_range_gap("h = :h AND begins_with(a, :a) AND b = :b", "a", "b"); + } + + #[test] + fn one_hash_multi_range_missing_hash_is_refused() { + for expr in [ + "a = :a AND b = :b AND c = :c", + "a = :a AND b = :b", + "a = :a", + ] { + let err = classify_one_hash(expr).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "HASH key attributes h must have equality conditions specified in the query"), + "expr {expr}: got {err:?}" + ); + } + } + + /// Classify against HASH `h1`, `h2` and RANGE `r`. + fn classify_two_hash(expr: &str) -> Result { + let tokens = tokenize(expr).unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&["h1", "h2"], &["r"], &HashMap::new())?; + Ok(kc) + } + + #[test] + fn two_hash_missing_hash_names_every_missing_attribute() { + for (expr, missing) in [ + ("h1 = :x", "h2"), + ("h2 = :x", "h1"), + ("h1 = :x AND r = :y", "h2"), + ("r = :y", "h1, h2"), + ("h1 = :x AND h2 > :y", "h2"), + ] { + let err = classify_two_hash(expr).unwrap_err(); + let want = format!( + "HASH key attributes {missing} must have equality conditions specified in the query" + ); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + } + assert!(classify_two_hash("h1 = :x AND h2 = :y").is_ok()); + assert!(classify_two_hash("h1 = :x AND h2 = :y AND r > :z").is_ok()); + } + + #[test] + fn a_second_condition_on_one_key_attribute_is_refused() { + const WANT: &str = "Invalid KeyConditionExpression: KeyConditionExpressions must only \ + contain one condition per key"; + for expr in [ + "h = :x AND h = :y", + "h = :x AND h = :y AND a = :z", + "h = :x AND a = :y AND a = :z", + "h = :h AND a = :a AND a > :x", + "h = :h AND a > :x AND a = :a", + "h = :h AND a = :a AND b = :n AND b > :lo", + "h = :h AND h > :x", + "h = :h AND h = :y AND c = :c", + "a = :a AND a = :y", + ] { + let err = classify_one_hash(expr).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg == WANT), + "expr {expr}: got {err:?}" + ); + } + for expr in [ + "h1 = :x AND h1 = :y AND h2 = :z", + "h1 = :x AND h1 = :y", + "h1 = :x AND h2 = :n AND r = :r AND r > :y", + "h1 = :x AND r = :r AND r = :y", + ] { + let err = classify_two_hash(expr).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg == WANT), + "expr {expr}: got {err:?}" + ); + } + } + + #[test] + fn a_condition_on_a_non_key_attribute_names_the_first_uncovered_range_attribute() { + for (expr, missing) in [ + ("h = :h AND z = :z", "a"), + ("h = :h AND z = :z AND y = :y", "a"), + ("h = :h AND z > :z", "a"), + ("h = :h AND a = :a AND z = :z", "b"), + ("h = :h AND a = :a AND z > :z", "b"), + ("h = :h AND a > :a AND z = :z", "b"), + ("h = :h AND a = :a AND b = :b AND z > :z", "c"), + ("h = :h AND a = :a AND b = :b AND c = :c AND z = :z", "d"), + ] { + let err = classify_one_hash(expr).unwrap_err(); + let want = format!("Query condition missed key schema element: {missing}"); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + } + let err = classify_two_hash("h1 = :x AND h2 = :y AND z = :z").unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Query condition missed key schema element: r")); + // A gap is reported before the non-key attribute. + let err = classify_one_hash("h = :h AND b = :b AND z = :z").unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) + if msg.starts_with("RANGE key attributes a must have equality conditions"))); + // A missing HASH attribute is reported before the non-key attribute. + let err = classify_two_hash("h1 = :x AND z = :z").unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "HASH key attributes h2 must have equality conditions specified in the query")); + } + + #[test] + fn more_conditions_than_key_attributes_are_refused_by_count() { + for (expr, n) in [ + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND z = :z", + 6, + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND z > :z", + 6, + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND PK = :p", + 6, + ), + ] { + let err = classify_one_hash(expr).unwrap_err(); + let want = format!( + "The number of query conditions ({n}) exceeds the number of key attributes \ + defined in the schema (5)" + ); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + } + let err = classify_two_hash("h1 = :x AND h2 = :y AND r = :r AND z = :z").unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "The number of query conditions (4) exceeds the number of key attributes defined in the schema (3)")); + } + + #[test] + fn the_count_is_refused_before_a_missing_hash_attribute_or_a_range_gap() { + // Each count case above has every key attribute, so they also pass + // with the count check moved after the missing-HASH and RANGE-gap + // checks. These lack the HASH attribute or leave a RANGE gap. + let count = |n: usize, attrs: usize| { + format!( + "The number of query conditions ({n}) exceeds the number of key attributes \ + defined in the schema ({attrs})" + ) + }; + for expr in [ + "a = :a AND b = :b AND c = :c AND d = :d AND z1 = :z AND z2 = :y", + "h = :h AND b = :b AND c = :c AND d = :d AND z1 = :z AND z2 = :y", + ] { + let err = classify_one_hash(expr).unwrap_err(); + let want = count(6, 5); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + } + let expr = "a = :a AND b = :b AND c = :c AND z1 = :z AND z2 = :y"; + let err = classify_two_hash(expr).unwrap_err(); + let want = count(5, 3); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + // The same five clauses on HASH `h` and RANGE `a`, `b`, `c`, `d` do not + // exceed its five key attributes, so the missing HASH attribute is + // reported. + let err = classify_one_hash(expr).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "HASH key attributes h must have equality conditions specified in the query"), + "expr {expr}: got {err:?}" + ); + } + + #[test] + fn one_hash_multi_range_resolves_name_references() { + let names: HashMap = [("hh", "h"), ("aa", "a"), ("bb", "b")] + .into_iter() + .map(|(k, v)| (k.to_owned(), v.to_owned())) + .collect(); + let kc = classify_with_names("#bb > :b AND #hh = :h AND #aa = :a", &names).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("#hh".to_owned())]); + assert_eq!(sk_shape(&kc), Some(("#bb".to_owned(), ">"))); + assert_eq!(extra_sk_names(&kc), vec!["#aa"]); + + let err = classify_with_names("#hh = :h AND #bb = :b", &names).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg.starts_with("RANGE key attributes a must have")), + "got {err:?}" + ); + } + + #[test] + fn one_hash_one_range_schema_is_left_unchanged() { + let tokens = tokenize("sk = :s AND pk = :p").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&["pk"], &["sk"], &HashMap::new()) + .unwrap(); + // Still the parser's assignment; resolve_pk_sk handles this schema. + assert_eq!(kc.pk_path, vec![PathElement::Attribute("sk".to_owned())]); + } + + #[test] + fn multi_hash_partition_key_need_not_come_first() { + // With two HASH attributes the first clause used to be checked as the + // first HASH attribute before reclassification, so this order was + // refused as missing `pk1`. + let tokens = tokenize("sk = :s AND pk1 = :a AND pk2 = :b").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&["pk1", "pk2"], &["sk"], &HashMap::new()) + .unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk1".to_owned())]); + assert_eq!( + kc.extra_pk_conditions[0].0, + vec![PathElement::Attribute("pk2".to_owned())] + ); + assert_eq!(sk_shape(&kc), Some(("sk".to_owned(), "="))); + assert!(kc.extra_sk_conditions.is_empty()); + } + + #[test] + fn multi_hash_multi_range_operator_after_equality_keeps_both() { + // The non-equality on the second RANGE attribute is the sort-key + // condition, and the equality on the first is carried with it. + let tokens = tokenize("pk1 = :a AND pk2 = :b AND s1 = :x AND s2 > :y").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&["pk1", "pk2"], &["s1", "s2"], &HashMap::new()) + .unwrap(); + assert_eq!(sk_shape(&kc), Some(("s2".to_owned(), ">"))); + assert_eq!(extra_sk_names(&kc), vec!["s1"]); + } + + fn key(name: &str, key_type: KeyType) -> KeySchemaElement { + KeySchemaElement { + attribute_name: name.to_owned(), + key_type, + } + } + + #[test] + fn resolve_key_schema_reads_a_multi_attribute_schema_by_name() { + // Authorization reads the partition key value from `pk_path` after this + // call, so it must name the first HASH attribute for every clause order + // the engine serves. + let two_hash = [ + key("g1", KeyType::Hash), + key("g2", KeyType::Hash), + key("gs", KeyType::Range), + ]; + let two_range = [ + key("h", KeyType::Hash), + key("a", KeyType::Range), + key("b", KeyType::Range), + ]; + for (schema, expr) in [ + (&two_hash[..], "g2 = :g AND g1 = :v AND gs = :x"), + (&two_hash[..], "gs = :x AND g1 = :v AND g2 = :g"), + (&two_hash[..], "g2 = :g AND g1 = :v AND gs > :lo"), + (&two_range[..], "a = :a AND h = :v AND b = :c"), + (&two_range[..], "b = :c AND h = :v AND a = :a"), + (&two_range[..], "b > :lo AND a = :a AND h = :v"), + (&two_range[..], "begins_with(b, :p) AND a = :a AND h = :v"), + ] { + let tokens = tokenize(expr).unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_key_schema(schema, &HashMap::new()) + .unwrap_or_else(|e| panic!("{expr}: {e:?}")); + let hash = &schema[0].attribute_name; + assert_eq!( + kc.pk_path, + vec![PathElement::Attribute(hash.clone())], + "{expr}" + ); + assert_eq!(kc.pk_value, Expr::Placeholder("v".to_owned()), "{expr}"); + } + } + + #[test] + fn resolve_key_schema_checks_values_before_resolve_pk_sk() { + let schema = [key("pk", KeyType::Hash), key("sk", KeyType::Range)]; + let tokens = tokenize("sk BETWEEN :lo AND :hi AND pk = :p").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + let mut checked = Vec::new(); + kc.resolve_key_schema_with(&schema, &HashMap::new(), |_, value| { + checked.push(format!("{value:?}")); + Ok(()) + }) + .unwrap(); + assert_eq!(checked.len(), 3, "{checked:?}"); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); + + let tokens = tokenize("sk = :s AND pk = :p").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + let err = kc + .resolve_key_schema_with(&schema, &HashMap::new(), |_, _| { + Err(validation_err("checked")) + }) + .unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) if msg == "checked")); + // The failed check left the parser's assignment in place. + assert_eq!(kc.pk_path, vec![PathElement::Attribute("sk".to_owned())]); + + kc.resolve_key_schema(&schema, &HashMap::new()).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); + } } diff --git a/crates/engine/src/legacy_filter.rs b/crates/engine/src/legacy_filter.rs index 27dbcd732..2ef07df77 100644 --- a/crates/engine/src/legacy_filter.rs +++ b/crates/engine/src/legacy_filter.rs @@ -22,6 +22,12 @@ pub fn desugar_key_conditions( conditions: &HashMap, key_schema: &[(String, bool)], // Vec of (attr_name, is_hash) ) -> Result<(KeyCondition, ExpressionMaps), DynamoDbError> { + let hash_count = key_schema.iter().filter(|(_, is_hash)| *is_hash).count(); + let range_count = key_schema.len() - hash_count; + if hash_count > 1 || range_count > 1 { + return desugar_multipart_key_conditions(conditions, key_schema); + } + // Find the hash key condition let hash_attr = key_schema .iter() @@ -162,6 +168,163 @@ fn desugar_sk_condition( } } +/// Desugar legacy `KeyConditions` on a key schema with several HASH or several +/// RANGE attributes into the same `KeyCondition` shape the expression parser +/// produces, so `KeyCondition::resolve_multipart` classifies both forms by one +/// rule (measured on the service 2026-10-21: `KeyConditions` on such an index +/// answer the same refusals as the equivalent `KeyConditionExpression`). +fn desugar_multipart_key_conditions( + conditions: &HashMap, + key_schema: &[(String, bool)], +) -> Result<(KeyCondition, ExpressionMaps), DynamoDbError> { + let mut values = HashMap::new(); + let mut equalities: Vec<(Vec, Expr)> = Vec::new(); + let mut non_equalities: Vec<(String, SortKeyCondition)> = Vec::new(); + + // Key-schema attributes first, in key-schema order, then any other name, + // so placeholder numbering and clause order are deterministic. + let mut ordered: Vec<&String> = key_schema + .iter() + .map(|(name, _)| name) + .filter(|name| conditions.contains_key(*name)) + .collect(); + let mut others: Vec<&String> = conditions + .keys() + .filter(|name| !key_schema.iter().any(|(k, _)| k == *name)) + .collect(); + others.sort(); + ordered.extend(others); + + for (i, attr) in ordered.into_iter().enumerate() { + let cond = &conditions[attr]; + if cond.comparison_operator == "EQ" { + if cond.attribute_value_list.len() != 1 { + return Err(DynamoDbError::ValidationException( + "Query key condition must have exactly one value for EQ".to_owned(), + )); + } + let placeholder = format!("_kc_eq{i}"); + values.insert(placeholder.clone(), cond.attribute_value_list[0].clone()); + equalities.push(( + vec![PathElement::Attribute(attr.clone())], + Expr::Placeholder(placeholder), + )); + } else { + let mut own_values = HashMap::new(); + let sk = desugar_sk_condition(cond, attr, &mut own_values)?; + // Rename this condition's placeholders so two non-equalities + // cannot collide on `_kc_sk`. + let sk = rename_placeholders(sk, &own_values, &format!("_kc_ne{i}_"), &mut values); + non_equalities.push((attr.clone(), sk)); + } + } + + let hash_attrs: Vec<&str> = key_schema + .iter() + .filter(|(_, is_hash)| *is_hash) + .map(|(name, _)| name.as_str()) + .collect(); + let range_attrs: Vec<&str> = key_schema + .iter() + .filter(|(_, is_hash)| !*is_hash) + .map(|(name, _)| name.as_str()) + .collect(); + + // Two non-equalities: the service names the RANGE attributes before the + // last conditioned one that lack an equality (measured: `h EQ, a GT, + // b GT` is `RANGE key attributes a must have equality conditions + // specified in the query because a condition is present on key attribute + // b`). + if non_equalities.len() > 1 { + let conditioned: Vec = conditions + .keys() + .filter_map(|name| range_attrs.iter().position(|r| r == name)) + .collect(); + if let Some(&last) = conditioned.iter().max() { + let without_eq: Vec<&str> = range_attrs[..last] + .iter() + .enumerate() + .filter(|(_, attr)| { + conditions + .get(**attr) + .is_none_or(|c| c.comparison_operator != "EQ") + }) + .map(|(_, attr)| *attr) + .collect(); + if !without_eq.is_empty() { + return Err(DynamoDbError::ValidationException(format!( + "RANGE key attributes {} must have equality conditions specified in the \ + query because a condition is present on key attribute {}", + without_eq.join(", "), + range_attrs[last] + ))); + } + } + return Err(DynamoDbError::ValidationException( + "Query key condition not supported".to_owned(), + )); + } + + let Some((pk_path, pk_value)) = equalities.first().cloned() else { + // No equality at all: every HASH attribute lacks one. + return Err(DynamoDbError::ValidationException(format!( + "HASH key attributes {} must have equality conditions specified in the query", + hash_attrs.join(", ") + ))); + }; + let extra_pk_conditions = equalities.into_iter().skip(1).collect(); + let sk_condition = non_equalities.into_iter().next().map(|(_, sk)| sk); + + Ok(( + KeyCondition { + pk_path, + pk_value, + sk_condition, + extra_pk_conditions, + extra_sk_conditions: Vec::new(), + }, + ExpressionMaps::new(HashMap::new(), values), + )) +} + +/// Move `own_values` into `values` under `prefix`-ed names and rewrite the +/// condition's placeholders to match. +fn rename_placeholders( + sk: SortKeyCondition, + own_values: &HashMap, + prefix: &str, + values: &mut HashMap, +) -> SortKeyCondition { + let mut rename = |expr: Expr| -> Expr { + match expr { + Expr::Placeholder(name) => { + let new_name = format!("{prefix}{name}"); + if let Some(v) = own_values.get(&name) { + values.insert(new_name.clone(), v.clone()); + } + Expr::Placeholder(new_name) + } + other => other, + } + }; + match sk { + SortKeyCondition::Compare { path, op, value } => SortKeyCondition::Compare { + path, + op, + value: rename(value), + }, + SortKeyCondition::Between { path, low, high } => SortKeyCondition::Between { + path, + low: rename(low), + high: rename(high), + }, + SortKeyCondition::BeginsWith { path, prefix: p } => SortKeyCondition::BeginsWith { + path, + prefix: rename(p), + }, + } +} + /// Desugar legacy `QueryFilter` or `ScanFilter` into a condition expression AST. /// /// Each entry maps an attribute name to a Condition with `ComparisonOperator` and values. diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index f45526af8..848f5fdbb 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -270,50 +270,67 @@ pub async fn handle_query( &maps }; - // Correct PK/SK assignment when both clauses are equality comparisons. - // The parser can't distinguish PK from SK without the key schema. let pk_attr = &query_key_info.key_schema[0].attribute_name; - key_condition.resolve_pk_sk(pk_attr, &effective_maps.names)?; - - // Validate that the partition key is actually referenced in the condition. - let pk_resolved = resolve_path_attr_name(&key_condition.pk_path, &effective_maps.names); - if pk_resolved.as_deref() != Some(pk_attr.as_str()) { - return Err(DynamoDbError::ValidationException(format!( - "Query condition missed key schema element: {pk_attr}" - ))); - } - - // For multi-part key schemas (GSIs with >1 HASH attribute), reclassify - // the parsed conditions so all HASH attributes go to pk_path/extra_pk_conditions - // and the RANGE condition stays as sk_condition. - if extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema) { - let hash_elements = extenddb_core::types::hash_key_elements(&query_key_info.key_schema); - let hash_attrs: Vec<&str> = hash_elements - .iter() - .map(|ks| ks.attribute_name.as_str()) - .collect(); - key_condition.resolve_multipart(&hash_attrs, &effective_maps.names)?; - - // Validate all HASH attributes are present in the KeyConditionExpression. - let provided_count = 1 + key_condition.extra_pk_conditions.len(); - if provided_count != hash_attrs.len() { - // Find the first missing HASH attribute for the error message. - let missing = hash_attrs + // Assign every clause to its key attribute through the classification + // authorization also uses to find dynamodb:LeadingKeys, so both read the + // same clause as the partition key. On a multi-attribute key schema (several + // HASH or several RANGE attributes) every clause is assigned by name, every + // HASH attribute needs an equality and the RANGE conditions must cover a + // leading run of the RANGE attributes with equalities before the last one. + // A wrong-typed value is reported where the service reports it: after those + // structural rules and before the non-key rule on a multi-attribute key + // schema, and ahead of the missing-key check on a single-attribute one + // (measured 2026-10-21: `pk = :n` on an S key with a second clause on a + // non-key attribute reports the type, not the missing key). + // + // On a multi-attribute key schema the classifier refuses a clause on an + // attribute outside the key schema (`Query condition missed key schema + // element`), so the type check reads only the queried key schema's + // definitions there. For an index query the table's list also holds the + // base table's key and the other indexes' key attributes. Measured + // 2026-10-30 on a GSI with HASH `GSI2PK` and RANGE `GSI2SK`: `GSI2PK = :h + // AND PK = :k` answers `Query condition missed key schema element: GSI2SK` + // whether `:k` has the table's type for `PK` or not, so the service's type + // check does not see an attribute outside the queried key schema. The + // single-attribute path has no non-key rule yet: a clause on another + // attribute reaches storage, which applies it to the first RANGE + // attribute, so it keeps checking against the table's definitions, which + // refuse a value of the wrong type before storage does. + let multipart = extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema); + let key_schema_definitions: Vec = query_key_info + .attribute_definitions + .iter() + .filter(|def| { + query_key_info + .key_schema .iter() - .find(|attr| { - let pk_name = - resolve_path_attr_name(&key_condition.pk_path, &effective_maps.names); - if pk_name.as_deref() == Some(*attr) { - return false; - } - !key_condition.extra_pk_conditions.iter().any(|(path, _)| { - resolve_path_attr_name(path, &effective_maps.names).as_deref() - == Some(*attr) - }) - }) - .unwrap_or(&hash_attrs[0]); + .any(|ks| ks.attribute_name == def.attribute_name) + }) + .cloned() + .collect(); + let type_definitions = if multipart { + &key_schema_definitions + } else { + &query_key_info.attribute_definitions + }; + key_condition.resolve_key_schema_with( + &query_key_info.key_schema, + &effective_maps.names, + |path, value| check_key_value_type(path, value, type_definitions, effective_maps), + )?; + refuse_begins_with_on_number( + &key_condition, + &query_key_info.attribute_definitions, + effective_maps, + )?; + // On a single-attribute key schema, validate that the partition key is + // actually referenced in the condition. The classifier has already required + // an equality on every HASH attribute of a multi-attribute one. + if !multipart { + let pk_resolved = resolve_path_attr_name(&key_condition.pk_path, &effective_maps.names); + if pk_resolved.as_deref() != Some(pk_attr.as_str()) { return Err(DynamoDbError::ValidationException(format!( - "Query condition missed key schema element: {missing}" + "Query condition missed key schema element: {pk_attr}" ))); } } @@ -574,6 +591,85 @@ fn resolve_path_attr_name( } } +/// Refuse a key condition value whose type differs from the attribute +/// definition of the attribute it names. +/// +/// Measured on the service 2026-10-21: an S key compared with an N value, +/// an N sort key compared with an S value in `=`, `>`, `begins_with` or a +/// BETWEEN bound, and the same on an index, all answer +/// `One or more parameter values were invalid: Condition parameter type does +/// not match schema type`, on single-attribute and multi-attribute key +/// schemas alike. A clause on an attribute without a definition in +/// `attribute_definitions` is not checked here. On a multi-attribute key +/// schema the caller passes the queried key schema's definitions only, and the +/// classifier reports a clause on any other attribute. +fn check_key_value_type( + path: &[PathElement], + expr: &extenddb_core::expression::Expr, + attribute_definitions: &[extenddb_core::types::AttributeDefinition], + maps: &ExpressionMaps, +) -> Result<(), DynamoDbError> { + use extenddb_core::expression::Expr; + use extenddb_core::types::{AttributeValue, ScalarAttributeType}; + + let Some(attr) = resolve_path_attr_name(path, &maps.names) else { + return Ok(()); + }; + let Some(def) = attribute_definitions + .iter() + .find(|d| d.attribute_name == attr) + else { + return Ok(()); + }; + let Expr::Placeholder(name) = expr else { + return Ok(()); + }; + let Some(value) = maps.values.get(name) else { + return Ok(()); + }; + let matches = matches!( + (def.attribute_type, value), + (ScalarAttributeType::S, AttributeValue::S(_)) + | (ScalarAttributeType::N, AttributeValue::N(_)) + | (ScalarAttributeType::B, AttributeValue::B(_)) + ); + if matches { + Ok(()) + } else { + Err(DynamoDbError::ValidationException( + "One or more parameter values were invalid: Condition parameter type does not match \ + schema type" + .to_owned(), + )) + } +} + +/// Refuse `begins_with` on a number key attribute (measured 2026-10-21: +/// `Invalid KeyConditionExpression: Incorrect operand type for operator or +/// function; operator or function: begins_with, operand type: N`). +fn refuse_begins_with_on_number( + key_condition: &extenddb_core::expression::KeyCondition, + attribute_definitions: &[extenddb_core::types::AttributeDefinition], + maps: &ExpressionMaps, +) -> Result<(), DynamoDbError> { + use extenddb_core::expression::SortKeyCondition; + use extenddb_core::types::ScalarAttributeType; + + if let Some(SortKeyCondition::BeginsWith { path, .. }) = &key_condition.sk_condition + && let Some(attr) = resolve_path_attr_name(path, &maps.names) + && attribute_definitions + .iter() + .any(|d| d.attribute_name == attr && d.attribute_type == ScalarAttributeType::N) + { + return Err(DynamoDbError::ValidationException( + "Invalid KeyConditionExpression: Incorrect operand type for operator or function; \ + operator or function: begins_with, operand type: N" + .to_owned(), + )); + } + Ok(()) +} + /// Validate that all `#name` references in an expression AST are defined. fn validate_name_refs_in_expr( expr: &extenddb_core::expression::Expr, diff --git a/crates/server/src/leading_keys.rs b/crates/server/src/leading_keys.rs index e5aa17a2f..0f2577985 100644 --- a/crates/server/src/leading_keys.rs +++ b/crates/server/src/leading_keys.rs @@ -30,7 +30,18 @@ //! value from the key condition (an LSI shares the base partition key). //! - `Query` on a global secondary index: the index's partition key value from //! the key condition. The service binds LeadingKeys to the queried index's -//! partition key, not the base table's. +//! partition key, not the base table's. On an index with several HASH +//! attributes it binds the first one's value only (measured 2026-10-20). The +//! clauses of a key condition on a multi-attribute key schema are assigned +//! to key attributes by name, through the same classification the engine +//! uses, so any clause order the engine serves binds the first HASH +//! attribute. When an equality names the first HASH attribute and the +//! classification refuses the condition (a missing HASH attribute, for one), +//! the key is left absent, as the service answers `ValidationException` to +//! every principal for `g1 = :a` on HASH `g1`, `g2` (measured 2026-10-21). A +//! single clause on the partition attribute under an operator the engine +//! rejects binds that clause's operand, the rule measured for `pk > :v` on a +//! base table. //! - `Scan` and `SearchVectors`: no LeadingKeys (neither targets a single //! partition, so the service sets none). //! - `BatchGetItem`/`BatchWriteItem`: one authorization per (action, table) @@ -55,7 +66,7 @@ use extenddb_core::expression::{ Expr, PathElement, Token, key_condition_clause_operands, parse_key_condition, tokenize_with_limit, }; -use extenddb_core::types::{KeyType, TableKeyInfo}; +use extenddb_core::types::{KeySchemaElement, KeyType, TableKeyInfo}; use serde_json::Value; /// A single `(action, table)` authorization to perform for a batch or @@ -235,21 +246,22 @@ pub(crate) fn single_table_leading_keys( /// schema (base table, or the named index). `None` when the key condition names /// no single value for that partition attribute. fn query_leading_keys(input: &Value, key_info: &TableKeyInfo, max_tokens: usize) -> Option { - // The partition attribute of whatever the query targets: the named index's - // partition key, or the base table's. A GSI has its own partition key; an - // LSI shares the base table's. An unknown index name is a request the engine - // rejects, so leave the key absent. - let pk_attr: String = match index_name(input) { + // The key schema the query reads: the named index's, or the base table's. + // A GSI has its own partition key; an LSI shares the base table's. An + // unknown index name is a request the engine rejects, so leave the key + // absent. + let key_schema: &[KeySchemaElement] = match index_name(input) { Some(index) => { - let info = key_info + &key_info .global_secondary_indexes .iter() .chain(key_info.local_secondary_indexes.iter()) - .find(|i| i.index_name == index)?; - hash_attr(&info.key_schema)?.to_owned() + .find(|i| i.index_name == index)? + .key_schema } - None => hash_attr(&key_info.key_schema)?.to_owned(), + None => &key_info.key_schema, }; + let pk_attr = hash_attr(key_schema)?; let names = names_map(input); let values = values_map(input); @@ -260,17 +272,17 @@ fn query_leading_keys(input: &Value, key_info: &TableKeyInfo, max_tokens: usize) let pk_clauses: Vec<_> = key_condition_clause_operands(&tokens) .ok()? .into_iter() - .filter(|clause| { - resolve_attr_name(&clause.path, &names).as_deref() == Some(pk_attr.as_str()) - }) + .filter(|clause| resolve_attr_name(&clause.path, &names).as_deref() == Some(pk_attr)) .collect(); // An equality on the partition attribute decides the key, as the - // engine reads it. When its value cannot be read here (an `N` that - // is not a number, a placeholder with no value), the key stays absent - // and no other clause's operand is bound in its place: the equality, - // not the other clause, names the partition. + // engine reads it. When the engine's classification refuses the + // condition (a missing HASH attribute on a multi-attribute key schema, + // for one) or the value cannot be read here (an `N` that is not a + // number, a placeholder with no value), the key stays absent and no + // other clause's operand is bound in its place: the equality, not the + // other clause, names the partition. if pk_clauses.iter().any(|clause| clause.equality) { - return equality_pk_value(&tokens, &pk_attr, &names, &values); + return equality_pk_value(&tokens, key_schema, pk_attr, &names, &values); } // Exactly one clause names the partition key, under an operator the // engine rejects, such as `pk > :v`. The service binds LeadingKeys to @@ -296,7 +308,7 @@ fn query_leading_keys(input: &Value, key_info: &TableKeyInfo, max_tokens: usize) // service's comparison operators is not read as one; both leave the key // absent. if let Some(conds) = input.get("KeyConditions").and_then(Value::as_object) { - let cond = conds.get(&pk_attr)?.as_object()?; + let cond = conds.get(pk_attr)?.as_object()?; let operator = cond.get("ComparisonOperator").and_then(Value::as_str)?; if !LEGACY_SINGLE_OPERAND_OPERATORS.contains(&operator) { return None; @@ -325,17 +337,24 @@ const LEGACY_SINGLE_OPERAND_OPERATORS: [&str; 9] = [ ]; /// The partition key value a `KeyConditionExpression` pins with an equality, -/// resolved the way the engine resolves it. `None` when the expression does not -/// parse as a key condition or does not pin `pk_attr` by equality. +/// resolved against the queried key schema the way the engine resolves it. +/// `None` when the expression does not parse as a key condition, when the +/// engine's classification refuses it, or when it does not pin `pk_attr` by +/// equality. fn equality_pk_value( tokens: &[Token], + key_schema: &[KeySchemaElement], pk_attr: &str, names: &HashMap, values: &HashMap, ) -> Option { let mut kc = parse_key_condition(tokens).ok()?; - // Assign PK vs SK using the partition attribute, mirroring the engine. - kc.resolve_pk_sk(pk_attr, names).ok()?; + // Assign every clause to its key attribute with the engine's own + // classification: by name on a multi-attribute key schema, where the + // first HASH attribute is the one LeadingKeys names, and by + // `resolve_pk_sk` on any other. A condition the classifier refuses is + // refused by the engine too. + kc.resolve_key_schema(key_schema, names).ok()?; // The condition must actually pin the partition key; if it does not, the // engine rejects the query. if resolve_attr_name(&kc.pk_path, names).as_deref() != Some(pk_attr) { diff --git a/crates/server/src/leading_keys/tests.rs b/crates/server/src/leading_keys/tests.rs index d68425039..c81e3baaa 100644 --- a/crates/server/src/leading_keys/tests.rs +++ b/crates/server/src/leading_keys/tests.rs @@ -435,6 +435,116 @@ fn query_on_multi_hash_gsi_is_clause_order_independent() { assert_eq!(r, Some(vec!["alice".to_owned()])); } +/// A table carrying a GSI with HASH `g1`, `g2` and RANGE `gs`, and a GSI with +/// HASH `h` and RANGE `a`, `b`. +fn multi_attribute_gsi_table() -> TableKeyInfo { + let gsi = |name: &str, key_schema: Vec| IndexInfo { + index_name: name.to_owned(), + index_id: name.to_owned(), + index_type: IndexType::Gsi, + key_schema, + projection: proj(), + }; + TableKeyInfo { + table_name: "mk".to_owned(), + key_schema: vec![ks("pk", KeyType::Hash)], + base_key_schema: vec![ks("pk", KeyType::Hash)], + global_secondary_indexes: vec![ + gsi( + "gsi3", + vec![ + ks("g1", KeyType::Hash), + ks("g2", KeyType::Hash), + ks("gs", KeyType::Range), + ], + ), + gsi( + "gsi4", + vec![ + ks("h", KeyType::Hash), + ks("a", KeyType::Range), + ks("b", KeyType::Range), + ], + ), + ], + ..Default::default() + } +} + +/// A Query on `index` of the multi-attribute table naming `alice` as the +/// first HASH attribute's value. +fn multi_attribute_query(index: &str, kce: &str, names: &[(&str, &str)]) -> Value { + let names: serde_json::Map = names + .iter() + .map(|(k, v)| ((*k).to_owned(), json!(v))) + .collect(); + json!({ + "TableName": "mk", + "IndexName": index, + "KeyConditionExpression": kce, + "ExpressionAttributeNames": names, + "ExpressionAttributeValues": { + ":v": {"S": "alice"}, ":g": {"S": "blue"}, ":x": {"S": "x1"}, + ":lo": {"S": "a"}, ":a": {"S": "a1"}, ":c": {"S": "b1"}, ":p": {"S": "b"}, + }, + }) +} + +/// One Query shape: the index, the key condition and its `#name` references. +type MultiAttributeCase<'a> = (&'a str, &'a str, &'a [(&'a str, &'a str)]); + +#[test] +fn query_on_multi_attribute_gsi_binds_the_first_hash_attribute_in_any_clause_order() { + // The engine assigns the clauses of a multi-attribute key schema by name + // and serves each of these orders. Authorization used to read them with + // `resolve_pk_sk`, found no partition clause and left the key absent, so + // a ForAllValues fence let the query through. + let hash_names = [("#g1", "g1"), ("#g2", "g2"), ("#gs", "gs")]; + let cases: &[MultiAttributeCase] = &[ + ("gsi3", "g2 = :g AND g1 = :v AND gs = :x", &[]), + ("gsi3", "gs = :x AND g1 = :v AND g2 = :g", &[]), + ("gsi3", "g2 = :g AND g1 = :v AND gs > :lo", &[]), + ("gsi3", "#g2 = :g AND #g1 = :v AND #gs = :x", &hash_names), + ("gsi3", "#gs = :x AND #g1 = :v AND #g2 = :g", &hash_names), + ("gsi3", "#g2 = :g AND #g1 = :v AND #gs > :lo", &hash_names), + ("gsi4", "a = :a AND h = :v AND b = :c", &[]), + ("gsi4", "b = :c AND h = :v AND a = :a", &[]), + ("gsi4", "a = :a AND h = :v AND b > :lo", &[]), + ("gsi4", "b > :lo AND a = :a AND h = :v", &[]), + ("gsi4", "begins_with(b, :p) AND a = :a AND h = :v", &[]), + // Orders authorization already read correctly. + ("gsi3", "g1 = :v AND g2 = :g AND gs = :x", &[]), + ("gsi4", "h = :v AND a = :a AND b = :c", &[]), + ]; + for (index, kce, names) in cases { + let input = multi_attribute_query(index, kce, names); + let r = single_table_leading_keys( + &input, + "Query", + Some(&multi_attribute_gsi_table()), + MAX_TOKENS, + ); + assert_eq!(r, Some(vec!["alice".to_owned()]), "{index}: {kce}"); + } +} + +#[test] +fn query_on_multi_hash_gsi_missing_a_hash_attribute_is_absent() { + // The engine refuses `g1 = :a` on HASH `g1`, `g2` (`HASH key attributes g2 + // must have equality conditions specified in the query`). Measured + // 2026-10-21: the service answers ValidationException to every principal + // for this shape, a principal fenced away from `alice` included, so the key + // is not bound. + let input = json!({ + "TableName": "m", + "IndexName": "gsi2", + "KeyConditionExpression": "g1 = :a", + "ExpressionAttributeValues": {":a": {"S": "alice"}}, + }); + let r = single_table_leading_keys(&input, "Query", Some(&multi_hash_gsi_table()), MAX_TOKENS); + assert_eq!(r, None); +} + // ---- query: shapes the engine rejects (key left absent) ----------------- #[test] diff --git a/crates/storage-cassandra/src/data/query.rs b/crates/storage-cassandra/src/data/query.rs index 65b0c242f..ceb604b6d 100644 --- a/crates/storage-cassandra/src/data/query.rs +++ b/crates/storage-cassandra/src/data/query.rs @@ -122,6 +122,13 @@ impl CassandraEngine { ) }; + extenddb_storage::util::refuse_conditions_beyond_first_range( + key_condition, + &query_key_schema, + &expression_maps.names, + "Cassandra", + )?; + // Step 1: Resolve partition key value — composite for multi-part HASH keys. let pk_av = resolve_expr_to_av(&key_condition.pk_value, expression_maps)?; let pk_text = if key_condition.extra_pk_conditions.is_empty() { @@ -225,6 +232,29 @@ impl CassandraEngine { && exclusive_start_key.is_some() && (base_sk_info_val.is_some() || sk_info_opt.is_some()); + // Every RANGE attribute of the index is a clustering column, in key-schema + // order, ahead of base_pk. CQL accepts an ORDER BY only over a prefix of + // the clustering columns in declared order, so an index with several + // RANGE attributes must name all of them. + let index_sk_columns: Vec = if index_name.is_some() { + super::ddl::all_sort_key_info(&query_key_schema, &key_info.attribute_definitions) + .iter() + .enumerate() + .map(|(i, &(_, sk_type))| extenddb_storage::util::sk_column_n(i, sk_type)) + .collect() + } else { + Vec::new() + }; + if index_sk_columns.len() > 1 && exclusive_start_key.is_some() { + // The cursor compares the first RANGE attribute and the base key + // only, which is not the clustering order of such an index. + return Err(StorageError::Validation(format!( + "ExclusiveStartKey on index {}: the Cassandra backend pages a Query over \ + an index with one RANGE attribute only", + index_name.unwrap_or_default() + ))); + } + // Special case: hash-only index needs base_pk pagination but uses single query let is_hash_only_index = index_name.is_some() && sk_info_opt.is_none(); @@ -273,8 +303,12 @@ impl CassandraEngine { let direction = if forward { "ASC" } else { "DESC" }; query.push_str(&format!(" ORDER BY {sk_col} {direction}")); if index_name.is_some() { - // base_pk is a clustering key after the index SK; include it - // so ties are broken consistently and the cursor is stable. + // The remaining index RANGE columns, then base_pk: all are + // clustering keys after the first index SK, so ties are + // broken consistently and the cursor is stable. + for col in index_sk_columns.iter().skip(1) { + query.push_str(&format!(", {col} {direction}")); + } query.push_str(&format!(", base_pk {direction}")); if let Some((_, base_sk_type)) = &base_sk_info_val { let base_sk_col = format!("base_{}", sk_column(*base_sk_type)); diff --git a/crates/storage-mongodb/src/data_engine.rs b/crates/storage-mongodb/src/data_engine.rs index dd8f4cc9f..b8e97fb86 100644 --- a/crates/storage-mongodb/src/data_engine.rs +++ b/crates/storage-mongodb/src/data_engine.rs @@ -1123,6 +1123,13 @@ impl MongoEngine { let mut filter = doc! { "pk": &pk_text }; + extenddb_storage::util::refuse_conditions_beyond_first_range( + key_condition, + &effective_key_schema, + &maps.names, + "MongoDB", + )?; + // Determine sort key field using effective key schema let sk_field = sk_field_name(&effective_key_schema, &key_info.attribute_definitions); diff --git a/crates/storage-postgres/src/data/query.rs b/crates/storage-postgres/src/data/query.rs index 6a84dc1b7..a57b69bec 100755 --- a/crates/storage-postgres/src/data/query.rs +++ b/crates/storage-postgres/src/data/query.rs @@ -145,9 +145,11 @@ pub(crate) fn build_sk_sql( /// /// # Parameter binding order: /// 1. `pk_text` (partition key) -/// 2. SK condition params (from `build_sk_sql`) -/// 3. Extra SK equality params (multi-range schemas) -/// 4. `ExclusiveStartKey` index SK value (if paginating with sort key) +/// 2. SK condition params (from `build_sk_sql`), as `sk_cond_type` +/// 3. RANGE equality params that are not the SK condition, in `extra_sk_binds` +/// order (the order their placeholders were written) +/// 4. `ExclusiveStartKey` values of every RANGE attribute in `range_keys` +/// (the index sort key columns of the cursor comparison) /// 5. Pagination extra binds (from `PaginationBinds` enum — built in `query_scan.rs` /// in the same branch that generates the SQL, so order cannot diverge) #[allow(clippy::too_many_arguments)] @@ -156,8 +158,9 @@ pub(crate) async fn execute_query_sql( pk_text: &str, key_condition: &KeyCondition, maps: &ExpressionMaps, - sk_info: Option<(&str, ScalarAttributeType)>, - extra_sk_col_indices: &[(usize, ScalarAttributeType)], + sk_cond_type: Option, + extra_sk_binds: &[(ScalarAttributeType, &Expr)], + range_keys: &[(&str, ScalarAttributeType)], exclusive_start_key: Option<&Item>, pagination_binds: &PaginationBinds, pool: &sqlx::PgPool, @@ -166,25 +169,26 @@ pub(crate) async fn execute_query_sql( query = query.bind(pk_text.to_owned()); // Bind sort key condition values - if let (Some(sk_cond), Some((_, sk_type))) = (&key_condition.sk_condition, sk_info) { + if let (Some(sk_cond), Some(sk_type)) = (&key_condition.sk_condition, sk_cond_type) { query = bind_sk_condition(query, sk_cond, sk_type, maps)?; } - // Bind extra RANGE key equality values - for (i, &(_pos, sk_type)) in extra_sk_col_indices.iter().enumerate() { - if let Some((_, value)) = key_condition.extra_sk_conditions.get(i) { - let av = resolve_expr_to_av(value, maps)?; - let sk = parse_sk(&av, sk_type)?; - query = bind_sk_value(query, &sk); - } + // Bind the other RANGE key equality values + for &(sk_type, value) in extra_sk_binds { + let av = resolve_expr_to_av(value, maps)?; + let sk = parse_sk(&av, sk_type)?; + query = bind_sk_value(query, &sk); } - // Bind exclusive start key SK value (index SK for the > / < comparison) - if let (Some(start_key), Some((sk_name, sk_type))) = (exclusive_start_key, sk_info) - && let Some(sk_val) = start_key.get(sk_name) - { - let sk = parse_sk(sk_val, sk_type)?; - query = bind_sk_value(query, &sk); + // Bind the exclusive start key's RANGE values (index SK columns of the + // > / < row comparison) + if let Some(start_key) = exclusive_start_key { + for &(sk_name, sk_type) in range_keys { + if let Some(sk_val) = start_key.get(sk_name) { + let sk = parse_sk(sk_val, sk_type)?; + query = bind_sk_value(query, &sk); + } + } } // Bind pagination extra values — order is determined by the enum variant, diff --git a/crates/storage-postgres/src/data/query_scan.rs b/crates/storage-postgres/src/data/query_scan.rs index c0f0c8b05..be6de8a6f 100644 --- a/crates/storage-postgres/src/data/query_scan.rs +++ b/crates/storage-postgres/src/data/query_scan.rs @@ -41,18 +41,18 @@ use crate::PostgresEngine; /// `ORDER BY` repeated the first page's items and skipped the rest. fn build_pagination_where( param_idx: u32, - sk_info_val: Option<(&str, ScalarAttributeType)>, + range_keys: &[(&str, ScalarAttributeType)], base_sk_info: &Option<(String, ScalarAttributeType)>, is_index: bool, is_lsi: bool, forward: bool, ) -> String { - let cols = order_by_columns(sk_info_val, base_sk_info.as_ref(), is_index, is_lsi); + let cols = order_by_columns(range_keys, base_sk_info.as_ref(), is_index, is_lsi); if cols.is_empty() { return String::new(); } let cmp = if forward { ">" } else { "<" }; - let last = param_idx + u32::try_from(cols.len()).expect("at most three cursor columns"); + let last = param_idx + u32::try_from(cols.len()).expect("at most six cursor columns"); let params = (param_idx..last) .map(|i| format!("${i}")) .collect::>(); @@ -67,18 +67,22 @@ fn build_pagination_where( /// page cursor compares, in order. Empty for a base-table Scan, whose rows are /// ordered by the primary key clause the caller writes. /// +/// `range_keys` is every RANGE attribute of the queried key schema, in +/// key-schema order; an index can declare up to four, ordered +/// lexicographically over all of them. +/// /// - Base table with a sort key: the sort key. /// - LSI: the index sort key, then the base sort key (every row shares the /// queried partition key, so the base sort key alone identifies a row). -/// - GSI with a sort key: the index sort key, then the FULL base primary key. -/// Rows in a GSI partition are unique on (index SK, base PK, base SK), not -/// on (index SK, base SK): many base partitions can project the same index -/// SK and the same base SK, and comparing base SK alone made a page-two -/// query return nothing whenever the rows sharing an index SK also shared a -/// base SK. +/// - GSI with a sort key: the index sort key columns, then the FULL base +/// primary key. Rows in a GSI partition are unique on (index SK, base PK, +/// base SK), not on (index SK, base SK): many base partitions can project +/// the same index SK and the same base SK, and comparing base SK alone made +/// a page-two query return nothing whenever the rows sharing an index SK +/// also shared a base SK. /// - Hash-only index: the full base primary key. fn order_by_columns( - sk_info_val: Option<(&str, ScalarAttributeType)>, + range_keys: &[(&str, ScalarAttributeType)], base_sk_info: Option<&(String, ScalarAttributeType)>, is_index: bool, is_lsi: bool, @@ -91,20 +95,24 @@ fn order_by_columns( } }; let base_sk = base_sk_info.map(|(_, t)| collated(&format!("base_{}", sk_column(*t)), *t)); - let mut cols = Vec::with_capacity(3); - if let Some((_, sk_type)) = sk_info_val { - cols.push(collated(sk_column(sk_type), sk_type)); - if !is_index { - return cols; - } - if !is_lsi { + let mut cols = Vec::with_capacity(range_keys.len() + 2); + if range_keys.is_empty() { + if is_index { cols.push("base_pk COLLATE \"C\"".to_owned()); + cols.extend(base_sk); } - cols.extend(base_sk); - } else if is_index { + return cols; + } + for (i, &(_, sk_type)) in range_keys.iter().enumerate() { + cols.push(collated(&sk_column_n(i, sk_type), sk_type)); + } + if !is_index { + return cols; + } + if !is_lsi { cols.push("base_pk COLLATE \"C\"".to_owned()); - cols.extend(base_sk); } + cols.extend(base_sk); cols } @@ -157,51 +165,55 @@ impl PostgresEngine { let mut sql = format!("SELECT item_data FROM {ddb_table} WHERE pk = $1"); let mut param_idx: u32 = 2; - // Sort key condition SQL fragment (first RANGE key). - let sk_sql_info = if let (Some(sk_cond), Some((_, sk_type))) = - (&key_condition.sk_condition, sk_info_val) - { - Some(build_sk_sql(sk_cond, sk_column(sk_type), &mut param_idx)) - } else { - None - }; - - if let Some(ref info) = sk_sql_info { - sql.push_str(&info.fragment); - } - - // Extra RANGE key equality conditions (multi-RANGE key schemas). - // Each extra SK condition is an equality on an additional RANGE attribute. - let mut extra_sk_col_indices: Vec<(usize, ScalarAttributeType)> = Vec::new(); - for (path, _value) in &key_condition.extra_sk_conditions { - let attr_name = match path.first() { + // Each RANGE condition applies to the column of the attribute it + // names. On a multi-RANGE index the non-equality condition can name a + // later RANGE attribute and the first one is an equality in + // `extra_sk_conditions`. A sort-key condition that names no RANGE + // attribute applies to the first one. + let range_column = |path: &[extenddb_core::expression::PathElement]| -> Option<(usize, ScalarAttributeType)> { + let name = match path.first() { Some(extenddb_core::expression::PathElement::Attribute(name)) => { - if let Some(ref_name) = name.strip_prefix('#') { - if let Some(resolved) = maps.names.get(ref_name) { - resolved.clone() - } else { - tracing::warn!(name_ref = %ref_name, "unresolved expression attribute name in extra SK condition, skipping"); - continue; - } - } else { - name.clone() + match name.strip_prefix('#') { + Some(ref_name) => maps.names.get(ref_name)?.clone(), + None => name.clone(), } } - _ => continue, + _ => return None, }; - // Find which RANGE key index this attribute corresponds to - if let Some(pos) = all_sks + all_sks .iter() - .position(|(sk_name, _)| *sk_name == attr_name) - { - // Skip index 0 — that's the primary SK handled above - if pos > 0 { - let (_, sk_type) = all_sks[pos]; - let col = sk_column_n(pos, sk_type); - let _ = write!(sql, " AND {col} = ${param_idx}"); - param_idx += 1; - extra_sk_col_indices.push((pos, sk_type)); - } + .position(|(sk_name, _)| *sk_name == name) + .map(|pos| (pos, all_sks[pos].1)) + }; + + // Sort key condition SQL fragment, and the type its values bind as. + let sk_cond_type = if let Some(sk_cond) = &key_condition.sk_condition + && let Some((pos, sk_type)) = range_column(sk_cond.path()) + .or_else(|| sk_info_val.map(|(_, sk_type)| (0, sk_type))) + { + let info = build_sk_sql(sk_cond, &sk_column_n(pos, sk_type), &mut param_idx); + sql.push_str(&info.fragment); + Some(sk_type) + } else { + None + }; + + // RANGE equalities that are not the sort-key condition, each with the + // value it binds. A name that is not a RANGE attribute has no column + // and is skipped. + let mut extra_sk_binds: Vec<(ScalarAttributeType, &extenddb_core::expression::Expr)> = + Vec::new(); + for (path, value) in &key_condition.extra_sk_conditions { + if let Some((pos, sk_type)) = range_column(path) { + let col = sk_column_n(pos, sk_type); + let collate = if sk_type == ScalarAttributeType::S { + " COLLATE \"C\"" + } else { + "" + }; + let _ = write!(sql, " AND {col}{collate} = ${param_idx}"); + param_idx += 1; + extra_sk_binds.push((sk_type, value)); } } @@ -223,7 +235,7 @@ impl PostgresEngine { if exclusive_start_key.is_some() { let pagination_sql = build_pagination_where( param_idx, - sk_info_val, + &all_sks, &base_sk_info, index_name.is_some(), is_lsi, @@ -238,7 +250,7 @@ impl PostgresEngine { // is a row comparison the key index can seek on. String columns carry // COLLATE "C" for DynamoDB's UTF-8 byte order. let order_cols = order_by_columns( - sk_info_val, + &all_sks, base_sk_info.as_ref(), index_name.is_some(), is_lsi, @@ -334,8 +346,9 @@ impl PostgresEngine { &pk_text, key_condition, maps, - sk_info_val, - &extra_sk_col_indices, + sk_cond_type, + &extra_sk_binds, + &all_sks, exclusive_start_key, &pagination_binds, &self.data_pool, @@ -611,11 +624,11 @@ mod tests { #[test] fn base_table_cursor_is_a_single_comparison() { assert_eq!( - build_pagination_where(3, Some(("sk", S)), &None, false, false, true), + build_pagination_where(3, &[("sk", S)], &None, false, false, true), " AND sk_s COLLATE \"C\" > $3" ); assert_eq!( - build_pagination_where(3, Some(("sk", N)), &None, false, false, false), + build_pagination_where(3, &[("sk", N)], &None, false, false, false), " AND sk_n < $3" ); } @@ -623,14 +636,14 @@ mod tests { #[test] fn forward_gsi_cursor_is_a_row_comparison_over_the_full_base_key() { assert_eq!( - build_pagination_where(3, Some(("gsk", S)), &base_s(), true, false, true), + build_pagination_where(3, &[("gsk", S)], &base_s(), true, false, true), " AND (sk_s COLLATE \"C\", base_pk COLLATE \"C\", base_sk_s COLLATE \"C\") > ($3, $4, $5)" ); // Numeric index sort key with a numeric base sort key: no collation on either. assert_eq!( build_pagination_where( 3, - Some(("gsk", N)), + &[("gsk", N)], &Some(("bsk".to_owned(), N)), true, false, @@ -646,7 +659,7 @@ mod tests { // the exact mirror of its forward one, including among items sharing // an index sort key (measured 2026-09-17). assert_eq!( - build_pagination_where(3, Some(("gsk", S)), &base_s(), true, false, false), + build_pagination_where(3, &[("gsk", S)], &base_s(), true, false, false), " AND (sk_s COLLATE \"C\", base_pk COLLATE \"C\", base_sk_s COLLATE \"C\") < ($3, $4, $5)" ); } @@ -654,11 +667,11 @@ mod tests { #[test] fn gsi_on_a_hash_only_base_table_uses_base_pk_as_the_tie_breaker() { assert_eq!( - build_pagination_where(3, Some(("gsk", S)), &None, true, false, true), + build_pagination_where(3, &[("gsk", S)], &None, true, false, true), " AND (sk_s COLLATE \"C\", base_pk COLLATE \"C\") > ($3, $4)" ); assert_eq!( - build_pagination_where(3, Some(("gsk", S)), &None, true, false, false), + build_pagination_where(3, &[("gsk", S)], &None, true, false, false), " AND (sk_s COLLATE \"C\", base_pk COLLATE \"C\") < ($3, $4)" ); } @@ -666,11 +679,11 @@ mod tests { #[test] fn lsi_cursor_is_a_row_comparison_in_both_directions() { assert_eq!( - build_pagination_where(3, Some(("lsk", S)), &base_s(), true, true, true), + build_pagination_where(3, &[("lsk", S)], &base_s(), true, true, true), " AND (sk_s COLLATE \"C\", base_sk_s COLLATE \"C\") > ($3, $4)" ); assert_eq!( - build_pagination_where(3, Some(("lsk", S)), &base_s(), true, true, false), + build_pagination_where(3, &[("lsk", S)], &base_s(), true, true, false), " AND (sk_s COLLATE \"C\", base_sk_s COLLATE \"C\") < ($3, $4)" ); } @@ -678,15 +691,15 @@ mod tests { #[test] fn hash_only_index_cursor_is_a_row_comparison_over_the_base_key() { assert_eq!( - build_pagination_where(3, None, &base_s(), true, false, true), + build_pagination_where(3, &[], &base_s(), true, false, true), " AND (base_pk COLLATE \"C\", base_sk_s COLLATE \"C\") > ($3, $4)" ); assert_eq!( - build_pagination_where(3, None, &None, true, false, true), + build_pagination_where(3, &[], &None, true, false, true), " AND base_pk COLLATE \"C\" > $3" ); assert_eq!( - build_pagination_where(3, None, &None, false, false, true), + build_pagination_where(3, &[], &None, false, false, true), "" ); } @@ -698,15 +711,15 @@ mod tests { // after (p5, p4) asked for keys above p4, returned p5 again, and // then ended with p0..p3 never delivered. assert_eq!( - build_pagination_where(3, None, &None, true, false, false), + build_pagination_where(3, &[], &None, true, false, false), " AND base_pk COLLATE \"C\" < $3" ); assert_eq!( - build_pagination_where(3, None, &base_s(), true, false, false), + build_pagination_where(3, &[], &base_s(), true, false, false), " AND (base_pk COLLATE \"C\", base_sk_s COLLATE \"C\") < ($3, $4)" ); assert_eq!( - build_pagination_where(3, None, &Some(("bsk".to_owned(), N)), true, false, false), + build_pagination_where(3, &[], &Some(("bsk".to_owned(), N)), true, false, false), " AND (base_pk COLLATE \"C\", base_sk_n) < ($3, $4)" ); } @@ -731,9 +744,9 @@ mod tests { (None, None, true, false), ]; for (sk, base, is_index, is_lsi) in shapes { - let cols = order_by_columns(sk, base.as_ref(), is_index, is_lsi); - let fwd = build_pagination_where(1, sk, &base, is_index, is_lsi, true); - let rev = build_pagination_where(1, sk, &base, is_index, is_lsi, false); + let cols = order_by_columns(sk.as_slice(), base.as_ref(), is_index, is_lsi); + let fwd = build_pagination_where(1, sk.as_slice(), &base, is_index, is_lsi, true); + let rev = build_pagination_where(1, sk.as_slice(), &base, is_index, is_lsi, false); let params = (1..=cols.len()) .map(|i| format!("${i}")) .collect::>(); @@ -756,6 +769,35 @@ mod tests { "reverse {sk:?} {base:?}" ); } - assert!(order_by_columns(None, None, false, false).is_empty()); + assert!(order_by_columns(&[], None, false, false).is_empty()); + } + + #[test] + fn multi_range_gsi_orders_and_resumes_over_every_range_attribute() { + // A GSI with RANGE attributes a (S), b (N), c (S) orders on all three + // in key-schema order before the base key, and its cursor compares + // the same columns. Ordering on the first one alone left ties on `a` + // in base-key order. + let range_keys = [("a", S), ("b", N), ("c", S)]; + assert_eq!( + order_by_columns(&range_keys, base_s().as_ref(), true, false), + vec![ + "sk_s COLLATE \"C\"", + "sk2_n", + "sk3_s COLLATE \"C\"", + "base_pk COLLATE \"C\"", + "base_sk_s COLLATE \"C\"", + ] + ); + assert_eq!( + build_pagination_where(4, &range_keys, &base_s(), true, false, true), + " AND (sk_s COLLATE \"C\", sk2_n, sk3_s COLLATE \"C\", base_pk COLLATE \"C\", \ + base_sk_s COLLATE \"C\") > ($4, $5, $6, $7, $8)" + ); + assert_eq!( + build_pagination_where(4, &range_keys, &base_s(), true, false, false), + " AND (sk_s COLLATE \"C\", sk2_n, sk3_s COLLATE \"C\", base_pk COLLATE \"C\", \ + base_sk_s COLLATE \"C\") < ($4, $5, $6, $7, $8)" + ); } } diff --git a/crates/storage-sqlite/src/data/query_scan.rs b/crates/storage-sqlite/src/data/query_scan.rs index 585debd28..3eabf42ae 100644 --- a/crates/storage-sqlite/src/data/query_scan.rs +++ b/crates/storage-sqlite/src/data/query_scan.rs @@ -75,25 +75,34 @@ impl SqliteEngine { let mut sql = format!("SELECT item_data FROM {ddb_table} WHERE pk = ?"); let mut binds: Vec = vec![BoundValue::Text(pk_text)]; - // Primary sort-key condition. - if let (Some(sk_cond), Some((_, sk_type))) = (&key_condition.sk_condition, sk_info_val) { + // Each RANGE condition applies to the column of the attribute it + // names. On a multi-RANGE index the non-equality condition can name a + // later RANGE attribute and the first one is an equality in + // `extra_sk_conditions`. A sort-key condition that names no RANGE + // attribute applies to the first one. + let range_column = |path: &[PathElement]| -> Option<(usize, ScalarAttributeType)> { + let name = resolve_attr_name(path, maps)?; + all_sks + .iter() + .position(|(n, _)| *n == name) + .map(|pos| (pos, all_sks[pos].1)) + }; + if let Some(sk_cond) = &key_condition.sk_condition + && let Some((pos, sk_type)) = range_column(sk_cond.path()) + .or_else(|| sk_info_val.map(|(_, sk_type)| (0, sk_type))) + { let (sk_sql, sk_binds) = - build_sk_sql_and_binds(sk_cond, sk_column(sk_type), sk_type, maps)?; + build_sk_sql_and_binds(sk_cond, &sk_column_n(pos, sk_type), sk_type, maps)?; sql.push_str(&sk_sql); for v in &sk_binds { binds.push(sk_bound(v)); } } - // Extra RANGE-key equality conditions (multi-RANGE schemas). + // RANGE equalities that are not the sort-key condition. A name that is + // not a RANGE attribute has no column and is skipped. for (path, value) in &key_condition.extra_sk_conditions { - let Some(attr_name) = resolve_attr_name(path, maps) else { - continue; - }; - if let Some(pos) = all_sks.iter().position(|(n, _)| *n == attr_name) - && pos > 0 - { - let (_, sk_type) = all_sks[pos]; + if let Some((pos, sk_type)) = range_column(path) { let _ = write!(sql, " AND {} = ?", sk_column_n(pos, sk_type)); binds.push(sk_bound(&parse_sk( &resolve_expr_to_av(value, maps)?, @@ -111,7 +120,7 @@ impl SqliteEngine { &mut sql, &mut binds, start_key, - sk_info_val, + &all_sks, base_sk_info.as_ref(), key_info, index_name.is_some(), @@ -125,7 +134,7 @@ impl SqliteEngine { // of the forward one (as DynamoDB's is) and the cursor predicate is a // row comparison over the key index. let order_cols = query_order_columns( - sk_info_val, + &all_sks, base_sk_info.as_ref(), index_name.is_some(), is_lsi, @@ -410,37 +419,45 @@ fn base_pk_from_start_key( /// in order, mirroring the PostgreSQL `order_by_columns` cases. Empty for a /// hash-only base table, which has no order to resume within a partition. /// +/// `range_keys` is every RANGE attribute of the queried key schema, in +/// key-schema order; an index can declare up to four, ordered +/// lexicographically over all of them. +/// /// - Base table with a sort key: the sort key. /// - LSI: the index sort key, then the base sort key (every row shares the /// queried partition key, so the base sort key alone identifies a row). -/// - GSI with a sort key: the index sort key, then the FULL base primary key. -/// Rows in a GSI partition are unique on (index SK, base PK, base SK), not -/// on (index SK, base SK): many base partitions can project the same index -/// SK and the same base SK, and comparing base SK alone made a page-two -/// query return nothing whenever the rows sharing an index SK also shared a -/// base SK. +/// - GSI with a sort key: the index sort key columns, then the FULL base +/// primary key. Rows in a GSI partition are unique on (index SK, base PK, +/// base SK), not on (index SK, base SK): many base partitions can project +/// the same index SK and the same base SK, and comparing base SK alone made +/// a page-two query return nothing whenever the rows sharing an index SK +/// also shared a base SK. /// - Hash-only index: the full base primary key. fn query_order_columns( - sk_info_val: Option<(&str, ScalarAttributeType)>, + range_keys: &[(&str, ScalarAttributeType)], base_sk_info: Option<&(String, ScalarAttributeType)>, is_index: bool, is_lsi: bool, ) -> Vec { let base_sk = base_sk_info.map(|(_, t)| format!("base_{}", sk_column(*t))); - let mut cols = Vec::with_capacity(3); - if let Some((_, sk_type)) = sk_info_val { - cols.push(sk_column(sk_type).to_owned()); - if !is_index { - return cols; - } - if !is_lsi { + let mut cols = Vec::with_capacity(range_keys.len() + 2); + if range_keys.is_empty() { + if is_index { cols.push("base_pk".to_owned()); + cols.extend(base_sk); } - cols.extend(base_sk); - } else if is_index { + return cols; + } + for (i, &(_, sk_type)) in range_keys.iter().enumerate() { + cols.push(sk_column_n(i, sk_type)); + } + if !is_index { + return cols; + } + if !is_lsi { cols.push("base_pk".to_owned()); - cols.extend(base_sk); } + cols.extend(base_sk); cols } @@ -462,14 +479,14 @@ fn append_query_pagination( sql: &mut String, binds: &mut Vec, start_key: &Item, - sk_info_val: Option<(&str, ScalarAttributeType)>, + range_keys: &[(&str, ScalarAttributeType)], base_sk_info: Option<&(String, ScalarAttributeType)>, key_info: &TableKeyInfo, is_index: bool, is_lsi: bool, forward: bool, ) -> Result<(), StorageError> { - let cols = query_order_columns(sk_info_val, base_sk_info, is_index, is_lsi); + let cols = query_order_columns(range_keys, base_sk_info, is_index, is_lsi); if cols.is_empty() { return Ok(()); } @@ -483,9 +500,19 @@ fn append_query_pagination( }; // Binds in the same order as the columns. let mut cursor: Vec = Vec::with_capacity(cols.len()); - if let Some((sk_name, sk_type)) = sk_info_val { - // The sort key first; a base table's cursor is the sort key alone. - cursor.push(sk_bind(sk_name, sk_type)?); + if range_keys.is_empty() { + cursor.push(BoundValue::Text(base_pk_from_start_key( + start_key, key_info, + )?)); + if let Some((base_name, base_type)) = base_sk_info { + cursor.push(sk_bind(base_name, *base_type)?); + } + } else { + // The sort key columns first; a base table's cursor is the sort key + // alone. + for &(sk_name, sk_type) in range_keys { + cursor.push(sk_bind(sk_name, sk_type)?); + } if is_index { if !is_lsi { cursor.push(BoundValue::Text(base_pk_from_start_key( @@ -496,13 +523,6 @@ fn append_query_pagination( cursor.push(sk_bind(base_name, *base_type)?); } } - } else { - cursor.push(BoundValue::Text(base_pk_from_start_key( - start_key, key_info, - )?)); - if let Some((base_name, base_type)) = base_sk_info { - cursor.push(sk_bind(base_name, *base_type)?); - } } debug_assert_eq!(cursor.len(), cols.len()); @@ -629,7 +649,7 @@ mod tests { &mut sql, &mut binds, key, - sk, + sk.as_slice(), base_sk_info.as_ref(), &ki, is_index, @@ -789,7 +809,7 @@ mod tests { ]) }; let base_sk_info = base_sk.map(|t| ("bsk".to_owned(), t)); - let cols = query_order_columns(sk, base_sk_info.as_ref(), is_index, is_lsi); + let cols = query_order_columns(sk.as_slice(), base_sk_info.as_ref(), is_index, is_lsi); let marks = vec!["?"; cols.len()].join(", "); let (cols_sql, marks_sql) = if cols.len() == 1 { (cols[0].clone(), "?".to_owned()) @@ -812,9 +832,52 @@ mod tests { assert_eq!(fwd_binds, rev_binds); } // A hash-only base table has nothing to resume within a partition. - assert!(query_order_columns(None, None, false, false).is_empty()); + assert!(query_order_columns(&[], None, false, false).is_empty()); let (sql, binds) = paginate(None, None, false, false, true, &start_key(&[("bpk", "p")])); assert!(sql.is_empty()); assert!(binds.is_empty()); } + + #[test] + fn multi_range_gsi_orders_and_resumes_over_every_range_attribute() { + // A GSI with RANGE attributes a (S), b (N), c (S) orders on all three + // in key-schema order before the base key, and its cursor compares + // the same columns. Ordering on the first one alone left ties on `a` + // in base-key order. + let range_keys = [("a", S), ("b", N), ("c", S)]; + let base_sk_info = Some(("bsk".to_owned(), S)); + assert_eq!( + query_order_columns(&range_keys, base_sk_info.as_ref(), true, false), + vec!["sk_s", "sk2_n", "sk3_s", "base_pk", "base_sk_s"] + ); + let ki = key_info(Some(S)); + let mut key = start_key(&[("a", "T"), ("c", "x"), ("bpk", "p"), ("bsk", "s")]); + key.insert("b".to_owned(), AttributeValue::N("7".to_owned())); + for (forward, cmp) in [(true, ">"), (false, "<")] { + let mut sql = String::new(); + let mut binds = Vec::new(); + append_query_pagination( + &mut sql, + &mut binds, + &key, + &range_keys, + base_sk_info.as_ref(), + &ki, + true, + false, + forward, + ) + .expect("well-formed start key"); + assert_eq!( + sql, + format!(" AND (sk_s, sk2_n, sk3_s, base_pk, base_sk_s) {cmp} (?, ?, ?, ?, ?)") + ); + let binds = texts(&binds); + assert_eq!(binds.len(), 5); + assert_eq!(binds[0], "T"); + assert_eq!(binds[2], "x"); + assert_eq!(binds[3], "p"); + assert_eq!(binds[4], "s"); + } + } } diff --git a/crates/storage/src/util/key.rs b/crates/storage/src/util/key.rs index bf01740c7..8177f6e50 100755 --- a/crates/storage/src/util/key.rs +++ b/crates/storage/src/util/key.rs @@ -123,6 +123,67 @@ pub fn sk_info<'a>( Some((&sk_element.attribute_name, attr_def.attribute_type)) } +/// Refuse a Query key condition that a backend applying the sort key +/// condition to the first RANGE attribute only cannot serve. +/// +/// On a key schema with several RANGE attributes the engine classifies a +/// condition on any of them; the MongoDB and Cassandra backends store and +/// filter the first RANGE attribute only, so a condition on a later one has +/// no column to apply to. Answering with the first attribute's filter alone +/// would return items that do not satisfy the stated condition. `backend` +/// names the backend in the message. +pub fn refuse_conditions_beyond_first_range( + key_condition: &extenddb_core::expression::KeyCondition, + key_schema: &[KeySchemaElement], + names: &std::collections::HashMap, + backend: &str, +) -> Result<(), StorageError> { + let range_attrs: Vec<&str> = key_schema + .iter() + .filter(|ks| ks.key_type == KeyType::Range) + .map(|ks| ks.attribute_name.as_str()) + .collect(); + let attr_of = |path: &[extenddb_core::expression::PathElement]| -> Option { + match path.first() { + Some(extenddb_core::expression::PathElement::Attribute(name)) => name + .strip_prefix('#') + .map_or_else(|| Some(name.clone()), |r| names.get(r).cloned()), + _ => None, + } + }; + // Every RANGE attribute with a condition, whichever slot the classifier + // put it in; the one with the smallest key-schema position past the first + // names the refusal. + let mut conditioned: Vec = key_condition + .extra_sk_conditions + .iter() + .filter_map(|(path, _)| attr_of(path)) + .collect(); + if let Some(sk) = &key_condition.sk_condition + && let Some(attr) = attr_of(sk.path()) + { + conditioned.push(attr); + } + let beyond = conditioned + .into_iter() + .filter_map(|attr| { + range_attrs + .iter() + .position(|r| *r == attr) + .filter(|pos| *pos > 0) + .map(|pos| (pos, attr)) + }) + .min() + .map(|(_, attr)| attr); + match beyond { + Some(attr) => Err(StorageError::Validation(format!( + "Key condition on {attr}: the {backend} backend applies a key condition to the first \ + RANGE attribute of a multi-attribute index only" + ))), + None => Ok(()), + } +} + /// Merge the attribute definitions supplied by an `UpdateTable` request into the /// table's persisted set, returning the union. /// diff --git a/crates/storage/src/util/mod.rs b/crates/storage/src/util/mod.rs index bb2493827..bb79c2103 100755 --- a/crates/storage/src/util/mod.rs +++ b/crates/storage/src/util/mod.rs @@ -26,8 +26,8 @@ pub use catalog_version::check_stored_catalog_version; pub use key::SortKeyValue; pub use key::{ composite_pk_to_text, effective_attribute_definitions, encode_netstring_composite, - merge_attribute_definitions, parse_sk, pk_to_text, recover_sort_key_definitions, sk_column, - sk_column_n, sk_info, + merge_attribute_definitions, parse_sk, pk_to_text, recover_sort_key_definitions, + refuse_conditions_beyond_first_range, sk_column, sk_column_n, sk_info, }; pub use shard_stream::shard_in_stream; diff --git a/docs/differences-from-dynamodb.md b/docs/differences-from-dynamodb.md index 80c1f671b..6bd1a1607 100755 --- a/docs/differences-from-dynamodb.md +++ b/docs/differences-from-dynamodb.md @@ -94,6 +94,7 @@ adaptation when switching between ExtendDB and the real service. | Multi-part base table keys | Not supported | Preview extension (opt-in via `enable_multipart_keys` setting). Standard single/composite keys work identically. | | GSI throughput on a `PROVISIONED` table | Every GSI needs `ProvisionedThroughput` of at least 1 read and 1 write unit | A GSI `ProvisionedThroughput` below 1 (including 0/0) is `ValidationException` on `CreateTable` of a `PROVISIONED` table and on `UpdateTable`'s GSI `Create` ("ReadCapacityUnits and WriteCapacityUnits must both be greater than or equal to 1 for index: "; the service's text was not captured). A GSI with no `ProvisionedThroughput` is still accepted on a `PROVISIONED` table, and `UpdateTable` to `PROVISIONED` leaves an on-demand table's GSIs without one; see the `BillingModeOverride` row under Backup and Restore for how a backup of such a table restores | | Projected attributes across all indexes | At most 100 | Enforced on `CreateTable` (and so on `RestoreTableFromBackup`, which validates its target the same way) and on `UpdateTable`'s GSI `Create`: every `NonKeyAttributes` entry of every GSI and LSI is counted, so an attribute projected into two indexes counts twice. Over 100 is `ValidationException` ("Number of projected attributes in all indexes exceeds limit of 100, projected: "; the service's text was not captured) | +| `Query` on a GSI with several RANGE attributes (MongoDB and Cassandra backends) | Orders results over every RANGE attribute in key-schema order and serves a key condition on each of them (equality on every one before the last, any operator on the last) | SQLite and PostgreSQL match the service. The MongoDB and Cassandra backends filter the first RANGE attribute of such an index only: a key condition on a later RANGE attribute is refused with `ValidationException` naming the attribute and the backend. MongoDB orders results by the first RANGE attribute then the base table key. Cassandra orders a page over every RANGE attribute then the base table key, and refuses a `Query` that carries an `ExclusiveStartKey` on such an index with `ValidationException` (the cursor compares the first RANGE attribute only). Every refusal the service gives for the condition's shape (one condition per key, condition count, missing HASH attribute, RANGE gap, non-key attribute, value type) is the same on every backend. | ## Backup and Restore diff --git a/tests/test_fgac_leading_keys.py b/tests/test_fgac_leading_keys.py index 2c7054ec6..905fcd1f1 100644 --- a/tests/test_fgac_leading_keys.py +++ b/tests/test_fgac_leading_keys.py @@ -44,6 +44,7 @@ from __future__ import annotations import os +import re import time import uuid from typing import Any @@ -53,7 +54,7 @@ from botocore.config import Config as BotoConfig from botocore.exceptions import ClientError -from conftest import wait_for_active, wait_for_deleted +from conftest import configured_backend, wait_for_active, wait_for_deleted from management_helpers import ManagementClient OWN = "alice" @@ -95,6 +96,28 @@ def _denied(exc: ClientError) -> bool: return exc.value.response["Error"]["Code"] == "AccessDeniedException" +# The values a Query on the multi-attribute key table may name; each query +# carries only the ones its key condition uses. +_MK_VALUES = {":v": {"S": OWN}, ":g": {"S": "blue"}, ":x": {"S": "x1"}, + ":lo": {"S": "a"}, ":a": {"S": "a1"}, ":c": {"S": "b1"}, + ":p": {"S": "b"}} + + +def _mk_query(index: str, kce: str) -> dict: + """Query arguments for `kce` on `index` of the multi-attribute key table, + with a `#name` reference for every attribute written as `#`.""" + kwargs: dict = { + "IndexName": index, + "KeyConditionExpression": kce, + "ExpressionAttributeValues": { + k: _MK_VALUES[k] for k in sorted(set(re.findall(r":\w+", kce)))}, + } + refs = sorted(set(re.findall(r"#\w+", kce))) + if refs: + kwargs["ExpressionAttributeNames"] = {r: r[1:] for r in refs} + return kwargs + + @pytest.fixture(scope="module") def auth_env(): return _require_auth_env() @@ -566,6 +589,74 @@ def test_multi_hash_gsi_base_partition_key_unchanged(self, mh_table): c.get_item(TableName=mh_table, Key={"pk": {"S": "i3"}}) assert _denied(e) + # -- GSIs with multi-attribute keys and three or more conditions --------- + # + # gsi3 has HASH g1, g2 and RANGE gs; gsi4 has HASH h and RANGE a, b. The + # engine assigns every clause of a key condition on such an index to its + # key attribute by name, so it serves any clause order. The Query shapes in + # QUERY_ACCEPTED below use both indexes. + + def _make_multi_key_gsi_table(self) -> tuple[str, Any]: + owner = f"owner-{uuid.uuid4().hex[:8]}" + resp = self.mgmt.create_user(self.account_id, owner, "TestPass123!") + assert resp.status_code == 201, resp.text + self._users.append(owner) + resp = self.mgmt.create_access_key(self.account_id, owner) + assert resp.status_code == 201, resp.text + creds = resp.json() + resp = self.mgmt.put_user_policy(self.account_id, owner, "full", ALLOW_ALL) + assert resp.status_code == 204, resp.text + client = _make_client(self.endpoint, creds["access_key_id"], + creds["secret_access_key"], self.region) + table = f"lk-mk-{uuid.uuid4().hex[:8]}" + client.create_table( + TableName=table, + AttributeDefinitions=[ + {"AttributeName": name, "AttributeType": "S"} + for name in ("pk", "g1", "g2", "gs", "h", "a", "b") + ], + KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], + GlobalSecondaryIndexes=[ + { + "IndexName": "gsi3", + "KeySchema": [ + {"AttributeName": "g1", "KeyType": "HASH"}, + {"AttributeName": "g2", "KeyType": "HASH"}, + {"AttributeName": "gs", "KeyType": "RANGE"}, + ], + "Projection": {"ProjectionType": "ALL"}, + }, + { + "IndexName": "gsi4", + "KeySchema": [ + {"AttributeName": "h", "KeyType": "HASH"}, + {"AttributeName": "a", "KeyType": "RANGE"}, + {"AttributeName": "b", "KeyType": "RANGE"}, + ], + "Projection": {"ProjectionType": "ALL"}, + }, + ], + BillingMode="PAY_PER_REQUEST", + ) + wait_for_active(client, table) + for pk, who, n in [("i1", OWN, "1"), ("i2", OWN, "2"), + ("i3", OTHER, "1"), ("i4", OTHER, "2")]: + client.put_item(TableName=table, Item={ + "pk": {"S": pk}, "g1": {"S": who}, "g2": {"S": "blue"}, + "gs": {"S": "x" + n}, "h": {"S": who}, "a": {"S": "a1"}, + "b": {"S": "b" + n}}) + return table, client + + @pytest.fixture() + def mk_table(self): + name, owner = self._make_multi_key_gsi_table() + yield name + try: + owner.delete_table(TableName=name) + wait_for_deleted(owner, name) + except Exception: + pass + # -- Numeric (N) partition key rendering -------------------------------- # # The service writes an N partition key into dynamodb:LeadingKeys in one of @@ -818,9 +909,7 @@ def test_num_noncanonical_fence_matches_nothing(self, num_table): # other principal. Every rejected shape returned ValidationException to the # owner and to the fenced principal. To the other principal the service # returned ValidationException, except where QUERY_REJECTED_OTHER_CODE - # names a different code. For the shapes in QUERY_REJECTED_CODE_DIVERGES - # ExtendDB still answers the other code, so the test asserts only that no - # data is returned. + # names a different code. QUERY_ACCEPTED = [ ("pk eq placeholder", "main", {"KeyConditionExpression": "pk = :v", @@ -895,8 +984,43 @@ def test_num_noncanonical_fence_matches_nothing(self, num_table): "ExpressionAttributeValues": {":a": {"S": "alice"}, ":b": {"S": "blue"}}}, "g1", "alice"), + # Three-clause orders on multi-attribute key GSIs. Each applies two + # measured rules, LeadingKeys is the first HASH attribute's value + # (2026-10-20, gsi2) and clause order is free (2026-10-21, #429); the + # shapes themselves were run against ExtendDB only. Before + # authorization assigned the clauses the way the engine does, it found + # no partition key in each of them and the other principal read the + # rows. + ("gsi3 g2 g1 gs", "mk", _mk_query("gsi3", "g2 = :g AND g1 = :v AND gs = :x"), + "g1", OWN), + ("gsi3 gs g1 g2", "mk", _mk_query("gsi3", "gs = :x AND g1 = :v AND g2 = :g"), + "g1", OWN), + ("gsi3 g2 g1 gs range", "mk", _mk_query("gsi3", "g2 = :g AND g1 = :v AND gs > :lo"), + "g1", OWN), + ("gsi3 names g2 g1 gs", "mk", + _mk_query("gsi3", "#g2 = :g AND #g1 = :v AND #gs = :x"), "g1", OWN), + ("gsi3 names gs g1 g2", "mk", + _mk_query("gsi3", "#gs = :x AND #g1 = :v AND #g2 = :g"), "g1", OWN), + ("gsi3 names g2 g1 gs range", "mk", + _mk_query("gsi3", "#g2 = :g AND #g1 = :v AND #gs > :lo"), "g1", OWN), + ("gsi4 a h b", "mk", _mk_query("gsi4", "a = :a AND h = :v AND b = :c"), "h", OWN), + ("gsi4 b h a", "mk", _mk_query("gsi4", "b = :c AND h = :v AND a = :a"), "h", OWN), + ("gsi4 a h b range", "mk", _mk_query("gsi4", "a = :a AND h = :v AND b > :lo"), + "h", OWN), + ("gsi4 b range a h", "mk", _mk_query("gsi4", "b > :lo AND a = :a AND h = :v"), + "h", OWN), + ("gsi4 begins_with b a h", "mk", + _mk_query("gsi4", "begins_with(b, :p) AND a = :a AND h = :v"), "h", OWN), ] + # Accepted shapes with a condition on a RANGE attribute after the first. + # The MongoDB and Cassandra backends refuse those with ValidationException + # (docs/differences-from-dynamodb.md). Authorization runs first, so the + # other principal is denied on those backends as well. + QUERY_LATER_RANGE = {"gsi4 a h b", "gsi4 b h a", "gsi4 a h b range", + "gsi4 b range a h", "gsi4 begins_with b a h"} + FIRST_RANGE_ONLY_BACKENDS = ("mongodb", "cassandra") + QUERY_REJECTED = [ ("non-eq pk", "main", {"KeyConditionExpression": "pk > :v", "ExpressionAttributeValues": {":v": {"S": "alice"}}}), @@ -927,14 +1051,6 @@ def test_num_noncanonical_fence_matches_nothing(self, num_table): "legacy begins_with pk": "AccessDeniedException", } - # Rejected shapes where ExtendDB still answers a different code than the - # service to the principal fenced away from the value. The service returned - # ValidationException to all three principals for the GSI shape below, while - # ExtendDB binds g1's value and denies. Tracked separately from issue #448. - QUERY_REJECTED_CODE_DIVERGES = { - "multi hash gsi missing g2": "ValidationException", - } - def _fence_policy(self, values: list[str], tables: list[str]) -> dict: resources = [] for t in tables: @@ -951,8 +1067,9 @@ def _fence_policy(self, values: list[str], tables: list[str]) -> dict: } def test_query_shapes_authorize_the_partition_the_engine_reads( - self, table, num_table, mh_table): - tables = {"main": table, "num": num_table, "mh": mh_table} + self, table, num_table, mh_table, mk_table): + tables = {"main": table, "num": num_table, "mh": mh_table, "mk": mk_table} + first_range_only = configured_backend() in self.FIRST_RANGE_ONLY_BACKENDS owner = self._user_client([("full", ALLOW_ALL)]) fenced = self._user_client([("lk", self._fence_policy( [OWN, G_OWN, "10"], list(tables.values())))]) @@ -979,6 +1096,17 @@ def call(client, table_key, kwargs): failures = [] for label, table_key, kwargs, attr, value in self.QUERY_ACCEPTED: + if first_range_only and label in self.QUERY_LATER_RANGE: + for who, client in (("owner", owner), ("fenced", fenced)): + _, err = call(client, table_key, kwargs) + if err != "ValidationException": + failures.append(f"{label}: {who} got {err or 'rows'}, expected the " + "backend's ValidationException") + _, err_o = call(other, table_key, kwargs) + if err_o != "AccessDeniedException": + failures.append(f"{label}: other principal got {err_o or 'rows'}, " + "expected denial") + continue got, err = call(owner, table_key, kwargs) if err is not None: failures.append(f"{label}: owner got {err}") @@ -1001,10 +1129,7 @@ def call(client, table_key, kwargs): failures.append(f"{label}: {who} got {err or 'rows'}, expected ValidationException") _, err_o = call(other, table_key, kwargs) want = self.QUERY_REJECTED_OTHER_CODE.get(label, "ValidationException") - if label in self.QUERY_REJECTED_CODE_DIVERGES: - if err_o is None: - failures.append(f"{label}: other principal got rows") - elif err_o != want: + if err_o != want: failures.append(f"{label}: other principal got {err_o or 'rows'}, " f"expected {want}") diff --git a/tests/test_gsi_multi_range_query.py b/tests/test_gsi_multi_range_query.py new file mode 100644 index 000000000..d5c407a05 --- /dev/null +++ b/tests/test_gsi_multi_range_query.py @@ -0,0 +1,664 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Query on a GSI with one HASH attribute and several RANGE attributes. + +The index key schema is HASH ``h`` and RANGE ``a``, ``b``, ``c``, ``d``. Amazon +DynamoDB orders an index partition lexicographically over all four RANGE +attributes, in key-schema order, and serves a reverse Query as the exact +mirror. A key condition may name a leading run of the RANGE attributes, with +an equality on every one before the last and any sort-key operator on the +last; a gap, or an operator before a later condition, is refused. Clause +order in the expression is free. + +The item values are chosen so that ties on ``a`` are broken by ``b``, ties on +``b`` by ``c`` and ties on ``c`` by ``d``, and so that the base key order of +the items is the reverse of their index order. An item without ``c`` is not +in the index. +""" + +from __future__ import annotations + +import pytest +from botocore.exceptions import ClientError + +from conftest import configured_backend, _is_real_dynamodb, scoped_table, wait_for_gsi_items + +INDEX = "gsi" +TWO_HASH_INDEX = "two-hash" +RANGE_ATTRS = ("a", "b", "c", "d") + +# PK -> (a, b, c, d). p6 has no c, so the index does not hold it. +ITEMS = { + "p1": ("T", "live", "b", "r1"), + "p2": ("T", "live", "a", "r2"), + "p3": ("T", "live", "a", "r1"), + "p4": ("T", "draft", "c", "r1"), + "p5": ("F", "live", "a", "r0"), + "p6": ("T", "live", None, "r1"), +} +INDEXED = 5 + + +def _s(name: str) -> dict: + return {"AttributeName": name, "AttributeType": "S"} + + +@pytest.fixture(scope="module") +def multi_range_table(dynamodb_client): + with scoped_table( + dynamodb_client, + attribute_definitions=[ + _s("PK"), + _s("h"), + *(_s(n) for n in RANGE_ATTRS), + _s("h1"), + _s("h2"), + {"AttributeName": "r", "AttributeType": "N"}, + ], + key_schema=[{"AttributeName": "PK", "KeyType": "HASH"}], + GlobalSecondaryIndexes=[ + { + "IndexName": INDEX, + "KeySchema": [ + {"AttributeName": "h", "KeyType": "HASH"}, + *({"AttributeName": n, "KeyType": "RANGE"} for n in RANGE_ATTRS), + ], + "Projection": {"ProjectionType": "ALL"}, + }, + { + "IndexName": TWO_HASH_INDEX, + "KeySchema": [ + {"AttributeName": "h1", "KeyType": "HASH"}, + {"AttributeName": "h2", "KeyType": "HASH"}, + {"AttributeName": "r", "KeyType": "RANGE"}, + ], + "Projection": {"ProjectionType": "ALL"}, + }, + ], + ) as name: + for pk, values in ITEMS.items(): + item = {"PK": {"S": pk}, "h": {"S": "o1"}} + for attr, value in zip(RANGE_ATTRS, values): + if value is not None: + item[attr] = {"S": value} + dynamodb_client.put_item(TableName=name, Item=item) + indexed = wait_for_gsi_items( + lambda: _query_all(dynamodb_client, name, "h = :h", {":h": "o1"}), + INDEXED, + ) + assert len(indexed) == INDEXED, indexed + yield name + + +FIRST_RANGE_ONLY_MARK = "applies a key condition to the first RANGE attribute" +FIRST_RANGE_ONLY_BACKENDS = ("mongodb", "cassandra") + + +@pytest.fixture(scope="module") +def first_range_only(dynamodb_client, multi_range_table) -> bool: + """True when the backend under test refuses a key condition on a later + RANGE attribute. + + The MongoDB and Cassandra backends store and filter the first RANGE + attribute of a multi-attribute index only and refuse the rest; the + service, SQLite and PostgreSQL serve them. The backend comes from + `EXTENDDB_TEST_BACKEND` or the server's config file, as tests/test_ttl.py + reads it, and the server's reply must match it: a backend that serves + these conditions and starts refusing them fails here instead of skipping + the served-result tests. + """ + backend = None if _is_real_dynamodb() else configured_backend() + expected = backend in FIRST_RANGE_ONLY_BACKENDS + try: + dynamodb_client.query( + TableName=multi_range_table, + IndexName=INDEX, + KeyConditionExpression="h = :h AND a = :a AND b = :b", + ExpressionAttributeValues=_values({":h": "o1", ":a": "T", ":b": "live"}), + ) + refused = None + except ClientError as exc: + refused = exc.response["Error"] + if expected and (refused is None or FIRST_RANGE_ONLY_MARK not in refused["Message"]): + pytest.fail(f"backend {backend} should refuse a condition on b, got {refused or 'items'}") + if not expected and refused is not None: + pytest.fail(f"backend {backend or 'unknown'} should serve a condition on b, got {refused}") + return expected + + +@pytest.fixture +def needs_all_range_attributes(first_range_only): + if first_range_only: + pytest.skip("this backend applies key conditions to the first RANGE attribute only") + + +def _values(values: dict[str, str]) -> dict: + return {k: {"S": v} for k, v in values.items()} + + +def _query(client, table: str, kce: str, values: dict[str, str], **kwargs) -> dict: + return client.query( + TableName=table, + IndexName=INDEX, + KeyConditionExpression=kce, + ExpressionAttributeValues=_values(values), + **kwargs, + ) + + +def _query_all(client, table: str, kce: str, values: dict[str, str], **kwargs) -> list[str]: + """Every page of the query, as the PK of each item in result order.""" + pks: list[str] = [] + start = None + while True: + extra = dict(kwargs) + if start is not None: + extra["ExclusiveStartKey"] = start + resp = _query(client, table, kce, values, **extra) + pks.extend(item["PK"]["S"] for item in resp["Items"]) + start = resp.get("LastEvaluatedKey") + if start is None: + return pks + + +def _refused(client, table: str, kce: str, values: dict[str, str]) -> dict: + with pytest.raises(ClientError) as exc_info: + _query(client, table, kce, values) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + return err + + +@pytest.mark.usefixtures("needs_all_range_attributes") +class TestOrder: + """Results are ordered over every RANGE attribute in key-schema order.""" + + def test_hash_only_orders_over_all_range_attributes(self, dynamodb_client, multi_range_table): + pks = _query_all(dynamodb_client, multi_range_table, "h = :h", {":h": "o1"}) + assert pks == ["p5", "p4", "p3", "p2", "p1"] + + def test_hash_only_reverse_is_the_mirror(self, dynamodb_client, multi_range_table): + pks = _query_all( + dynamodb_client, + multi_range_table, + "h = :h", + {":h": "o1"}, + ScanIndexForward=False, + ) + assert pks == ["p1", "p2", "p3", "p4", "p5"] + + def test_first_range_equality_orders_by_the_rest(self, dynamodb_client, multi_range_table): + pks = _query_all( + dynamodb_client, + multi_range_table, + "h = :h AND a = :a", + {":h": "o1", ":a": "T"}, + ) + assert pks == ["p4", "p3", "p2", "p1"] + + def test_first_range_equality_reverse(self, dynamodb_client, multi_range_table): + pks = _query_all( + dynamodb_client, + multi_range_table, + "h = :h AND a = :a", + {":h": "o1", ":a": "T"}, + ScanIndexForward=False, + ) + assert pks == ["p1", "p2", "p3", "p4"] + + @pytest.mark.parametrize("forward", [True, False]) + def test_one_item_pages_resume_over_all_range_attributes( + self, dynamodb_client, multi_range_table, forward + ): + pks = _query_all( + dynamodb_client, + multi_range_table, + "h = :h", + {":h": "o1"}, + Limit=1, + ScanIndexForward=forward, + ) + want = ["p5", "p4", "p3", "p2", "p1"] + assert pks == (want if forward else want[::-1]) + + def test_limit_page_and_last_evaluated_key_shape(self, dynamodb_client, multi_range_table): + resp = _query( + dynamodb_client, + multi_range_table, + "h = :h AND a = :a", + {":h": "o1", ":a": "T"}, + Limit=2, + ) + assert [item["PK"]["S"] for item in resp["Items"]] == ["p4", "p3"] + lek = resp["LastEvaluatedKey"] + assert set(lek) == {"PK", "h", "a", "b", "c", "d"} + assert lek["PK"] == {"S": "p3"} + + +@pytest.mark.usefixtures("needs_all_range_attributes") +class TestAcceptedKeyConditions: + """A leading run of RANGE attributes, an operator only on the last one.""" + + @pytest.mark.parametrize( + ("kce", "values", "want"), + [ + ( + "h = :h AND a = :a AND b = :b", + {":h": "o1", ":a": "T", ":b": "live"}, + ["p3", "p2", "p1"], + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c", + {":h": "o1", ":a": "T", ":b": "live", ":c": "a"}, + ["p3", "p2"], + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d", + {":h": "o1", ":a": "T", ":b": "live", ":c": "a", ":d": "r1"}, + ["p3"], + ), + ( + "h = :h AND a = :a AND b = :b AND c > :c", + {":h": "o1", ":a": "T", ":b": "live", ":c": "a"}, + ["p1"], + ), + ( + "h = :h AND a = :a AND b < :b", + {":h": "o1", ":a": "T", ":b": "live"}, + ["p4"], + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND begins_with(d, :d)", + {":h": "o1", ":a": "T", ":b": "live", ":c": "a", ":d": "r"}, + ["p3", "p2"], + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d BETWEEN :x AND :y", + {":h": "o1", ":a": "T", ":b": "live", ":c": "a", ":x": "r1", ":y": "r1"}, + ["p3"], + ), + ], + ids=[ + "eq-a-b", + "eq-a-b-c", + "eq-all-four", + "gt-on-c", + "lt-on-b", + "begins-with-on-d", + "between-on-d", + ], + ) + def test_condition_is_served(self, dynamodb_client, multi_range_table, kce, values, want): + assert _query_all(dynamodb_client, multi_range_table, kce, values) == want + + @pytest.mark.parametrize( + "kce", + [ + "b = :b AND h = :h AND a = :a", + "a = :a AND b = :b AND h = :h", + ], + ids=["hash-in-the-middle", "hash-last"], + ) + def test_clause_order_is_free(self, dynamodb_client, multi_range_table, kce): + pks = _query_all( + dynamodb_client, + multi_range_table, + kce, + {":h": "o1", ":a": "T", ":b": "live"}, + ) + assert pks == ["p3", "p2", "p1"] + + +class TestRefusedKeyConditions: + """A gap in the RANGE attributes, or an operator before a later condition.""" + + @pytest.mark.parametrize( + ("kce", "values", "message"), + [ + ( + "h = :h AND b = :b", + {":h": "o1", ":b": "live"}, + "RANGE key attributes a must have equality conditions specified in the " + "query because a condition is present on key attribute b", + ), + ( + "h = :h AND a = :a AND c = :c", + {":h": "o1", ":a": "T", ":c": "a"}, + "RANGE key attributes b must have equality conditions specified in the " + "query because a condition is present on key attribute c", + ), + ( + "h = :h AND a = :a AND b > :b AND c = :c", + {":h": "o1", ":a": "T", ":b": "draft", ":c": "a"}, + "RANGE key attributes b must have equality conditions specified in the " + "query because a condition is present on key attribute c", + ), + ], + ids=["gap-at-a", "gap-at-b", "operator-before-c"], + ) + def test_refused_with_the_attribute_named( + self, dynamodb_client, multi_range_table, kce, values, message + ): + err = _refused(dynamodb_client, multi_range_table, kce, values) + assert err["Message"] == message + + def test_two_missing_range_attributes_are_refused(self, dynamodb_client, multi_range_table): + err = _refused( + dynamodb_client, + multi_range_table, + "h = :h AND c = :c", + {":h": "o1", ":c": "a"}, + ) + assert "must have equality conditions specified in the query" in err["Message"] + + @pytest.mark.parametrize( + "kce,values", + [ + ("a = :a AND b = :b AND c = :c", {":a": "T", ":b": "live", ":c": "a"}), + ("a = :a", {":a": "T"}), + ], + ids=["three-range-no-hash", "one-range-no-hash"], + ) + def test_missing_hash_is_refused(self, dynamodb_client, multi_range_table, kce, values): + err = _refused(dynamodb_client, multi_range_table, kce, values) + assert err["Message"] == ( + "HASH key attributes h must have equality conditions specified in the query" + ) + + @pytest.mark.parametrize( + "kce,values,missing", + [ + ("h1 = :x", {":x": "1"}, "h2"), + ("h2 = :x", {":x": "1"}, "h1"), + ("h1 = :x AND r = :y", {":x": "1", ":y": "2"}, "h2"), + ("r = :y", {":y": "2"}, "h1, h2"), + ("h1 = :x AND h2 > :y", {":x": "1", ":y": "2"}, "h2"), + ], + ids=["h1-only", "h2-only", "h1-and-range", "range-only", "non-equality-on-h2"], + ) + def test_two_hash_index_names_every_missing_hash_attribute( + self, dynamodb_client, multi_range_table, kce, values, missing + ): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.query( + TableName=multi_range_table, + IndexName=TWO_HASH_INDEX, + KeyConditionExpression=kce, + ExpressionAttributeValues=_values(values), + ) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + assert err["Message"] == ( + f"HASH key attributes {missing} must have equality conditions specified in the query" + ) + + @pytest.mark.parametrize( + "kce,values", + [ + ("h = :x AND h = :y", {":x": "o1", ":y": "o2"}), + ("h = :x AND h = :y AND a = :z", {":x": "o1", ":y": "o2", ":z": "T"}), + ("h = :x AND a = :y AND a = :z", {":x": "o1", ":y": "T", ":z": "F"}), + ("h = :h AND a = :a AND a > :x", {":h": "o1", ":a": "T", ":x": "A"}), + ("h = :h AND a > :x AND a = :a", {":h": "o1", ":a": "T", ":x": "A"}), + ("h = :h AND a = :a AND b = :b AND b > :lo", {":h": "o1", ":a": "T", ":b": "live", ":lo": "a"}), + ("h = :h AND h > :x", {":h": "o1", ":x": "a"}), + ], + ids=[ + "hash-twice", + "hash-twice-with-range", + "range-twice", + "range-eq-then-gt", + "range-gt-then-eq", + "second-range-eq-then-gt", + "hash-eq-then-gt", + ], + ) + def test_a_second_condition_on_one_key_attribute_is_refused( + self, dynamodb_client, multi_range_table, kce, values + ): + err = _refused(dynamodb_client, multi_range_table, kce, values) + assert err["Message"] == ( + "Invalid KeyConditionExpression: KeyConditionExpressions must only contain " + "one condition per key" + ) + + @pytest.mark.parametrize( + "kce,values,missing", + [ + ("h = :h AND z = :z", {":h": "o1", ":z": "zz"}, "a"), + ("h = :h AND z = :z AND y = :y", {":h": "o1", ":z": "zz", ":y": "yy"}, "a"), + ("h = :h AND z > :z", {":h": "o1", ":z": "zz"}, "a"), + ("h = :h AND a = :a AND z = :z", {":h": "o1", ":a": "T", ":z": "zz"}, "b"), + ("h = :h AND a = :a AND z > :z", {":h": "o1", ":a": "T", ":z": "zz"}, "b"), + ("h = :h AND a > :a AND z = :z", {":h": "o1", ":a": "A", ":z": "zz"}, "b"), + ("h = :h AND a = :a AND b = :b AND z > :z", {":h": "o1", ":a": "T", ":b": "live", ":z": "zz"}, "c"), + ("h = :h AND PK = :p", {":h": "o1", ":p": "p1"}, "a"), + ], + ids=[ + "nonkey-eq", + "two-nonkey-eq", + "nonkey-gt", + "nonkey-eq-after-a", + "nonkey-gt-after-a", + "nonkey-eq-after-a-gt", + "nonkey-gt-after-a-b", + "base-table-key", + ], + ) + def test_a_condition_on_a_non_key_attribute_names_the_first_uncovered_range_attribute( + self, dynamodb_client, multi_range_table, kce, values, missing + ): + err = _refused(dynamodb_client, multi_range_table, kce, values) + assert err["Message"] == f"Query condition missed key schema element: {missing}" + + def test_a_non_key_condition_on_a_two_hash_index_names_the_range_attribute( + self, dynamodb_client, multi_range_table + ): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.query( + TableName=multi_range_table, + IndexName=TWO_HASH_INDEX, + KeyConditionExpression="h1 = :x AND h2 = :y AND z = :z", + ExpressionAttributeValues=_values({":x": "1", ":y": "2", ":z": "3"}), + ) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + assert err["Message"] == "Query condition missed key schema element: r" + + @pytest.mark.parametrize( + "kce,values,count", + [ + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND z = :z", + {":h": "o1", ":a": "T", ":b": "live", ":c": "a", ":d": "r1", ":z": "zz"}, + 6, + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND z > :z", + {":h": "o1", ":a": "T", ":b": "live", ":c": "a", ":d": "r1", ":z": "zz"}, + 6, + ), + ], + ids=["six-with-nonkey-eq", "six-with-nonkey-gt"], + ) + def test_more_conditions_than_key_attributes_are_refused_by_count( + self, dynamodb_client, multi_range_table, kce, values, count + ): + err = _refused(dynamodb_client, multi_range_table, kce, values) + assert err["Message"] == ( + f"The number of query conditions ({count}) exceeds the number of key attributes " + "defined in the schema (5)" + ) + + @pytest.mark.parametrize( + "kce,values", + [ + ("h = :h", {":h": {"N": "1"}}), + ("h = :h AND a = :a", {":h": {"S": "o1"}, ":a": {"N": "1"}}), + ("h = :h AND a = :a AND b = :b", {":h": {"S": "o1"}, ":a": {"S": "T"}, ":b": {"N": "1"}}), + ("h = :h AND a = :a AND b > :b", {":h": {"S": "o1"}, ":a": {"S": "T"}, ":b": {"N": "1"}}), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND begins_with(d, :d)", + {":h": {"S": "o1"}, ":a": {"S": "T"}, ":b": {"S": "live"}, ":c": {"S": "a"}, ":d": {"B": b"r"}}, + ), + ("h = :h AND z = :z", {":h": {"N": "1"}, ":z": {"S": "zz"}}), + ], + ids=["hash-n", "a-n", "b-n", "b-gt-n", "d-begins-with-b", "hash-n-with-nonkey"], + ) + def test_a_value_of_the_wrong_type_is_refused(self, dynamodb_client, multi_range_table, kce, values): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.query( + TableName=multi_range_table, + IndexName=INDEX, + KeyConditionExpression=kce, + ExpressionAttributeValues=values, + ) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + assert err["Message"] == ( + "One or more parameter values were invalid: Condition parameter type does not match " + "schema type" + ) + + @pytest.mark.parametrize( + "index,kce,values,missing", + [ + (INDEX, "h = :h AND PK = :p", {":h": {"S": "o1"}, ":p": {"N": "1"}}, "a"), + (INDEX, "h = :h AND r = :r", {":h": {"S": "o1"}, ":r": {"S": "1"}}, "a"), + ( + TWO_HASH_INDEX, + "h1 = :x AND h2 = :y AND a = :a", + {":x": {"S": "1"}, ":y": {"S": "2"}, ":a": {"N": "1"}}, + "r", + ), + ], + ids=["base-table-key-n", "other-index-range-s", "two-hash-other-index-range-n"], + ) + def test_a_value_on_an_attribute_outside_the_index_key_schema_is_not_type_checked( + self, dynamodb_client, multi_range_table, index, kce, values, missing + ): + # The table defines the attribute (the base table key, or the other + # index's key) with a different type, but the queried index's key schema + # does not hold it, so the non-key rule answers. Measured 2026-10-30 on a + # GSI with HASH GSI2PK and RANGE GSI2SK (one RANGE attribute): + # `GSI2PK = :h AND PK = :k` answers `Query condition missed key schema + # element: GSI2SK` with `:k` an S, PK's defined type, and with `:k` an N. + with pytest.raises(ClientError) as exc_info: + dynamodb_client.query( + TableName=multi_range_table, + IndexName=index, + KeyConditionExpression=kce, + ExpressionAttributeValues=values, + ) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + assert err["Message"] == f"Query condition missed key schema element: {missing}" + + + def test_begins_with_on_a_number_attribute_is_refused(self, dynamodb_client, multi_range_table): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.query( + TableName=multi_range_table, + IndexName=TWO_HASH_INDEX, + KeyConditionExpression="h1 = :x AND h2 = :y AND begins_with(r, :r)", + ExpressionAttributeValues={":x": {"S": "1"}, ":y": {"S": "2"}, ":r": {"N": "1"}}, + ) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + assert err["Message"] == ( + "Invalid KeyConditionExpression: Incorrect operand type for operator or function; " + "operator or function: begins_with, operand type: N" + ) + + @pytest.mark.parametrize( + "kce,values", + [("h = :h AND #q = :a", {":h": "o1", ":a": "T"}), ("h = :h AND a = :a AND #q > :x", {":h": "o1", ":a": "T", ":x": "A"})], + ids=["undefined-name-eq", "undefined-name-gt"], + ) + def test_an_undefined_name_reference_is_refused(self, dynamodb_client, multi_range_table, kce, values): + err = _refused(dynamodb_client, multi_range_table, kce, values) + assert err["Message"] == ( + "Invalid KeyConditionExpression: An expression attribute name used in the document " + "path is not defined; attribute name: #q" + ) + + +@pytest.mark.usefixtures("needs_all_range_attributes") +class TestLegacyKeyConditions: + """`KeyConditions` on a multi-attribute index follow the same rules.""" + + @staticmethod + def _kc(**conds): + return { + attr: {"ComparisonOperator": op, "AttributeValueList": [{"S": v} for v in vals]} + for attr, (op, *vals) in conds.items() + } + + def test_all_equalities_are_served(self, dynamodb_client, multi_range_table): + resp = dynamodb_client.query( + TableName=multi_range_table, + IndexName=INDEX, + KeyConditions=self._kc(h=("EQ", "o1"), a=("EQ", "T"), b=("EQ", "live")), + ) + assert [i["PK"]["S"] for i in resp["Items"]] == ["p3", "p2", "p1"] + + def test_trailing_operator_is_served(self, dynamodb_client, multi_range_table): + resp = dynamodb_client.query( + TableName=multi_range_table, + IndexName=INDEX, + KeyConditions=self._kc(h=("EQ", "o1"), a=("EQ", "T"), b=("GT", "draft")), + ) + assert [i["PK"]["S"] for i in resp["Items"]] == ["p3", "p2", "p1"] + + @pytest.mark.parametrize( + "conds,message", + [ + ( + {"h": ("EQ", "o1"), "b": ("EQ", "live")}, + "RANGE key attributes a must have equality conditions specified in the query " + "because a condition is present on key attribute b", + ), + ( + {"h": ("EQ", "o1"), "a": ("GT", "A"), "b": ("GT", "a")}, + "RANGE key attributes a must have equality conditions specified in the query " + "because a condition is present on key attribute b", + ), + ({"h": ("EQ", "o1"), "z": ("EQ", "zz")}, "Query condition missed key schema element: a"), + ( + {"a": ("EQ", "T")}, + "HASH key attributes h must have equality conditions specified in the query", + ), + ], + ids=["gap-at-a", "two-operators", "nonkey", "no-hash"], + ) + def test_refusals_match_the_expression_form( + self, dynamodb_client, multi_range_table, conds, message + ): + with pytest.raises(ClientError) as exc_info: + dynamodb_client.query( + TableName=multi_range_table, IndexName=INDEX, KeyConditions=self._kc(**conds) + ) + err = exc_info.value.response["Error"] + assert err["Code"] == "ValidationException", err + assert err["Message"] == message + + +class TestFirstRangeOnlyBackends: + """Backends that filter the first RANGE attribute only refuse the rest.""" + + def test_a_condition_beyond_the_first_range_attribute_is_refused( + self, dynamodb_client, multi_range_table, first_range_only + ): + if not first_range_only: + pytest.skip("this target serves conditions on every RANGE attribute") + for kce, values in [ + ("h = :h AND a = :a AND b = :b", {":h": "o1", ":a": "T", ":b": "live"}), + ("h = :h AND a = :a AND b > :b", {":h": "o1", ":a": "T", ":b": "a"}), + ]: + err = _refused(dynamodb_client, multi_range_table, kce, values) + assert err["Message"].startswith("Key condition on b: ") + assert FIRST_RANGE_ONLY_MARK in err["Message"] + # The first RANGE attribute alone is served. + resp = _query(dynamodb_client, multi_range_table, "h = :h AND a = :a", {":h": "o1", ":a": "T"}) + assert resp["Count"] == 4 diff --git a/tests/test_ttl.py b/tests/test_ttl.py index b73e5ef94..16d93e65e 100755 --- a/tests/test_ttl.py +++ b/tests/test_ttl.py @@ -18,30 +18,7 @@ import pytest from botocore.exceptions import ClientError -from conftest import _is_real_dynamodb, wait_for_active, wait_for_deleted - - -def _configured_backend(): - """The backend under test: `EXTENDDB_TEST_BACKEND` (exported by - devtools/run-tests), else the `[storage] backend` of the server's config - file, else None (real DynamoDB, or a server started elsewhere).""" - explicit = os.environ.get("EXTENDDB_TEST_BACKEND", "").strip() - if explicit: - return explicit - path = os.environ.get("EXTENDDB_CONFIG", "extenddb.toml") - try: - with open(path) as f: - text = f.read() - except OSError: - return None - section = None - for line in text.splitlines(): - line = line.split("#", 1)[0].strip() - if line.startswith("["): - section = line.strip("[]").strip() - elif section == "storage" and line.split("=", 1)[0].strip() == "backend": - return line.split("=", 1)[1].strip().strip('"').strip("'") - return None +from conftest import configured_backend, _is_real_dynamodb, wait_for_active, wait_for_deleted @pytest.fixture() @@ -136,7 +113,7 @@ def test_enable_ttl_with_a_gsi_is_applied_or_refused_as_a_client_error( The Cassandra refusal itself is pinned by the storage-level test test_ttl_on_a_table_with_an_async_gsi_is_refused_as_a_validation_error. """ - backend = None if _is_real_dynamodb() else _configured_backend() + backend = None if _is_real_dynamodb() else configured_backend() may_refuse = backend == "cassandra" or (backend is None and not _is_real_dynamodb()) print(f"backend under test: {backend or 'unknown'}; refusal allowed: {may_refuse}") dynamodb_client.create_table(