From 4a2d3828a1906345bf530231f1d65babba9c6040 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Sat, 31 Oct 2026 19:05:54 +0000 Subject: [PATCH] fix(query): reject a KeyConditionExpression with two conditions on one key A key condition that named the same key attribute twice was served. For `pk = :a AND pk = :b` the parser took the first equality as the partition key condition and the second as the sort key condition, so on a table with pk HASH and sk RANGE the request returned the items in partition `:a` whose sort key equalled `:b`. The same hole accepted two conditions on the sort key, including BETWEEN or begins_with paired with another sort key condition. DynamoDB rejects all of these with "Invalid KeyConditionExpression: KeyConditionExpressions must only contain one condition per key" (measured 2026-10-24, us-east-1). It resolves every `#name` and `:value` reference before it looks for the duplicate, so an undefined name or value is reported first, and it reports the duplicate ahead of the key value type check and the unused-value check (measured 2026-10-31). parse_key_condition_with_maps takes the request's expression attribute names and values. After the parser's own checks, and before any clause is classified, it refuses two conditions on one key attribute, written with plain names or through `#name` references. When there is a duplicate it reports an undefined `#name` and then an undefined `:value` first. A key condition without a duplicate takes the same path as before. The engine's Query handler parses through it, and the server's LeadingKeys extraction runs the same check through key_condition_clause_operands, so a duplicate leaves dynamodb:LeadingKeys absent whichever way it is written. Fixes #430 --- crates/core/src/expression/key_condition.rs | 442 +++++++++++++++++- crates/core/src/expression/mod.rs | 2 +- crates/engine/src/expression_helpers.rs | 121 ++++- crates/engine/src/query.rs | 7 +- crates/server/src/leading_keys.rs | 29 +- crates/server/src/leading_keys/tests.rs | 52 ++- tests/test_fgac_leading_keys.py | 52 ++- tests/test_query_key_condition_one_per_key.py | 318 +++++++++++++ 8 files changed, 962 insertions(+), 61 deletions(-) create mode 100644 tests/test_query_key_condition_one_per_key.py diff --git a/crates/core/src/expression/key_condition.rs b/crates/core/src/expression/key_condition.rs index edaa1f8e5..211aa3729 100755 --- a/crates/core/src/expression/key_condition.rs +++ b/crates/core/src/expression/key_condition.rs @@ -389,7 +389,9 @@ impl KeyCondition { /// 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. + /// HASH attribute. Both parse with [`parse_key_condition_with_maps`], + /// which refuses a second condition on one key attribute before this runs, + /// so a duplicate is reported ahead of `value_check`. /// /// # Errors /// @@ -462,6 +464,75 @@ impl KeyCondition { } } +/// The service's message when a `KeyConditionExpression` carries more than one +/// condition on the same key attribute. Measured 2026-10-24 (us-east-1). +const ONE_CONDITION_PER_KEY_ERR: &str = "Invalid KeyConditionExpression: KeyConditionExpressions must only contain one condition per key"; + +/// Refuse a key condition with more than one condition on one key attribute. +/// +/// Two clauses name the same attribute when their paths resolve to the same +/// name through `names`, whether each is written as a plain name or as a +/// `#name` reference. Two clauses on one `#name` reference name the same +/// attribute whether or not `names` defines it. DynamoDB allows one condition +/// per key attribute, on the partition key and on the sort key alike, whatever +/// the operators are (measured 2026-10-24). +/// +/// The service resolves every `#name` and `:value` reference before it looks +/// for a duplicate (measured 2026-10-31), so when a duplicate is found an +/// undefined `#name` in any clause is reported first, then an undefined +/// `:value`, each with the service's text. A key condition without a duplicate +/// passes, and the checks that follow report its undefined references. +fn reject_duplicate_key_attrs( + clauses: &[RawClause], + names: &std::collections::HashMap, + values: &std::collections::HashMap, +) -> Result<(), DynamoDbError> { + let mut seen: Vec<&str> = Vec::with_capacity(clauses.len()); + let mut duplicated = false; + for clause in clauses { + let Some(PathElement::Attribute(name)) = raw_clause_path(clause).first() else { + continue; + }; + // An undefined reference keeps its `#`, which no plain name carries. + let attr = match name.strip_prefix('#') { + Some(reference) => names.get(reference).map_or(name.as_str(), String::as_str), + None => name.as_str(), + }; + if seen.contains(&attr) { + duplicated = true; + break; + } + seen.push(attr); + } + if !duplicated { + return Ok(()); + } + for clause in clauses { + if let Some(PathElement::Attribute(name)) = raw_clause_path(clause).first() + && let Some(reference) = name.strip_prefix('#') + && !names.contains_key(reference) + { + return Err(validation_err(&format!( + "Invalid KeyConditionExpression: An expression attribute name used in the \ + document path is not defined; attribute name: #{reference}" + ))); + } + } + for clause in clauses { + for value in raw_clause_values(clause) { + if let Expr::Placeholder(placeholder) = value + && !values.contains_key(placeholder) + { + return Err(validation_err(&format!( + "Invalid KeyConditionExpression: An expression attribute value used in \ + expression is not defined; attribute value: :{placeholder}" + ))); + } + } + } + Err(validation_err(ONE_CONDITION_PER_KEY_ERR)) +} + /// Resolve a path's top-level attribute name, handling `#name` references. fn resolve_path_attr( path: &[PathElement], @@ -526,12 +597,49 @@ impl SortKeyCondition { /// Supports multi-part keys: up to 8 AND-ed clauses (4 HASH + 4 RANGE). /// Rejects OR, NOT, and unsupported functions. /// +/// This does not look for a second condition on one key attribute, which +/// needs the request's `ExpressionAttributeNames` and +/// `ExpressionAttributeValues`. A request's key condition is parsed with +/// [`parse_key_condition_with_maps`]. +/// /// # Errors /// /// Returns `ValidationException` for syntax errors or unsupported constructs. pub fn parse_key_condition(tokens: &[Token]) -> Result { - let mut clauses = parse_clauses(tokens)?; + classify_clauses(parse_clauses(tokens)?) +} + +/// [`parse_key_condition`] with the request's `ExpressionAttributeNames` +/// (`names`, keyed without the leading `#`) and `ExpressionAttributeValues` +/// (`values`, keyed without the leading `:`; only which placeholders are +/// defined is read). +/// +/// After the parser's own checks, and before any clause is read as the +/// partition or the sort key condition, this refuses a key condition with more +/// than one condition on one key attribute, written with plain names or +/// through `#name` references. When there is such a duplicate, an undefined +/// `#name` and then an undefined `:value` are reported ahead of it, as the +/// service reports them. The engine reads a `Query` key condition through +/// this, and authorization runs the same check through +/// [`key_condition_clause_operands`]. +/// +/// # Errors +/// +/// As [`parse_key_condition`], plus `ValidationException` for a second +/// condition on one key attribute. +pub fn parse_key_condition_with_maps( + tokens: &[Token], + names: &std::collections::HashMap, + values: &std::collections::HashMap, +) -> Result { + let clauses = parse_clauses(tokens)?; + reject_duplicate_key_attrs(&clauses, names, values)?; + classify_clauses(clauses) +} +/// Read the parsed clauses as a partition key condition and sort key +/// conditions. +fn classify_clauses(mut clauses: Vec) -> Result { match clauses.len() { 1 => classify_single(clauses.remove(0)), 2 => classify_pair(clauses.remove(0), clauses.remove(0)), @@ -548,14 +656,24 @@ pub fn parse_key_condition(tokens: &[Token]) -> Result( tokens: &[Token], + names: &std::collections::HashMap, + values: &std::collections::HashMap, ) -> Result, DynamoDbError> { - Ok(parse_clauses(tokens)? + let clauses = parse_clauses(tokens)?; + reject_duplicate_key_attrs(&clauses, names, values)?; + Ok(clauses .into_iter() .filter_map(|clause| match clause { RawClause::Eq(path, value) => Some(KeyClauseOperand { @@ -645,6 +763,15 @@ fn raw_clause_path(clause: &RawClause) -> &[PathElement] { } } +/// The value operands of a raw clause, in the order they are written. +fn raw_clause_values(clause: &RawClause) -> Vec<&Expr> { + match clause { + RawClause::Eq(_, value) | RawClause::Compare(_, _, value) => vec![value], + RawClause::Between(_, low, high) => vec![low, high], + RawClause::BeginsWith(_, prefix) => vec![prefix], + } +} + /// Reject any key path that is not a single top-level attribute. /// /// `parse_path` always emits an `Attribute` (or `#name`) as element 0, so a path @@ -998,6 +1125,7 @@ fn validation_err(msg: &str) -> DynamoDbError { mod tests { use super::*; use crate::expression::tokenize; + use crate::types::AttributeValue; use std::collections::HashMap; #[test] @@ -1355,17 +1483,14 @@ mod tests { "pk < :v AND sk > :s", &[("pk", "v", false), ("sk", "s", false)], ), - ( - "pk = :u AND pk > :v", - &[("pk", "u", true), ("pk", "v", false)], - ), // BETWEEN names two operands, so it contributes none. ("pk BETWEEN :lo AND :hi", &[]), ("pk = :v AND sk BETWEEN :lo AND :hi", &[("pk", "v", true)]), ]; for (expr, want) in cases { let tokens = tokenize(expr).unwrap(); - let got = key_condition_clause_operands(&tokens).unwrap(); + let got = + key_condition_clause_operands(&tokens, &HashMap::new(), &values_of(expr)).unwrap(); let got: Vec<(String, String, bool)> = got .into_iter() .map(|clause| { @@ -1404,7 +1529,7 @@ mod tests { continue; }; assert!( - key_condition_clause_operands(&tokens).is_err(), + key_condition_clause_operands(&tokens, &HashMap::new(), &no_values()).is_err(), "expr {expr} should be rejected" ); } @@ -1902,4 +2027,297 @@ mod tests { kc.resolve_key_schema(&schema, &HashMap::new()).unwrap(); assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); } + + const UNDEFINED_NAME_U: &str = "Invalid KeyConditionExpression: An expression attribute \ + name used in the document path is not defined; attribute \ + name: #u"; + const TYPE_ERR: &str = "One or more parameter values were invalid: Condition parameter \ + type does not match schema type"; + + /// No `ExpressionAttributeValues`. + fn no_values() -> HashMap { + HashMap::new() + } + + /// `ExpressionAttributeValues` that define every placeholder of `expr`, + /// keyed without the leading `:`. + fn values_of(expr: &str) -> HashMap { + tokenize(expr) + .unwrap() + .into_iter() + .filter_map(|token| match token { + Token::Placeholder(name) => Some((name, AttributeValue::S("v".to_owned()))), + _ => None, + }) + .collect() + } + + /// `ExpressionAttributeNames` from `(reference, attribute)` pairs, keyed + /// without the leading `#`. + fn names_of(pairs: &[(&str, &str)]) -> HashMap { + pairs + .iter() + .map(|(reference, attr)| ((*reference).to_owned(), (*attr).to_owned())) + .collect() + } + + /// Parse `expr` as the engine parses a request's key condition, with every + /// placeholder defined. + fn parse_request( + expr: &str, + names: &HashMap, + ) -> Result { + let tokens = tokenize(expr).unwrap(); + parse_key_condition_with_maps(&tokens, names, &values_of(expr)) + } + + fn assert_refused( + result: Result, + want: &str, + expr: &str, + ) { + match result { + Err(DynamoDbError::ValidationException(msg)) => assert_eq!(msg, want, "expr {expr}"), + other => panic!("expr {expr}: got {other:?}"), + } + } + + /// A second condition on a key attribute, written with plain names or + /// through a `#name` reference, is refused before the clauses are handed + /// to authorization, so the LeadingKeys path reads no partition operand + /// from such a request. + #[test] + fn clause_operands_reject_duplicate_key_attribute() { + let names = names_of(&[("p", "pk"), ("s", "sk")]); + for expr in [ + "pk = :a AND pk = :b", + "#p = :a AND pk = :b", + "pk = :u AND pk > :v", + "#p = :u AND pk > :v", + "pk > :v AND sk = :s AND sk = :t", + "pk > :v AND #s = :s AND sk = :t", + ] { + let tokens = tokenize(expr).unwrap(); + assert_refused( + key_condition_clause_operands(&tokens, &names, &values_of(expr)), + ONE_CONDITION_PER_KEY_ERR, + expr, + ); + } + } + + #[test] + fn duplicate_condition_on_one_key_attribute_rejected() { + // Measured 2026-10-24 (us-east-1): every one of these shapes answers + // the one-condition-per-key message, whatever the operators are and + // whichever key attribute is named twice. + for expr in [ + "pk = :a AND pk = :b", + "pk = :a AND pk = :a", + "pk = :a AND pk = :b AND sk = :c", + "pk = :a AND pk > :b", + "pk > :b AND pk = :a", + ":a = pk AND pk = :b", + "(pk = :a) AND (pk = :b)", + "pk = :a AND sk = :b AND sk = :c", + "pk = :a AND sk = :b AND sk = :b", + "pk = :a AND sk BETWEEN :b AND :c AND sk = :b", + "pk = :a AND sk BETWEEN :b AND :c AND begins_with(sk, :b)", + "pk = :a AND begins_with(sk, :b) AND sk = :c", + "pk = :a AND sk > :b AND sk < :c", + "pk = :a AND begins_with(sk, :b) AND begins_with(sk, :c)", + "pk = :a AND pk = :b AND sk = :b AND sk = :c", + "sk = :b AND sk = :c", + ] { + assert_refused( + parse_request(expr, &HashMap::new()), + ONE_CONDITION_PER_KEY_ERR, + expr, + ); + } + } + + #[test] + fn duplicate_condition_through_name_reference_rejected() { + // `#p` and `pk` name the same attribute once `#p` is resolved, on a + // single-attribute key schema and a multi-attribute one alike. + let names = names_of(&[("p", "pk"), ("hh", "h")]); + for expr in ["#p = :a AND pk = :b", "#hh = :x AND h = :y AND a = :z"] { + assert_refused(parse_request(expr, &names), ONE_CONDITION_PER_KEY_ERR, expr); + } + } + + #[test] + fn duplicate_condition_through_two_name_references_rejected() { + let names = names_of(&[("p", "pk"), ("q", "pk")]); + let expr = "#p = :a AND #q = :b"; + assert_refused(parse_request(expr, &names), ONE_CONDITION_PER_KEY_ERR, expr); + } + + #[test] + fn duplicate_condition_on_sort_key_through_name_reference_rejected() { + let names = names_of(&[("s", "sk")]); + let expr = "pk = :a AND #s = :b AND sk = :c"; + assert_refused(parse_request(expr, &names), ONE_CONDITION_PER_KEY_ERR, expr); + } + + /// With a duplicate present, an undefined `#name` anywhere in the + /// expression is reported first. Measured 2026-10-24 for + /// `#u = :a AND #u = :b` and 2026-10-31 for the next three; the + /// multi-attribute form is not measured. + #[test] + fn undefined_name_reported_ahead_of_duplicate() { + for expr in [ + "#u = :a AND #u = :b", + "#u = :a AND #u > :b", + "#u = :a AND sk = :b AND sk = :c", + "pk = :a AND pk = :b AND #u = :c", + "h = :a AND h = :b AND #u = :c", + ] { + assert_refused(parse_request(expr, &HashMap::new()), UNDEFINED_NAME_U, expr); + } + // Without a duplicate the parse passes, and `resolve_pk_sk` reports + // the name. + let mut kc = parse_request("#u = :a AND sk = :b", &HashMap::new()).unwrap(); + assert_refused( + kc.resolve_pk_sk("pk", &HashMap::new()), + UNDEFINED_NAME_U, + "#u = :a AND sk = :b", + ); + } + + /// With a duplicate present, an undefined `:value` is reported ahead of it + /// (measured 2026-10-31: `pk = :a AND pk = :b` without + /// `ExpressionAttributeValues` answers the undefined-value message for + /// `:a`). Which of an undefined name and an undefined value the service + /// reports first is not measured; the name is checked first here. + #[test] + fn undefined_value_reported_ahead_of_duplicate() { + let undefined = |value: &str| { + format!( + "Invalid KeyConditionExpression: An expression attribute value used in \ + expression is not defined; attribute value: :{value}" + ) + }; + let tokens = tokenize("pk = :a AND pk = :b").unwrap(); + assert_refused( + parse_key_condition_with_maps(&tokens, &HashMap::new(), &no_values()), + &undefined("a"), + "pk = :a AND pk = :b", + ); + let tokens = tokenize("pk = :a AND pk = :missing").unwrap(); + assert_refused( + parse_key_condition_with_maps(&tokens, &HashMap::new(), &values_of("pk = :a")), + &undefined("missing"), + "pk = :a AND pk = :missing", + ); + let tokens = tokenize("pk = :a AND pk = :b AND #u = :c").unwrap(); + assert_refused( + parse_key_condition_with_maps(&tokens, &HashMap::new(), &no_values()), + UNDEFINED_NAME_U, + "pk = :a AND pk = :b AND #u = :c", + ); + // Without a duplicate the parse does not read the values. + let tokens = tokenize("pk = :a AND sk = :b").unwrap(); + assert!(parse_key_condition_with_maps(&tokens, &HashMap::new(), &no_values()).is_ok()); + } + + /// The duplicate is reported by the parse, so it comes ahead of the key + /// value type check that `resolve_key_schema_with` runs and of the engine's + /// unused-value check, whichever way it is spelled (measured 2026-10-31: + /// `pk = :a AND pk = :n`, `#p = :a AND pk = :n` and + /// `pk = :a AND #s = :b AND sk = :n` with `:n` a number on an S key, and + /// `pk = :a AND pk = :b` with an unused `:z`, answer the + /// one-condition-per-key message; `pk = :n` alone answers the type). + #[test] + fn duplicate_reported_ahead_of_type_and_unused_value_checks() { + let schema = [key("pk", KeyType::Hash), key("sk", KeyType::Range)]; + let names = names_of(&[("p", "pk"), ("s", "sk")]); + let values: HashMap = [ + ("a", AttributeValue::S("alice".to_owned())), + ("b", AttributeValue::S("bob".to_owned())), + ("n", AttributeValue::N("1".to_owned())), + ("z", AttributeValue::S("zed".to_owned())), + ] + .into_iter() + .map(|(k, v)| (k.to_owned(), v)) + .collect(); + for expr in [ + "pk = :a AND pk = :n", + "#p = :a AND pk = :n", + "pk = :a AND #s = :b AND sk = :n", + "pk = :a AND pk = :b", + ] { + let tokens = tokenize(expr).unwrap(); + assert_refused( + parse_key_condition_with_maps(&tokens, &names, &values), + ONE_CONDITION_PER_KEY_ERR, + expr, + ); + } + let tokens = tokenize("pk = :n").unwrap(); + let mut kc = parse_key_condition_with_maps(&tokens, &names, &values).unwrap(); + assert_refused( + kc.resolve_key_schema_with(&schema, &names, |_, value| match value { + Expr::Placeholder(name) if name == "n" => Err(validation_err(TYPE_ERR)), + _ => Ok(()), + }), + TYPE_ERR, + "pk = :n", + ); + } + + #[test] + fn nested_path_reported_ahead_of_duplicate() { + // Measured 2026-10-24: both of these answer their own message, not the + // one-condition-per-key message. + assert_refused( + parse_request("pk = :a AND pk.foo = :b", &HashMap::new()), + NESTED_KEY_ERR, + "pk = :a AND pk.foo = :b", + ); + assert_refused( + parse_request("((pk = :a)) AND (pk = :b)", &HashMap::new()), + "Invalid KeyConditionExpression: The expression has redundant parentheses;", + "((pk = :a)) AND (pk = :b)", + ); + } + + #[test] + fn distinct_key_attributes_accepted_after_duplicate_check() { + // One condition per key attribute passes the check and resolves under + // the key schema the engine reads it with: the check is per attribute + // name, not per clause count. + let names = names_of(&[("p", "pk"), ("s", "sk")]); + let pk_sk = [key("pk", KeyType::Hash), key("sk", KeyType::Range)]; + let two_hash = [ + key("pk1", KeyType::Hash), + key("pk2", KeyType::Hash), + key("sk", KeyType::Range), + ]; + let two_by_two = [ + key("pk1", KeyType::Hash), + key("pk2", KeyType::Hash), + key("sk1", KeyType::Range), + key("sk2", KeyType::Range), + ]; + for (schema, expr) in [ + (&pk_sk[..], "pk = :a"), + (&pk_sk[..], "pk = :a AND sk = :b"), + (&pk_sk[..], "sk = :b AND pk = :a"), + (&pk_sk[..], "pk = :a AND sk BETWEEN :b AND :c"), + (&pk_sk[..], "pk = :a AND begins_with(sk, :b)"), + (&pk_sk[..], "#p = :a AND #s = :b"), + (&two_hash[..], "pk1 = :a AND pk2 = :b AND sk = :c"), + ( + &two_by_two[..], + "pk1 = :a AND pk2 = :b AND sk1 = :c AND sk2 = :d", + ), + ] { + let mut kc = parse_request(expr, &names) + .unwrap_or_else(|err| panic!("expr {expr} should parse: {err:?}")); + kc.resolve_key_schema(schema, &names) + .unwrap_or_else(|err| panic!("expr {expr} should resolve: {err:?}")); + } + } } diff --git a/crates/core/src/expression/mod.rs b/crates/core/src/expression/mod.rs index c9f8e207b..28061f18c 100755 --- a/crates/core/src/expression/mod.rs +++ b/crates/core/src/expression/mod.rs @@ -26,7 +26,7 @@ pub use ast::{ArithOp, CompareOp, Expr, PathElement, UpdateAction}; pub use evaluator::evaluate_condition; pub use key_condition::{ KeyClauseOperand, KeyCondition, SortKeyCondition, key_condition_clause_operands, - parse_key_condition, + parse_key_condition, parse_key_condition_with_maps, }; pub use kind::ExpressionKind; pub use parser::{parse_condition, parse_condition_with_depth_limit}; diff --git a/crates/engine/src/expression_helpers.rs b/crates/engine/src/expression_helpers.rs index 913333015..9a5dc0f65 100755 --- a/crates/engine/src/expression_helpers.rs +++ b/crates/engine/src/expression_helpers.rs @@ -8,8 +8,9 @@ use std::collections::HashMap; use extenddb_core::error::DynamoDbError; use extenddb_core::expression::{ Expr, ExpressionKind, ExpressionMaps, KeyCondition, PathElement, Token, UpdateAction, - parse_condition_with_depth_limit, parse_key_condition, parse_projection, parse_update_from, - tokenize_for, tokenize_with_limit, validate_no_reserved_words, validate_ordering_operand_types, + parse_condition_with_depth_limit, parse_key_condition_with_maps, parse_projection, + parse_update_from, tokenize_for, tokenize_with_limit, validate_no_reserved_words, + validate_ordering_operand_types, }; use extenddb_core::limits::LimitsConfig; use extenddb_core::types::{AttributeValue, ConditionalOperator, ExpectedAttributeValue}; @@ -237,14 +238,19 @@ pub fn parse_projection_expr( /// Size-check, tokenize, reserved-word check, and parse a `KeyConditionExpression`. /// +/// `maps` holds the request's `ExpressionAttributeNames` and +/// `ExpressionAttributeValues`, which the parse reads to refuse a second +/// condition on one key attribute. +/// /// Errors carry the `KeyConditionExpression` prefix, matching Amazon `DynamoDB`. /// /// # Errors /// /// Returns `DynamoDbError::ValidationException` for over-size, syntax, or -/// reserved-word errors. +/// reserved-word errors, and for a second condition on one key attribute. pub fn parse_key_condition_expr( expr: &str, + maps: &ExpressionMaps, limits: &LimitsConfig, ) -> Result { check_expression_size(expr, ExpressionKind::KeyCondition, limits)?; @@ -257,7 +263,7 @@ pub fn parse_key_condition_expr( if limits.enforce_reserved_keywords { validate_no_reserved_words(&tokens)?; } - parse_key_condition(&tokens) + parse_key_condition_with_maps(&tokens, &maps.names, &maps.values) }) .map_err(|e| prefix_expression_error(e, ExpressionKind::KeyCondition)) } @@ -407,4 +413,111 @@ mod tests { "got {err:?}" ); } + + /// A key condition request and the message it answers: the expression, + /// its `#`-prefixed names, its `:`-prefixed values (`None` for no + /// `ExpressionAttributeValues`) and the expected message. + type KceCase = ( + &'static str, + Vec<(&'static str, &'static str)>, + Option>, + &'static str, + ); + + /// Parse `expr` as `Query` does, with `#`-prefixed `names` and + /// `:`-prefixed `values` as a request carries them. + fn parse_kce( + expr: &str, + names: &[(&str, &str)], + values: Option<&[(&str, AttributeValue)]>, + ) -> Result { + let names: HashMap = names + .iter() + .map(|(k, v)| ((*k).to_owned(), (*v).to_owned())) + .collect(); + let values: Option> = values.map(|vs| { + vs.iter() + .map(|(k, v)| ((*k).to_owned(), v.clone())) + .collect() + }); + let maps = build_expression_maps(Some(&names), values.as_ref()); + parse_key_condition_expr(expr, &maps, &LimitsConfig::default()) + } + + /// The order of the one-condition-per-key check through the engine's + /// entry point, on the requests measured 2026-10-31 (table `pk` S HASH, + /// `sk` S RANGE). The key value type check and the unused-value check + /// run after this parse. + #[test] + fn key_condition_duplicate_check_order_matches_the_service() { + const ONE_PER_KEY: &str = "Invalid KeyConditionExpression: KeyConditionExpressions \ + must only contain one condition per key"; + const UNDEFINED_U: &str = "Invalid KeyConditionExpression: An expression attribute \ + name used in the document path is not defined; attribute \ + name: #u"; + const UNDEFINED_A: &str = "Invalid KeyConditionExpression: An expression attribute \ + value used in expression is not defined; attribute value: :a"; + let s = |v: &str| AttributeValue::S(v.to_owned()); + let n = AttributeValue::N("1".to_owned()); + let abc = [(":a", s("alice")), (":b", s("bob")), (":c", s("carol"))]; + let cases: Vec = vec![ + ( + "pk = :a AND pk = :b", + vec![], + Some(abc[..2].to_vec()), + ONE_PER_KEY, + ), + ( + "#p = :a AND pk = :n", + vec![("#p", "pk")], + Some(vec![(":a", s("alice")), (":n", n.clone())]), + ONE_PER_KEY, + ), + ( + "pk = :a AND #s = :b AND sk = :n", + vec![("#s", "sk")], + Some(vec![ + (":a", s("alice")), + (":b", s("bob")), + (":n", n.clone()), + ]), + ONE_PER_KEY, + ), + ( + "#u = :a AND sk = :b AND sk = :c", + vec![], + Some(abc.to_vec()), + UNDEFINED_U, + ), + ( + "pk = :a AND pk = :b AND #u = :c", + vec![], + Some(abc.to_vec()), + UNDEFINED_U, + ), + ("pk = :a AND pk = :b", vec![], None, UNDEFINED_A), + ( + "pk = :a AND pk = :b", + vec![], + Some(vec![(":a", s("alice")), (":b", s("bob")), (":z", s("zed"))]), + ONE_PER_KEY, + ), + ( + "#u = :a AND #u > :b", + vec![], + Some(abc[..2].to_vec()), + UNDEFINED_U, + ), + ]; + for (expr, names, values, want) in cases { + let got = parse_kce(expr, &names, values.as_deref()); + assert!( + matches!(&got, Err(DynamoDbError::ValidationException(msg)) if msg == want), + "expr {expr}: got {got:?}" + ); + } + // Without a duplicate the parse leaves the value to the later checks. + let got = parse_kce("pk = :n", &[], Some(&[(":n", n)])); + assert!(got.is_ok(), "pk = :n: got {got:?}"); + } } diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index 848f5fdbb..1ed90459b 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -237,7 +237,12 @@ pub async fn handle_query( let (mut key_condition, legacy_kc_maps) = if let Some(kce_str) = input.key_condition_expression.as_deref() { - let parsed = crate::expression_helpers::parse_key_condition_expr(kce_str, &ctx.limits)?; + // The parse refuses a second condition on one key attribute, after + // the expression's `#name` and `:value` references are checked and + // before the clauses are classified, which is where the service + // reports it (measured 2026-10-31). + let parsed = + crate::expression_helpers::parse_key_condition_expr(kce_str, &maps, &ctx.limits)?; (parsed, None) } else if let Some(ref kc) = input.key_conditions { let key_schema_pairs: Vec<(String, bool)> = query_key_info diff --git a/crates/server/src/leading_keys.rs b/crates/server/src/leading_keys.rs index 0f2577985..46cc90af3 100644 --- a/crates/server/src/leading_keys.rs +++ b/crates/server/src/leading_keys.rs @@ -63,7 +63,7 @@ use std::collections::HashMap; use extenddb_core::expression::{ - Expr, PathElement, Token, key_condition_clause_operands, parse_key_condition, + Expr, PathElement, Token, key_condition_clause_operands, parse_key_condition_with_maps, tokenize_with_limit, }; use extenddb_core::types::{KeySchemaElement, KeyType, TableKeyInfo}; @@ -268,8 +268,11 @@ fn query_leading_keys(input: &Value, key_info: &TableKeyInfo, max_tokens: usize) if let Some(kce) = input.get("KeyConditionExpression").and_then(Value::as_str) { let tokens = tokenize_with_limit(kce, max_tokens).ok()?; - // The clauses that name the partition attribute, as written. - let pk_clauses: Vec<_> = key_condition_clause_operands(&tokens) + // The clauses that name the partition attribute. A key condition with + // a second condition on one key attribute, written with plain names + // or through a `#name` reference, is refused here with the check the + // engine runs, so it leaves the key absent whatever its operators. + let pk_clauses: Vec<_> = key_condition_clause_operands(&tokens, &names, &values) .ok()? .into_iter() .filter(|clause| resolve_attr_name(&clause.path, &names).as_deref() == Some(pk_attr)) @@ -278,21 +281,17 @@ fn query_leading_keys(input: &Value, key_info: &TableKeyInfo, max_tokens: usize) // 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. + // number, a placeholder with no value), the key stays absent. if pk_clauses.iter().any(|clause| clause.equality) { 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 - // that operand and evaluates the policy before it rejects the shape, - // so a principal fenced away from the value gets AccessDeniedException + // The one clause on the partition key uses an operator the engine + // rejects, such as `pk > :v`. The service binds LeadingKeys to that + // operand and evaluates the policy before it rejects the shape, so a + // principal fenced away from the value gets AccessDeniedException // while one fenced to it gets the shape's ValidationException - // (measured 2026-10-21 for `pk > :v`). A condition that does not - // parse, one that names no partition clause, and one with two - // non-equality partition clauses (which value the service binds for - // that shape is not measured) leave the key absent. + // (measured 2026-10-21 for `pk > :v`). A condition that does not parse + // and one that names no partition clause leave the key absent. let [clause] = pk_clauses.as_slice() else { return None; }; @@ -348,7 +347,7 @@ fn equality_pk_value( names: &HashMap, values: &HashMap, ) -> Option { - let mut kc = parse_key_condition(tokens).ok()?; + let mut kc = parse_key_condition_with_maps(tokens, names, values).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 diff --git a/crates/server/src/leading_keys/tests.rs b/crates/server/src/leading_keys/tests.rs index c81e3baaa..9936f1e6d 100644 --- a/crates/server/src/leading_keys/tests.rs +++ b/crates/server/src/leading_keys/tests.rs @@ -622,11 +622,10 @@ fn query_between_on_the_partition_key_is_absent() { assert_eq!(r, None); } -/// An equality on the partition key decides the key, even when its value -/// cannot be read and another clause names the partition key: the other -/// clause's operand is never bound in its place. With `:u = {"N": "bob"}` -/// (not a number) the key stays absent, as on main, rather than becoming -/// `alice`. +/// An equality on the partition key and a second clause on the partition key +/// name the partition key twice, so the duplicate check refuses the condition +/// and the key stays absent: the other clause's operand (`alice`) is never +/// bound, whether the equality's value can be read or not. #[test] fn query_unreadable_partition_equality_binds_no_other_clause() { for (kce, u) in [ @@ -634,6 +633,7 @@ fn query_unreadable_partition_equality_binds_no_other_clause() { ("pk > :v AND pk = :u", json!({"N": "bob"})), ("#p = :u AND pk > :v", json!({"N": "bob"})), ("pk = :u AND begins_with(pk, :v)", json!({"N": "1e"})), + ("pk = :u AND pk > :v", json!({"S": "bob"})), ] { let input = json!({ "TableName": "t", @@ -644,18 +644,40 @@ fn query_unreadable_partition_equality_binds_no_other_clause() { let r = single_table_leading_keys(&input, "Query", Some(&main_table()), MAX_TOKENS); assert_eq!(r, None, "{kce}"); } - // A readable equality keeps its own value with another partition clause. - let input = json!({ - "TableName": "t", - "KeyConditionExpression": "pk = :u AND pk > :v", - "ExpressionAttributeValues": {":u": {"S": "bob"}, ":v": {"S": "alice"}}, - }); - let r = single_table_leading_keys(&input, "Query", Some(&main_table()), MAX_TOKENS); - assert_ne!(r, Some(vec!["alice".to_owned()])); } -/// Two non-equality clauses on the partition key: which operand the service -/// binds is not measured, so neither is bound, in either clause order. +/// A second condition on one key attribute, written with plain names or +/// through a `#name` reference, leaves the key absent, including when the +/// only partition clause is a non-equality whose operand would otherwise be +/// bound. The engine refuses all of these. +#[test] +fn query_duplicate_key_condition_leaves_the_key_absent() { + for kce in [ + "pk = :a AND pk = :b", + "#p = :a AND pk = :b", + "pk = :a AND sk = :b AND sk = :c", + "pk = :a AND #s = :b AND sk = :c", + "pk > :a AND sk = :b AND sk = :c", + "pk > :a AND #s = :b AND sk = :c", + ] { + let input = json!({ + "TableName": "t", + "KeyConditionExpression": kce, + "ExpressionAttributeNames": {"#p": "pk", "#s": "sk"}, + "ExpressionAttributeValues": { + ":a": {"S": "alice"}, + ":b": {"S": "bob"}, + ":c": {"S": "carol"}, + }, + }); + let r = single_table_leading_keys(&input, "Query", Some(&main_table()), MAX_TOKENS); + assert_eq!(r, None, "{kce}"); + } +} + +/// Two non-equality clauses on the partition key name it twice, so the +/// duplicate check refuses the condition and neither operand is bound, in +/// either clause order. #[test] fn query_two_non_equality_partition_clauses_bind_nothing() { for kce in [ diff --git a/tests/test_fgac_leading_keys.py b/tests/test_fgac_leading_keys.py index 905fcd1f1..ee3882909 100644 --- a/tests/test_fgac_leading_keys.py +++ b/tests/test_fgac_leading_keys.py @@ -96,6 +96,12 @@ def _denied(exc: ClientError) -> bool: return exc.value.response["Error"]["Code"] == "AccessDeniedException" +ONE_PER_KEY = ( + "Invalid KeyConditionExpression: " + "KeyConditionExpressions must only contain one condition per key" +) + + # 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"}, @@ -371,12 +377,31 @@ def test_scan_denied_under_for_any_value(self, table): c.scan(TableName=table) assert _denied(e) - # -- An equality on the partition key decides the key ------------------ + # -- A second clause on the partition key binds nothing ---------------- def test_unreadable_partition_equality_does_not_bind_another_clause(self, table): - # An equality on the partition key decides the key. When its value - # cannot be read (`:u = {"N": "bob"}`), a second clause naming the - # fenced value must not be bound in its place. + # An equality on the partition key and a second clause on it name the + # partition key twice, so the duplicate check leaves + # dynamodb:LeadingKeys absent and the fenced value in the second + # clause (`:v`) is never bound. A principal allowed everything gets + # the one-condition-per-key error. A policy that needs a LeadingKeys + # value denies the request, whether the equality's value can be read + # (`:u = {"S": "bob"}`) or not (`:u = {"N": "bob"}`, refused either + # way). + kces = ("pk = :u AND pk > :v", "pk > :v AND pk = :u") + readable = {":u": {"S": OTHER}, ":v": {"S": OWN}} + unreadable = {":u": {"N": OTHER}, ":v": {"S": OWN}} + owner = self._user_client([("all", { + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": "dynamodb:*", "Resource": "*"}], + })]) + for kce in kces: + with pytest.raises(ClientError) as e: + owner.query(TableName=table, KeyConditionExpression=kce, + ExpressionAttributeValues=readable) + err = e.value.response["Error"] + assert err["Code"] == "ValidationException", kce + assert err["Message"] == ONE_PER_KEY, kce for form in ("ForAnyValue", "ForAllValues+Null"): if form == "ForAnyValue": policy = self._leading_keys_policy("ForAnyValue", [OWN], table) @@ -385,15 +410,16 @@ def test_unreadable_partition_equality_does_not_bind_another_clause(self, table) stmt = policy["Statement"][0] stmt["Condition"]["Null"] = {"dynamodb:LeadingKeys": "false"} c = self._user_client([("lk", policy)]) - for kce in ("pk = :u AND pk > :v", "pk > :v AND pk = :u"): - try: - got = c.query( - TableName=table, KeyConditionExpression=kce, - ExpressionAttributeValues={":u": {"N": OTHER}, ":v": {"S": OWN}}) - except ClientError: - continue - parts = {it["pk"]["S"] for it in got["Items"]} - assert OTHER not in parts, (form, kce, parts) + for kce in kces: + with pytest.raises(ClientError) as e: + c.query(TableName=table, KeyConditionExpression=kce, + ExpressionAttributeValues=readable) + assert _denied(e), (form, kce) + with pytest.raises(ClientError) as e: + c.query(TableName=table, KeyConditionExpression=kce, + ExpressionAttributeValues=unreadable) + assert e.value.response["Error"]["Code"] in ( + "AccessDeniedException", "ValidationException"), (form, kce) def test_non_equality_partition_with_sort_clause_is_refused(self, table): # The fix binds the partition operand of these shapes; the engine still diff --git a/tests/test_query_key_condition_one_per_key.py b/tests/test_query_key_condition_one_per_key.py new file mode 100644 index 000000000..3aa118c86 --- /dev/null +++ b/tests/test_query_key_condition_one_per_key.py @@ -0,0 +1,318 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""A KeyConditionExpression carries one condition per key attribute. + +Amazon DynamoDB rejects a key condition that names the same key attribute twice, +on the partition key and on the sort key alike, whatever the operators are: + + ValidationException: Invalid KeyConditionExpression: KeyConditionExpressions + must only contain one condition per key + +Measured 2026-10-24T00:27:56Z and 00:30:39Z in us-east-1 on a table with +pk (S) HASH and sk (S) RANGE. Issue #430. + +The order against undefined references, the key value type check and an +unused value was measured 2026-10-31T17:37Z in us-east-1 on a table with the +same key schema, two samples each: an undefined #name anywhere in the +expression and an undefined :value are reported ahead of the duplicate, and the +duplicate is reported ahead of the type check and the unused-value check. + +Dual-target against Amazon DynamoDB and extenddb. +""" + +from __future__ import annotations + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +ONE_PER_KEY = ( + "Invalid KeyConditionExpression: " + "KeyConditionExpressions must only contain one condition per key" +) +NESTED = ( + "Invalid KeyConditionExpression: " + "KeyConditionExpressions cannot have conditions on nested attributes" +) +REDUNDANT_PARENS = ( + "Invalid KeyConditionExpression: The expression has redundant parentheses;" +) +UNDEFINED_NAME = ( + "An expression attribute name used in the document path is not defined; " + "attribute name: #u" +) +UNDEFINED_NAME_EXACT = "Invalid KeyConditionExpression: " + UNDEFINED_NAME +UNDEFINED_VALUE_A = ( + "Invalid KeyConditionExpression: An expression attribute value used in " + "expression is not defined; attribute value: :a" +) + +A = {"S": "alice"} +B = {"S": "bob"} +C = {"S": "carol"} +N1 = {"N": "1"} + + +@pytest.fixture(scope="class") +def range_table(dynamodb_client): + """pk (S) HASH, sk (S) RANGE, holding alice/bob and alice/carol.""" + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", "AttributeType": "S"}, + {"AttributeName": "sk", "AttributeType": "S"}, + ], + key_schema=[ + {"AttributeName": "pk", "KeyType": "HASH"}, + {"AttributeName": "sk", "KeyType": "RANGE"}, + ], + ) as name: + for sk in ("bob", "carol"): + dynamodb_client.put_item( + TableName=name, Item={"pk": {"S": "alice"}, "sk": {"S": sk}} + ) + yield name + + +def _error(func) -> tuple[str, str]: + with pytest.raises(ClientError) as exc_info: + func() + err = exc_info.value.response["Error"] + return err["Code"], err["Message"] + + +class TestOneConditionPerKey: + """Every measured duplicate shape, and the shapes that keep their own error.""" + + @pytest.mark.parametrize( + ("expr", "values"), + [ + # The shape in the issue: a second equality on the partition key is + # served on extenddb before the fix, reading :b as a sort key value. + ("pk = :a AND pk = :b", {":a": A, ":b": B}), + ("pk = :a AND pk = :a", {":a": A}), + ("pk = :a AND pk = :b AND sk = :c", {":a": A, ":b": B, ":c": C}), + ("pk = :a AND pk > :b", {":a": A, ":b": B}), + ("pk > :b AND pk = :a", {":a": A, ":b": B}), + (":a = pk AND pk = :b", {":a": A, ":b": B}), + ("(pk = :a) AND (pk = :b)", {":a": A, ":b": B}), + ], + ) + def test_duplicate_partition_key_condition( + self, dynamodb_client, range_table, expr, values + ): + code, msg = _error( + lambda: dynamodb_client.query( + TableName=range_table, + KeyConditionExpression=expr, + ExpressionAttributeValues=values, + ) + ) + assert code == "ValidationException" + assert msg == ONE_PER_KEY + + @pytest.mark.parametrize( + ("expr", "values"), + [ + ("pk = :a AND sk = :b AND sk = :c", {":a": A, ":b": B, ":c": C}), + ("pk = :a AND sk = :b AND sk = :b", {":a": A, ":b": B}), + ( + "pk = :a AND sk BETWEEN :b AND :c AND sk = :b", + {":a": A, ":b": B, ":c": C}, + ), + ( + "pk = :a AND sk BETWEEN :b AND :c AND begins_with(sk, :b)", + {":a": A, ":b": B, ":c": C}, + ), + ("pk = :a AND begins_with(sk, :b) AND sk = :c", {":a": A, ":b": B, ":c": C}), + ("pk = :a AND sk > :b AND sk < :c", {":a": A, ":b": B, ":c": C}), + ( + "pk = :a AND begins_with(sk, :b) AND begins_with(sk, :c)", + {":a": A, ":b": B, ":c": C}, + ), + ("pk = :a AND pk = :b AND sk = :b AND sk = :c", {":a": A, ":b": B, ":c": C}), + # No partition key condition at all: the duplicate is reported + # ahead of the missing key schema element. + ("sk = :b AND sk = :c", {":b": B, ":c": C}), + ], + ) + def test_duplicate_sort_key_condition( + self, dynamodb_client, range_table, expr, values + ): + code, msg = _error( + lambda: dynamodb_client.query( + TableName=range_table, + KeyConditionExpression=expr, + ExpressionAttributeValues=values, + ) + ) + assert code == "ValidationException" + assert msg == ONE_PER_KEY + + @pytest.mark.parametrize( + ("expr", "values", "names"), + [ + ("#p = :a AND pk = :b", {":a": A, ":b": B}, {"#p": "pk"}), + ("#p = :a AND #q = :b", {":a": A, ":b": B}, {"#p": "pk", "#q": "pk"}), + ( + "pk = :a AND #s = :b AND sk = :c", + {":a": A, ":b": B, ":c": C}, + {"#s": "sk"}, + ), + ], + ) + def test_duplicate_through_name_reference( + self, dynamodb_client, range_table, expr, values, names + ): + """An #name alias resolving to the same attribute is a duplicate.""" + code, msg = _error( + lambda: dynamodb_client.query( + TableName=range_table, + KeyConditionExpression=expr, + ExpressionAttributeValues=values, + ExpressionAttributeNames=names, + ) + ) + assert code == "ValidationException" + assert msg == ONE_PER_KEY + + def test_distinct_key_conditions_still_served(self, dynamodb_client, range_table): + """One condition per key attribute, in either order, returns rows.""" + pk_only = dynamodb_client.query( + TableName=range_table, + KeyConditionExpression="pk = :a", + ExpressionAttributeValues={":a": A}, + ) + assert pk_only["Count"] == 2 + + pk_and_sk = dynamodb_client.query( + TableName=range_table, + KeyConditionExpression="pk = :a AND sk = :b", + ExpressionAttributeValues={":a": A, ":b": B}, + ) + assert pk_and_sk["Count"] == 1 + + sk_first = dynamodb_client.query( + TableName=range_table, + KeyConditionExpression="sk = :b AND pk = :a", + ExpressionAttributeValues={":a": A, ":b": B}, + ) + assert sk_first["Count"] == 1 + + sk_range = dynamodb_client.query( + TableName=range_table, + KeyConditionExpression="pk = :a AND sk BETWEEN :b AND :c", + ExpressionAttributeValues={":a": A, ":b": B, ":c": C}, + ) + assert sk_range["Count"] == 2 + + def test_nested_path_reported_ahead_of_duplicate( + self, dynamodb_client, range_table + ): + code, msg = _error( + lambda: dynamodb_client.query( + TableName=range_table, + KeyConditionExpression="pk = :a AND pk.foo = :b", + ExpressionAttributeValues={":a": A, ":b": B}, + ) + ) + assert code == "ValidationException" + assert msg == NESTED + + def test_redundant_parens_reported_ahead_of_duplicate( + self, dynamodb_client, range_table + ): + code, msg = _error( + lambda: dynamodb_client.query( + TableName=range_table, + KeyConditionExpression="((pk = :a)) AND (pk = :b)", + ExpressionAttributeValues={":a": A, ":b": B}, + ) + ) + assert code == "ValidationException" + assert msg == REDUNDANT_PARENS + + def test_undefined_name_reported_ahead_of_duplicate( + self, dynamodb_client, range_table + ): + """Two conditions on an undefined #name: the name is reported.""" + code, msg = _error( + lambda: dynamodb_client.query( + TableName=range_table, + KeyConditionExpression="#u = :a AND #u = :b", + ExpressionAttributeValues={":a": A, ":b": B}, + ) + ) + assert code == "ValidationException" + assert UNDEFINED_NAME in msg + + +class TestDuplicateCheckOrder: + """Where the duplicate is reported against the other checks (2026-10-31).""" + + @pytest.mark.parametrize( + ("expr", "values"), + [ + ("#u = :a AND sk = :b AND sk = :c", {":a": A, ":b": B, ":c": C}), + ("pk = :a AND pk = :b AND #u = :c", {":a": A, ":b": B, ":c": C}), + ("#u = :a AND #u > :b", {":a": A, ":b": B}), + ], + ) + def test_undefined_name_in_any_clause_reported_ahead_of_duplicate( + self, dynamodb_client, range_table, expr, values + ): + """No ExpressionAttributeNames: the undefined #u is reported.""" + code, msg = _error( + lambda: dynamodb_client.query( + TableName=range_table, + KeyConditionExpression=expr, + ExpressionAttributeValues=values, + ) + ) + assert code == "ValidationException" + assert msg == UNDEFINED_NAME_EXACT + + def test_undefined_value_reported_ahead_of_duplicate( + self, dynamodb_client, range_table + ): + """No ExpressionAttributeValues: the first undefined value is reported.""" + code, msg = _error( + lambda: dynamodb_client.query( + TableName=range_table, + KeyConditionExpression="pk = :a AND pk = :b", + ) + ) + assert code == "ValidationException" + assert msg == UNDEFINED_VALUE_A + + @pytest.mark.parametrize( + ("expr", "values", "names"), + [ + # A number on the S key: the duplicate is reported ahead of the + # key value type check, in either spelling. + ("#p = :a AND pk = :n", {":a": A, ":n": N1}, {"#p": "pk"}), + ( + "pk = :a AND #s = :b AND sk = :n", + {":a": A, ":b": B, ":n": N1}, + {"#s": "sk"}, + ), + # An unused value: the duplicate is reported ahead of the + # unused-value check. + ("pk = :a AND pk = :b", {":a": A, ":b": B, ":z": {"S": "zed"}}, None), + ], + ) + def test_duplicate_reported_ahead_of_value_checks( + self, dynamodb_client, range_table, expr, values, names + ): + kwargs = { + "TableName": range_table, + "KeyConditionExpression": expr, + "ExpressionAttributeValues": values, + } + if names is not None: + kwargs["ExpressionAttributeNames"] = names + code, msg = _error(lambda: dynamodb_client.query(**kwargs)) + assert code == "ValidationException" + assert msg == ONE_PER_KEY