From ac10fb79c9608d2217e3f800bcd7f90539c04ec6 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:19:48 +0000 Subject: [PATCH 1/5] fix(query): classify and order key conditions on a multi-attribute GSI the way the service does On a global secondary index with one HASH attribute and several RANGE attributes, Query ordered results by the first RANGE attribute and then the table key, refused every key condition with three or more clauses as "Query condition missed key schema element: h", and accepted a condition that skipped the first RANGE attribute while returning no items. KeyCondition::resolve_multipart returned early when the schema had one HASH attribute, so the parser's assignment stood: the first equality became the partition key and the rest sat where the engine's HASH-count check refused them. Measured against the service on 2026-10-21, with the issue's schema (HASH h, RANGE a, b, c, d) and with two HASH attributes: - Results are ordered over every RANGE attribute in key-schema order, then the base table key; ScanIndexForward = false is the exact mirror. LastEvaluatedKey carries every key attribute. - A key condition is served when the HASH attributes all have an equality and the RANGE conditions cover a leading run of the RANGE attributes with an equality on every one before the last; the last may use <, <=, >, >=, BETWEEN or begins_with. Clause order in the expression is free. - Refusals, in the order the service applies them: a second condition on one key attribute ("KeyConditionExpressions must only contain one condition per key"); more conditions than key attributes ("The number of query conditions (6) exceeds the number of key attributes defined in the schema (5)"); a HASH attribute without an equality ("HASH key attributes h1, h2 must have equality conditions specified in the query", every missing one named); a RANGE gap ("RANGE key attributes a must have equality conditions specified in the query because a condition is present on key attribute b"); a value whose type differs from the attribute definition ("One or more parameter values were invalid: Condition parameter type does not match schema type"); a condition on an attribute outside the key schema, named by the first RANGE attribute without a condition ("Query condition missed key schema element: a"). begins_with on a number attribute is "Incorrect operand type for operator or function; operator or function: begins_with, operand type: N". The single-attribute wording "Query condition missed key schema element" stays on single-attribute schemas. resolve_multipart now classifies every clause by name against the HASH and RANGE attribute lists and applies those rules in that order; the value type check runs inside it through resolve_multipart_with, with the engine supplying the attribute definitions. The engine's single-attribute path gets the same type check ahead of its missing-key check, which turns a 500 on a wrong-typed sort key value into the service's refusal. Legacy KeyConditions on a multi-attribute schema go through the same classifier. The SQLite and PostgreSQL backends apply every RANGE condition to its column and order and page over all RANGE columns; the cursor keeps its shape. The MongoDB and Cassandra backends filter the first RANGE attribute only and now refuse a condition on a later one instead of applying it to the wrong column. On Cassandra every Query on an index with several RANGE attributes answered 500 before this change: the index table clusters on every RANGE column and CQL rejects an ORDER BY that skips one, so the ORDER BY now names every index RANGE column in declared order, and a Query with an ExclusiveStartKey on such an index is refused instead of answering 500, because the cursor compares the first RANGE attribute only. docs/differences-from-dynamodb.md records both backends. tests/test_gsi_multi_range_query.py pins 60 cases, all measured on the service: order, pages and LastEvaluatedKey, every accepted shape, every refusal and its text, legacy KeyConditions, and the MongoDB and Cassandra refusal. 60 pass on the service and on SQLite and PostgreSQL; on MongoDB 39 pass and the 22 served-result cases skip; on Cassandra the same 22 skip. On main 30 of the 30 then-existing cases failed. Unit tests in core cover the classifier rules; the storage plan and cursor tests cover the ORDER BY and cursor columns. Closes #381 --- crates/core/src/expression/key_condition.rs | 665 ++++++++++++++++-- crates/engine/src/legacy_filter.rs | 163 +++++ crates/engine/src/query.rs | 207 ++++-- crates/storage-cassandra/src/data/query.rs | 38 +- crates/storage-mongodb/src/data_engine.rs | 7 + crates/storage-postgres/src/data/query.rs | 42 +- .../storage-postgres/src/data/query_scan.rs | 208 +++--- crates/storage-sqlite/src/data/query_scan.rs | 153 ++-- crates/storage/src/util/key.rs | 61 ++ crates/storage/src/util/mod.rs | 4 +- docs/differences-from-dynamodb.md | 1 + tests/test_gsi_multi_range_query.py | 619 ++++++++++++++++ 12 files changed, 1927 insertions(+), 241 deletions(-) create mode 100644 tests/test_gsi_multi_range_query.py diff --git a/crates/core/src/expression/key_condition.rs b/crates/core/src/expression/key_condition.rs index 4e306512d..916cb4734 100755 --- a/crates/core/src/expression/key_condition.rs +++ b/crates/core/src/expression/key_condition.rs @@ -80,8 +80,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,88 +115,258 @@ 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(), + )); + } - // 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() { + // 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})" + ))); + } + + // 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(()) } @@ -212,8 +382,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 +418,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 +1081,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 +1096,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 +1114,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 +1133,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 +1319,383 @@ 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 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"]); + } } 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..7e43fb7f2 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -270,50 +270,70 @@ 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 + // A key schema with several HASH or several RANGE attributes: assign + // every clause by name, require an equality on each HASH attribute, + // and require the RANGE conditions to cover a leading run of the + // RANGE attributes with equalities before the last one. + let hash_attrs: Vec<&str> = + extenddb_core::types::hash_key_elements(&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]); + .map(|ks| ks.attribute_name.as_str()) + .collect(); + let range_attrs: Vec<&str> = + extenddb_core::types::range_key_elements(&query_key_info.key_schema) + .iter() + .map(|ks| ks.attribute_name.as_str()) + .collect(); + // A wrong-typed value is reported after the structural rules and + // before the non-key rule (measured 2026-10-21), so the type check + // runs inside the classifier. + key_condition.resolve_multipart_with( + &hash_attrs, + &range_attrs, + &effective_maps.names, + |path, value| { + check_key_value_type( + path, + value, + &query_key_info.attribute_definitions, + effective_maps, + ) + }, + )?; + refuse_begins_with_on_number( + &key_condition, + &query_key_info.attribute_definitions, + effective_maps, + )?; + } else { + // On a single-attribute key schema the service reports a wrong-typed + // value ahead of the missing-key check (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). + for_each_key_clause(&key_condition, |path, value| { + check_key_value_type( + path, + value, + &query_key_info.attribute_definitions, + effective_maps, + ) + })?; + refuse_begins_with_on_number( + &key_condition, + &query_key_info.attribute_definitions, + effective_maps, + )?; + // Correct PK/SK assignment when both clauses are equality comparisons. + // The parser can't distinguish PK from SK without the key schema. + 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: {missing}" + "Query condition missed key schema element: {pk_attr}" ))); } } @@ -574,6 +594,113 @@ fn resolve_path_attr_name( } } +/// Call `f` with every clause of a key condition: the partition key clause, +/// the extra HASH and RANGE equalities, and each operand of the sort key +/// condition. +fn for_each_key_clause( + key_condition: &extenddb_core::expression::KeyCondition, + mut f: F, +) -> Result<(), DynamoDbError> +where + F: FnMut(&[PathElement], &extenddb_core::expression::Expr) -> Result<(), DynamoDbError>, +{ + use extenddb_core::expression::SortKeyCondition; + + f(&key_condition.pk_path, &key_condition.pk_value)?; + for (path, value) in &key_condition.extra_pk_conditions { + f(path, value)?; + } + for (path, value) in &key_condition.extra_sk_conditions { + f(path, value)?; + } + match &key_condition.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(()), + } +} + +/// 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 is not +/// checked here; the classifier reports it. +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/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_gsi_multi_range_query.py b/tests/test_gsi_multi_range_query.py new file mode 100644 index 000000000..321a65acf --- /dev/null +++ b/tests/test_gsi_multi_range_query.py @@ -0,0 +1,619 @@ +# 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 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" + + +@pytest.fixture(scope="module") +def first_range_only(dynamodb_client, multi_range_table) -> bool: + """True when the server 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. + """ + 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"}), + ) + except ClientError as exc: + if FIRST_RANGE_ONLY_MARK in exc.response["Error"]["Message"]: + return True + raise + return False + + +@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" + ) + + + 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 From 5f7452a70e215f056c666160856e45b5372ed0cb Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 09:46:06 +0000 Subject: [PATCH 2/5] fix(auth): bind LeadingKeys through the engine's key condition classification On a key schema with several HASH or several RANGE attributes the engine assigns every key condition clause to its key attribute by name, so it serves clause orders such as `g2 = :g AND g1 = :v AND gs = :x` and `b > :lo AND a = :a AND h = :v`. Authorization still read the partition equality with `resolve_pk_sk`, found no partition key in those orders and left dynamodb:LeadingKeys absent, so a ForAllValues:StringEquals fence let the query read any partition of the index. `KeyCondition::resolve_key_schema_with` holds the choice between the multi-attribute classifier and `resolve_pk_sk`. The engine calls it, and so does `equality_pk_value`, which `query_leading_keys` calls for a condition with an equality on the partition attribute. When a clause compares the first HASH attribute by equality, authorization binds that attribute's value for every clause order the engine serves, and a condition the classifier refuses leaves the key absent. For `g1 = :a` on HASH `g1`, `g2` that gives the answer the service gives every principal, ValidationException (measured 2026-10-21). A single clause on the partition attribute under an operator the engine rejects still binds that clause's operand, as it does on main since #511. tests/test_fgac_leading_keys.py adds the three-clause orders from the review on a GSI with HASH `g1`, `g2` and RANGE `gs` and a GSI with HASH `h` and RANGE `a`, `b`, and expects ValidationException for the missing-g2 shape from the principal fenced away from the value. That shape was the only entry in the list of codes that still differed from the service, so the list is removed. It and tests/test_ttl.py read the backend through `configured_backend()` in tests/conftest.py, so this file can expect the MongoDB and Cassandra refusal of a condition on `b`. --- crates/core/src/expression/key_condition.rs | 164 ++++++++++++++++++++ crates/engine/src/query.rs | 109 ++++--------- crates/server/src/leading_keys.rs | 67 +++++--- crates/server/src/leading_keys/tests.rs | 110 +++++++++++++ tests/test_fgac_leading_keys.py | 161 ++++++++++++++++--- tests/test_ttl.py | 27 +--- 6 files changed, 488 insertions(+), 150 deletions(-) diff --git a/crates/core/src/expression/key_condition.rs b/crates/core/src/expression/key_condition.rs index 916cb4734..ca5a53c3e 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. /// @@ -370,6 +373,93 @@ impl KeyCondition { 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. @@ -1698,4 +1788,78 @@ mod tests { 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/query.rs b/crates/engine/src/query.rs index 7e43fb7f2..514279c35 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -271,65 +271,38 @@ pub async fn handle_query( }; let pk_attr = &query_key_info.key_schema[0].attribute_name; - if extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema) { - // A key schema with several HASH or several RANGE attributes: assign - // every clause by name, require an equality on each HASH attribute, - // and require the RANGE conditions to cover a leading run of the - // RANGE attributes with equalities before the last one. - let hash_attrs: Vec<&str> = - extenddb_core::types::hash_key_elements(&query_key_info.key_schema) - .iter() - .map(|ks| ks.attribute_name.as_str()) - .collect(); - let range_attrs: Vec<&str> = - extenddb_core::types::range_key_elements(&query_key_info.key_schema) - .iter() - .map(|ks| ks.attribute_name.as_str()) - .collect(); - // A wrong-typed value is reported after the structural rules and - // before the non-key rule (measured 2026-10-21), so the type check - // runs inside the classifier. - key_condition.resolve_multipart_with( - &hash_attrs, - &range_attrs, - &effective_maps.names, - |path, value| { - check_key_value_type( - path, - value, - &query_key_info.attribute_definitions, - effective_maps, - ) - }, - )?; - refuse_begins_with_on_number( - &key_condition, - &query_key_info.attribute_definitions, - effective_maps, - )?; - } else { - // On a single-attribute key schema the service reports a wrong-typed - // value ahead of the missing-key check (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). - for_each_key_clause(&key_condition, |path, value| { + // 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). + key_condition.resolve_key_schema_with( + &query_key_info.key_schema, + &effective_maps.names, + |path, value| { check_key_value_type( path, value, &query_key_info.attribute_definitions, effective_maps, ) - })?; - refuse_begins_with_on_number( - &key_condition, - &query_key_info.attribute_definitions, - effective_maps, - )?; - // Correct PK/SK assignment when both clauses are equality comparisons. - // The parser can't distinguish PK from SK without the key schema. - key_condition.resolve_pk_sk(pk_attr, &effective_maps.names)?; - - // Validate that the partition key is actually referenced in the condition. + }, + )?; + 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 !extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema) { 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!( @@ -594,36 +567,6 @@ fn resolve_path_attr_name( } } -/// Call `f` with every clause of a key condition: the partition key clause, -/// the extra HASH and RANGE equalities, and each operand of the sort key -/// condition. -fn for_each_key_clause( - key_condition: &extenddb_core::expression::KeyCondition, - mut f: F, -) -> Result<(), DynamoDbError> -where - F: FnMut(&[PathElement], &extenddb_core::expression::Expr) -> Result<(), DynamoDbError>, -{ - use extenddb_core::expression::SortKeyCondition; - - f(&key_condition.pk_path, &key_condition.pk_value)?; - for (path, value) in &key_condition.extra_pk_conditions { - f(path, value)?; - } - for (path, value) in &key_condition.extra_sk_conditions { - f(path, value)?; - } - match &key_condition.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(()), - } -} - /// Refuse a key condition value whose type differs from the attribute /// definition of the attribute it names. /// 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/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_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( From 853d20b17ce53947b2d59e57f3c85d6960d6ed7f Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 09:48:32 +0000 Subject: [PATCH 3/5] test(core): pin the condition count refusal ahead of the HASH and RANGE checks The service reports more conditions than key attributes ahead of which key attribute is missing (measured 2026-10-21), and the classifier checks the count before the missing-HASH and RANGE-gap rules. Every count case in the tests had every key attribute, so moving the count check after both rules still passed them. The new cases have six conditions with `h` missing and six with a RANGE gap on HASH `h` and RANGE `a`, `b`, `c`, `d`, and five conditions on HASH `h1`, `h2` and RANGE `r`. The same five clauses on the five key attributes of `h` and `a` to `d` are not more than the key attribute count, so the missing HASH attribute answers them. --- crates/core/src/expression/key_condition.rs | 40 +++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/crates/core/src/expression/key_condition.rs b/crates/core/src/expression/key_condition.rs index ca5a53c3e..edaa1f8e5 100755 --- a/crates/core/src/expression/key_condition.rs +++ b/crates/core/src/expression/key_condition.rs @@ -1730,6 +1730,46 @@ mod tests { 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")] From 2269f00482974a30de40bb558d87ca23d1e11fa4 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 09:50:51 +0000 Subject: [PATCH 4/5] test(query): take the first-RANGE-only branch from the backend under test `first_range_only` decided from the server's reply whether to skip the served-result tests, so a SQLite or PostgreSQL server that answered a condition on `b` with the MongoDB and Cassandra refusal would have reported 39 passed and 22 skipped and stayed green. The fixture now reads the backend from `EXTENDDB_TEST_BACKEND` or the server's config file, as tests/test_ttl.py does, expects the refusal on MongoDB and Cassandra only, and fails when the reply does not match the backend. --- tests/test_gsi_multi_range_query.py | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/tests/test_gsi_multi_range_query.py b/tests/test_gsi_multi_range_query.py index 321a65acf..8a4c9acab 100644 --- a/tests/test_gsi_multi_range_query.py +++ b/tests/test_gsi_multi_range_query.py @@ -22,7 +22,7 @@ import pytest from botocore.exceptions import ClientError -from conftest import scoped_table, wait_for_gsi_items +from conftest import configured_backend, _is_real_dynamodb, scoped_table, wait_for_gsi_items INDEX = "gsi" TWO_HASH_INDEX = "two-hash" @@ -92,16 +92,24 @@ def multi_range_table(dynamodb_client): 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 server refuses a key condition on a later RANGE attribute. + """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. + 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, @@ -109,11 +117,14 @@ def first_range_only(dynamodb_client, multi_range_table) -> bool: KeyConditionExpression="h = :h AND a = :a AND b = :b", ExpressionAttributeValues=_values({":h": "o1", ":a": "T", ":b": "live"}), ) + refused = None except ClientError as exc: - if FIRST_RANGE_ONLY_MARK in exc.response["Error"]["Message"]: - return True - raise - return False + 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 From a7e14d8e5ff9cc541e9f7e00346556210f81ca59 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 30 Oct 2026 09:54:38 +0000 Subject: [PATCH 5/5] fix(query): type-check only the queried key schema on a multi-attribute index For an index query the attribute definitions are the table's whole list, which also holds the base table key and the other indexes' key attributes. A clause on one of those attributes with a value of another type got `Condition parameter type does not match schema type`, where the classifier's non-key rule gives `Query condition missed key schema element: `, the first RANGE attribute without a condition. On a multi-attribute key schema the type check now reads the definitions of the queried key schema only, as its doc comment says. Measured on the service on 2026-10-30, on a GSI with HASH `GSI2PK` and RANGE `GSI2SK` of a table keyed on `PK` (S): `GSI2PK = :h AND PK = :k` answers `Query condition missed key schema element: GSI2SK` with `:k` an S and with `:k` an N, two samples each. The service's type check does not see an attribute outside the queried key schema. A single-attribute key schema keeps the table's definitions. Its path has no non-key rule: a clause on another attribute reaches storage, which applies it to the first RANGE attribute, and the type check is what refuses a value of the wrong type there. Reading only the key schema's definitions on that path would turn the N case above into a 500, and the S case would still be applied to the first RANGE attribute. That path's divergence from the measurement is older than this change and is tracked separately. --- crates/engine/src/query.rs | 48 ++++++++++++++++++++++------- tests/test_gsi_multi_range_query.py | 34 ++++++++++++++++++++ 2 files changed, 71 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index 514279c35..848f5fdbb 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -282,17 +282,41 @@ pub async fn handle_query( // 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() + .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, - &query_key_info.attribute_definitions, - effective_maps, - ) - }, + |path, value| check_key_value_type(path, value, type_definitions, effective_maps), )?; refuse_begins_with_on_number( &key_condition, @@ -302,7 +326,7 @@ pub async fn handle_query( // 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 !extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema) { + 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!( @@ -575,8 +599,10 @@ fn resolve_path_attr_name( /// 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 is not -/// checked here; the classifier reports it. +/// 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, diff --git a/tests/test_gsi_multi_range_query.py b/tests/test_gsi_multi_range_query.py index 8a4c9acab..d5c407a05 100644 --- a/tests/test_gsi_multi_range_query.py +++ b/tests/test_gsi_multi_range_query.py @@ -521,6 +521,40 @@ def test_a_value_of_the_wrong_type_is_refused(self, dynamodb_client, multi_range "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: