diff --git a/crates/core/src/expression/key_condition.rs b/crates/core/src/expression/key_condition.rs index edaa1f8e5..6b1cdeb8f 100755 --- a/crates/core/src/expression/key_condition.rs +++ b/crates/core/src/expression/key_condition.rs @@ -14,12 +14,11 @@ //! - No OR, NOT, nested conditions, or other functions use super::ast::{CompareOp, Expr, PathElement}; +use super::kind::ExpressionKind; use super::parser_common; +use super::syntax_error::{is_operator_or_keyword, syntax_error}; 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. /// @@ -42,450 +41,6 @@ pub struct KeyCondition { pub extra_sk_conditions: Vec<(Vec, Expr)>, } -impl KeyCondition { - /// Correct PK/SK assignment using the table's key schema. - /// - /// For single-attribute keys: when both the PK and SK conditions are equality - /// comparisons, the parser cannot distinguish which is which without knowing - /// the key schema. This method resolves `#name` references and swaps PK/SK - /// if the current PK path actually refers to the sort key attribute. - /// - /// For multi-part keys: all equality conditions are initially parsed as - /// PK + SK pairs. This method reclassifies them based on the key schema, - /// moving HASH attribute conditions to `extra_pk_conditions` and keeping - /// the RANGE condition as `sk_condition`. - /// - /// Call this after parsing and before passing to the storage layer. - /// - /// # Errors - /// - /// Returns `ValidationException` if a `#name` reference cannot be resolved. - pub fn resolve_pk_sk( - &mut self, - pk_attr: &str, - names: &std::collections::HashMap, - ) -> Result<(), DynamoDbError> { - // Only ambiguous when SK is an Eq comparison (both clauses were Eq). - let sk_is_eq = matches!( - self.sk_condition, - Some(SortKeyCondition::Compare { - op: CompareOp::Eq, - .. - }) - ); - if !sk_is_eq { - return Ok(()); - } - - // Resolve the current PK path's top-level attribute name. - let pk_name = match self.pk_path.first() { - Some(PathElement::Attribute(name)) => { - if let Some(ref_name) = name.strip_prefix('#') { - names.get(ref_name).map(String::as_str).ok_or_else(|| { - DynamoDbError::ValidationException(format!( - "Invalid KeyConditionExpression: An expression attribute name used in \ - the document path is not defined; attribute name: #{ref_name}" - )) - })? - } else { - name.as_str() - } - } - _ => return Ok(()), - }; - - // If the current PK path already matches the actual PK attribute, no swap needed. - if pk_name == pk_attr { - return Ok(()); - } - - // Swap: current PK is actually the SK, and the SK condition holds the real PK. - if let Some(SortKeyCondition::Compare { - path: sk_path, - op: CompareOp::Eq, - value: sk_value, - }) = self.sk_condition.take() - { - let old_pk_path = std::mem::replace(&mut self.pk_path, sk_path); - let old_pk_value = std::mem::replace(&mut self.pk_value, sk_value); - self.sk_condition = Some(SortKeyCondition::Compare { - path: old_pk_path, - op: CompareOp::Eq, - value: old_pk_value, - }); - } - - Ok(()) - } - - /// Classify and validate the conditions of a multi-attribute key schema. - /// - /// 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. - /// - /// 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> { - 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(()); - } - - // 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); - - // 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 => non_eq = Some(other), - } - } - - 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) { - 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 { - other_eqs.push((path, value)); - } - } - - // The non-equality clause's attribute: its RANGE position when it - // names a RANGE attribute, its HASH position when it names a HASH - // attribute, neither when it names a non-key attribute. - let non_eq_attr = match &non_eq { - Some(cond) => Some(resolve_path_attr(cond.path(), names)?), - None => None, - }; - let non_eq_pos = non_eq_attr - .as_deref() - .and_then(|a| range_attrs.iter().position(|r| *r == a)); - let non_eq_hash_pos = non_eq_attr - .as_deref() - .and_then(|a| hash_attrs.iter().position(|h| *h == a)); - - // One condition per key attribute. A second condition on one HASH or - // RANGE attribute, equality or not, is refused before any coverage - // check (measured on the service, 2026-10-21). - hash_eqs.sort_by_key(|(pos, _, _)| *pos); - range_eqs.sort_by_key(|(pos, _, _)| *pos); - let duplicated = hash_eqs.windows(2).any(|w| w[0].0 == w[1].0) - || range_eqs.windows(2).any(|w| w[0].0 == w[1].0) - || non_eq_pos.is_some_and(|p| range_eqs.iter().any(|(q, _, _)| *q == p)) - || non_eq_hash_pos.is_some_and(|p| hash_eqs.iter().any(|(q, _, _)| *q == p)); - if duplicated { - return Err(DynamoDbError::ValidationException( - "Invalid KeyConditionExpression: KeyConditionExpressions must only contain \ - one condition per key" - .to_string(), - )); - } - - // More conditions than key attributes (measured 2026-10-21: the count - // is reported ahead of which attribute is missing). - let condition_count = - hash_eqs.len() + range_eqs.len() + other_eqs.len() + usize::from(non_eq.is_some()); - let key_attribute_count = hash_attrs.len() + range_attrs.len(); - if condition_count > key_attribute_count { - return Err(DynamoDbError::ValidationException(format!( - "The number of query conditions ({condition_count}) exceeds the number of key \ - attributes defined in the schema ({key_attribute_count})" - ))); - } - - // 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 = hash_iter.collect(); - } - - 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 = range_eqs; - - Ok(()) - } - - /// Assign every clause to its key attribute under `key_schema`, the key - /// schema of the table or index the query reads. - /// - /// A multi-attribute key schema goes through - /// [`Self::resolve_multipart_with`], which classifies every clause by name, - /// applies the service's rules for such a schema and calls `value_check` - /// where the service reports a value of the wrong type. On any other key - /// schema `value_check` runs on every clause first, which is where the - /// service reports it for such a schema (measured 2026-10-21), and then - /// [`Self::resolve_pk_sk`] picks the partition key clause. - /// - /// The engine reads a `Query` key condition through this, and so does - /// authorization when it finds the partition key value for - /// `dynamodb:LeadingKeys`, so both read the same clause as the partition - /// key. On a multi-attribute key schema `pk_path` then names the first - /// HASH attribute. - /// - /// # Errors - /// - /// As [`Self::resolve_multipart_with`] on a multi-attribute key schema and - /// as [`Self::resolve_pk_sk`] on any other, plus whatever `value_check` - /// returns. - pub fn resolve_key_schema_with( - &mut self, - key_schema: &[KeySchemaElement], - names: &std::collections::HashMap, - mut value_check: F, - ) -> Result<(), DynamoDbError> - where - F: FnMut(&[PathElement], &Expr) -> Result<(), DynamoDbError>, - { - if is_multipart_key_schema(key_schema) { - let hash_attrs: Vec<&str> = hash_key_elements(key_schema) - .iter() - .map(|ks| ks.attribute_name.as_str()) - .collect(); - let range_attrs: Vec<&str> = range_key_elements(key_schema) - .iter() - .map(|ks| ks.attribute_name.as_str()) - .collect(); - return self.resolve_multipart_with(&hash_attrs, &range_attrs, names, value_check); - } - self.for_each_clause(&mut value_check)?; - match key_schema.iter().find(|ks| ks.key_type == KeyType::Hash) { - Some(hash) => self.resolve_pk_sk(&hash.attribute_name, names), - None => Ok(()), - } - } - - /// [`Self::resolve_key_schema_with`] without a value check. - /// - /// # Errors - /// - /// As [`Self::resolve_key_schema_with`]. - pub fn resolve_key_schema( - &mut self, - key_schema: &[KeySchemaElement], - names: &std::collections::HashMap, - ) -> Result<(), DynamoDbError> { - self.resolve_key_schema_with(key_schema, names, |_, _| Ok(())) - } - - /// Call `f` with the path and value of every clause: the partition key - /// clause, the extra HASH and RANGE equalities, and each operand of the - /// sort key condition. - fn for_each_clause(&self, f: &mut F) -> Result<(), DynamoDbError> - where - F: FnMut(&[PathElement], &Expr) -> Result<(), DynamoDbError>, - { - f(&self.pk_path, &self.pk_value)?; - for (path, value) in &self.extra_pk_conditions { - f(path, value)?; - } - for (path, value) in &self.extra_sk_conditions { - f(path, value)?; - } - match &self.sk_condition { - Some(SortKeyCondition::Compare { path, value, .. }) => f(path, value), - Some(SortKeyCondition::Between { path, low, high }) => { - f(path, low)?; - f(path, high) - } - Some(SortKeyCondition::BeginsWith { path, prefix }) => f(path, prefix), - None => Ok(()), - } - } -} - -/// Resolve a path's top-level attribute name, handling `#name` references. -fn resolve_path_attr( - path: &[PathElement], - names: &std::collections::HashMap, -) -> Result { - match path.first() { - Some(PathElement::Attribute(name)) => { - if let Some(ref_name) = name.strip_prefix('#') { - names.get(ref_name).cloned().ok_or_else(|| { - DynamoDbError::ValidationException(format!( - "Invalid KeyConditionExpression: An expression attribute name used in \ - the document path is not defined; attribute name: #{ref_name}" - )) - }) - } else { - Ok(name.clone()) - } - } - _ => Err(DynamoDbError::ValidationException( - "Invalid key condition path".to_owned(), - )), - } -} - /// Sort key condition variants. #[derive(Debug, Clone)] pub enum SortKeyCondition { @@ -530,7 +85,23 @@ impl SortKeyCondition { /// /// Returns `ValidationException` for syntax errors or unsupported constructs. pub fn parse_key_condition(tokens: &[Token]) -> Result { - let mut clauses = parse_clauses(tokens)?; + parse_key_condition_from(tokens, "") +} + +/// Parse a `KeyConditionExpression` whose tokens came from `source`, so a +/// syntax error can quote the source window around the offending token as +/// Amazon `DynamoDB` does (`pk = = :v` answers `Syntax error; token: "=", +/// near: "= = :v"`; `pk = :v AND` answers `token: "", near: "AND"`, +/// measured 2026-10-25). An empty `source` joins the token texts instead. +/// +/// # Errors +/// +/// Returns `ValidationException` for syntax errors or unsupported constructs. +pub fn parse_key_condition_from( + tokens: &[Token], + source: &str, +) -> Result { + let mut clauses = parse_clauses(tokens, source)?; match clauses.len() { 1 => classify_single(clauses.remove(0)), @@ -555,7 +126,7 @@ pub fn parse_key_condition(tokens: &[Token]) -> Result Result, DynamoDbError> { - Ok(parse_clauses(tokens)? + Ok(parse_clauses(tokens, "")? .into_iter() .filter_map(|clause| match clause { RawClause::Eq(path, value) => Some(KeyClauseOperand { @@ -589,25 +160,35 @@ pub struct KeyClauseOperand { /// Parse a `KeyConditionExpression` token stream into its AND-ed clauses, /// before any of them is read as the partition or the sort key condition. -fn parse_clauses(tokens: &[Token]) -> Result, DynamoDbError> { +/// A syntax error quotes `source`, the text the tokens came from; an empty +/// `source` joins the token texts instead. +fn parse_clauses(tokens: &[Token], source: &str) -> Result, DynamoDbError> { parser_common::check_redundant_parens(tokens) .map_err(|body| validation_err(&format!("Invalid KeyConditionExpression: {body}")))?; // Strip outer parentheses: "(pk = :pk AND sk > :sk)" → "pk = :pk AND sk > :sk" - let tokens = if tokens.len() >= 2 + let all = tokens; + let (tokens, offset) = if tokens.len() >= 2 && tokens[0] == Token::LParen && tokens[tokens.len() - 1] == Token::RParen && outer_parens_match(tokens) { - &tokens[1..tokens.len() - 1] + (&tokens[1..tokens.len() - 1], 1) } else { - tokens + (tokens, 0) + }; + // Syntax errors quote the window from the full token list, so a position + // inside the stripped slice is shifted back by the stripped parenthesis. + let src = Source { + text: source, + all, + offset, }; let mut pos = 0; let mut clauses = Vec::new(); - clauses.push(parse_key_clause(tokens, &mut pos)?); + clauses.push(parse_key_clause(tokens, &src, &mut pos)?); while pos < tokens.len() { if tokens[pos] != Token::And { @@ -616,7 +197,7 @@ fn parse_clauses(tokens: &[Token]) -> Result, DynamoDbError> { )); } pos += 1; - clauses.push(parse_key_clause(tokens, &mut pos)?); + clauses.push(parse_key_clause(tokens, &src, &mut pos)?); } if pos < tokens.len() { @@ -635,6 +216,27 @@ fn parse_clauses(tokens: &[Token]) -> Result, DynamoDbError> { Ok(clauses) } +/// The expression text and full token list a clause parser reports syntax +/// errors against; `offset` is the number of tokens stripped before the slice +/// being parsed. +struct Source<'a> { + text: &'a str, + all: &'a [Token], + offset: usize, +} + +impl Source<'_> { + /// The service's syntax error for the token at `pos` of the parsed slice. + fn syntax_error(&self, pos: usize) -> DynamoDbError { + syntax_error( + ExpressionKind::KeyCondition, + self.text, + self.all, + pos + self.offset, + ) + } +} + /// The attribute-side path of a raw clause. fn raw_clause_path(clause: &RawClause) -> &[PathElement] { match clause { @@ -777,17 +379,25 @@ fn raw_to_sk(clause: RawClause) -> SortKeyCondition { } } -fn parse_key_clause(tokens: &[Token], pos: &mut usize) -> Result { +fn parse_key_clause( + tokens: &[Token], + src: &Source<'_>, + pos: &mut usize, +) -> Result { if *pos >= tokens.len() { - return Err(validation_err( - "Invalid KeyConditionExpression: unexpected end of expression", - )); + if tokens.is_empty() { + return Err(validation_err( + "Invalid KeyConditionExpression: unexpected end of expression", + )); + } + // Input ends where a clause is required (`pk = :v AND`). + return Err(src.syntax_error(*pos)); } // Allow one level of parentheses around a single clause if tokens[*pos] == Token::LParen { *pos += 1; - let clause = parse_key_clause_inner(tokens, pos)?; + let clause = parse_key_clause_inner(tokens, src, pos)?; if *pos < tokens.len() && tokens[*pos] == Token::RParen { *pos += 1; return Ok(clause); @@ -797,14 +407,16 @@ fn parse_key_clause(tokens: &[Token], pos: &mut usize) -> Result Result { +fn parse_key_clause_inner( + tokens: &[Token], + src: &Source<'_>, + pos: &mut usize, +) -> Result { if *pos >= tokens.len() { - return Err(validation_err( - "Invalid KeyConditionExpression: unexpected end of expression", - )); + return Err(src.syntax_error(*pos)); } // begins_with(path, :val) @@ -815,15 +427,15 @@ fn parse_key_clause_inner(tokens: &[Token], pos: &mut usize) -> Result= tokens.len() { return Err(validation_err( @@ -846,7 +458,7 @@ fn parse_key_clause_inner(tokens: &[Token], pos: &mut usize) -> Result Result Result Result Result { +fn parse_key_operand( + tokens: &[Token], + src: &Source<'_>, + pos: &mut usize, +) -> Result { if *pos >= tokens.len() { - return Err(validation_err( - "Invalid KeyConditionExpression: unexpected end of expression", - )); + return Err(src.syntax_error(*pos)); } match &tokens[*pos] { @@ -903,9 +517,11 @@ fn parse_key_operand(tokens: &[Token], pos: &mut usize) -> Result { - let value = parse_key_value(tokens, pos)?; + let value = parse_key_value(tokens, src, pos)?; Ok(KeyOperand::Value(value)) } + // An operator or keyword where an operand is required (`AND pk = :v`). + token if is_operator_or_keyword(token) => Err(src.syntax_error(*pos)), _ => Err(validation_err( "Invalid KeyConditionExpression: expected attribute path or value placeholder", )), @@ -914,27 +530,30 @@ fn parse_key_operand(tokens: &[Token], pos: &mut usize) -> Result, pos: &mut usize, ) -> Result, DynamoDbError> { if *pos >= tokens.len() { - return Err(validation_err( - "Invalid KeyConditionExpression: expected attribute path", - )); + return Err(src.syntax_error(*pos)); } match &tokens[*pos] { Token::Ident(_) | Token::NameRef(_) => parser_common::parse_path(tokens, pos), + token if is_operator_or_keyword(token) => Err(src.syntax_error(*pos)), _ => Err(validation_err( "Invalid KeyConditionExpression: expected attribute path", )), } } -fn parse_key_value(tokens: &[Token], pos: &mut usize) -> Result { +fn parse_key_value( + tokens: &[Token], + src: &Source<'_>, + pos: &mut usize, +) -> Result { if *pos >= tokens.len() { - return Err(validation_err( - "Invalid KeyConditionExpression: expected value", - )); + // Input ends where a value is required (`pk =`). + return Err(src.syntax_error(*pos)); } match &tokens[*pos] { Token::Placeholder(name) => { @@ -942,6 +561,9 @@ fn parse_key_value(tokens: &[Token], pos: &mut usize) -> Result Err(src.syntax_error(*pos)), _ => Err(validation_err( "Invalid KeyConditionExpression: key condition values must be expression attribute value placeholders", )), @@ -995,911 +617,5 @@ fn validation_err(msg: &str) -> DynamoDbError { } #[cfg(test)] -mod tests { - use super::*; - use crate::expression::tokenize; - use std::collections::HashMap; - - #[test] - fn key_condition_redundant_parens_rejected_with_canonical_message() { - for expr in ["((pk = :v))", "((#pk = :pk)) AND (#sk = :sk)"] { - let tokens = tokenize(expr).unwrap(); - let err = parse_key_condition(&tokens).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) - if msg == "Invalid KeyConditionExpression: The expression has redundant parentheses;"), - "expr {expr}: got {err:?}" - ); - } - } - - #[test] - fn key_condition_valid_parens_accepted() { - for expr in [ - "(pk = :v)", - "(#pk = :pk) AND (#sk = :sk)", - "(#pk = :pk AND #sk = :sk)", - "(#pk = :pk AND (#sk = :sk))", - ] { - let tokens = tokenize(expr).unwrap(); - assert!( - parse_key_condition(&tokens).is_ok(), - "expr {expr} should parse" - ); - } - } - - #[test] - fn resolve_pk_sk_swaps_when_reversed() { - // "sk = :sk AND pk = :pk" — parser assigns first Eq as PK - let tokens = tokenize("sk = :sk AND pk = :pk").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("sk".to_owned())]); - - kc.resolve_pk_sk("pk", &HashMap::new()).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - match &kc.sk_condition { - Some(SortKeyCondition::Compare { - path, - op: CompareOp::Eq, - .. - }) => { - assert_eq!(path, &vec![PathElement::Attribute("sk".to_owned())]); - } - other => panic!("expected Eq SK condition, got {other:?}"), - } - } - - #[test] - fn resolve_pk_sk_no_swap_when_correct() { - let tokens = tokenize("pk = :pk AND sk = :sk").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - - kc.resolve_pk_sk("pk", &HashMap::new()).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - } - - #[test] - fn resolve_pk_sk_with_name_refs() { - let tokens = tokenize("#s = :sk AND #p = :pk").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - - let mut names = HashMap::new(); - names.insert("s".to_owned(), "sortkey".to_owned()); - names.insert("p".to_owned(), "partkey".to_owned()); - - kc.resolve_pk_sk("partkey", &names).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("#p".to_owned())]); - } - - #[test] - fn resolve_pk_sk_noop_for_non_eq_sk() { - // "pk = :pk AND sk > :val" — SK is not Eq, no ambiguity - let tokens = tokenize("pk = :pk AND sk > :val").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - kc.resolve_pk_sk("pk", &HashMap::new()).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - } - - #[test] - fn resolve_pk_sk_noop_for_single_clause() { - let tokens = tokenize("pk = :pk").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - kc.resolve_pk_sk("pk", &HashMap::new()).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - } - - #[test] - fn parse_three_clause_multipart() { - // pk1 = :v1 AND pk2 = :v2 AND sk = :v3 - let tokens = tokenize("pk1 = :v1 AND pk2 = :v2 AND sk = :v3").unwrap(); - let kc = parse_key_condition(&tokens).unwrap(); - // First clause is pk_path, one extra_pk, one sk - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk1".to_owned())]); - assert_eq!(kc.extra_pk_conditions.len(), 1); - assert!(kc.sk_condition.is_some()); - } - - #[test] - fn parse_four_clause_multipart() { - let tokens = tokenize("a = :a AND b = :b AND c = :c AND d = :d").unwrap(); - let kc = parse_key_condition(&tokens).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("a".to_owned())]); - assert_eq!(kc.extra_pk_conditions.len(), 2); - assert!(kc.sk_condition.is_some()); - } - - #[test] - fn parse_multipart_with_range_condition() { - // pk1 = :v1 AND pk2 = :v2 AND sk > :v3 - let tokens = tokenize("pk1 = :v1 AND pk2 = :v2 AND sk > :v3").unwrap(); - let kc = parse_key_condition(&tokens).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk1".to_owned())]); - // pk2 is in extra_pk_conditions (will be reclassified by resolve_multipart) - assert_eq!(kc.extra_pk_conditions.len(), 1); - match &kc.sk_condition { - Some(SortKeyCondition::Compare { - op: CompareOp::Gt, .. - }) => {} - other => panic!("expected Gt SK condition, got {other:?}"), - } - } - - #[test] - fn parse_reversed_sort_key_comparison() { - let tokens = tokenize("pk = :pk AND :lo <= sk").unwrap(); - let kc = parse_key_condition(&tokens).unwrap(); - - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - match &kc.sk_condition { - Some(SortKeyCondition::Compare { - path, - op: CompareOp::Ge, - value: Expr::Placeholder(name), - }) => { - assert_eq!(path, &vec![PathElement::Attribute("sk".to_owned())]); - assert_eq!(name, "lo"); - } - other => panic!("expected reversed Ge SK condition, got {other:?}"), - } - } - - #[test] - fn parse_reversed_partition_key_equality() { - let tokens = tokenize(":pk = pk").unwrap(); - let kc = parse_key_condition(&tokens).unwrap(); - - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - assert_eq!(kc.pk_value, Expr::Placeholder("pk".to_owned())); - } - - #[test] - fn reverse_comparator_maps_each_operator_and_is_involutive() { - use CompareOp::{Eq, Ge, Gt, Le, Lt, Ne}; - - let cases = [(Eq, Eq), (Ne, Ne), (Lt, Gt), (Le, Ge), (Gt, Lt), (Ge, Le)]; - - for (op, reversed) in cases { - assert_eq!(reverse_comparator(op), reversed); - assert_eq!(reverse_comparator(reverse_comparator(op)), op); - } - } - - #[test] - fn resolve_multipart_reclassifies() { - 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"], &["sk1"], &names) - .unwrap(); - - // After resolve_multipart, pk_path + extra_pk should have pk1 and pk2 - let pk_count = 1 + kc.extra_pk_conditions.len(); - assert_eq!(pk_count, 2); - // sk_condition should be the sk1 equality - assert!(kc.sk_condition.is_some()); - } - - #[test] - fn resolve_multipart_with_range() { - 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"], &["sk1"], &names) - .unwrap(); - - assert_eq!(1 + kc.extra_pk_conditions.len(), 2); - match &kc.sk_condition { - Some(SortKeyCondition::Compare { - op: CompareOp::Gt, .. - }) => {} - other => panic!("expected Gt SK condition, got {other:?}"), - } - } - - #[test] - fn resolve_multipart_reorders_to_schema_order() { - // Expression order: pk2, pk1, sk — reversed from key schema order [pk1, pk2] - 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"], &["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())]); - // extra_pk_conditions should have pk2 - assert_eq!(kc.extra_pk_conditions.len(), 1); - assert_eq!( - kc.extra_pk_conditions[0].0, - vec![PathElement::Attribute("pk2".to_owned())] - ); - } - - #[test] - fn resolve_multipart_multi_range() { - // 2 HASH + 2 RANGE, all equality conditions - 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"], &["sk1", "sk2"], &names) - .unwrap(); - - // 2 PK conditions - assert_eq!(1 + kc.extra_pk_conditions.len(), 2); - // Primary SK condition is sk1 - match &kc.sk_condition { - Some(SortKeyCondition::Compare { - path, - op: CompareOp::Eq, - .. - }) => { - assert_eq!(path, &vec![PathElement::Attribute("sk1".to_owned())]); - } - other => panic!("expected Eq SK condition on sk1, got {other:?}"), - } - // Extra SK condition is sk2 - assert_eq!(kc.extra_sk_conditions.len(), 1); - assert_eq!( - kc.extra_sk_conditions[0].0, - vec![PathElement::Attribute("sk2".to_owned())] - ); - } - - const NESTED_KEY_ERR: &str = "Invalid KeyConditionExpression: KeyConditionExpressions cannot have conditions on nested attributes"; - - #[test] - fn nested_dot_path_on_key_rejected() { - for expr in [ - "pk.foo = :v", - "pk = :p AND sk.foo = :v", - "begins_with(sk.foo, :v)", - "foo.bar = :v", - ] { - let tokens = tokenize(expr).unwrap(); - let err = parse_key_condition(&tokens).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if msg == NESTED_KEY_ERR), - "expr {expr}: got {err:?}" - ); - } - } - - #[test] - fn nested_path_in_all_clause_shapes_rejected() { - // Exercises every `raw_clause_path` arm (Compare/Between/BeginsWith) and a - // 3-clause multi-part shape with a nested middle clause. Multi-part keys - // are an ExtendDB extension real DynamoDB cannot express, hence a - // parser-level test. The reject loop runs before classification, so a - // nested clause in any position is rejected up front. - for expr in [ - "pk.foo > :v", // single non-eq (Compare arm) - "pk = :p AND sk.foo BETWEEN :a AND :b", // Between arm - "pk = :p AND begins_with(sk[0], :v)", // BeginsWith arm, index - "pk = :p AND :v < sk.foo", // reversed value-first - "pk1 = :v1 AND pk2.x = :v2 AND sk = :v3", // multi-part, nested middle - ] { - let tokens = tokenize(expr).unwrap(); - let err = parse_key_condition(&tokens).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if msg == NESTED_KEY_ERR), - "expr {expr}: got {err:?}" - ); - } - } - - #[test] - fn index_path_on_key_rejected() { - for expr in ["pk[0] = :v", "pk = :p AND sk[0] = :v"] { - let tokens = tokenize(expr).unwrap(); - let err = parse_key_condition(&tokens).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if msg == NESTED_KEY_ERR), - "expr {expr}: got {err:?}" - ); - } - } - - #[test] - fn nested_path_via_name_placeholder_rejected() { - // The check is structural (path length), so it fires before #name - // resolution. - let tokens = tokenize("#d.foo = :v").unwrap(); - let err = parse_key_condition(&tokens).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if msg == NESTED_KEY_ERR), - "got {err:?}" - ); - } - - #[test] - fn top_level_key_paths_accepted() { - for expr in ["pk = :v", "pk = :p AND sk = :s", "#pk = :p AND #sk = :s"] { - let tokens = tokenize(expr).unwrap(); - assert!( - parse_key_condition(&tokens).is_ok(), - "expr {expr} should parse" - ); - } - } - - /// One expected clause: attribute, operand placeholder, equality. - type ExpectedClause = (&'static str, &'static str, bool); - - /// The attribute and operand of each clause, and whether it is an - /// equality, which is what authorization reads to find the partition value - /// of a key condition the engine rejects for its operator. - #[test] - fn clause_operands_name_every_single_operand_clause() { - let cases: &[(&str, &[ExpectedClause])] = &[ - ("pk = :v", &[("pk", "v", true)]), - ( - "pk = :v AND sk = :s", - &[("pk", "v", true), ("sk", "s", true)], - ), - ("pk > :v", &[("pk", "v", false)]), - (":v > pk", &[("pk", "v", false)]), - ("begins_with(pk, :v)", &[("pk", "v", false)]), - ("(pk >= :v)", &[("pk", "v", false)]), - ("#p <= :v", &[("#p", "v", false)]), - ( - "pk = :v AND sk < :s", - &[("pk", "v", true), ("sk", "s", false)], - ), - ( - "pk > :v AND sk = :s", - &[("pk", "v", false), ("sk", "s", true)], - ), - ( - "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: Vec<(String, String, bool)> = got - .into_iter() - .map(|clause| { - let name = match &clause.path[0] { - PathElement::Attribute(n) => n.clone(), - PathElement::Index(i) => format!("[{i}]"), - }; - let value = match clause.value { - Expr::Placeholder(n) => n, - other => format!("{other:?}"), - }; - (name, value, clause.equality) - }) - .collect(); - let want: Vec<(String, String, bool)> = want - .iter() - .map(|(a, v, e)| ((*a).to_owned(), (*v).to_owned(), *e)) - .collect(); - assert_eq!(got, want, "expr {expr}"); - } - } - - /// The same syntax errors and unsupported constructs `parse_key_condition` - /// rejects before classification. - #[test] - fn clause_operands_reject_what_the_parser_rejects() { - for expr in [ - "pk :: :v", - "pk > :v OR sk = :s", - "((pk = :v))", - "pk.sub > :v", - "pk <> :v", - "pk >", - ] { - let Ok(tokens) = tokenize(expr) else { - continue; - }; - assert!( - key_condition_clause_operands(&tokens).is_err(), - "expr {expr} should be rejected" - ); - } - } - - /// The index key schema the single-HASH cases use: HASH `h`, then RANGE - /// `a`, `b`, `c`, `d`. - const ONE_HASH: [&str; 1] = ["h"]; - const FOUR_RANGE: [&str; 4] = ["a", "b", "c", "d"]; - - /// Parse `expr` and classify it against `h` + `a, b, c, d`. - fn classify_one_hash(expr: &str) -> Result { - classify_with_names(expr, &HashMap::new()) - } - - fn classify_with_names( - expr: &str, - names: &HashMap, - ) -> Result { - let tokens = tokenize(expr).unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - kc.resolve_multipart(&ONE_HASH, &FOUR_RANGE, names)?; - Ok(kc) - } - - /// The attribute names of `extra_sk_conditions`, in order. - fn extra_sk_names(kc: &KeyCondition) -> Vec { - kc.extra_sk_conditions - .iter() - .map(|(path, _)| match path.first() { - Some(PathElement::Attribute(name)) => name.clone(), - other => panic!("unexpected path {other:?}"), - }) - .collect() - } - - /// The sort-key condition's attribute and a label for its operator. - fn sk_shape(kc: &KeyCondition) -> Option<(String, &'static str)> { - let cond = kc.sk_condition.as_ref()?; - let name = match cond.path().first() { - Some(PathElement::Attribute(name)) => name.clone(), - other => panic!("unexpected path {other:?}"), - }; - let op = match cond { - SortKeyCondition::Compare { op, .. } => match op { - CompareOp::Eq => "=", - CompareOp::Ne => "<>", - CompareOp::Lt => "<", - CompareOp::Le => "<=", - CompareOp::Gt => ">", - CompareOp::Ge => ">=", - }, - SortKeyCondition::Between { .. } => "BETWEEN", - SortKeyCondition::BeginsWith { .. } => "begins_with", - }; - Some((name, op)) - } - - fn assert_range_gap(expr: &str, without_eq: &str, present: &str) { - let err = classify_one_hash(expr).unwrap_err(); - let want = format!( - "RANGE key attributes {without_eq} must have equality conditions specified in the \ - query because a condition is present on key attribute {present}" - ); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), - "expr {expr}: got {err:?}, want {want}" - ); - } - - #[test] - fn one_hash_multi_range_hash_only_condition_is_accepted() { - let kc = classify_one_hash("h = :h").unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("h".to_owned())]); - assert!(kc.extra_pk_conditions.is_empty()); - assert!(kc.sk_condition.is_none()); - assert!(kc.extra_sk_conditions.is_empty()); - } - - #[test] - fn one_hash_multi_range_equality_prefixes_are_accepted() { - // Equalities on a leading run of the RANGE attributes, up to all four. - for (expr, extra) in [ - ("h = :h AND a = :a", vec![]), - ("h = :h AND a = :a AND b = :b", vec!["b"]), - ("h = :h AND a = :a AND b = :b AND c = :c", vec!["b", "c"]), - ( - "h = :h AND a = :a AND b = :b AND c = :c AND d = :d", - vec!["b", "c", "d"], - ), - ] { - let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); - assert_eq!( - kc.pk_path, - vec![PathElement::Attribute("h".to_owned())], - "{expr}" - ); - assert!(kc.extra_pk_conditions.is_empty(), "{expr}"); - assert_eq!(sk_shape(&kc), Some(("a".to_owned(), "=")), "{expr}"); - assert_eq!(extra_sk_names(&kc), extra, "{expr}"); - } - } - - #[test] - fn one_hash_multi_range_clause_order_is_free() { - // The defect: the first clause was taken as the partition key, so - // `b = :b AND h = :h AND a = :a` was refused as missing `h`. - for expr in [ - "b = :b AND h = :h AND a = :a", - "a = :a AND b = :b AND h = :h", - "b = :b AND a = :a AND h = :h", - ] { - let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); - assert_eq!( - kc.pk_path, - vec![PathElement::Attribute("h".to_owned())], - "{expr}" - ); - assert_eq!(sk_shape(&kc), Some(("a".to_owned(), "=")), "{expr}"); - assert_eq!(extra_sk_names(&kc), vec!["b"], "{expr}"); - } - } - - #[test] - fn one_hash_multi_range_operator_on_the_last_given_attribute_is_accepted() { - for (expr, sk, extra) in [ - ("h = :h AND a > :a", ("a", ">"), vec![]), - ("h = :h AND a = :a AND b < :b", ("b", "<"), vec!["a"]), - ( - "h = :h AND a = :a AND b = :b AND c > :c", - ("c", ">"), - vec!["a", "b"], - ), - ( - "h = :h AND a = :a AND begins_with(b, :b)", - ("b", "begins_with"), - vec!["a"], - ), - ( - "h = :h AND a = :a AND b = :b AND c = :c AND begins_with(d, :d)", - ("d", "begins_with"), - vec!["a", "b", "c"], - ), - ( - "h = :h AND a = :a AND b = :b AND c = :c AND d BETWEEN :x AND :y", - ("d", "BETWEEN"), - vec!["a", "b", "c"], - ), - ( - "c > :c AND b = :b AND h = :h AND a = :a", - ("c", ">"), - vec!["a", "b"], - ), - ] { - let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); - assert_eq!( - kc.pk_path, - vec![PathElement::Attribute("h".to_owned())], - "{expr}" - ); - assert_eq!(sk_shape(&kc), Some((sk.0.to_owned(), sk.1)), "{expr}"); - assert_eq!(extra_sk_names(&kc), extra, "{expr}"); - } - } - - #[test] - fn one_hash_multi_range_gap_is_refused() { - // A condition on a RANGE attribute needs an equality on every RANGE - // attribute before it. The defect accepted `h = :h AND b = :b` and - // returned no items. - assert_range_gap("h = :h AND b = :b", "a", "b"); - assert_range_gap("h = :h AND a = :a AND c = :c", "b", "c"); - assert_range_gap("h = :h AND a = :a AND b = :b AND d > :d", "c", "d"); - } - - #[test] - fn one_hash_multi_range_operator_before_a_later_condition_is_refused() { - assert_range_gap("h = :h AND a = :a AND b > :b AND c = :c", "b", "c"); - assert_range_gap("h = :h AND begins_with(a, :a) AND b = :b", "a", "b"); - } - - #[test] - fn one_hash_multi_range_missing_hash_is_refused() { - for expr in [ - "a = :a AND b = :b AND c = :c", - "a = :a AND b = :b", - "a = :a", - ] { - let err = classify_one_hash(expr).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) - if msg == "HASH key attributes h must have equality conditions specified in the query"), - "expr {expr}: got {err:?}" - ); - } - } - - /// Classify against HASH `h1`, `h2` and RANGE `r`. - fn classify_two_hash(expr: &str) -> Result { - let tokens = tokenize(expr).unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - kc.resolve_multipart(&["h1", "h2"], &["r"], &HashMap::new())?; - Ok(kc) - } - - #[test] - fn two_hash_missing_hash_names_every_missing_attribute() { - for (expr, missing) in [ - ("h1 = :x", "h2"), - ("h2 = :x", "h1"), - ("h1 = :x AND r = :y", "h2"), - ("r = :y", "h1, h2"), - ("h1 = :x AND h2 > :y", "h2"), - ] { - let err = classify_two_hash(expr).unwrap_err(); - let want = format!( - "HASH key attributes {missing} must have equality conditions specified in the query" - ); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), - "expr {expr}: got {err:?}" - ); - } - assert!(classify_two_hash("h1 = :x AND h2 = :y").is_ok()); - assert!(classify_two_hash("h1 = :x AND h2 = :y AND r > :z").is_ok()); - } - - #[test] - fn a_second_condition_on_one_key_attribute_is_refused() { - const WANT: &str = "Invalid KeyConditionExpression: KeyConditionExpressions must only \ - contain one condition per key"; - for expr in [ - "h = :x AND h = :y", - "h = :x AND h = :y AND a = :z", - "h = :x AND a = :y AND a = :z", - "h = :h AND a = :a AND a > :x", - "h = :h AND a > :x AND a = :a", - "h = :h AND a = :a AND b = :n AND b > :lo", - "h = :h AND h > :x", - "h = :h AND h = :y AND c = :c", - "a = :a AND a = :y", - ] { - let err = classify_one_hash(expr).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if msg == WANT), - "expr {expr}: got {err:?}" - ); - } - for expr in [ - "h1 = :x AND h1 = :y AND h2 = :z", - "h1 = :x AND h1 = :y", - "h1 = :x AND h2 = :n AND r = :r AND r > :y", - "h1 = :x AND r = :r AND r = :y", - ] { - let err = classify_two_hash(expr).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if msg == WANT), - "expr {expr}: got {err:?}" - ); - } - } - - #[test] - fn a_condition_on_a_non_key_attribute_names_the_first_uncovered_range_attribute() { - for (expr, missing) in [ - ("h = :h AND z = :z", "a"), - ("h = :h AND z = :z AND y = :y", "a"), - ("h = :h AND z > :z", "a"), - ("h = :h AND a = :a AND z = :z", "b"), - ("h = :h AND a = :a AND z > :z", "b"), - ("h = :h AND a > :a AND z = :z", "b"), - ("h = :h AND a = :a AND b = :b AND z > :z", "c"), - ("h = :h AND a = :a AND b = :b AND c = :c AND z = :z", "d"), - ] { - let err = classify_one_hash(expr).unwrap_err(); - let want = format!("Query condition missed key schema element: {missing}"); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), - "expr {expr}: got {err:?}" - ); - } - let err = classify_two_hash("h1 = :x AND h2 = :y AND z = :z").unwrap_err(); - assert!(matches!(&err, DynamoDbError::ValidationException(msg) - if msg == "Query condition missed key schema element: r")); - // A gap is reported before the non-key attribute. - let err = classify_one_hash("h = :h AND b = :b AND z = :z").unwrap_err(); - assert!(matches!(&err, DynamoDbError::ValidationException(msg) - if msg.starts_with("RANGE key attributes a must have equality conditions"))); - // A missing HASH attribute is reported before the non-key attribute. - let err = classify_two_hash("h1 = :x AND z = :z").unwrap_err(); - assert!(matches!(&err, DynamoDbError::ValidationException(msg) - if msg == "HASH key attributes h2 must have equality conditions specified in the query")); - } - - #[test] - fn more_conditions_than_key_attributes_are_refused_by_count() { - for (expr, n) in [ - ( - "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND z = :z", - 6, - ), - ( - "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND z > :z", - 6, - ), - ( - "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND PK = :p", - 6, - ), - ] { - let err = classify_one_hash(expr).unwrap_err(); - let want = format!( - "The number of query conditions ({n}) exceeds the number of key attributes \ - defined in the schema (5)" - ); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), - "expr {expr}: got {err:?}" - ); - } - let err = classify_two_hash("h1 = :x AND h2 = :y AND r = :r AND z = :z").unwrap_err(); - assert!(matches!(&err, DynamoDbError::ValidationException(msg) - if msg == "The number of query conditions (4) exceeds the number of key attributes defined in the schema (3)")); - } - - #[test] - fn the_count_is_refused_before_a_missing_hash_attribute_or_a_range_gap() { - // Each count case above has every key attribute, so they also pass - // with the count check moved after the missing-HASH and RANGE-gap - // checks. These lack the HASH attribute or leave a RANGE gap. - let count = |n: usize, attrs: usize| { - format!( - "The number of query conditions ({n}) exceeds the number of key attributes \ - defined in the schema ({attrs})" - ) - }; - for expr in [ - "a = :a AND b = :b AND c = :c AND d = :d AND z1 = :z AND z2 = :y", - "h = :h AND b = :b AND c = :c AND d = :d AND z1 = :z AND z2 = :y", - ] { - let err = classify_one_hash(expr).unwrap_err(); - let want = count(6, 5); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), - "expr {expr}: got {err:?}" - ); - } - let expr = "a = :a AND b = :b AND c = :c AND z1 = :z AND z2 = :y"; - let err = classify_two_hash(expr).unwrap_err(); - let want = count(5, 3); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), - "expr {expr}: got {err:?}" - ); - // The same five clauses on HASH `h` and RANGE `a`, `b`, `c`, `d` do not - // exceed its five key attributes, so the missing HASH attribute is - // reported. - let err = classify_one_hash(expr).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) - if msg == "HASH key attributes h must have equality conditions specified in the query"), - "expr {expr}: got {err:?}" - ); - } - - #[test] - fn one_hash_multi_range_resolves_name_references() { - let names: HashMap = [("hh", "h"), ("aa", "a"), ("bb", "b")] - .into_iter() - .map(|(k, v)| (k.to_owned(), v.to_owned())) - .collect(); - let kc = classify_with_names("#bb > :b AND #hh = :h AND #aa = :a", &names).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("#hh".to_owned())]); - assert_eq!(sk_shape(&kc), Some(("#bb".to_owned(), ">"))); - assert_eq!(extra_sk_names(&kc), vec!["#aa"]); - - let err = classify_with_names("#hh = :h AND #bb = :b", &names).unwrap_err(); - assert!( - matches!(&err, DynamoDbError::ValidationException(msg) - if msg.starts_with("RANGE key attributes a must have")), - "got {err:?}" - ); - } - - #[test] - fn one_hash_one_range_schema_is_left_unchanged() { - let tokens = tokenize("sk = :s AND pk = :p").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - kc.resolve_multipart(&["pk"], &["sk"], &HashMap::new()) - .unwrap(); - // Still the parser's assignment; resolve_pk_sk handles this schema. - assert_eq!(kc.pk_path, vec![PathElement::Attribute("sk".to_owned())]); - } - - #[test] - fn multi_hash_partition_key_need_not_come_first() { - // With two HASH attributes the first clause used to be checked as the - // first HASH attribute before reclassification, so this order was - // refused as missing `pk1`. - let tokens = tokenize("sk = :s AND pk1 = :a AND pk2 = :b").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - kc.resolve_multipart(&["pk1", "pk2"], &["sk"], &HashMap::new()) - .unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk1".to_owned())]); - assert_eq!( - kc.extra_pk_conditions[0].0, - vec![PathElement::Attribute("pk2".to_owned())] - ); - assert_eq!(sk_shape(&kc), Some(("sk".to_owned(), "="))); - assert!(kc.extra_sk_conditions.is_empty()); - } - - #[test] - fn multi_hash_multi_range_operator_after_equality_keeps_both() { - // The non-equality on the second RANGE attribute is the sort-key - // condition, and the equality on the first is carried with it. - let tokens = tokenize("pk1 = :a AND pk2 = :b AND s1 = :x AND s2 > :y").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - kc.resolve_multipart(&["pk1", "pk2"], &["s1", "s2"], &HashMap::new()) - .unwrap(); - assert_eq!(sk_shape(&kc), Some(("s2".to_owned(), ">"))); - assert_eq!(extra_sk_names(&kc), vec!["s1"]); - } - - fn key(name: &str, key_type: KeyType) -> KeySchemaElement { - KeySchemaElement { - attribute_name: name.to_owned(), - key_type, - } - } - - #[test] - fn resolve_key_schema_reads_a_multi_attribute_schema_by_name() { - // Authorization reads the partition key value from `pk_path` after this - // call, so it must name the first HASH attribute for every clause order - // the engine serves. - let two_hash = [ - key("g1", KeyType::Hash), - key("g2", KeyType::Hash), - key("gs", KeyType::Range), - ]; - let two_range = [ - key("h", KeyType::Hash), - key("a", KeyType::Range), - key("b", KeyType::Range), - ]; - for (schema, expr) in [ - (&two_hash[..], "g2 = :g AND g1 = :v AND gs = :x"), - (&two_hash[..], "gs = :x AND g1 = :v AND g2 = :g"), - (&two_hash[..], "g2 = :g AND g1 = :v AND gs > :lo"), - (&two_range[..], "a = :a AND h = :v AND b = :c"), - (&two_range[..], "b = :c AND h = :v AND a = :a"), - (&two_range[..], "b > :lo AND a = :a AND h = :v"), - (&two_range[..], "begins_with(b, :p) AND a = :a AND h = :v"), - ] { - let tokens = tokenize(expr).unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - kc.resolve_key_schema(schema, &HashMap::new()) - .unwrap_or_else(|e| panic!("{expr}: {e:?}")); - let hash = &schema[0].attribute_name; - assert_eq!( - kc.pk_path, - vec![PathElement::Attribute(hash.clone())], - "{expr}" - ); - assert_eq!(kc.pk_value, Expr::Placeholder("v".to_owned()), "{expr}"); - } - } - - #[test] - fn resolve_key_schema_checks_values_before_resolve_pk_sk() { - let schema = [key("pk", KeyType::Hash), key("sk", KeyType::Range)]; - let tokens = tokenize("sk BETWEEN :lo AND :hi AND pk = :p").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - let mut checked = Vec::new(); - kc.resolve_key_schema_with(&schema, &HashMap::new(), |_, value| { - checked.push(format!("{value:?}")); - Ok(()) - }) - .unwrap(); - assert_eq!(checked.len(), 3, "{checked:?}"); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - - let tokens = tokenize("sk = :s AND pk = :p").unwrap(); - let mut kc = parse_key_condition(&tokens).unwrap(); - let err = kc - .resolve_key_schema_with(&schema, &HashMap::new(), |_, _| { - Err(validation_err("checked")) - }) - .unwrap_err(); - assert!(matches!(&err, DynamoDbError::ValidationException(msg) if msg == "checked")); - // The failed check left the parser's assignment in place. - assert_eq!(kc.pk_path, vec![PathElement::Attribute("sk".to_owned())]); - - kc.resolve_key_schema(&schema, &HashMap::new()).unwrap(); - assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); - } -} +#[path = "key_condition_tests.rs"] +mod tests; diff --git a/crates/core/src/expression/key_condition_resolve.rs b/crates/core/src/expression/key_condition_resolve.rs new file mode 100644 index 000000000..8ce4b3813 --- /dev/null +++ b/crates/core/src/expression/key_condition_resolve.rs @@ -0,0 +1,458 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Key schema resolution for a parsed [`KeyCondition`]: correcting the +//! partition and sort key assignment once the table's key schema is known. + +use std::collections::HashMap; + +use super::ast::{CompareOp, Expr, PathElement}; +use super::key_condition::{KeyCondition, SortKeyCondition}; +use crate::error::DynamoDbError; +use crate::types::{ + KeySchemaElement, KeyType, hash_key_elements, is_multipart_key_schema, range_key_elements, +}; + +impl KeyCondition { + /// Correct PK/SK assignment using the table's key schema. + /// + /// For single-attribute keys: when both the PK and SK conditions are equality + /// comparisons, the parser cannot distinguish which is which without knowing + /// the key schema. This method resolves `#name` references and swaps PK/SK + /// if the current PK path actually refers to the sort key attribute. + /// + /// For multi-part keys: all equality conditions are initially parsed as + /// PK + SK pairs. This method reclassifies them based on the key schema, + /// moving HASH attribute conditions to `extra_pk_conditions` and keeping + /// the RANGE condition as `sk_condition`. + /// + /// Call this after parsing and before passing to the storage layer. + /// + /// # Errors + /// + /// Returns `ValidationException` if a `#name` reference cannot be resolved. + pub fn resolve_pk_sk( + &mut self, + pk_attr: &str, + names: &HashMap, + ) -> Result<(), DynamoDbError> { + // Only ambiguous when SK is an Eq comparison (both clauses were Eq). + let sk_is_eq = matches!( + self.sk_condition, + Some(SortKeyCondition::Compare { + op: CompareOp::Eq, + .. + }) + ); + if !sk_is_eq { + return Ok(()); + } + + // Resolve the current PK path's top-level attribute name. + let pk_name = match self.pk_path.first() { + Some(PathElement::Attribute(name)) => { + if let Some(ref_name) = name.strip_prefix('#') { + names.get(ref_name).map(String::as_str).ok_or_else(|| { + DynamoDbError::ValidationException(format!( + "Invalid KeyConditionExpression: An expression attribute name used in \ + the document path is not defined; attribute name: #{ref_name}" + )) + })? + } else { + name.as_str() + } + } + _ => return Ok(()), + }; + + // If the current PK path already matches the actual PK attribute, no swap needed. + if pk_name == pk_attr { + return Ok(()); + } + + // Swap: current PK is actually the SK, and the SK condition holds the real PK. + if let Some(SortKeyCondition::Compare { + path: sk_path, + op: CompareOp::Eq, + value: sk_value, + }) = self.sk_condition.take() + { + let old_pk_path = std::mem::replace(&mut self.pk_path, sk_path); + let old_pk_value = std::mem::replace(&mut self.pk_value, sk_value); + self.sk_condition = Some(SortKeyCondition::Compare { + path: old_pk_path, + op: CompareOp::Eq, + value: old_pk_value, + }); + } + + Ok(()) + } + + /// Classify and validate the conditions of a multi-attribute key schema. + /// + /// 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. + /// + /// 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: &HashMap, + ) -> Result<(), DynamoDbError> { + 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: &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(()); + } + + // 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); + + // 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 => non_eq = Some(other), + } + } + + 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) { + 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 { + other_eqs.push((path, value)); + } + } + + // The non-equality clause's attribute: its RANGE position when it + // names a RANGE attribute, its HASH position when it names a HASH + // attribute, neither when it names a non-key attribute. + let non_eq_attr = match &non_eq { + Some(cond) => Some(resolve_path_attr(cond.path(), names)?), + None => None, + }; + let non_eq_pos = non_eq_attr + .as_deref() + .and_then(|a| range_attrs.iter().position(|r| *r == a)); + let non_eq_hash_pos = non_eq_attr + .as_deref() + .and_then(|a| hash_attrs.iter().position(|h| *h == a)); + + // One condition per key attribute. A second condition on one HASH or + // RANGE attribute, equality or not, is refused before any coverage + // check (measured on the service, 2026-10-21). + hash_eqs.sort_by_key(|(pos, _, _)| *pos); + range_eqs.sort_by_key(|(pos, _, _)| *pos); + let duplicated = hash_eqs.windows(2).any(|w| w[0].0 == w[1].0) + || range_eqs.windows(2).any(|w| w[0].0 == w[1].0) + || non_eq_pos.is_some_and(|p| range_eqs.iter().any(|(q, _, _)| *q == p)) + || non_eq_hash_pos.is_some_and(|p| hash_eqs.iter().any(|(q, _, _)| *q == p)); + if duplicated { + return Err(DynamoDbError::ValidationException( + "Invalid KeyConditionExpression: KeyConditionExpressions must only contain \ + one condition per key" + .to_string(), + )); + } + + // More conditions than key attributes (measured 2026-10-21: the count + // is reported ahead of which attribute is missing). + let condition_count = + hash_eqs.len() + range_eqs.len() + other_eqs.len() + usize::from(non_eq.is_some()); + let key_attribute_count = hash_attrs.len() + range_attrs.len(); + if condition_count > key_attribute_count { + return Err(DynamoDbError::ValidationException(format!( + "The number of query conditions ({condition_count}) exceeds the number of key \ + attributes defined in the schema ({key_attribute_count})" + ))); + } + + // 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 = hash_iter.collect(); + } + + 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 = range_eqs; + + Ok(()) + } + + /// Assign every clause to its key attribute under `key_schema`, the key + /// schema of the table or index the query reads. + /// + /// A multi-attribute key schema goes through + /// [`Self::resolve_multipart_with`], which classifies every clause by name, + /// applies the service's rules for such a schema and calls `value_check` + /// where the service reports a value of the wrong type. On any other key + /// schema `value_check` runs on every clause first, which is where the + /// service reports it for such a schema (measured 2026-10-21), and then + /// [`Self::resolve_pk_sk`] picks the partition key clause. + /// + /// The engine reads a `Query` key condition through this, and so does + /// authorization when it finds the partition key value for + /// `dynamodb:LeadingKeys`, so both read the same clause as the partition + /// key. On a multi-attribute key schema `pk_path` then names the first + /// HASH attribute. + /// + /// # Errors + /// + /// As [`Self::resolve_multipart_with`] on a multi-attribute key schema and + /// as [`Self::resolve_pk_sk`] on any other, plus whatever `value_check` + /// returns. + pub fn resolve_key_schema_with( + &mut self, + key_schema: &[KeySchemaElement], + names: &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: &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. +fn resolve_path_attr( + path: &[PathElement], + names: &HashMap, +) -> Result { + match path.first() { + Some(PathElement::Attribute(name)) => { + if let Some(ref_name) = name.strip_prefix('#') { + names.get(ref_name).cloned().ok_or_else(|| { + DynamoDbError::ValidationException(format!( + "Invalid KeyConditionExpression: An expression attribute name used in \ + the document path is not defined; attribute name: #{ref_name}" + )) + }) + } else { + Ok(name.clone()) + } + } + _ => Err(DynamoDbError::ValidationException( + "Invalid key condition path".to_owned(), + )), + } +} diff --git a/crates/core/src/expression/key_condition_tests.rs b/crates/core/src/expression/key_condition_tests.rs new file mode 100644 index 000000000..cbb9308c6 --- /dev/null +++ b/crates/core/src/expression/key_condition_tests.rs @@ -0,0 +1,997 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +use super::*; +use crate::expression::tokenize; +use crate::types::{KeySchemaElement, KeyType}; +use std::collections::HashMap; + +#[test] +fn key_condition_redundant_parens_rejected_with_canonical_message() { + for expr in ["((pk = :v))", "((#pk = :pk)) AND (#sk = :sk)"] { + let tokens = tokenize(expr).unwrap(); + let err = parse_key_condition(&tokens).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Invalid KeyConditionExpression: The expression has redundant parentheses;"), + "expr {expr}: got {err:?}" + ); + } +} + +#[test] +fn key_condition_valid_parens_accepted() { + for expr in [ + "(pk = :v)", + "(#pk = :pk) AND (#sk = :sk)", + "(#pk = :pk AND #sk = :sk)", + "(#pk = :pk AND (#sk = :sk))", + ] { + let tokens = tokenize(expr).unwrap(); + assert!( + parse_key_condition(&tokens).is_ok(), + "expr {expr} should parse" + ); + } +} + +#[test] +fn resolve_pk_sk_swaps_when_reversed() { + // "sk = :sk AND pk = :pk" — parser assigns first Eq as PK + let tokens = tokenize("sk = :sk AND pk = :pk").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("sk".to_owned())]); + + kc.resolve_pk_sk("pk", &HashMap::new()).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); + match &kc.sk_condition { + Some(SortKeyCondition::Compare { + path, + op: CompareOp::Eq, + .. + }) => { + assert_eq!(path, &vec![PathElement::Attribute("sk".to_owned())]); + } + other => panic!("expected Eq SK condition, got {other:?}"), + } +} + +#[test] +fn resolve_pk_sk_no_swap_when_correct() { + let tokens = tokenize("pk = :pk AND sk = :sk").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); + + kc.resolve_pk_sk("pk", &HashMap::new()).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); +} + +#[test] +fn resolve_pk_sk_with_name_refs() { + let tokens = tokenize("#s = :sk AND #p = :pk").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + + let mut names = HashMap::new(); + names.insert("s".to_owned(), "sortkey".to_owned()); + names.insert("p".to_owned(), "partkey".to_owned()); + + kc.resolve_pk_sk("partkey", &names).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("#p".to_owned())]); +} + +#[test] +fn resolve_pk_sk_noop_for_non_eq_sk() { + // "pk = :pk AND sk > :val" — SK is not Eq, no ambiguity + let tokens = tokenize("pk = :pk AND sk > :val").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_pk_sk("pk", &HashMap::new()).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); +} + +#[test] +fn resolve_pk_sk_noop_for_single_clause() { + let tokens = tokenize("pk = :pk").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_pk_sk("pk", &HashMap::new()).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); +} + +#[test] +fn parse_three_clause_multipart() { + // pk1 = :v1 AND pk2 = :v2 AND sk = :v3 + let tokens = tokenize("pk1 = :v1 AND pk2 = :v2 AND sk = :v3").unwrap(); + let kc = parse_key_condition(&tokens).unwrap(); + // First clause is pk_path, one extra_pk, one sk + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk1".to_owned())]); + assert_eq!(kc.extra_pk_conditions.len(), 1); + assert!(kc.sk_condition.is_some()); +} + +#[test] +fn parse_four_clause_multipart() { + let tokens = tokenize("a = :a AND b = :b AND c = :c AND d = :d").unwrap(); + let kc = parse_key_condition(&tokens).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("a".to_owned())]); + assert_eq!(kc.extra_pk_conditions.len(), 2); + assert!(kc.sk_condition.is_some()); +} + +#[test] +fn parse_multipart_with_range_condition() { + // pk1 = :v1 AND pk2 = :v2 AND sk > :v3 + let tokens = tokenize("pk1 = :v1 AND pk2 = :v2 AND sk > :v3").unwrap(); + let kc = parse_key_condition(&tokens).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk1".to_owned())]); + // pk2 is in extra_pk_conditions (will be reclassified by resolve_multipart) + assert_eq!(kc.extra_pk_conditions.len(), 1); + match &kc.sk_condition { + Some(SortKeyCondition::Compare { + op: CompareOp::Gt, .. + }) => {} + other => panic!("expected Gt SK condition, got {other:?}"), + } +} + +#[test] +fn parse_reversed_sort_key_comparison() { + let tokens = tokenize("pk = :pk AND :lo <= sk").unwrap(); + let kc = parse_key_condition(&tokens).unwrap(); + + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); + match &kc.sk_condition { + Some(SortKeyCondition::Compare { + path, + op: CompareOp::Ge, + value: Expr::Placeholder(name), + }) => { + assert_eq!(path, &vec![PathElement::Attribute("sk".to_owned())]); + assert_eq!(name, "lo"); + } + other => panic!("expected reversed Ge SK condition, got {other:?}"), + } +} + +#[test] +fn parse_reversed_partition_key_equality() { + let tokens = tokenize(":pk = pk").unwrap(); + let kc = parse_key_condition(&tokens).unwrap(); + + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); + assert_eq!(kc.pk_value, Expr::Placeholder("pk".to_owned())); +} + +#[test] +fn reverse_comparator_maps_each_operator_and_is_involutive() { + use CompareOp::{Eq, Ge, Gt, Le, Lt, Ne}; + + let cases = [(Eq, Eq), (Ne, Ne), (Lt, Gt), (Le, Ge), (Gt, Lt), (Ge, Le)]; + + for (op, reversed) in cases { + assert_eq!(reverse_comparator(op), reversed); + assert_eq!(reverse_comparator(reverse_comparator(op)), op); + } +} + +#[test] +fn resolve_multipart_reclassifies() { + 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"], &["sk1"], &names) + .unwrap(); + + // After resolve_multipart, pk_path + extra_pk should have pk1 and pk2 + let pk_count = 1 + kc.extra_pk_conditions.len(); + assert_eq!(pk_count, 2); + // sk_condition should be the sk1 equality + assert!(kc.sk_condition.is_some()); +} + +#[test] +fn resolve_multipart_with_range() { + 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"], &["sk1"], &names) + .unwrap(); + + assert_eq!(1 + kc.extra_pk_conditions.len(), 2); + match &kc.sk_condition { + Some(SortKeyCondition::Compare { + op: CompareOp::Gt, .. + }) => {} + other => panic!("expected Gt SK condition, got {other:?}"), + } +} + +#[test] +fn resolve_multipart_reorders_to_schema_order() { + // Expression order: pk2, pk1, sk — reversed from key schema order [pk1, pk2] + 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"], &["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())]); + // extra_pk_conditions should have pk2 + assert_eq!(kc.extra_pk_conditions.len(), 1); + assert_eq!( + kc.extra_pk_conditions[0].0, + vec![PathElement::Attribute("pk2".to_owned())] + ); +} + +#[test] +fn resolve_multipart_multi_range() { + // 2 HASH + 2 RANGE, all equality conditions + 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"], &["sk1", "sk2"], &names) + .unwrap(); + + // 2 PK conditions + assert_eq!(1 + kc.extra_pk_conditions.len(), 2); + // Primary SK condition is sk1 + match &kc.sk_condition { + Some(SortKeyCondition::Compare { + path, + op: CompareOp::Eq, + .. + }) => { + assert_eq!(path, &vec![PathElement::Attribute("sk1".to_owned())]); + } + other => panic!("expected Eq SK condition on sk1, got {other:?}"), + } + // Extra SK condition is sk2 + assert_eq!(kc.extra_sk_conditions.len(), 1); + assert_eq!( + kc.extra_sk_conditions[0].0, + vec![PathElement::Attribute("sk2".to_owned())] + ); +} + +const NESTED_KEY_ERR: &str = "Invalid KeyConditionExpression: KeyConditionExpressions cannot have conditions on nested attributes"; + +#[test] +fn nested_dot_path_on_key_rejected() { + for expr in [ + "pk.foo = :v", + "pk = :p AND sk.foo = :v", + "begins_with(sk.foo, :v)", + "foo.bar = :v", + ] { + let tokens = tokenize(expr).unwrap(); + let err = parse_key_condition(&tokens).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg == NESTED_KEY_ERR), + "expr {expr}: got {err:?}" + ); + } +} + +#[test] +fn nested_path_in_all_clause_shapes_rejected() { + // Exercises every `raw_clause_path` arm (Compare/Between/BeginsWith) and a + // 3-clause multi-part shape with a nested middle clause. Multi-part keys + // are an ExtendDB extension real DynamoDB cannot express, hence a + // parser-level test. The reject loop runs before classification, so a + // nested clause in any position is rejected up front. + for expr in [ + "pk.foo > :v", // single non-eq (Compare arm) + "pk = :p AND sk.foo BETWEEN :a AND :b", // Between arm + "pk = :p AND begins_with(sk[0], :v)", // BeginsWith arm, index + "pk = :p AND :v < sk.foo", // reversed value-first + "pk1 = :v1 AND pk2.x = :v2 AND sk = :v3", // multi-part, nested middle + ] { + let tokens = tokenize(expr).unwrap(); + let err = parse_key_condition(&tokens).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg == NESTED_KEY_ERR), + "expr {expr}: got {err:?}" + ); + } +} + +#[test] +fn index_path_on_key_rejected() { + for expr in ["pk[0] = :v", "pk = :p AND sk[0] = :v"] { + let tokens = tokenize(expr).unwrap(); + let err = parse_key_condition(&tokens).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg == NESTED_KEY_ERR), + "expr {expr}: got {err:?}" + ); + } +} + +#[test] +fn nested_path_via_name_placeholder_rejected() { + // The check is structural (path length), so it fires before #name + // resolution. + let tokens = tokenize("#d.foo = :v").unwrap(); + let err = parse_key_condition(&tokens).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg == NESTED_KEY_ERR), + "got {err:?}" + ); +} + +#[test] +fn top_level_key_paths_accepted() { + for expr in ["pk = :v", "pk = :p AND sk = :s", "#pk = :p AND #sk = :s"] { + let tokens = tokenize(expr).unwrap(); + assert!( + parse_key_condition(&tokens).is_ok(), + "expr {expr} should parse" + ); + } +} + +/// One expected clause: attribute, operand placeholder, equality. +type ExpectedClause = (&'static str, &'static str, bool); + +/// The attribute and operand of each clause, and whether it is an +/// equality, which is what authorization reads to find the partition value +/// of a key condition the engine rejects for its operator. +#[test] +fn clause_operands_name_every_single_operand_clause() { + let cases: &[(&str, &[ExpectedClause])] = &[ + ("pk = :v", &[("pk", "v", true)]), + ( + "pk = :v AND sk = :s", + &[("pk", "v", true), ("sk", "s", true)], + ), + ("pk > :v", &[("pk", "v", false)]), + (":v > pk", &[("pk", "v", false)]), + ("begins_with(pk, :v)", &[("pk", "v", false)]), + ("(pk >= :v)", &[("pk", "v", false)]), + ("#p <= :v", &[("#p", "v", false)]), + ( + "pk = :v AND sk < :s", + &[("pk", "v", true), ("sk", "s", false)], + ), + ( + "pk > :v AND sk = :s", + &[("pk", "v", false), ("sk", "s", true)], + ), + ( + "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: Vec<(String, String, bool)> = got + .into_iter() + .map(|clause| { + let name = match &clause.path[0] { + PathElement::Attribute(n) => n.clone(), + PathElement::Index(i) => format!("[{i}]"), + }; + let value = match clause.value { + Expr::Placeholder(n) => n, + other => format!("{other:?}"), + }; + (name, value, clause.equality) + }) + .collect(); + let want: Vec<(String, String, bool)> = want + .iter() + .map(|(a, v, e)| ((*a).to_owned(), (*v).to_owned(), *e)) + .collect(); + assert_eq!(got, want, "expr {expr}"); + } +} + +/// The same syntax errors and unsupported constructs `parse_key_condition` +/// rejects before classification, with the same error: both read the clauses +/// through one parser. +#[test] +fn clause_operands_reject_what_the_parser_rejects() { + for expr in [ + "pk :: :v", + "pk > :v OR sk = :s", + "((pk = :v))", + "pk.sub > :v", + "pk <> :v", + "pk >", + ] { + let Ok(tokens) = tokenize(expr) else { + continue; + }; + let Err(operands_err) = key_condition_clause_operands(&tokens) else { + panic!("expr {expr} should be rejected"); + }; + let Err(parser_err) = parse_key_condition(&tokens) else { + panic!("expr {expr} should be rejected by parse_key_condition"); + }; + assert_eq!( + format!("{operands_err:?}"), + format!("{parser_err:?}"), + "expr {expr}" + ); + } +} + +/// The index key schema the single-HASH cases use: HASH `h`, then RANGE +/// `a`, `b`, `c`, `d`. +const ONE_HASH: [&str; 1] = ["h"]; +const FOUR_RANGE: [&str; 4] = ["a", "b", "c", "d"]; + +/// Parse `expr` and classify it against `h` + `a, b, c, d`. +fn classify_one_hash(expr: &str) -> Result { + classify_with_names(expr, &HashMap::new()) +} + +fn classify_with_names( + expr: &str, + names: &HashMap, +) -> Result { + let tokens = tokenize(expr).unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&ONE_HASH, &FOUR_RANGE, names)?; + Ok(kc) +} + +/// The attribute names of `extra_sk_conditions`, in order. +fn extra_sk_names(kc: &KeyCondition) -> Vec { + kc.extra_sk_conditions + .iter() + .map(|(path, _)| match path.first() { + Some(PathElement::Attribute(name)) => name.clone(), + other => panic!("unexpected path {other:?}"), + }) + .collect() +} + +/// The sort-key condition's attribute and a label for its operator. +fn sk_shape(kc: &KeyCondition) -> Option<(String, &'static str)> { + let cond = kc.sk_condition.as_ref()?; + let name = match cond.path().first() { + Some(PathElement::Attribute(name)) => name.clone(), + other => panic!("unexpected path {other:?}"), + }; + let op = match cond { + SortKeyCondition::Compare { op, .. } => match op { + CompareOp::Eq => "=", + CompareOp::Ne => "<>", + CompareOp::Lt => "<", + CompareOp::Le => "<=", + CompareOp::Gt => ">", + CompareOp::Ge => ">=", + }, + SortKeyCondition::Between { .. } => "BETWEEN", + SortKeyCondition::BeginsWith { .. } => "begins_with", + }; + Some((name, op)) +} + +fn assert_range_gap(expr: &str, without_eq: &str, present: &str) { + let err = classify_one_hash(expr).unwrap_err(); + let want = format!( + "RANGE key attributes {without_eq} must have equality conditions specified in the \ + query because a condition is present on key attribute {present}" + ); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}, want {want}" + ); +} + +#[test] +fn one_hash_multi_range_hash_only_condition_is_accepted() { + let kc = classify_one_hash("h = :h").unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("h".to_owned())]); + assert!(kc.extra_pk_conditions.is_empty()); + assert!(kc.sk_condition.is_none()); + assert!(kc.extra_sk_conditions.is_empty()); +} + +#[test] +fn one_hash_multi_range_equality_prefixes_are_accepted() { + // Equalities on a leading run of the RANGE attributes, up to all four. + for (expr, extra) in [ + ("h = :h AND a = :a", vec![]), + ("h = :h AND a = :a AND b = :b", vec!["b"]), + ("h = :h AND a = :a AND b = :b AND c = :c", vec!["b", "c"]), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d", + vec!["b", "c", "d"], + ), + ] { + let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); + assert_eq!( + kc.pk_path, + vec![PathElement::Attribute("h".to_owned())], + "{expr}" + ); + assert!(kc.extra_pk_conditions.is_empty(), "{expr}"); + assert_eq!(sk_shape(&kc), Some(("a".to_owned(), "=")), "{expr}"); + assert_eq!(extra_sk_names(&kc), extra, "{expr}"); + } +} + +#[test] +fn one_hash_multi_range_clause_order_is_free() { + // The defect: the first clause was taken as the partition key, so + // `b = :b AND h = :h AND a = :a` was refused as missing `h`. + for expr in [ + "b = :b AND h = :h AND a = :a", + "a = :a AND b = :b AND h = :h", + "b = :b AND a = :a AND h = :h", + ] { + let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); + assert_eq!( + kc.pk_path, + vec![PathElement::Attribute("h".to_owned())], + "{expr}" + ); + assert_eq!(sk_shape(&kc), Some(("a".to_owned(), "=")), "{expr}"); + assert_eq!(extra_sk_names(&kc), vec!["b"], "{expr}"); + } +} + +#[test] +fn one_hash_multi_range_operator_on_the_last_given_attribute_is_accepted() { + for (expr, sk, extra) in [ + ("h = :h AND a > :a", ("a", ">"), vec![]), + ("h = :h AND a = :a AND b < :b", ("b", "<"), vec!["a"]), + ( + "h = :h AND a = :a AND b = :b AND c > :c", + ("c", ">"), + vec!["a", "b"], + ), + ( + "h = :h AND a = :a AND begins_with(b, :b)", + ("b", "begins_with"), + vec!["a"], + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND begins_with(d, :d)", + ("d", "begins_with"), + vec!["a", "b", "c"], + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d BETWEEN :x AND :y", + ("d", "BETWEEN"), + vec!["a", "b", "c"], + ), + ( + "c > :c AND b = :b AND h = :h AND a = :a", + ("c", ">"), + vec!["a", "b"], + ), + ] { + let kc = classify_one_hash(expr).unwrap_or_else(|e| panic!("{expr}: {e:?}")); + assert_eq!( + kc.pk_path, + vec![PathElement::Attribute("h".to_owned())], + "{expr}" + ); + assert_eq!(sk_shape(&kc), Some((sk.0.to_owned(), sk.1)), "{expr}"); + assert_eq!(extra_sk_names(&kc), extra, "{expr}"); + } +} + +#[test] +fn one_hash_multi_range_gap_is_refused() { + // A condition on a RANGE attribute needs an equality on every RANGE + // attribute before it. The defect accepted `h = :h AND b = :b` and + // returned no items. + assert_range_gap("h = :h AND b = :b", "a", "b"); + assert_range_gap("h = :h AND a = :a AND c = :c", "b", "c"); + assert_range_gap("h = :h AND a = :a AND b = :b AND d > :d", "c", "d"); +} + +#[test] +fn one_hash_multi_range_operator_before_a_later_condition_is_refused() { + assert_range_gap("h = :h AND a = :a AND b > :b AND c = :c", "b", "c"); + assert_range_gap("h = :h AND begins_with(a, :a) AND b = :b", "a", "b"); +} + +#[test] +fn one_hash_multi_range_missing_hash_is_refused() { + for expr in [ + "a = :a AND b = :b AND c = :c", + "a = :a AND b = :b", + "a = :a", + ] { + let err = classify_one_hash(expr).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "HASH key attributes h must have equality conditions specified in the query"), + "expr {expr}: got {err:?}" + ); + } +} + +/// Classify against HASH `h1`, `h2` and RANGE `r`. +fn classify_two_hash(expr: &str) -> Result { + let tokens = tokenize(expr).unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&["h1", "h2"], &["r"], &HashMap::new())?; + Ok(kc) +} + +#[test] +fn two_hash_missing_hash_names_every_missing_attribute() { + for (expr, missing) in [ + ("h1 = :x", "h2"), + ("h2 = :x", "h1"), + ("h1 = :x AND r = :y", "h2"), + ("r = :y", "h1, h2"), + ("h1 = :x AND h2 > :y", "h2"), + ] { + let err = classify_two_hash(expr).unwrap_err(); + let want = format!( + "HASH key attributes {missing} must have equality conditions specified in the query" + ); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + } + assert!(classify_two_hash("h1 = :x AND h2 = :y").is_ok()); + assert!(classify_two_hash("h1 = :x AND h2 = :y AND r > :z").is_ok()); +} + +#[test] +fn a_second_condition_on_one_key_attribute_is_refused() { + const WANT: &str = "Invalid KeyConditionExpression: KeyConditionExpressions must only \ + contain one condition per key"; + for expr in [ + "h = :x AND h = :y", + "h = :x AND h = :y AND a = :z", + "h = :x AND a = :y AND a = :z", + "h = :h AND a = :a AND a > :x", + "h = :h AND a > :x AND a = :a", + "h = :h AND a = :a AND b = :n AND b > :lo", + "h = :h AND h > :x", + "h = :h AND h = :y AND c = :c", + "a = :a AND a = :y", + ] { + let err = classify_one_hash(expr).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg == WANT), + "expr {expr}: got {err:?}" + ); + } + for expr in [ + "h1 = :x AND h1 = :y AND h2 = :z", + "h1 = :x AND h1 = :y", + "h1 = :x AND h2 = :n AND r = :r AND r > :y", + "h1 = :x AND r = :r AND r = :y", + ] { + let err = classify_two_hash(expr).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if msg == WANT), + "expr {expr}: got {err:?}" + ); + } +} + +#[test] +fn a_condition_on_a_non_key_attribute_names_the_first_uncovered_range_attribute() { + for (expr, missing) in [ + ("h = :h AND z = :z", "a"), + ("h = :h AND z = :z AND y = :y", "a"), + ("h = :h AND z > :z", "a"), + ("h = :h AND a = :a AND z = :z", "b"), + ("h = :h AND a = :a AND z > :z", "b"), + ("h = :h AND a > :a AND z = :z", "b"), + ("h = :h AND a = :a AND b = :b AND z > :z", "c"), + ("h = :h AND a = :a AND b = :b AND c = :c AND z = :z", "d"), + ] { + let err = classify_one_hash(expr).unwrap_err(); + let want = format!("Query condition missed key schema element: {missing}"); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + } + let err = classify_two_hash("h1 = :x AND h2 = :y AND z = :z").unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Query condition missed key schema element: r")); + // A gap is reported before the non-key attribute. + let err = classify_one_hash("h = :h AND b = :b AND z = :z").unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) + if msg.starts_with("RANGE key attributes a must have equality conditions"))); + // A missing HASH attribute is reported before the non-key attribute. + let err = classify_two_hash("h1 = :x AND z = :z").unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "HASH key attributes h2 must have equality conditions specified in the query")); +} + +#[test] +fn more_conditions_than_key_attributes_are_refused_by_count() { + for (expr, n) in [ + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND z = :z", + 6, + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND z > :z", + 6, + ), + ( + "h = :h AND a = :a AND b = :b AND c = :c AND d = :d AND PK = :p", + 6, + ), + ] { + let err = classify_one_hash(expr).unwrap_err(); + let want = format!( + "The number of query conditions ({n}) exceeds the number of key attributes \ + defined in the schema (5)" + ); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + } + let err = classify_two_hash("h1 = :x AND h2 = :y AND r = :r AND z = :z").unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "The number of query conditions (4) exceeds the number of key attributes defined in the schema (3)")); +} + +#[test] +fn the_count_is_refused_before_a_missing_hash_attribute_or_a_range_gap() { + // Each count case above has every key attribute, so they also pass + // with the count check moved after the missing-HASH and RANGE-gap + // checks. These lack the HASH attribute or leave a RANGE gap. + let count = |n: usize, attrs: usize| { + format!( + "The number of query conditions ({n}) exceeds the number of key attributes \ + defined in the schema ({attrs})" + ) + }; + for expr in [ + "a = :a AND b = :b AND c = :c AND d = :d AND z1 = :z AND z2 = :y", + "h = :h AND b = :b AND c = :c AND d = :d AND z1 = :z AND z2 = :y", + ] { + let err = classify_one_hash(expr).unwrap_err(); + let want = count(6, 5); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + } + let expr = "a = :a AND b = :b AND c = :c AND z1 = :z AND z2 = :y"; + let err = classify_two_hash(expr).unwrap_err(); + let want = count(5, 3); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) if *msg == want), + "expr {expr}: got {err:?}" + ); + // The same five clauses on HASH `h` and RANGE `a`, `b`, `c`, `d` do not + // exceed its five key attributes, so the missing HASH attribute is + // reported. + let err = classify_one_hash(expr).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "HASH key attributes h must have equality conditions specified in the query"), + "expr {expr}: got {err:?}" + ); +} + +#[test] +fn one_hash_multi_range_resolves_name_references() { + let names: HashMap = [("hh", "h"), ("aa", "a"), ("bb", "b")] + .into_iter() + .map(|(k, v)| (k.to_owned(), v.to_owned())) + .collect(); + let kc = classify_with_names("#bb > :b AND #hh = :h AND #aa = :a", &names).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("#hh".to_owned())]); + assert_eq!(sk_shape(&kc), Some(("#bb".to_owned(), ">"))); + assert_eq!(extra_sk_names(&kc), vec!["#aa"]); + + let err = classify_with_names("#hh = :h AND #bb = :b", &names).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg.starts_with("RANGE key attributes a must have")), + "got {err:?}" + ); +} + +#[test] +fn one_hash_one_range_schema_is_left_unchanged() { + let tokens = tokenize("sk = :s AND pk = :p").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&["pk"], &["sk"], &HashMap::new()) + .unwrap(); + // Still the parser's assignment; resolve_pk_sk handles this schema. + assert_eq!(kc.pk_path, vec![PathElement::Attribute("sk".to_owned())]); +} + +#[test] +fn multi_hash_partition_key_need_not_come_first() { + // With two HASH attributes the first clause used to be checked as the + // first HASH attribute before reclassification, so this order was + // refused as missing `pk1`. + let tokens = tokenize("sk = :s AND pk1 = :a AND pk2 = :b").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&["pk1", "pk2"], &["sk"], &HashMap::new()) + .unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk1".to_owned())]); + assert_eq!( + kc.extra_pk_conditions[0].0, + vec![PathElement::Attribute("pk2".to_owned())] + ); + assert_eq!(sk_shape(&kc), Some(("sk".to_owned(), "="))); + assert!(kc.extra_sk_conditions.is_empty()); +} + +#[test] +fn multi_hash_multi_range_operator_after_equality_keeps_both() { + // The non-equality on the second RANGE attribute is the sort-key + // condition, and the equality on the first is carried with it. + let tokens = tokenize("pk1 = :a AND pk2 = :b AND s1 = :x AND s2 > :y").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_multipart(&["pk1", "pk2"], &["s1", "s2"], &HashMap::new()) + .unwrap(); + assert_eq!(sk_shape(&kc), Some(("s2".to_owned(), ">"))); + assert_eq!(extra_sk_names(&kc), vec!["s1"]); +} + +fn key(name: &str, key_type: KeyType) -> KeySchemaElement { + KeySchemaElement { + attribute_name: name.to_owned(), + key_type, + } +} + +#[test] +fn resolve_key_schema_reads_a_multi_attribute_schema_by_name() { + // Authorization reads the partition key value from `pk_path` after this + // call, so it must name the first HASH attribute for every clause order + // the engine serves. + let two_hash = [ + key("g1", KeyType::Hash), + key("g2", KeyType::Hash), + key("gs", KeyType::Range), + ]; + let two_range = [ + key("h", KeyType::Hash), + key("a", KeyType::Range), + key("b", KeyType::Range), + ]; + for (schema, expr) in [ + (&two_hash[..], "g2 = :g AND g1 = :v AND gs = :x"), + (&two_hash[..], "gs = :x AND g1 = :v AND g2 = :g"), + (&two_hash[..], "g2 = :g AND g1 = :v AND gs > :lo"), + (&two_range[..], "a = :a AND h = :v AND b = :c"), + (&two_range[..], "b = :c AND h = :v AND a = :a"), + (&two_range[..], "b > :lo AND a = :a AND h = :v"), + (&two_range[..], "begins_with(b, :p) AND a = :a AND h = :v"), + ] { + let tokens = tokenize(expr).unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + kc.resolve_key_schema(schema, &HashMap::new()) + .unwrap_or_else(|e| panic!("{expr}: {e:?}")); + let hash = &schema[0].attribute_name; + assert_eq!( + kc.pk_path, + vec![PathElement::Attribute(hash.clone())], + "{expr}" + ); + assert_eq!(kc.pk_value, Expr::Placeholder("v".to_owned()), "{expr}"); + } +} + +#[test] +fn resolve_key_schema_checks_values_before_resolve_pk_sk() { + let schema = [key("pk", KeyType::Hash), key("sk", KeyType::Range)]; + let tokens = tokenize("sk BETWEEN :lo AND :hi AND pk = :p").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + let mut checked = Vec::new(); + kc.resolve_key_schema_with(&schema, &HashMap::new(), |_, value| { + checked.push(format!("{value:?}")); + Ok(()) + }) + .unwrap(); + assert_eq!(checked.len(), 3, "{checked:?}"); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); + + let tokens = tokenize("sk = :s AND pk = :p").unwrap(); + let mut kc = parse_key_condition(&tokens).unwrap(); + let err = kc + .resolve_key_schema_with(&schema, &HashMap::new(), |_, _| { + Err(validation_err("checked")) + }) + .unwrap_err(); + assert!(matches!(&err, DynamoDbError::ValidationException(msg) if msg == "checked")); + // The failed check left the parser's assignment in place. + assert_eq!(kc.pk_path, vec![PathElement::Attribute("sk".to_owned())]); + + kc.resolve_key_schema(&schema, &HashMap::new()).unwrap(); + assert_eq!(kc.pk_path, vec![PathElement::Attribute("pk".to_owned())]); +} + +// Syntax error wording measured 2026-10-25 (us-east-1) on Query. + +fn kce_msg(src: &str) -> String { + match parse_key_condition_from(&tokenize(src).unwrap(), src) { + Err(DynamoDbError::ValidationException(m)) => m, + other => panic!("expected ValidationException, got {other:?}"), + } +} + +#[test] +fn repeated_operator_quotes_the_source_window() { + // [d22] `pk = = :v` + assert_eq!( + kce_msg("pk = = :v"), + "Invalid KeyConditionExpression: Syntax error; token: \"=\", near: \"= = :v\"" + ); + assert_eq!( + kce_msg("pk = :v AND sk = = :s"), + "Invalid KeyConditionExpression: Syntax error; token: \"=\", near: \"= = :s\"" + ); +} + +#[test] +fn a_lower_case_keyword_is_shown_as_written() { + // Measured 2026-10-25: the token and the window are the source text. + assert_eq!( + kce_msg("pk = :p and and sk = :s"), + "Invalid KeyConditionExpression: Syntax error; token: \"and\", near: \"and and sk\"" + ); +} + +#[test] +fn trailing_keyword_is_eof_with_the_keyword_as_window() { + // [d23] `pk = :v AND` + assert_eq!( + kce_msg("pk = :v AND"), + "Invalid KeyConditionExpression: Syntax error; token: \"\", near: \"AND\"" + ); + assert_eq!( + kce_msg("pk ="), + "Invalid KeyConditionExpression: Syntax error; token: \"\", near: \"=\"" + ); +} + +#[test] +fn stripped_outer_parentheses_keep_the_source_window() { + // The window is taken from the full token list, so the stripped + // parenthesis does not shift the quoted text. + assert_eq!( + kce_msg("(pk = = :v)"), + "Invalid KeyConditionExpression: Syntax error; token: \"=\", near: \"= = :v\"" + ); +} + +#[test] +fn a_fault_at_the_first_token_keeps_leading_whitespace() { + // Measured 2026-10-25: the window starts at the start of the expression. + assert_eq!( + kce_msg(" = pk = :v"), + "Invalid KeyConditionExpression: Syntax error; token: \"=\", near: \" = pk\"" + ); +} + +#[test] +fn other_key_condition_texts_are_unchanged() { + assert_eq!( + kce_msg("pk = :v OR sk = :s"), + "Invalid KeyConditionExpression: only AND is supported between key conditions" + ); + assert_eq!( + kce_msg("pk"), + "Invalid KeyConditionExpression: expected operator after operand" + ); + assert_eq!( + kce_msg("pk = :v AND sk = sk2"), + "Invalid KeyConditionExpression: key condition values must be expression attribute value placeholders" + ); +} diff --git a/crates/core/src/expression/mod.rs b/crates/core/src/expression/mod.rs index c9f8e207b..dea971f6a 100755 --- a/crates/core/src/expression/mod.rs +++ b/crates/core/src/expression/mod.rs @@ -11,13 +11,16 @@ mod ast; mod evaluator; mod key_condition; +mod key_condition_resolve; mod kind; +mod operator_count; mod parser; mod parser_common; mod projection; mod reserved_words; mod resolver; mod search_condition; +mod syntax_error; mod tokenizer; mod update_evaluator; mod update_parser; @@ -26,11 +29,12 @@ 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_from, }; pub use kind::ExpressionKind; -pub use parser::{parse_condition, parse_condition_with_depth_limit}; -pub use projection::{Projection, parse_projection}; +pub use operator_count::{MAX_EXPRESSION_OPERATORS, count_operators, validate_operator_count}; +pub use parser::{parse_condition, parse_condition_from, parse_condition_with_depth_limit}; +pub use projection::{Projection, parse_projection, parse_projection_from}; pub use reserved_words::validate_no_reserved_words; pub use resolver::{ ExpressionMaps, collect_key_condition_refs, collect_value_placeholders, resolve_element_name, @@ -42,6 +46,7 @@ pub use search_condition::{ SearchCondition, validate_conditions_against_search_schema, validate_search_condition_expression, }; +pub use syntax_error::{is_operator_or_keyword, syntax_error, token_text}; pub use tokenizer::{Token, tokenize, tokenize_for, tokenize_with_limit}; pub use update_evaluator::apply_update_validated; pub use update_parser::{parse_update, parse_update_from}; diff --git a/crates/core/src/expression/operator_count.rs b/crates/core/src/expression/operator_count.rs new file mode 100644 index 000000000..a0548d11d --- /dev/null +++ b/crates/core/src/expression/operator_count.rs @@ -0,0 +1,161 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! The operator cap Amazon `DynamoDB` applies to condition, filter and key +//! condition expressions. +//! +//! Measured 2026-10-25 (us-east-1) with `PutItem`, `Scan` and `Query`: every +//! comparison (`=`, `<>`, `<`, `<=`, `>`, `>=`), `AND`, `OR`, `NOT`, `BETWEEN` +//! (its inner `AND` not counted), `IN` (list elements not counted) and every +//! function call (`size` included) counts one; parentheses, paths and +//! placeholders count zero. An expression whose count exceeds 300 is refused +//! with `Invalid : The expression contains too many operators; operator +//! count: `; a count of exactly 300 is accepted. + +use super::ast::Expr; +use super::kind::ExpressionKind; +use crate::error::DynamoDbError; + +/// The largest operator count the service accepts. +pub const MAX_EXPRESSION_OPERATORS: usize = 300; + +/// Count the operators in a parsed condition as the service counts them. +#[must_use] +pub fn count_operators(expr: &Expr) -> usize { + match expr { + Expr::Path(_) | Expr::Placeholder(_) => 0, + Expr::Not(inner) => 1 + count_operators(inner), + Expr::And(left, right) | Expr::Or(left, right) => { + 1 + count_operators(left) + count_operators(right) + } + Expr::Compare { left, right, .. } | Expr::Arithmetic { left, right, .. } => { + 1 + count_operators(left) + count_operators(right) + } + Expr::Between { operand, low, high } => { + 1 + count_operators(operand) + count_operators(low) + count_operators(high) + } + Expr::In { operand, list } => { + 1 + count_operators(operand) + list.iter().map(count_operators).sum::() + } + Expr::Function { args, .. } => 1 + args.iter().map(count_operators).sum::(), + } +} + +/// Refuse an expression with more than [`MAX_EXPRESSION_OPERATORS`] operators. +/// +/// # Errors +/// +/// Returns `ValidationException` carrying the `Invalid :` prefix and the +/// full operator count. +pub fn validate_operator_count(expr: &Expr, kind: ExpressionKind) -> Result<(), DynamoDbError> { + let count = count_operators(expr); + if count > MAX_EXPRESSION_OPERATORS { + return Err(DynamoDbError::ValidationException(format!( + "Invalid {kind}: The expression contains too many operators; operator count: {count}" + ))); + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::expression::parser::parse_condition; + use crate::expression::tokenizer::tokenize; + + fn count(src: &str) -> usize { + count_operators(&parse_condition(&tokenize(src).unwrap()).unwrap()) + } + + fn chain(term: &str, n: usize, joiner: &str) -> String { + vec![term; n].join(joiner) + } + + // Expected counts are the service's answers of 2026-10-25 (probe cases in + // brackets). + + #[test] + fn comparisons_and_joiners_count_one_each() { + assert_eq!(count(&chain("#v = :v", 300, " OR ")), 599); // [a02] + assert_eq!(count(&chain("#v <> :v", 300, " OR ")), 599); // [a05] + assert_eq!(count(&chain("#v >= :v", 300, " AND ")), 599); // [a09, a03] + assert_eq!( + count(&format!( + "attribute_not_exists(pk) OR {}", + chain("#v = :v", 300, " OR ") + )), + 601 + ); // [a01] + } + + #[test] + fn not_functions_between_in_and_size_count_one_each() { + assert_eq!(count(&chain("NOT #v = :v", 200, " OR ")), 599); // [a10] + assert_eq!(count(&chain("NOT NOT #v = :v", 200, " OR ")), 799); // [a22] + assert_eq!(count(&chain("#v BETWEEN :v AND :v", 160, " OR ")), 319); // [a40] + assert_eq!(count(&chain("#v IN (:v, :v, :v)", 170, " OR ")), 339); // [a41] + assert_eq!(count(&chain("#v IN (:v)", 200, " OR ")), 399); // [a12] + assert_eq!(count(&chain("attribute_exists(#v)", 170, " OR ")), 339); // [a43] + assert_eq!(count(&chain("contains(#v, :v)", 200, " OR ")), 399); // [a45] + assert_eq!(count(&chain("size(#v) = :v", 230, " OR ")), 689); // [a47] + assert_eq!( + count(&chain("size(#v) BETWEEN :v AND :v", 130, " OR ")), + 389 + ); // [a53] + } + + #[test] + fn parentheses_count_zero() { + assert_eq!(count(&chain("(#v = :v)", 300, " OR ")), 599); // [a19] + assert_eq!(count(&chain("((#v = :v) OR (#v = :v))", 140, " OR ")), 559); // [a51] + assert_eq!( + count(&format!( + "attribute_exists(pk) AND ({})", + chain("#v = :v", 300, " OR ") + )), + 601 + ); // [a04] + assert_eq!( + count(&format!( + "attribute_exists(pk) AND NOT ({})", + chain("#v = :v", 150, " OR ") + )), + 302 + ); // [a52] + } + + #[test] + fn cap_is_exclusive_at_300() { + let kind = ExpressionKind::Condition; + let at_cap = format!( + "NOT attribute_not_exists(pk) OR {}", + chain("#v = :v", 149, " OR ") + ); // [a49] + assert_eq!(count(&at_cap), 300); + assert!( + validate_operator_count(&parse_condition(&tokenize(&at_cap).unwrap()).unwrap(), kind) + .is_ok() + ); + + let over = chain("#v = :v", 151, " OR "); // [a30] + let err = + validate_operator_count(&parse_condition(&tokenize(&over).unwrap()).unwrap(), kind) + .unwrap_err(); + assert!(matches!(err, DynamoDbError::ValidationException(m) + if m == "Invalid ConditionExpression: The expression contains too many operators; operator count: 301")); + + let under = chain("#v = :v", 150, " OR "); // [a30] + assert!( + validate_operator_count(&parse_condition(&tokenize(&under).unwrap()).unwrap(), kind) + .is_ok() + ); + } + + #[test] + fn message_carries_the_kind() { + let expr = parse_condition(&tokenize(&chain("#v = :v", 300, " OR ")).unwrap()).unwrap(); + let err = validate_operator_count(&expr, ExpressionKind::Filter).unwrap_err(); + assert!(matches!(err, DynamoDbError::ValidationException(m) + if m == "Invalid FilterExpression: The expression contains too many operators; operator count: 599")); // [b01] + } +} diff --git a/crates/core/src/expression/parser.rs b/crates/core/src/expression/parser.rs index f44bdfab6..cc49d73d3 100755 --- a/crates/core/src/expression/parser.rs +++ b/crates/core/src/expression/parser.rs @@ -20,7 +20,9 @@ //! ``` use super::ast::{CompareOp, Expr}; +use super::kind::ExpressionKind; use super::parser_common; +use super::syntax_error::{is_operator_or_keyword, syntax_error}; use super::tokenizer::Token; use crate::error::DynamoDbError; @@ -41,13 +43,37 @@ pub fn parse_condition(tokens: &[Token]) -> Result { pub fn parse_condition_with_depth_limit( tokens: &[Token], max_depth: usize, +) -> Result { + parse_condition_from(tokens, "", max_depth) +} + +/// Parse a condition expression whose tokens came from `source`, so a syntax +/// error can quote the source window around the offending token as Amazon +/// `DynamoDB` does (`Syntax error; token: "=", near: "= = :v"`). An empty +/// `source` joins the token texts instead. +/// +/// # Errors +/// +/// Returns `ValidationException` for syntax errors or depth exceeded. +pub fn parse_condition_from( + tokens: &[Token], + source: &str, + max_depth: usize, ) -> Result { parser_common::check_redundant_parens(tokens) .map_err(|body| validation_err(&format!("Invalid ConditionExpression: {body}")))?; let mut pos = 0; let mut depth = 0; - let expr = parse_or(tokens, &mut pos, &mut depth, max_depth)?; + let expr = parse_or(tokens, source, &mut pos, &mut depth, max_depth)?; if pos < tokens.len() { + // A token after a complete condition. Measured 2026-10-25 for an + // operator (`#v = :v =`) and a path (`#v = :v #v = :v`); other tokens + // keep the previous text. + if is_operator_or_keyword(&tokens[pos]) + || matches!(tokens[pos], Token::Ident(_) | Token::NameRef(_)) + { + return Err(syntax_error(ExpressionKind::Condition, source, tokens, pos)); + } return Err(validation_err(&format!( "Invalid ConditionExpression: unexpected token at position {pos}" ))); @@ -57,14 +83,15 @@ pub fn parse_condition_with_depth_limit( fn parse_or( tokens: &[Token], + src: &str, pos: &mut usize, depth: &mut usize, max_depth: usize, ) -> Result { - let mut left = parse_and(tokens, pos, depth, max_depth)?; + let mut left = parse_and(tokens, src, pos, depth, max_depth)?; while *pos < tokens.len() && tokens[*pos] == Token::Or { *pos += 1; - let right = parse_and(tokens, pos, depth, max_depth)?; + let right = parse_and(tokens, src, pos, depth, max_depth)?; left = Expr::Or(Box::new(left), Box::new(right)); } Ok(left) @@ -72,14 +99,15 @@ fn parse_or( fn parse_and( tokens: &[Token], + src: &str, pos: &mut usize, depth: &mut usize, max_depth: usize, ) -> Result { - let mut left = parse_not(tokens, pos, depth, max_depth)?; + let mut left = parse_not(tokens, src, pos, depth, max_depth)?; while *pos < tokens.len() && tokens[*pos] == Token::And { *pos += 1; - let right = parse_not(tokens, pos, depth, max_depth)?; + let right = parse_not(tokens, src, pos, depth, max_depth)?; left = Expr::And(Box::new(left), Box::new(right)); } Ok(left) @@ -87,6 +115,7 @@ fn parse_and( fn parse_not( tokens: &[Token], + src: &str, pos: &mut usize, depth: &mut usize, max_depth: usize, @@ -99,23 +128,29 @@ fn parse_not( "Invalid ConditionExpression: expression nesting depth exceeded", )); } - let expr = parse_not(tokens, pos, depth, max_depth)?; + let expr = parse_not(tokens, src, pos, depth, max_depth)?; *depth -= 1; return Ok(Expr::Not(Box::new(expr))); } - parse_primary(tokens, pos, depth, max_depth) + parse_primary(tokens, src, pos, depth, max_depth) } fn parse_primary( tokens: &[Token], + src: &str, pos: &mut usize, depth: &mut usize, max_depth: usize, ) -> Result { if *pos >= tokens.len() { - return Err(validation_err( - "Invalid ConditionExpression: unexpected end of expression", - )); + if tokens.is_empty() { + return Err(validation_err( + "Invalid ConditionExpression: unexpected end of expression", + )); + } + // Input ends where a condition is required (`#v = :v AND`): measured + // 2026-10-25 as token "" with the last token as the window. + return Err(syntax_error(ExpressionKind::Condition, src, tokens, *pos)); } // Parenthesized expression @@ -127,7 +162,7 @@ fn parse_primary( "Invalid ConditionExpression: expression nesting depth exceeded", )); } - let expr = parse_or(tokens, pos, depth, max_depth)?; + let expr = parse_or(tokens, src, pos, depth, max_depth)?; *depth -= 1; parser_common::expect_token(tokens, pos, &Token::RParen, ")", "ConditionExpression")?; return Ok(expr); @@ -140,17 +175,17 @@ fn parse_primary( && *pos + 1 < tokens.len() && tokens[*pos + 1] == Token::LParen { - return parse_function_call(tokens, pos); + return parse_function_call(tokens, src, pos); } } // Operand — then check for comparator, BETWEEN, or IN - let left = parse_operand(tokens, pos)?; + let left = parse_operand(tokens, src, pos)?; if *pos < tokens.len() { if let Some(op) = try_comparator(&tokens[*pos]) { *pos += 1; - let right = parse_operand(tokens, pos)?; + let right = parse_operand(tokens, src, pos)?; return Ok(Expr::Compare { left: Box::new(left), op, @@ -161,9 +196,9 @@ fn parse_primary( // BETWEEN operand AND operand if tokens[*pos] == Token::Between { *pos += 1; - let low = parse_operand(tokens, pos)?; + let low = parse_operand(tokens, src, pos)?; parser_common::expect_token(tokens, pos, &Token::And, "AND", "ConditionExpression")?; - let high = parse_operand(tokens, pos)?; + let high = parse_operand(tokens, src, pos)?; return Ok(Expr::Between { operand: Box::new(left), low: Box::new(low), @@ -175,10 +210,10 @@ fn parse_primary( if tokens[*pos] == Token::In { *pos += 1; parser_common::expect_token(tokens, pos, &Token::LParen, "(", "ConditionExpression")?; - let mut list = vec![parse_operand(tokens, pos)?]; + let mut list = vec![parse_operand(tokens, src, pos)?]; while *pos < tokens.len() && tokens[*pos] == Token::Comma { *pos += 1; - list.push(parse_operand(tokens, pos)?); + list.push(parse_operand(tokens, src, pos)?); } parser_common::expect_token(tokens, pos, &Token::RParen, ")", "ConditionExpression")?; return Ok(Expr::In { @@ -194,11 +229,10 @@ fn parse_primary( )) } -fn parse_operand(tokens: &[Token], pos: &mut usize) -> Result { +fn parse_operand(tokens: &[Token], src: &str, pos: &mut usize) -> Result { if *pos >= tokens.len() { - return Err(validation_err( - "Invalid ConditionExpression: expected operand", - )); + // Input ends where an operand is required (`#v = :v AND NOT`). + return Err(syntax_error(ExpressionKind::Condition, src, tokens, *pos)); } match &tokens[*pos] { @@ -213,7 +247,7 @@ fn parse_operand(tokens: &[Token], pos: &mut usize) -> Result Result { + Err(syntax_error(ExpressionKind::Condition, src, tokens, *pos)) + } _ => Err(validation_err( "Invalid ConditionExpression: expected attribute path or value placeholder", )), } } -fn parse_function_call(tokens: &[Token], pos: &mut usize) -> Result { +fn parse_function_call( + tokens: &[Token], + src: &str, + pos: &mut usize, +) -> Result { let name = match &tokens[*pos] { Token::Ident(n) => n.to_ascii_lowercase(), _ => { @@ -243,10 +286,10 @@ fn parse_function_call(tokens: &[Token], pos: &mut usize) -> Result Result>, DynamoDbError> { + parse_projection_from(tokens, "") +} + +/// Parse a `ProjectionExpression` whose tokens came from `source`, so a syntax +/// error can quote the source window around the offending token as Amazon +/// `DynamoDB` does. An empty `source` joins the token texts instead. +/// +/// # Errors +/// +/// Returns `ValidationException` for syntax errors. +pub fn parse_projection_from( + tokens: &[Token], + source: &str, +) -> Result>, DynamoDbError> { if tokens.is_empty() { return Ok(Vec::new()); } let mut pos = 0; - let mut paths = vec![super::parser_common::parse_path(tokens, &mut pos)?]; + let mut paths = vec![parse_projection_path(tokens, source, &mut pos)?]; while pos < tokens.len() { if tokens[pos] != Token::Comma { return Err(DynamoDbError::ValidationException(format!( @@ -34,11 +50,37 @@ pub fn parse_projection(tokens: &[Token]) -> Result>, Dynam ))); } pos += 1; - paths.push(super::parser_common::parse_path(tokens, &mut pos)?); + paths.push(parse_projection_path(tokens, source, &mut pos)?); } Ok(paths) } +/// Parse one projected path. Input that ends where a path is required (`#v,`) +/// and a comma, operator or keyword there (`#v, , #v`) answer the service's +/// syntax error (measured 2026-10-25: token ",", near ", , #v"; token "", +/// near ","). Any other token keeps the previous text. +fn parse_projection_path( + tokens: &[Token], + source: &str, + pos: &mut usize, +) -> Result, DynamoDbError> { + match tokens.get(*pos) { + None | Some(Token::Comma) => Err(syntax_error( + ExpressionKind::Projection, + source, + tokens, + *pos, + )), + Some(token) if is_operator_or_keyword(token) => Err(syntax_error( + ExpressionKind::Projection, + source, + tokens, + *pos, + )), + Some(_) => super::parser_common::parse_path(tokens, pos), + } +} + /// A node in the projection trie. /// /// Each node selects part of the item. `terminal` means the whole value at @@ -259,426 +301,5 @@ fn project_value(value: &AttributeValue, node: &ProjNode) -> Option, - ) -> Result { - let tokens = tokenize(expr_str)?; - let paths = parse_projection(&tokens)?; - let maps = ExpressionMaps::new(names, HashMap::new()); - apply_projection(item, &paths, &maps) - } - - fn sample_item() -> Item { - let mut inner = BTreeMap::new(); - inner.insert("city".into(), AttributeValue::S("NYC".into())); - inner.insert("zip".into(), AttributeValue::S("10001".into())); - let mut item = BTreeMap::new(); - item.insert("pk".into(), AttributeValue::S("key1".into())); - item.insert("name".into(), AttributeValue::S("Alice".into())); - item.insert("age".into(), AttributeValue::N("30".into())); - item.insert("address".into(), AttributeValue::M(inner)); - item - } - - #[test] - fn project_single_attribute() { - let item = sample_item(); - let result = project("name", &item, HashMap::new()).unwrap(); - assert_eq!(result.len(), 1); - assert_eq!(result.get("name"), Some(&AttributeValue::S("Alice".into()))); - } - - #[test] - fn project_multiple_attributes() { - let item = sample_item(); - let result = project("name, age", &item, HashMap::new()).unwrap(); - assert_eq!(result.len(), 2); - assert!(result.contains_key("name")); - assert!(result.contains_key("age")); - } - - #[test] - fn project_missing_attribute_omitted() { - let item = sample_item(); - let result = project("name, missing", &item, HashMap::new()).unwrap(); - assert_eq!(result.len(), 1); - assert!(result.contains_key("name")); - } - - #[test] - fn project_nested_path() { - let item = sample_item(); - let result = project("address.city", &item, HashMap::new()).unwrap(); - assert!(result.contains_key("address")); - if let Some(AttributeValue::M(m)) = result.get("address") { - assert_eq!(m.get("city"), Some(&AttributeValue::S("NYC".into()))); - assert!(!m.contains_key("zip")); // Only city projected - } else { - panic!("Expected M"); - } - } - - #[test] - fn project_with_name_ref() { - let item = sample_item(); - let mut names = HashMap::new(); - names.insert("n".into(), "name".into()); - let result = project("#n", &item, names).unwrap(); - assert_eq!(result.len(), 1); - assert_eq!(result.get("name"), Some(&AttributeValue::S("Alice".into()))); - } - - #[test] - fn project_empty_expression() { - let item = sample_item(); - let tokens = tokenize("").unwrap(); - let paths = parse_projection(&tokens).unwrap(); - assert!(paths.is_empty()); - let maps = ExpressionMaps::new(HashMap::new(), HashMap::new()); - let result = apply_projection(&item, &paths, &maps).unwrap(); - assert!(result.is_empty()); - } - - #[test] - fn project_all_attributes() { - let item = sample_item(); - let result = project("pk, name, age, address", &item, HashMap::new()).unwrap(); - assert_eq!(result.len(), 4); - } - - #[test] - fn project_list_index() { - let mut item = BTreeMap::new(); - item.insert("pk".into(), AttributeValue::S("k1".into())); - item.insert( - "mylist".into(), - AttributeValue::L(vec![ - AttributeValue::S("zero".into()), - AttributeValue::S("one".into()), - AttributeValue::S("two".into()), - ]), - ); - - let result = project("mylist[0]", &item, HashMap::new()).unwrap(); - assert_eq!(result.len(), 1); - match result.get("mylist") { - Some(AttributeValue::L(list)) => { - assert_eq!(list.len(), 1); - assert_eq!(list[0], AttributeValue::S("zero".into())); - } - other => panic!("Expected L, got {other:?}"), - } - - let result = project("mylist[1]", &item, HashMap::new()).unwrap(); - match result.get("mylist") { - Some(AttributeValue::L(list)) => { - assert_eq!(list.len(), 1); - assert_eq!(list[0], AttributeValue::S("one".into())); - } - other => panic!("Expected L, got {other:?}"), - } - - // Out-of-bounds index returns empty - let result = project("mylist[5]", &item, HashMap::new()).unwrap(); - assert!(result.is_empty()); - } - - fn list_item() -> Item { - let mut item = BTreeMap::new(); - item.insert("pk".into(), AttributeValue::S("p1".into())); - item.insert( - "mylist".into(), - AttributeValue::L(vec![ - AttributeValue::S("zero".into()), - AttributeValue::S("one".into()), - AttributeValue::S("two".into()), - AttributeValue::S("three".into()), - ]), - ); - item.insert( - "with_null".into(), - AttributeValue::L(vec![ - AttributeValue::S("keep0".into()), - AttributeValue::Null, - AttributeValue::S("keep2".into()), - ]), - ); - item - } - - fn assert_list(result: &Item, key: &str, expected: &[AttributeValue]) { - match result.get(key) { - Some(AttributeValue::L(list)) => assert_eq!(list.as_slice(), expected), - other => panic!("Expected L for {key}, got {other:?}"), - } - } - - #[test] - fn project_two_list_indices_compacted() { - let item = list_item(); - let result = project("mylist[0], mylist[2]", &item, HashMap::new()).unwrap(); - assert_list( - &result, - "mylist", - &[ - AttributeValue::S("zero".into()), - AttributeValue::S("two".into()), - ], - ); - } - - #[test] - fn project_list_indices_ordered_by_index_not_expression() { - let item = list_item(); - // Reversed expression order still comes out in ascending index order. - let result = project("mylist[2], mylist[0]", &item, HashMap::new()).unwrap(); - assert_list( - &result, - "mylist", - &[ - AttributeValue::S("zero".into()), - AttributeValue::S("two".into()), - ], - ); - } - - #[test] - fn project_list_index_gap_compacted() { - let item = list_item(); - let result = project("mylist[1], mylist[3]", &item, HashMap::new()).unwrap(); - assert_list( - &result, - "mylist", - &[ - AttributeValue::S("one".into()), - AttributeValue::S("three".into()), - ], - ); - } - - #[test] - fn project_null_element_by_index_preserved() { - let item = list_item(); - let result = project("with_null[1]", &item, HashMap::new()).unwrap(); - assert_list(&result, "with_null", &[AttributeValue::Null]); - } - - #[test] - fn project_unselected_null_dropped() { - let item = list_item(); - let result = project("with_null[0], with_null[2]", &item, HashMap::new()).unwrap(); - assert_list( - &result, - "with_null", - &[ - AttributeValue::S("keep0".into()), - AttributeValue::S("keep2".into()), - ], - ); - } - - #[test] - fn project_whole_list_preserves_null() { - let item = list_item(); - let result = project("with_null", &item, HashMap::new()).unwrap(); - assert_list( - &result, - "with_null", - &[ - AttributeValue::S("keep0".into()), - AttributeValue::Null, - AttributeValue::S("keep2".into()), - ], - ); - } - - #[test] - fn project_list_of_maps_subfield_multi() { - let mut item = BTreeMap::new(); - item.insert("pk".into(), AttributeValue::S("p".into())); - let mk = |v: &str, x: &str| { - let mut m = BTreeMap::new(); - m.insert("val".into(), AttributeValue::S(v.into())); - m.insert("x".into(), AttributeValue::S(x.into())); - AttributeValue::M(m) - }; - item.insert( - "lom".into(), - AttributeValue::L(vec![mk("a0", "x0"), mk("a1", "x1"), mk("a2", "x2")]), - ); - - let result = project("lom[0].val, lom[2].val", &item, HashMap::new()).unwrap(); - let only_val = |v: &str| { - let mut m = BTreeMap::new(); - m.insert("val".into(), AttributeValue::S(v.into())); - AttributeValue::M(m) - }; - assert_list(&result, "lom", &[only_val("a0"), only_val("a2")]); - } - - #[test] - fn project_same_index_merges_subfields() { - let mut item = BTreeMap::new(); - item.insert("pk".into(), AttributeValue::S("p".into())); - let mut m = BTreeMap::new(); - m.insert("a".into(), AttributeValue::S("av".into())); - m.insert("b".into(), AttributeValue::S("bv".into())); - m.insert("c".into(), AttributeValue::S("cv".into())); - item.insert("l".into(), AttributeValue::L(vec![AttributeValue::M(m)])); - - // Two paths selecting the same element merge their subfields. - let result = project("l[0].a, l[0].c", &item, HashMap::new()).unwrap(); - let mut expected = BTreeMap::new(); - expected.insert("a".into(), AttributeValue::S("av".into())); - expected.insert("c".into(), AttributeValue::S("cv".into())); - assert_list(&result, "l", &[AttributeValue::M(expected)]); - } - - fn check_overlap(expr: &str, names: HashMap) -> Result<(), DynamoDbError> { - let tokens = tokenize(expr).unwrap(); - let paths = parse_projection(&tokens).unwrap(); - let maps = ExpressionMaps::new(names, HashMap::new()); - Projection::compile(&paths, &maps, true).map(|_| ()) - } - - #[test] - fn compile_without_overlap_rejection_accepts_duplicates() { - // Desugared AttributesToGet keeps the legacy accept-duplicates behavior. - let tokens = tokenize("a, a").unwrap(); - let paths = parse_projection(&tokens).unwrap(); - let maps = ExpressionMaps::new(HashMap::new(), HashMap::new()); - assert!(Projection::compile(&paths, &maps, false).is_ok()); - } - - #[test] - fn compiled_projection_reusable_across_items() { - let tokens = tokenize("name").unwrap(); - let paths = parse_projection(&tokens).unwrap(); - let maps = ExpressionMaps::new(HashMap::new(), HashMap::new()); - let proj = Projection::compile(&paths, &maps, true).unwrap(); - let item = sample_item(); - for _ in 0..2 { - let out = proj.apply(&item); - assert_eq!(out.len(), 1); - assert!(out.contains_key("name")); - } - } - - fn overlap_msg(expr: &str) -> String { - match check_overlap(expr, HashMap::new()) { - Err(DynamoDbError::ValidationException(m)) => m, - other => panic!("expected ValidationException, got {other:?}"), - } - } - - #[test] - fn overlap_parent_then_child() { - assert_eq!( - overlap_msg("a, a.b"), - "Invalid ProjectionExpression: Two document paths overlap with each other; \ - must remove or rewrite one of these paths; path one: [a], path two: [a, b]" - ); - } - - #[test] - fn overlap_child_then_parent() { - assert_eq!( - overlap_msg("a.b, a"), - "Invalid ProjectionExpression: Two document paths overlap with each other; \ - must remove or rewrite one of these paths; path one: [a, b], path two: [a]" - ); - } - - #[test] - fn overlap_exact_duplicate() { - assert_eq!( - overlap_msg("a, a"), - "Invalid ProjectionExpression: Two document paths overlap with each other; \ - must remove or rewrite one of these paths; path one: [a], path two: [a]" - ); - } - - #[test] - fn overlap_index_path_rendering() { - assert_eq!( - overlap_msg("a[0], a"), - "Invalid ProjectionExpression: Two document paths overlap with each other; \ - must remove or rewrite one of these paths; path one: [a, [0]], path two: [a]" - ); - assert_eq!( - overlap_msg("a[0].b, a[0]"), - "Invalid ProjectionExpression: Two document paths overlap with each other; \ - must remove or rewrite one of these paths; path one: [a, [0], b], path two: [a, [0]]" - ); - } - - #[test] - fn overlap_reports_first_pair_in_order() { - assert_eq!( - overlap_msg("x, a.b, a"), - "Invalid ProjectionExpression: Two document paths overlap with each other; \ - must remove or rewrite one of these paths; path one: [a, b], path two: [a]" - ); - } - - #[test] - fn overlap_uses_resolved_names() { - let mut names = HashMap::new(); - names.insert("p".into(), "a".into()); - names.insert("q".into(), "b".into()); - match check_overlap("#p, #p.#q", names) { - Err(DynamoDbError::ValidationException(m)) => assert_eq!( - m, - "Invalid ProjectionExpression: Two document paths overlap with each other; \ - must remove or rewrite one of these paths; path one: [a], path two: [a, b]" - ), - other => panic!("expected ValidationException, got {other:?}"), - } - } - - #[test] - fn no_overlap_for_siblings() { - assert!(check_overlap("a.b, a.c", HashMap::new()).is_ok()); - assert!(check_overlap("a[0], a[1]", HashMap::new()).is_ok()); - assert!(check_overlap("a, b, c", HashMap::new()).is_ok()); - } - - #[test] - fn project_list_index_into_map_preserves_structure() { - let mut item = BTreeMap::new(); - item.insert("pk".into(), AttributeValue::S("p".into())); - let mut map0 = BTreeMap::new(); - map0.insert("val".into(), AttributeValue::S("target".into())); - map0.insert("x".into(), AttributeValue::S("0".into())); - let mut map1 = BTreeMap::new(); - map1.insert("x".into(), AttributeValue::S("0".into())); - item.insert( - "a_list".into(), - AttributeValue::L(vec![AttributeValue::M(map0), AttributeValue::M(map1)]), - ); - - let result = project("a_list[0].val", &item, HashMap::new()).unwrap(); - // Should preserve: {"a_list": [{"val": "target"}]} - match result.get("a_list") { - Some(AttributeValue::L(list)) => { - assert_eq!(list.len(), 1); - match &list[0] { - AttributeValue::M(m) => { - assert_eq!(m.get("val"), Some(&AttributeValue::S("target".into()))); - assert!(!m.contains_key("x")); - } - other => panic!("Expected M, got {other:?}"), - } - } - other => panic!("Expected L, got {other:?}"), - } - } -} +#[path = "projection_tests.rs"] +mod tests; diff --git a/crates/core/src/expression/projection_tests.rs b/crates/core/src/expression/projection_tests.rs new file mode 100644 index 000000000..c347c9cb9 --- /dev/null +++ b/crates/core/src/expression/projection_tests.rs @@ -0,0 +1,463 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +use super::*; +use crate::expression::resolver::ExpressionMaps; +use crate::expression::tokenizer::tokenize; +use std::collections::HashMap; + +fn project( + expr_str: &str, + item: &Item, + names: HashMap, +) -> Result { + let tokens = tokenize(expr_str)?; + let paths = parse_projection(&tokens)?; + let maps = ExpressionMaps::new(names, HashMap::new()); + apply_projection(item, &paths, &maps) +} + +fn sample_item() -> Item { + let mut inner = BTreeMap::new(); + inner.insert("city".into(), AttributeValue::S("NYC".into())); + inner.insert("zip".into(), AttributeValue::S("10001".into())); + let mut item = BTreeMap::new(); + item.insert("pk".into(), AttributeValue::S("key1".into())); + item.insert("name".into(), AttributeValue::S("Alice".into())); + item.insert("age".into(), AttributeValue::N("30".into())); + item.insert("address".into(), AttributeValue::M(inner)); + item +} + +#[test] +fn project_single_attribute() { + let item = sample_item(); + let result = project("name", &item, HashMap::new()).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result.get("name"), Some(&AttributeValue::S("Alice".into()))); +} + +#[test] +fn project_multiple_attributes() { + let item = sample_item(); + let result = project("name, age", &item, HashMap::new()).unwrap(); + assert_eq!(result.len(), 2); + assert!(result.contains_key("name")); + assert!(result.contains_key("age")); +} + +#[test] +fn project_missing_attribute_omitted() { + let item = sample_item(); + let result = project("name, missing", &item, HashMap::new()).unwrap(); + assert_eq!(result.len(), 1); + assert!(result.contains_key("name")); +} + +#[test] +fn project_nested_path() { + let item = sample_item(); + let result = project("address.city", &item, HashMap::new()).unwrap(); + assert!(result.contains_key("address")); + if let Some(AttributeValue::M(m)) = result.get("address") { + assert_eq!(m.get("city"), Some(&AttributeValue::S("NYC".into()))); + assert!(!m.contains_key("zip")); // Only city projected + } else { + panic!("Expected M"); + } +} + +#[test] +fn project_with_name_ref() { + let item = sample_item(); + let mut names = HashMap::new(); + names.insert("n".into(), "name".into()); + let result = project("#n", &item, names).unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result.get("name"), Some(&AttributeValue::S("Alice".into()))); +} + +#[test] +fn project_empty_expression() { + let item = sample_item(); + let tokens = tokenize("").unwrap(); + let paths = parse_projection(&tokens).unwrap(); + assert!(paths.is_empty()); + let maps = ExpressionMaps::new(HashMap::new(), HashMap::new()); + let result = apply_projection(&item, &paths, &maps).unwrap(); + assert!(result.is_empty()); +} + +#[test] +fn project_all_attributes() { + let item = sample_item(); + let result = project("pk, name, age, address", &item, HashMap::new()).unwrap(); + assert_eq!(result.len(), 4); +} + +#[test] +fn project_list_index() { + let mut item = BTreeMap::new(); + item.insert("pk".into(), AttributeValue::S("k1".into())); + item.insert( + "mylist".into(), + AttributeValue::L(vec![ + AttributeValue::S("zero".into()), + AttributeValue::S("one".into()), + AttributeValue::S("two".into()), + ]), + ); + + let result = project("mylist[0]", &item, HashMap::new()).unwrap(); + assert_eq!(result.len(), 1); + match result.get("mylist") { + Some(AttributeValue::L(list)) => { + assert_eq!(list.len(), 1); + assert_eq!(list[0], AttributeValue::S("zero".into())); + } + other => panic!("Expected L, got {other:?}"), + } + + let result = project("mylist[1]", &item, HashMap::new()).unwrap(); + match result.get("mylist") { + Some(AttributeValue::L(list)) => { + assert_eq!(list.len(), 1); + assert_eq!(list[0], AttributeValue::S("one".into())); + } + other => panic!("Expected L, got {other:?}"), + } + + // Out-of-bounds index returns empty + let result = project("mylist[5]", &item, HashMap::new()).unwrap(); + assert!(result.is_empty()); +} + +fn list_item() -> Item { + let mut item = BTreeMap::new(); + item.insert("pk".into(), AttributeValue::S("p1".into())); + item.insert( + "mylist".into(), + AttributeValue::L(vec![ + AttributeValue::S("zero".into()), + AttributeValue::S("one".into()), + AttributeValue::S("two".into()), + AttributeValue::S("three".into()), + ]), + ); + item.insert( + "with_null".into(), + AttributeValue::L(vec![ + AttributeValue::S("keep0".into()), + AttributeValue::Null, + AttributeValue::S("keep2".into()), + ]), + ); + item +} + +fn assert_list(result: &Item, key: &str, expected: &[AttributeValue]) { + match result.get(key) { + Some(AttributeValue::L(list)) => assert_eq!(list.as_slice(), expected), + other => panic!("Expected L for {key}, got {other:?}"), + } +} + +#[test] +fn project_two_list_indices_compacted() { + let item = list_item(); + let result = project("mylist[0], mylist[2]", &item, HashMap::new()).unwrap(); + assert_list( + &result, + "mylist", + &[ + AttributeValue::S("zero".into()), + AttributeValue::S("two".into()), + ], + ); +} + +#[test] +fn project_list_indices_ordered_by_index_not_expression() { + let item = list_item(); + // Reversed expression order still comes out in ascending index order. + let result = project("mylist[2], mylist[0]", &item, HashMap::new()).unwrap(); + assert_list( + &result, + "mylist", + &[ + AttributeValue::S("zero".into()), + AttributeValue::S("two".into()), + ], + ); +} + +#[test] +fn project_list_index_gap_compacted() { + let item = list_item(); + let result = project("mylist[1], mylist[3]", &item, HashMap::new()).unwrap(); + assert_list( + &result, + "mylist", + &[ + AttributeValue::S("one".into()), + AttributeValue::S("three".into()), + ], + ); +} + +#[test] +fn project_null_element_by_index_preserved() { + let item = list_item(); + let result = project("with_null[1]", &item, HashMap::new()).unwrap(); + assert_list(&result, "with_null", &[AttributeValue::Null]); +} + +#[test] +fn project_unselected_null_dropped() { + let item = list_item(); + let result = project("with_null[0], with_null[2]", &item, HashMap::new()).unwrap(); + assert_list( + &result, + "with_null", + &[ + AttributeValue::S("keep0".into()), + AttributeValue::S("keep2".into()), + ], + ); +} + +#[test] +fn project_whole_list_preserves_null() { + let item = list_item(); + let result = project("with_null", &item, HashMap::new()).unwrap(); + assert_list( + &result, + "with_null", + &[ + AttributeValue::S("keep0".into()), + AttributeValue::Null, + AttributeValue::S("keep2".into()), + ], + ); +} + +#[test] +fn project_list_of_maps_subfield_multi() { + let mut item = BTreeMap::new(); + item.insert("pk".into(), AttributeValue::S("p".into())); + let mk = |v: &str, x: &str| { + let mut m = BTreeMap::new(); + m.insert("val".into(), AttributeValue::S(v.into())); + m.insert("x".into(), AttributeValue::S(x.into())); + AttributeValue::M(m) + }; + item.insert( + "lom".into(), + AttributeValue::L(vec![mk("a0", "x0"), mk("a1", "x1"), mk("a2", "x2")]), + ); + + let result = project("lom[0].val, lom[2].val", &item, HashMap::new()).unwrap(); + let only_val = |v: &str| { + let mut m = BTreeMap::new(); + m.insert("val".into(), AttributeValue::S(v.into())); + AttributeValue::M(m) + }; + assert_list(&result, "lom", &[only_val("a0"), only_val("a2")]); +} + +#[test] +fn project_same_index_merges_subfields() { + let mut item = BTreeMap::new(); + item.insert("pk".into(), AttributeValue::S("p".into())); + let mut m = BTreeMap::new(); + m.insert("a".into(), AttributeValue::S("av".into())); + m.insert("b".into(), AttributeValue::S("bv".into())); + m.insert("c".into(), AttributeValue::S("cv".into())); + item.insert("l".into(), AttributeValue::L(vec![AttributeValue::M(m)])); + + // Two paths selecting the same element merge their subfields. + let result = project("l[0].a, l[0].c", &item, HashMap::new()).unwrap(); + let mut expected = BTreeMap::new(); + expected.insert("a".into(), AttributeValue::S("av".into())); + expected.insert("c".into(), AttributeValue::S("cv".into())); + assert_list(&result, "l", &[AttributeValue::M(expected)]); +} + +fn check_overlap(expr: &str, names: HashMap) -> Result<(), DynamoDbError> { + let tokens = tokenize(expr).unwrap(); + let paths = parse_projection(&tokens).unwrap(); + let maps = ExpressionMaps::new(names, HashMap::new()); + Projection::compile(&paths, &maps, true).map(|_| ()) +} + +#[test] +fn compile_without_overlap_rejection_accepts_duplicates() { + // Desugared AttributesToGet keeps the legacy accept-duplicates behavior. + let tokens = tokenize("a, a").unwrap(); + let paths = parse_projection(&tokens).unwrap(); + let maps = ExpressionMaps::new(HashMap::new(), HashMap::new()); + assert!(Projection::compile(&paths, &maps, false).is_ok()); +} + +#[test] +fn compiled_projection_reusable_across_items() { + let tokens = tokenize("name").unwrap(); + let paths = parse_projection(&tokens).unwrap(); + let maps = ExpressionMaps::new(HashMap::new(), HashMap::new()); + let proj = Projection::compile(&paths, &maps, true).unwrap(); + let item = sample_item(); + for _ in 0..2 { + let out = proj.apply(&item); + assert_eq!(out.len(), 1); + assert!(out.contains_key("name")); + } +} + +fn overlap_msg(expr: &str) -> String { + match check_overlap(expr, HashMap::new()) { + Err(DynamoDbError::ValidationException(m)) => m, + other => panic!("expected ValidationException, got {other:?}"), + } +} + +#[test] +fn overlap_parent_then_child() { + assert_eq!( + overlap_msg("a, a.b"), + "Invalid ProjectionExpression: Two document paths overlap with each other; \ + must remove or rewrite one of these paths; path one: [a], path two: [a, b]" + ); +} + +#[test] +fn overlap_child_then_parent() { + assert_eq!( + overlap_msg("a.b, a"), + "Invalid ProjectionExpression: Two document paths overlap with each other; \ + must remove or rewrite one of these paths; path one: [a, b], path two: [a]" + ); +} + +#[test] +fn overlap_exact_duplicate() { + assert_eq!( + overlap_msg("a, a"), + "Invalid ProjectionExpression: Two document paths overlap with each other; \ + must remove or rewrite one of these paths; path one: [a], path two: [a]" + ); +} + +#[test] +fn overlap_index_path_rendering() { + assert_eq!( + overlap_msg("a[0], a"), + "Invalid ProjectionExpression: Two document paths overlap with each other; \ + must remove or rewrite one of these paths; path one: [a, [0]], path two: [a]" + ); + assert_eq!( + overlap_msg("a[0].b, a[0]"), + "Invalid ProjectionExpression: Two document paths overlap with each other; \ + must remove or rewrite one of these paths; path one: [a, [0], b], path two: [a, [0]]" + ); +} + +#[test] +fn overlap_reports_first_pair_in_order() { + assert_eq!( + overlap_msg("x, a.b, a"), + "Invalid ProjectionExpression: Two document paths overlap with each other; \ + must remove or rewrite one of these paths; path one: [a, b], path two: [a]" + ); +} + +#[test] +fn overlap_uses_resolved_names() { + let mut names = HashMap::new(); + names.insert("p".into(), "a".into()); + names.insert("q".into(), "b".into()); + match check_overlap("#p, #p.#q", names) { + Err(DynamoDbError::ValidationException(m)) => assert_eq!( + m, + "Invalid ProjectionExpression: Two document paths overlap with each other; \ + must remove or rewrite one of these paths; path one: [a], path two: [a, b]" + ), + other => panic!("expected ValidationException, got {other:?}"), + } +} + +#[test] +fn no_overlap_for_siblings() { + assert!(check_overlap("a.b, a.c", HashMap::new()).is_ok()); + assert!(check_overlap("a[0], a[1]", HashMap::new()).is_ok()); + assert!(check_overlap("a, b, c", HashMap::new()).is_ok()); +} + +#[test] +fn project_list_index_into_map_preserves_structure() { + let mut item = BTreeMap::new(); + item.insert("pk".into(), AttributeValue::S("p".into())); + let mut map0 = BTreeMap::new(); + map0.insert("val".into(), AttributeValue::S("target".into())); + map0.insert("x".into(), AttributeValue::S("0".into())); + let mut map1 = BTreeMap::new(); + map1.insert("x".into(), AttributeValue::S("0".into())); + item.insert( + "a_list".into(), + AttributeValue::L(vec![AttributeValue::M(map0), AttributeValue::M(map1)]), + ); + + let result = project("a_list[0].val", &item, HashMap::new()).unwrap(); + // Should preserve: {"a_list": [{"val": "target"}]} + match result.get("a_list") { + Some(AttributeValue::L(list)) => { + assert_eq!(list.len(), 1); + match &list[0] { + AttributeValue::M(m) => { + assert_eq!(m.get("val"), Some(&AttributeValue::S("target".into()))); + assert!(!m.contains_key("x")); + } + other => panic!("Expected M, got {other:?}"), + } + } + other => panic!("Expected L, got {other:?}"), + } +} + +// Syntax error wording measured 2026-10-25 (us-east-1) on GetItem. + +fn proj_msg(src: &str) -> String { + match parse_projection_from(&tokenize(src).unwrap(), src) { + Err(DynamoDbError::ValidationException(m)) => m, + other => panic!("expected ValidationException, got {other:?}"), + } +} + +#[test] +fn doubled_comma_quotes_the_source_window() { + // [d27] `#v, , #v` + assert_eq!( + proj_msg("#v, , #v"), + "Invalid ProjectionExpression: Syntax error; token: \",\", near: \", , #v\"" + ); +} + +#[test] +fn trailing_comma_is_eof_with_the_comma_as_window() { + // [d28] `#v,` + assert_eq!( + proj_msg("#v,"), + "Invalid ProjectionExpression: Syntax error; token: \"\", near: \",\"" + ); +} + +#[test] +fn other_projection_texts_are_unchanged() { + assert_eq!( + proj_msg("#v #w"), + "Invalid ProjectionExpression: unexpected token at position 1" + ); + assert_eq!( + proj_msg("#v, )"), + "Invalid expression: expected attribute name" + ); +} diff --git a/crates/core/src/expression/syntax_error.rs b/crates/core/src/expression/syntax_error.rs new file mode 100644 index 000000000..2ae935c5f --- /dev/null +++ b/crates/core/src/expression/syntax_error.rs @@ -0,0 +1,265 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! The `Syntax error; token: "", near: ""` wording Amazon `DynamoDB` +//! uses when a parser meets a token it cannot accept. +//! +//! Measured 2026-10-25 (us-east-1) on `ConditionExpression`, `FilterExpression`, +//! `KeyConditionExpression`, `UpdateExpression` and `ProjectionExpression`: +//! the token is the offending token's text, or `` when the input ends +//! where a token is required; the window is the source text from the start of +//! the token before the offending one to the end of the token after it, with +//! the source's own whitespace kept (`#v==:v` answers `near: "==:v"`) and a +//! missing neighbour dropped (`= #v = :v` answers `near: "= #v"`, `#v = :v =` +//! answers `near: ":v ="`). For `` the window is the last token's text +//! (`#v = :v AND` answers `token: "", near: "AND"`). + +use std::ops::Range; + +use super::kind::ExpressionKind; +use super::tokenizer::Token; +use crate::error::DynamoDbError; + +/// The text a token is shown as in a syntax error when the source it came +/// from is not available. With a source, the token is shown as written. +#[must_use] +pub fn token_text(token: &Token) -> String { + match token { + Token::Ident(s) => s.clone(), + Token::NameRef(s) => format!("#{s}"), + Token::Placeholder(s) => format!(":{s}"), + Token::Eq => "=".to_owned(), + Token::Ne => "<>".to_owned(), + Token::Lt => "<".to_owned(), + Token::Le => "<=".to_owned(), + Token::Gt => ">".to_owned(), + Token::Ge => ">=".to_owned(), + Token::Plus => "+".to_owned(), + Token::Minus => "-".to_owned(), + Token::Comma => ",".to_owned(), + Token::Dot => ".".to_owned(), + Token::LBracket => "[".to_owned(), + Token::RBracket => "]".to_owned(), + Token::LParen => "(".to_owned(), + Token::RParen => ")".to_owned(), + Token::And => "AND".to_owned(), + Token::Or => "OR".to_owned(), + Token::Not => "NOT".to_owned(), + Token::Between => "BETWEEN".to_owned(), + Token::In => "IN".to_owned(), + Token::Set => "SET".to_owned(), + Token::Remove => "REMOVE".to_owned(), + Token::Add => "ADD".to_owned(), + Token::Delete => "DELETE".to_owned(), + } +} + +/// Whether `token` is an operator (`=`, `<>`, `<`, `<=`, `>`, `>=`, `+`, `-`) or +/// a keyword (`AND`, `OR`, `NOT`, `BETWEEN`, `IN`, `SET`, `REMOVE`, `ADD`, +/// `DELETE`): the token classes measured to answer the service's syntax error +/// when they appear where an operand or a connector is required. Punctuation +/// and operands are not in the measured set and keep their previous texts. +#[must_use] +pub fn is_operator_or_keyword(token: &Token) -> bool { + !matches!( + token, + Token::Ident(_) + | Token::NameRef(_) + | Token::Placeholder(_) + | Token::Comma + | Token::Dot + | Token::LBracket + | Token::RBracket + | Token::LParen + | Token::RParen + ) +} + +/// The byte length a token occupies in the source it was read from. Keywords +/// are matched case-insensitively by the tokenizer, so their length is the +/// length of the keyword; every other token's text is reproduced verbatim. +fn token_len(token: &Token) -> usize { + match token { + Token::Ident(s) => s.len(), + Token::NameRef(s) | Token::Placeholder(s) => 1 + s.len(), + _ => token_text(token).len(), + } +} + +/// Byte ranges of `tokens` in `source`, recovered by walking the source with +/// the tokenizer's whitespace rule (space, tab, newline, carriage return). +/// Returns `None` when the tokens do not line up with the source (a caller +/// that passed a source the tokens did not come from), so the window falls +/// back to the token texts. +fn token_spans(source: &str, tokens: &[Token]) -> Option>> { + let bytes = source.as_bytes(); + let mut spans = Vec::with_capacity(tokens.len()); + let mut i = 0; + for token in tokens { + while i < bytes.len() && matches!(bytes[i], b' ' | b'\t' | b'\n' | b'\r') { + i += 1; + } + let end = i + token_len(token); + if end > bytes.len() { + return None; + } + spans.push(i..end); + i = end; + } + Some(spans) +} + +/// Build the service's syntax error for the token at `pos` (or `` when +/// `pos == tokens.len()`), prefixed with `Invalid :`. +/// +/// `source` is the expression text the tokens came from; an empty source +/// (a caller holding only tokens) joins the token texts with single spaces. +#[must_use] +pub fn syntax_error( + kind: ExpressionKind, + source: &str, + tokens: &[Token], + pos: usize, +) -> DynamoDbError { + // The token is shown as the client wrote it: a lower-case `and` answers + // `token: "and"` (measured 2026-10-25 on FilterExpression, + // ConditionExpression and KeyConditionExpression). + let spans = (!source.is_empty()) + .then(|| token_spans(source, tokens)) + .flatten(); + let token = match (tokens.get(pos), &spans) { + (None, _) => "".to_owned(), + (Some(_), Some(spans)) => source[spans[pos].clone()].to_owned(), + (Some(t), None) => token_text(t), + }; + let near = near_window(source, tokens, pos); + DynamoDbError::ValidationException(format!( + "Invalid {kind}: Syntax error; token: \"{token}\", near: \"{near}\"" + )) +} + +/// The window of source text around the token at `pos`: from the previous +/// token to the next one, each dropped when absent. When the faulty token is +/// the first one, the window starts at the start of the source, leading +/// whitespace kept (measured 2026-10-25: ` = #v = :v` answers +/// `near: " = #v"`). At end of input the window is the last token alone. +fn near_window(source: &str, tokens: &[Token], pos: usize) -> String { + if tokens.is_empty() { + return String::new(); + } + let (first, last) = if pos >= tokens.len() { + (tokens.len() - 1, tokens.len() - 1) + } else { + (pos.saturating_sub(1), (pos + 1).min(tokens.len() - 1)) + }; + if let Some(spans) = (!source.is_empty()) + .then(|| token_spans(source, tokens)) + .flatten() + { + let start = if pos == 0 { 0 } else { spans[first].start }; + return source[start..spans[last].end].to_owned(); + } + tokens[first..=last] + .iter() + .map(token_text) + .collect::>() + .join(" ") +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::expression::tokenizer::tokenize; + + fn msg(kind: ExpressionKind, source: &str, pos: usize) -> String { + let tokens = tokenize(source).unwrap(); + match syntax_error(kind, source, &tokens, pos) { + DynamoDbError::ValidationException(m) => m, + other => panic!("{other:?}"), + } + } + + // Expected texts measured 2026-10-25 (us-east-1), probe cases in brackets. + + #[test] + fn repeated_operator_in_the_middle_shows_three_tokens() { + // [d01] `#v = = :v`: the second `=` is the offending token. + assert_eq!( + msg(ExpressionKind::Condition, "#v = = :v", 2), + "Invalid ConditionExpression: Syntax error; token: \"=\", near: \"= = :v\"" + ); + } + + #[test] + fn window_keeps_the_source_whitespace() { + // [d17] no spaces, [d18] a tab, [d12] longer tokens. + assert!(msg(ExpressionKind::Condition, "#v==:v", 2).ends_with("near: \"==:v\"")); + assert!(msg(ExpressionKind::Condition, "#v =\t= :v", 2).ends_with("near: \"=\t= :v\"")); + assert!( + msg(ExpressionKind::Condition, "#longname = = :longvalue", 2) + .ends_with("near: \"= = :longvalue\"") + ); + } + + #[test] + fn missing_neighbours_are_dropped() { + // [d03] at the start, [d04] at the end, [d29] alone, [d30] two tokens. + assert!(msg(ExpressionKind::Condition, "= #v = :v", 0).ends_with("near: \"= #v\"")); + assert!(msg(ExpressionKind::Condition, "#v = :v =", 3).ends_with("near: \":v =\"")); + assert!(msg(ExpressionKind::Condition, "=", 0).ends_with("near: \"=\"")); + assert!(msg(ExpressionKind::Condition, "= =", 0).ends_with("near: \"= =\"")); + } + + #[test] + fn a_fault_at_the_first_token_keeps_leading_whitespace() { + // Measured 2026-10-25 on FilterExpression, KeyConditionExpression and + // ProjectionExpression. + assert!(msg(ExpressionKind::Filter, " = #v = :v", 0).ends_with("near: \" = #v\"")); + assert!(msg(ExpressionKind::Filter, "\t= #v = :v", 0).ends_with("near: \"\t= #v\"")); + assert!(msg(ExpressionKind::Filter, " AND #v = :v", 0).ends_with("near: \" AND #v\"")); + assert!(msg(ExpressionKind::KeyCondition, " = pk = :v", 0).ends_with("near: \" = pk\"")); + assert!(msg(ExpressionKind::Projection, " , #v", 0).ends_with("near: \" , #v\"")); + assert!(msg(ExpressionKind::Filter, "= #v = :v", 0).ends_with("near: \"= #v\"")); + } + + #[test] + fn end_of_input_is_eof_with_the_last_token() { + // [d05] trailing AND, [d26] trailing comma on an update. + assert_eq!( + msg(ExpressionKind::Condition, "#v = :v AND", 4), + "Invalid ConditionExpression: Syntax error; token: \"\", near: \"AND\"" + ); + assert_eq!( + msg(ExpressionKind::Update, "SET #v = :v,", 5), + "Invalid UpdateExpression: Syntax error; token: \"\", near: \",\"" + ); + } + + #[test] + fn keyword_tokens_show_the_source_text() { + // [d07] doubled AND; a lower-case keyword (measured 2026-10-25) is + // shown as written, in the token and in the window. + assert_eq!( + msg(ExpressionKind::Condition, "#v = :v AND AND #v = :v", 4), + "Invalid ConditionExpression: Syntax error; token: \"AND\", near: \"AND AND #v\"" + ); + assert_eq!( + msg(ExpressionKind::Filter, "#v = :v and and #v = :v", 4), + "Invalid FilterExpression: Syntax error; token: \"and\", near: \"and and #v\"" + ); + } + + #[test] + fn without_a_source_the_window_joins_token_texts() { + let tokens = tokenize("#v = = :v").unwrap(); + let DynamoDbError::ValidationException(m) = + syntax_error(ExpressionKind::Filter, "", &tokens, 2) + else { + panic!() + }; + assert_eq!( + m, + "Invalid FilterExpression: Syntax error; token: \"=\", near: \"= = :v\"" + ); + } +} diff --git a/crates/core/src/expression/update_parser.rs b/crates/core/src/expression/update_parser.rs index a67c22e6c..333e9302b 100755 --- a/crates/core/src/expression/update_parser.rs +++ b/crates/core/src/expression/update_parser.rs @@ -17,59 +17,30 @@ //! ``` use super::ast::{ArithOp, Expr, UpdateAction}; +use super::kind::ExpressionKind; use super::parser_common; +use super::syntax_error::{is_operator_or_keyword, syntax_error}; use super::tokenizer::Token; use crate::error::DynamoDbError; +const KIND: ExpressionKind = ExpressionKind::Update; + /// Parse an update expression token stream into a list of actions. /// /// # Errors /// /// Returns `ValidationException` for syntax errors. pub fn parse_update(tokens: &[Token]) -> Result, DynamoDbError> { - parser_common::check_redundant_parens(tokens).map_err(|msg| { - DynamoDbError::ValidationException(format!("Invalid UpdateExpression: {msg}")) - })?; - - let mut pos = 0; - let mut actions = Vec::new(); - - while pos < tokens.len() { - match &tokens[pos] { - Token::Set => { - pos += 1; - parse_set_actions(tokens, &mut pos, &mut actions)?; - } - Token::Remove => { - pos += 1; - parse_remove_actions(tokens, &mut pos, &mut actions)?; - } - Token::Add => { - pos += 1; - parse_add_actions(tokens, &mut pos, &mut actions)?; - } - Token::Delete => { - pos += 1; - parse_delete_actions(tokens, &mut pos, &mut actions)?; - } - _ => { - return Err(validation_err( - "Invalid UpdateExpression: expected SET, REMOVE, ADD, or DELETE", - )); - } - } - } - - if actions.is_empty() { - return Err(validation_err( - "Invalid UpdateExpression: expression must contain at least one action", - )); - } - - Ok(actions) + parse_update_from(tokens, "") } -/// Parse an update expression with DynamoDB-compatible syntax error messages. +/// Parse an update expression whose tokens came from `source`, so a syntax +/// error can quote the source window around the offending token as Amazon +/// `DynamoDB` does. An empty `source` joins the token texts instead. +/// +/// # Errors +/// +/// Returns `ValidationException` for syntax errors. pub fn parse_update_from( tokens: &[Token], source: &str, @@ -85,27 +56,24 @@ pub fn parse_update_from( match &tokens[pos] { Token::Set => { pos += 1; - parse_set_actions(tokens, &mut pos, &mut actions)?; + parse_set_actions(tokens, source, &mut pos, &mut actions)?; } Token::Remove => { pos += 1; - parse_remove_actions(tokens, &mut pos, &mut actions)?; + parse_remove_actions(tokens, source, &mut pos, &mut actions)?; } Token::Add => { pos += 1; - parse_add_actions(tokens, &mut pos, &mut actions)?; + parse_add_actions(tokens, source, &mut pos, &mut actions)?; } Token::Delete => { pos += 1; - parse_delete_actions(tokens, &mut pos, &mut actions)?; - } - token => { - let token_text = token_display_text(token); - let near = build_near_context(source, &token_text); - return Err(validation_err(&format!( - "Invalid UpdateExpression: Syntax error; token: \"{token_text}\", near: \"{near}\"" - ))); + parse_delete_actions(tokens, source, &mut pos, &mut actions)?; } + // A token where a section keyword is required, such as the second + // `+` of `SET #v = :v + :v + :v`, which answers token "+", near + // ":v + :v" (measured 2026-10-25). + _ => return Err(syntax_error(KIND, source, tokens, pos)), } } @@ -118,58 +86,46 @@ pub fn parse_update_from( Ok(actions) } -fn token_display_text(token: &Token) -> String { - match token { - Token::Ident(s) => s.clone(), - Token::NameRef(s) => format!("#{s}"), - Token::Placeholder(s) => format!(":{s}"), - Token::Eq => "=".to_owned(), - Token::Ne => "<>".to_owned(), - Token::Lt => "<".to_owned(), - Token::Le => "<=".to_owned(), - Token::Gt => ">".to_owned(), - Token::Ge => ">=".to_owned(), - Token::Plus => "+".to_owned(), - Token::Minus => "-".to_owned(), - Token::Comma => ",".to_owned(), - Token::Dot => ".".to_owned(), - Token::LBracket => "[".to_owned(), - Token::RBracket => "]".to_owned(), - Token::LParen => "(".to_owned(), - Token::RParen => ")".to_owned(), - Token::And => "AND".to_owned(), - Token::Or => "OR".to_owned(), - Token::Not => "NOT".to_owned(), - Token::Between => "BETWEEN".to_owned(), - Token::In => "IN".to_owned(), - Token::Set => "SET".to_owned(), - Token::Remove => "REMOVE".to_owned(), - Token::Add => "ADD".to_owned(), - Token::Delete => "DELETE".to_owned(), - } -} - -fn build_near_context(source: &str, token_text: &str) -> String { - if source.is_empty() { - return token_text.to_owned(); - } - if let Some(start) = source.find(token_text) { - let end = std::cmp::min(start + token_text.len() + 7, source.len()); - source[start..end].trim_end().to_owned() - } else { - token_text.to_owned() +/// Parse the path that starts an action. Input that ends here (`REMOVE`, +/// `SET #v = :v,`) or an operator here (`SET = :v`) is a syntax error in the +/// service's wording (measured 2026-10-25). +fn parse_action_path( + tokens: &[Token], + src: &str, + pos: &mut usize, +) -> Result, DynamoDbError> { + match tokens.get(*pos) { + None => Err(syntax_error(KIND, src, tokens, *pos)), + Some(token) if is_operator_or_keyword(token) => Err(syntax_error(KIND, src, tokens, *pos)), + _ => parser_common::parse_path(tokens, pos), } } fn parse_set_actions( tokens: &[Token], + src: &str, pos: &mut usize, actions: &mut Vec, ) -> Result<(), DynamoDbError> { loop { - let path = parser_common::parse_path(tokens, pos)?; - parser_common::expect_token(tokens, pos, &Token::Eq, "=", "UpdateExpression")?; - let value = parse_value_expr(tokens, pos)?; + let path = parse_action_path(tokens, src, pos)?; + // `SET #v :v` answers token ":v", near "#v :v" (measured 2026-10-25); + // end of input and an operator or keyword here follow the same rule. + // Any other token keeps the previous text. + match tokens.get(*pos) { + Some(Token::Eq) => {} + None | Some(Token::Placeholder(_)) => { + return Err(syntax_error(KIND, src, tokens, *pos)); + } + Some(token) if is_operator_or_keyword(token) => { + return Err(syntax_error(KIND, src, tokens, *pos)); + } + Some(_) => { + parser_common::expect_token(tokens, pos, &Token::Eq, "=", "UpdateExpression")?; + } + } + *pos += 1; + let value = parse_value_expr(tokens, src, pos)?; actions.push(UpdateAction::Set { path, value }); if *pos < tokens.len() && tokens[*pos] == Token::Comma { @@ -183,11 +139,12 @@ fn parse_set_actions( fn parse_remove_actions( tokens: &[Token], + src: &str, pos: &mut usize, actions: &mut Vec, ) -> Result<(), DynamoDbError> { loop { - let path = parser_common::parse_path(tokens, pos)?; + let path = parse_action_path(tokens, src, pos)?; actions.push(UpdateAction::Remove { path }); if *pos < tokens.len() && tokens[*pos] == Token::Comma { @@ -200,8 +157,8 @@ fn parse_remove_actions( } /// Parse a SET value expression: `operand` or `operand +/- operand`. -fn parse_value_expr(tokens: &[Token], pos: &mut usize) -> Result { - let left = parse_set_operand(tokens, pos)?; +fn parse_value_expr(tokens: &[Token], src: &str, pos: &mut usize) -> Result { + let left = parse_set_operand(tokens, src, pos)?; if *pos < tokens.len() { let arith_op = match &tokens[*pos] { @@ -211,7 +168,7 @@ fn parse_value_expr(tokens: &[Token], pos: &mut usize) -> Result Result Result { +fn parse_set_operand(tokens: &[Token], src: &str, pos: &mut usize) -> Result { if *pos >= tokens.len() { - return Err(validation_err("Invalid UpdateExpression: expected operand")); + // `SET #v = :v +` answers token "", near "+" (measured 2026-10-25). + return Err(syntax_error(KIND, src, tokens, *pos)); } match &tokens[*pos] { @@ -241,7 +199,7 @@ fn parse_set_operand(tokens: &[Token], pos: &mut usize) -> Result Result parser_common::parse_path(tokens, pos).map(Expr::Path), Token::LParen => { *pos += 1; // consume '(' - let expr = parse_value_expr(tokens, pos)?; + let expr = parse_value_expr(tokens, src, pos)?; if *pos >= tokens.len() || tokens[*pos] != Token::RParen { return Err(validation_err( "Invalid UpdateExpression: expected closing ')'", @@ -258,13 +216,21 @@ fn parse_set_operand(tokens: &[Token], pos: &mut usize) -> Result Err(syntax_error(KIND, src, tokens, *pos)), _ => Err(validation_err( "Invalid UpdateExpression: expected path, placeholder, or function", )), } } -fn parse_set_function_call(tokens: &[Token], pos: &mut usize) -> Result { +fn parse_set_function_call( + tokens: &[Token], + src: &str, + pos: &mut usize, +) -> Result { let name = match &tokens[*pos] { Token::Ident(n) => n.to_ascii_lowercase(), _ => { @@ -278,10 +244,10 @@ fn parse_set_function_call(tokens: &[Token], pos: &mut usize) -> Result bool { /// Parse ADD actions: `ADD path value (, path value)*` fn parse_add_actions( tokens: &[Token], + src: &str, pos: &mut usize, actions: &mut Vec, ) -> Result<(), DynamoDbError> { loop { - let path = parser_common::parse_path(tokens, pos)?; + let path = parse_action_path(tokens, src, pos)?; let value = parse_add_delete_value(tokens, pos)?; actions.push(UpdateAction::Add { path, value }); if *pos < tokens.len() && tokens[*pos] == Token::Comma { @@ -315,11 +282,12 @@ fn parse_add_actions( /// Parse DELETE actions: `DELETE path value (, path value)*` fn parse_delete_actions( tokens: &[Token], + src: &str, pos: &mut usize, actions: &mut Vec, ) -> Result<(), DynamoDbError> { loop { - let path = parser_common::parse_path(tokens, pos)?; + let path = parse_action_path(tokens, src, pos)?; let value = parse_add_delete_value(tokens, pos)?; actions.push(UpdateAction::Delete { path, value }); if *pos < tokens.len() && tokens[*pos] == Token::Comma { @@ -517,10 +485,11 @@ mod tests { #[test] fn parse_invalid_action_keyword_rejected() { + // A token where a section keyword is required is the service's syntax + // error; without a source the window joins the token texts. let err = parse("price = :v").unwrap_err(); - assert!( - matches!(err, DynamoDbError::ValidationException(msg) if msg.contains("expected SET")) - ); + assert!(matches!(err, DynamoDbError::ValidationException(msg) + if msg == "Invalid UpdateExpression: Syntax error; token: \"price\", near: \"price =\"")); } #[test] @@ -568,4 +537,41 @@ mod tests { "Expected validation error for stray ')', got: {err:?}" ); } + + // Syntax error wording measured 2026-10-25 (us-east-1) on UpdateItem. + + fn upd_msg(src: &str) -> String { + match parse_update_from(&tokenize(src).unwrap(), src) { + Err(DynamoDbError::ValidationException(m)) => m, + other => panic!("expected ValidationException, got {other:?}"), + } + } + + fn syntax(token: &str, near: &str) -> String { + format!("Invalid UpdateExpression: Syntax error; token: \"{token}\", near: \"{near}\"") + } + + #[test] + fn measured_update_syntax_errors() { + assert_eq!(upd_msg("SET #v = = :v"), syntax("=", "= = :v")); // [d24] + assert_eq!(upd_msg("SET #v = :v + + :v"), syntax("+", "+ + :v")); // [d25] + assert_eq!(upd_msg("SET #v = :v,"), syntax("", ",")); // [d26] + assert_eq!(upd_msg("SET #v = :v + :v + :v"), syntax("+", ":v + :v")); // [b07] + assert_eq!(upd_msg("SET #v = :v +"), syntax("", "+")); // [u08] + assert_eq!(upd_msg("SET #v = :v REMOVE"), syntax("", "REMOVE")); // [u10] + assert_eq!(upd_msg("SET = :v"), syntax("=", "SET = :v")); // [u11] + assert_eq!(upd_msg("SET #v :v"), syntax(":v", "#v :v")); // [u12] + } + + #[test] + fn other_update_texts_are_unchanged() { + assert_eq!( + upd_msg("SET #v #w = :v"), + "Invalid UpdateExpression: expected '='" + ); + assert_eq!( + upd_msg("SET #v = (:v"), + "Invalid UpdateExpression: expected closing ')'" + ); + } } diff --git a/crates/core/src/serde_helpers.rs b/crates/core/src/serde_helpers.rs index 3a4ffb3cd..292bb9bfc 100644 --- a/crates/core/src/serde_helpers.rs +++ b/crates/core/src/serde_helpers.rs @@ -26,24 +26,16 @@ fn is_placeholder_ident(rest: &str) -> bool { /// Validate one placeholder key. Length is checked before syntax. /// -/// `include_size_in_too_long`: the Names too-long message appends -/// `; size of key: N`, the Values one does not (Amazon `DynamoDB` quirk). -fn validate_placeholder_key( - key: &str, - prefix: char, - field_name: &str, - include_size_in_too_long: bool, -) -> Result<(), String> { +/// The too-long message carries `; size of key: N` for both maps: measured +/// 2026-10-25 (us-east-1) on PutItem, Scan and Query for +/// `ExpressionAttributeValues` as well as `ExpressionAttributeNames`. +fn validate_placeholder_key(key: &str, prefix: char, field_name: &str) -> Result<(), String> { if key.len() > MAX_PLACEHOLDER_KEY_BYTES { - let mut msg = format!( + return Err(format!( "{field_name} contains invalid key: The expression attribute map \ - contains a key that is too long;" - ); - if include_size_in_too_long { - use std::fmt::Write as _; - let _ = write!(msg, " size of key: {}", key.len()); - } - return Err(msg); + contains a key that is too long; size of key: {}", + key.len() + )); } if !key.strip_prefix(prefix).is_some_and(is_placeholder_ident) { return Err(format!( @@ -61,7 +53,6 @@ fn deserialize_placeholder_map<'de, D, T>( deserializer: D, prefix: char, field_name: &str, - include_size_in_too_long: bool, check_value: impl Fn(&str, &Value) -> Result<(), String>, convert: impl Fn(Value) -> Result, ) -> Result>, D::Error> @@ -76,8 +67,7 @@ where } let mut out = HashMap::with_capacity(raw.len()); for (key, value) in raw { - validate_placeholder_key(&key, prefix, field_name, include_size_in_too_long) - .map_err(de::Error::custom)?; + validate_placeholder_key(&key, prefix, field_name).map_err(de::Error::custom)?; check_value(&key, &value).map_err(de::Error::custom)?; let converted = convert(value).map_err(de::Error::custom)?; out.insert(key, converted); @@ -101,7 +91,6 @@ where deserializer, '#', "ExpressionAttributeNames", - true, |key, value| { // Only a genuine empty *string* is rejected. A non-string falls // through to `convert` -> type error -> SerializationException, @@ -134,7 +123,6 @@ where deserializer, ':', "ExpressionAttributeValues", - false, |key, value| { // Empty AttributeValue (no datatype). Caught here so the error names // its key. Mirrors the empty-object check in `types::attribute_value`; @@ -265,13 +253,19 @@ mod tests { } #[test] - fn values_key_too_long_omits_size() { + fn values_key_too_long_reports_size() { + // Measured 2026-10-25 (us-east-1), PutItem, Scan and Query: the Values + // message carries the size suffix like the Names one. let key = format!(":{}", "a".repeat(255)); // 256 bytes including ':' let json = format!(r#"{{"values":{{"{key}":{{"S":"x"}}}}}}"#); let msg = values_err(&json); - assert!(msg.contains("contains a key that is too long;"), "{msg}"); - // Quirk: the values variant omits "; size of key: N". - assert!(!msg.contains("size of key"), "{msg}"); + assert!( + msg.contains( + "ExpressionAttributeValues contains invalid key: The expression \ + attribute map contains a key that is too long; size of key: 256" + ), + "{msg}" + ); } #[test] diff --git a/crates/engine/src/expression_helpers.rs b/crates/engine/src/expression_helpers.rs index 913333015..571dc849c 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_from, parse_key_condition_from, parse_projection_from, parse_update_from, + tokenize_for, tokenize_with_limit, validate_no_reserved_words, validate_operator_count, + validate_ordering_operand_types, }; use extenddb_core::limits::LimitsConfig; use extenddb_core::types::{AttributeValue, ConditionalOperator, ExpectedAttributeValue}; @@ -89,10 +90,26 @@ pub fn check_expression_size( pub fn parse_condition_expr(expr: &str, limits: &LimitsConfig) -> Result { check_expression_size(expr, ExpressionKind::Condition, limits)?; tokenize_expression(expr, limits) - .and_then(|tokens| parse_condition_with_depth_limit(&tokens, limits.max_expression_depth)) + .and_then(|tokens| parse_condition_from(&tokens, expr, limits.max_expression_depth)) .map_err(|e| prefix_expression_error(e, ExpressionKind::Condition)) } +/// Refuse a condition or filter whose operator count exceeds the service's cap +/// of 300 (`Invalid : The expression contains too many operators; +/// operator count: `). +/// +/// Measured 2026-10-25 (us-east-1): the check runs after the syntax, reserved +/// word, undefined placeholder and operand type checks and before the unused +/// placeholder check and the table lookup. `TransactWriteItems` counts a +/// different total for the same text and does not call this. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` when the count is over the cap. +pub fn check_operator_count(expr: &Expr, kind: ExpressionKind) -> Result<(), DynamoDbError> { + validate_operator_count(expr, kind) +} + /// Tokenize, reserved-word check, and parse an `UpdateExpression`. /// /// Errors carry the `UpdateExpression` prefix, matching Amazon DynamoDB. @@ -205,6 +222,9 @@ pub fn resolve_condition( if let Some(ref expr) = condition { validate_ordering_operand_types(expr, &maps) .map_err(|e| prefix_expression_error(e, ExpressionKind::Condition))?; + // After the operand type check (an ordering operator on a BOOL beats + // the count), before the unused placeholder check and the table lookup. + check_operator_count(expr, ExpressionKind::Condition)?; } Ok((condition, maps)) } @@ -230,7 +250,7 @@ pub fn parse_projection_expr( if limits.enforce_reserved_keywords { validate_no_reserved_words(&tokens)?; } - parse_projection(&tokens) + parse_projection_from(&tokens, proj_str) }); result.map_err(|e| prefix_expression_error(e, ExpressionKind::Projection)) } @@ -257,7 +277,17 @@ pub fn parse_key_condition_expr( if limits.enforce_reserved_keywords { validate_no_reserved_words(&tokens)?; } - parse_key_condition(&tokens) + // The service counts operators on the whole expression before it + // applies the key condition grammar: `pk = :v OR` + 150 terms answers + // the count, not the operator refusal, and 151 AND-joined equalities + // answer the count ahead of the one-condition-per-key check (measured + // 2026-10-25). The general condition parser supplies the count; when + // it does not accept the text, the key condition parser reports the + // error in its own words. + if let Ok(expr) = parse_condition_from(&tokens, expr, limits.max_expression_depth) { + check_operator_count(&expr, ExpressionKind::KeyCondition)?; + } + parse_key_condition_from(&tokens, expr) }) .map_err(|e| prefix_expression_error(e, ExpressionKind::KeyCondition)) } @@ -407,4 +437,94 @@ mod tests { "got {err:?}" ); } + + // Operator cap placement, measured 2026-10-25 (us-east-1). + + fn chain(n: usize) -> String { + vec!["#v = :v"; n].join(" OR ") + } + + #[test] + fn count_over_the_cap_is_refused_with_the_full_count() { + let limits = LimitsConfig::default(); + // 151 comparisons and 150 ORs: 301 [a30]. + let err = + resolve_condition(Some(&chain(151)), None, None, None, None, &limits).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Invalid ConditionExpression: The expression contains too many operators; operator count: 301"), + "got {err:?}" + ); + // 150 comparisons and 149 ORs: 299, accepted [a30]. + assert!(resolve_condition(Some(&chain(150)), None, None, None, None, &limits).is_ok()); + } + + #[test] + fn ordering_operand_type_beats_the_count() { + // [p17] 299 operators plus `#v < :b` with a BOOL value. + let limits = LimitsConfig::default(); + let expr = format!("{} OR #v < :b", chain(150)); + let mut values = HashMap::new(); + values.insert(":v".to_owned(), AttributeValue::S("x".to_owned())); + values.insert(":b".to_owned(), AttributeValue::Bool(true)); + let err = + resolve_condition(Some(&expr), None, Some(&values), None, None, &limits).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg.contains("Incorrect operand type for operator or function; operator or function: <, operand type: BOOL")), + "got {err:?}" + ); + } + + #[test] + fn syntax_error_beats_the_count() { + // [e04] a 300-term chain followed by ` = =`. + let limits = LimitsConfig::default(); + let expr = format!("{} = =", chain(300)); + let err = resolve_condition(Some(&expr), None, None, None, None, &limits).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Invalid ConditionExpression: Syntax error; token: \"=\", near: \":v = =\""), + "got {err:?}" + ); + } + + #[test] + fn key_condition_count_runs_before_the_key_grammar() { + let limits = LimitsConfig::default(); + // [a62] `pk = :v OR` + 150 terms: 301, the count, not the OR refusal. + let expr = format!("pk = :v OR {}", vec!["#v = :v"; 150].join(" AND ")); + let err = parse_key_condition_expr(&expr, &limits).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Invalid KeyConditionExpression: The expression contains too many operators; operator count: 301"), + "got {err:?}" + ); + // [a63] two conditions joined by OR under the cap: the key grammar answers. + let err = parse_key_condition_expr("pk = :v OR sk = :s", &limits).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Invalid KeyConditionExpression: only AND is supported between key conditions"), + "got {err:?}" + ); + // [d23] a syntax error is reported by the key grammar in the service's words. + let err = parse_key_condition_expr("pk = :v AND", &limits).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Invalid KeyConditionExpression: Syntax error; token: \"\", near: \"AND\""), + "got {err:?}" + ); + } + + #[test] + fn projection_syntax_error_quotes_the_source() { + // [d27] + let limits = LimitsConfig::default(); + let err = parse_projection_expr("#v, , #v", &limits).unwrap_err(); + assert!( + matches!(&err, DynamoDbError::ValidationException(msg) + if msg == "Invalid ProjectionExpression: Syntax error; token: \",\", near: \", , #v\""), + "got {err:?}" + ); + } } diff --git a/crates/engine/src/put_item.rs b/crates/engine/src/put_item.rs index ffd6f70b4..f03bd67b5 100755 --- a/crates/engine/src/put_item.rs +++ b/crates/engine/src/put_item.rs @@ -74,25 +74,11 @@ pub async fn handle_put_item( )?; crate::validate_put_delete_return_values(&body)?; - let input: PutItemInput = serde_json::from_value(body).map_err(|e| { - let msg = e.to_string(); - if msg.contains("parameter values were invalid") - || msg.contains("may not be empty") - || msg.contains("contains duplicates") - || msg.contains("Null attribute value") - || msg.contains("validation error detected") - || msg.contains("must not be empty") - || msg.contains("Syntax error; key") - || msg.contains("AttributeValue is empty") - || msg.contains("AttributeValue has more than one datatypes set") - { - DynamoDbError::ValidationException(msg) - } else { - DynamoDbError::SerializationException(format!( - "Start of structure or map found where not expected: {e}" - )) - } - })?; + // The shared classifier also recognises the placeholder map errors + // (`contains invalid key`, `contains invalid value`): a too-long + // ExpressionAttributeNames or ExpressionAttributeValues key is a + // ValidationException, as on every other handler (measured 2026-10-25). + let input: PutItemInput = serde_json::from_value(body).map_err(crate::deserialize_error)?; // Reject mixing legacy and expression parameters, then EAN/EAV supplied // without a referencing expression. diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index 848f5fdbb..efe7d6843 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -358,6 +358,13 @@ pub async fn handle_query( if let Some(ref filter_expr) = filter { let names = input.expression_attribute_names.as_ref(); validate_name_refs_in_expr(filter_expr, names, ExpressionKind::Filter)?; + // The operator cap follows the undefined name check and precedes the + // unused placeholder check (measured 2026-10-25). It applies to a + // FilterExpression only: a legacy QueryFilter of 160 conditions is + // accepted (measured 2026-10-25). + if filter_maps.is_none() { + crate::expression_helpers::check_operator_count(filter_expr, ExpressionKind::Filter)?; + } } // Parse ProjectionExpression or desugar legacy AttributesToGet diff --git a/crates/engine/src/scan.rs b/crates/engine/src/scan.rs index 1c1de2274..4b413b9a5 100755 --- a/crates/engine/src/scan.rs +++ b/crates/engine/src/scan.rs @@ -139,6 +139,16 @@ pub async fn handle_scan( ) }; + // The operator cap runs before the unused placeholder check and the table + // lookup (measured 2026-10-25: the count beats an unused value, a missing + // table and an unknown IndexName). It applies to a FilterExpression only: + // a legacy ScanFilter of 160 conditions is accepted (measured 2026-10-25). + if filter_maps.is_none() + && let Some(ref f) = filter + { + crate::expression_helpers::check_operator_count(f, ExpressionKind::Filter)?; + } + // Parse ProjectionExpression or desugar legacy AttributesToGet let (effective_projection_str, extra_proj_names) = if input.projection_expression.is_some() { (input.projection_expression.clone(), HashMap::new()) diff --git a/tests/test_expression_attribute_map_validation.py b/tests/test_expression_attribute_map_validation.py index f073452a4..763dfc725 100644 --- a/tests/test_expression_attribute_map_validation.py +++ b/tests/test_expression_attribute_map_validation.py @@ -183,11 +183,11 @@ def test_value_key_too_long(self, dynamodb_client_no_validation, hash_table): ) err = exc_info.value.response["Error"] assert err["Code"] == "ValidationException" - # Quirk: unlike ExpressionAttributeNames, the values variant omits the - # "; size of key: N" suffix. + # Measured 2026-10-25 (us-east-1) on Scan, Query and PutItem: the values + # variant carries the "; size of key: N" suffix like the names one. assert err["Message"] == ( "ExpressionAttributeValues contains invalid key: The expression " - "attribute map contains a key that is too long;" + "attribute map contains a key that is too long; size of key: 256" ) def test_value_empty_attribute_value(self, dynamodb_client_no_validation, hash_table): diff --git a/tests/test_expression_operator_limits.py b/tests/test_expression_operator_limits.py new file mode 100644 index 000000000..211f39a8d --- /dev/null +++ b/tests/test_expression_operator_limits.py @@ -0,0 +1,652 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Expression operator cap, placeholder key length and syntax error wording. + +Three request shapes Amazon DynamoDB refuses with texts ExtendDB did not match: + + - an expression with more than 300 operators: + ``Invalid : The expression contains too many operators; operator + count: ``, on ConditionExpression (PutItem, UpdateItem, DeleteItem), + FilterExpression (Scan, Query) and KeyConditionExpression (Query). Every + comparison operator, AND, OR, NOT, BETWEEN, IN and function call counts + one; parentheses, paths, placeholders and list elements count zero. + - an ExpressionAttributeNames or ExpressionAttributeValues key over 255 + bytes on PutItem: a ValidationException carrying ``size of key: ``, + not a SerializationException. + - a repeated or misplaced operator or keyword: ``Syntax error; token: "", + near: ""`` where the window is the source text from the token + before the offending one to the token after it, and ```` with the + last token as the window when the input ends early. + +Every expected text was measured against Amazon DynamoDB on 2026-10-25 +(us-east-1); the probe case id is noted on each assertion. The write paths +carry a ``1 validation error detected: `` prefix on the service that ExtendDB +does not emit; those assertions compare the text after the prefix. + +Dual-target against Amazon DynamoDB and extenddb. +""" + +from __future__ import annotations + +import os + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +PREFIX = "1 validation error detected: " + +TOO_MANY = "The expression contains too many operators; operator count: " + +# On MongoDB an UpdateItem condition is compiled to a filter whose OR and AND +# chains nest one level per operator, and a chain of 150 terms fails inside +# MongoDB with InternalServerError. This change does not touch that compiler. +# The MongoDB test runner sets EXTENDDB_TEST_MONGODB_CONTAINER. +# TODO: remove this marker once the MongoDB filter compiler flattens chains. +XFAIL_ON_MONGODB_DEEP_OR = pytest.mark.xfail( + bool(os.environ.get("EXTENDDB_TEST_MONGODB_CONTAINER", "").strip()), + reason="MongoDB answers InternalServerError for a condition chain of 150 terms", + raises=AssertionError, + strict=True, +) + + +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"] + + +def _validation(func) -> str: + code, msg = _error(func) + assert code == "ValidationException", (code, msg) + return msg + + +def _after_prefix(msg: str) -> str: + return msg[len(PREFIX) :] if msg.startswith(PREFIX) else msg + + +def chain(term: str, n: int, joiner: str = " OR ") -> str: + return joiner.join([term] * n) + + +def syntax(kind: str, token: str, near: str) -> str: + return f'Invalid {kind}: Syntax error; token: "{token}", near: "{near}"' + + +@pytest.fixture(scope="module") +def table(dynamodb_client): + """Composite-key table with one item for the Query and Scan rows.""" + 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: + dynamodb_client.put_item( + TableName=name, + Item={"pk": {"S": "p1"}, "sk": {"S": "s1"}, "v": {"S": "x"}}, + ) + yield name + + +NAMES = {"#v": "v"} +VALUES = {":v": {"S": "x"}} + + +def put(client, table, condition, **kwargs): + args = { + "TableName": table, + "Item": {"pk": {"S": "absent"}, "sk": {"S": "absent"}}, + "ConditionExpression": condition, + "ExpressionAttributeNames": NAMES, + "ExpressionAttributeValues": VALUES, + } + args.update(kwargs) + return lambda: client.put_item(**args) + + +def scan(client, table, filter_expr, **kwargs): + args = { + "TableName": table, + "FilterExpression": filter_expr, + "ExpressionAttributeNames": NAMES, + "ExpressionAttributeValues": VALUES, + } + args.update(kwargs) + return lambda: client.scan(**args) + + +def query(client, table, kce, **kwargs): + args = { + "TableName": table, + "KeyConditionExpression": kce, + "ExpressionAttributeNames": NAMES, + "ExpressionAttributeValues": VALUES, + } + args.update(kwargs) + return lambda: client.query(**args) + + +class TestOperatorCount: + """More than 300 operators is refused with the full count.""" + + @pytest.mark.parametrize( + "case, expr, count", + [ + ("a01", "attribute_not_exists(pk) OR " + chain("#v = :v", 300), 601), + ("a02", chain("#v = :v", 300), 599), + ("a03", chain("#v = :v", 300, " AND "), 599), + ("a05", chain("#v <> :v", 300), 599), + ("a09", chain("#v >= :v", 300), 599), + ("a10", chain("NOT #v = :v", 200), 599), + ("a22", chain("NOT NOT #v = :v", 200), 799), + ("a12", chain("#v IN (:v)", 200), 399), + ("a19", chain("(#v = :v)", 300), 599), + ("a40", chain("#v BETWEEN :v AND :v", 160), 319), + ("a41", chain("#v IN (:v, :v, :v)", 170), 339), + ("a43", chain("attribute_exists(#v)", 170), 339), + ("a45", chain("contains(#v, :v)", 200), 399), + ("a47", chain("size(#v) = :v", 230), 689), + ("a48", chain("#v = :v AND #v = :v", 170), 679), + ("a51", chain("((#v = :v) OR (#v = :v))", 140), 559), + ("a52", "attribute_exists(pk) AND NOT (" + chain("#v = :v", 150) + ")", 302), + ("a53", chain("size(#v) BETWEEN :v AND :v", 130), 389), + ("a30", chain("#v = :v", 151), 301), + ("a50", "NOT attribute_not_exists(pk) OR " + chain("#v = :v", 150), 302), + ], + ) + def test_put_item_condition_count(self, dynamodb_client, table, case, expr, count): + msg = _validation(put(dynamodb_client, table, expr)) + assert _after_prefix(msg) == f"Invalid ConditionExpression: {TOO_MANY}{count}", case + + @pytest.mark.parametrize( + "case, expr", + [ + ("a30", chain("#v = :v", 150)), # 299 + ("a49", "NOT attribute_not_exists(pk) OR " + chain("#v = :v", 149)), # 300 + ("a42", chain("#v IN (:v, :v, :v, :v, :v)", 130)), # 259 + ("a54", chain("#v IN (:v)", 150)), # 299 + ("a46", chain("attribute_type(#v, :v)", 150)), # 299 + ], + ) + def test_put_item_condition_at_or_under_the_cap_is_evaluated( + self, dynamodb_client, table, case, expr + ): + # The condition is false for the absent key, so nothing is written. + code, _ = _error(put(dynamodb_client, table, expr)) + assert code == "ConditionalCheckFailedException", case + + def test_update_item_condition_count(self, dynamodb_client, table): + # [u01] 599 refused; [u03] 300 accepted (condition evaluated). + msg = _validation( + lambda: dynamodb_client.update_item( + TableName=table, + Key={"pk": {"S": "absent"}, "sk": {"S": "absent"}}, + UpdateExpression="SET #w = :v", + ConditionExpression=chain("#v = :v", 300), + ExpressionAttributeNames={"#v": "v", "#w": "w"}, + ExpressionAttributeValues=VALUES, + ) + ) + assert _after_prefix(msg) == f"Invalid ConditionExpression: {TOO_MANY}599" + + @XFAIL_ON_MONGODB_DEEP_OR + def test_update_item_condition_at_the_cap_is_evaluated(self, dynamodb_client, table): + # [u03] 300 accepted (condition evaluated). + code, _ = _error( + lambda: dynamodb_client.update_item( + TableName=table, + Key={"pk": {"S": "absent"}, "sk": {"S": "absent"}}, + UpdateExpression="SET #w = :v", + ConditionExpression="NOT attribute_not_exists(pk) OR " + chain("#v = :v", 149), + ExpressionAttributeNames={"#v": "v", "#w": "w"}, + ExpressionAttributeValues=VALUES, + ) + ) + assert code == "ConditionalCheckFailedException" + + def test_delete_item_condition_count(self, dynamodb_client, table): + # [d01] + msg = _validation( + lambda: dynamodb_client.delete_item( + TableName=table, + Key={"pk": {"S": "absent"}, "sk": {"S": "absent"}}, + ConditionExpression=chain("#v = :v", 300), + ExpressionAttributeNames=NAMES, + ExpressionAttributeValues=VALUES, + ) + ) + assert _after_prefix(msg) == f"Invalid ConditionExpression: {TOO_MANY}599" + + def test_scan_filter_count(self, dynamodb_client, table): + # [b01] 599 refused, no prefix; [b02] 299 accepted; [b03] 301 refused; + # [a57] BETWEEN counts one on Scan too. + msg = _validation(scan(dynamodb_client, table, chain("#v = :v", 300))) + assert msg == f"Invalid FilterExpression: {TOO_MANY}599" + assert "Count" in scan(dynamodb_client, table, chain("#v = :v", 150))() + msg = _validation(scan(dynamodb_client, table, chain("#v = :v", 151))) + assert msg == f"Invalid FilterExpression: {TOO_MANY}301" + msg = _validation(scan(dynamodb_client, table, chain("#v BETWEEN :v AND :v", 160))) + assert msg == f"Invalid FilterExpression: {TOO_MANY}319" + + def test_query_filter_count(self, dynamodb_client, table): + # [b06] + msg = _validation( + query( + dynamodb_client, + table, + "pk = :p", + FilterExpression=chain("#v = :v", 300), + ExpressionAttributeValues={":v": {"S": "x"}, ":p": {"S": "p1"}}, + ) + ) + assert msg == f"Invalid FilterExpression: {TOO_MANY}599" + + def test_key_condition_count(self, dynamodb_client, table): + # [b04] 601; [a58] BETWEEN; [a59] begins_with; [a61] 151 equalities. + rows = [ + ("b04", "pk = :v AND " + chain("#v = :v", 300, " AND "), 601), + ("a58", "pk = :v AND " + chain("#v BETWEEN :v AND :v", 150, " AND "), 301), + ("a59", "pk = :v AND " + chain("begins_with(#v, :v)", 170, " AND "), 341), + ("a61", chain("#v = :v", 151, " AND "), 301), + ] + for case, kce, count in rows: + msg = _validation(query(dynamodb_client, table, kce)) + assert msg == f"Invalid KeyConditionExpression: {TOO_MANY}{count}", case + + def test_key_condition_count_precedes_the_key_grammar(self, dynamodb_client, table): + # [a62] `pk = :v OR` + 150 terms answers the count, not the operator + # refusal. The service also runs the count ahead of its + # one-condition-per-key check (b05, a60); ExtendDB has no such check. + msg = _validation(query(dynamodb_client, table, "pk = :v OR " + chain("#v = :v", 150, " AND "))) + assert msg == f"Invalid KeyConditionExpression: {TOO_MANY}301" + + +class TestOperatorCountOrder: + """Where the count falls against the other request checks.""" + + def test_syntax_error_beats_the_count(self, dynamodb_client, table): + # [e04], [e05] + msg = _validation(put(dynamodb_client, table, chain("#v = :v", 300) + " = =")) + assert _after_prefix(msg) == syntax("ConditionExpression", "=", ":v = =") + msg = _validation(put(dynamodb_client, table, "= = " + chain("#v = :v", 300))) + assert _after_prefix(msg) == syntax("ConditionExpression", "=", "= =") + + def test_redundant_parentheses_and_reserved_word_beat_the_count(self, dynamodb_client, table): + # [p14], [p12] + msg = _validation(put(dynamodb_client, table, "((#v = :v)) OR " + chain("#v = :v", 299))) + assert _after_prefix(msg) == ( + "Invalid ConditionExpression: The expression has redundant parentheses;" + ) + msg = _validation(put(dynamodb_client, table, chain("#v = :v", 299) + " OR status = :v")) + assert _after_prefix(msg) == ( + "Invalid ConditionExpression: Attribute name is a reserved keyword; " + "reserved keyword: status" + ) + + def test_ordering_operand_type_beats_the_count(self, dynamodb_client, table): + # [p17] + msg = _validation( + put( + dynamodb_client, + table, + chain("#v = :v", 299) + " OR #v < :b", + ExpressionAttributeValues={":v": {"S": "x"}, ":b": {"BOOL": True}}, + ) + ) + assert _after_prefix(msg) == ( + "Invalid ConditionExpression: Incorrect operand type for operator or " + "function; operator or function: <, operand type: BOOL" + ) + + def test_count_beats_unused_value_missing_table_and_key_mismatch( + self, dynamodb_client, table + ): + # [e02], [p05], [p06] + expected = f"Invalid ConditionExpression: {TOO_MANY}599" + msg = _validation( + put( + dynamodb_client, + table, + chain("#v = :v", 300), + ExpressionAttributeValues={":v": {"S": "x"}, ":u": {"S": "y"}}, + ) + ) + assert _after_prefix(msg) == expected + msg = _validation( + put(dynamodb_client, "extenddb-missing-b501", chain("#v = :v", 300)) + ) + assert _after_prefix(msg) == expected + msg = _validation( + put(dynamodb_client, table, chain("#v = :v", 300), Item={"pk": {"N": "1"}, "sk": {"S": "a"}}) + ) + assert _after_prefix(msg) == expected + + def test_scan_count_beats_unused_missing_table_and_unknown_index( + self, dynamodb_client, table + ): + # [s02], [s07], [s08] + expected = f"Invalid FilterExpression: {TOO_MANY}599" + msg = _validation( + scan( + dynamodb_client, + table, + chain("#v = :v", 300), + ExpressionAttributeValues={":v": {"S": "x"}, ":u": {"S": "y"}}, + ) + ) + assert msg == expected + assert _validation(scan(dynamodb_client, "extenddb-missing-b501", chain("#v = :v", 300))) == expected + assert _validation(scan(dynamodb_client, table, chain("#v = :v", 300), IndexName="nope")) == expected + + def test_update_item_condition_count_beats_update_expression_faults( + self, dynamodb_client, table + ): + # [u04] update syntax error, [u07] update undefined name: the condition's + # count is reported first. + for update in ("SET #w = = :v", "SET #w = :v"): + msg = _validation( + lambda update=update: dynamodb_client.update_item( + TableName=table, + Key={"pk": {"S": "absent"}, "sk": {"S": "absent"}}, + UpdateExpression=update, + ConditionExpression=chain("#v = :v", 300), + ExpressionAttributeNames=NAMES, + ExpressionAttributeValues=VALUES, + ) + ) + assert _after_prefix(msg) == f"Invalid ConditionExpression: {TOO_MANY}599", update + + +class TestPlaceholderKeyLength: + """A placeholder key over 255 bytes is a ValidationException with its size.""" + + @staticmethod + def _names_msg(field: str, size: int) -> str: + return ( + f"{field} contains invalid key: The expression attribute map contains " + f"a key that is too long; size of key: {size}" + ) + + def test_put_item_names_key(self, dynamodb_client_no_validation, table): + # [c01] 255 accepted (condition evaluated), 256 and 257 refused. + ok = "#" + "a" * 254 + code, _ = _error( + put(dynamodb_client_no_validation, table, f"{ok} = :v", ExpressionAttributeNames={ok: "v"}) + ) + assert code == "ConditionalCheckFailedException" + for size in (256, 257): + key = "#" + "a" * (size - 1) + msg = _validation( + put(dynamodb_client_no_validation, table, f"{key} = :v", ExpressionAttributeNames={key: "v"}) + ) + assert _after_prefix(msg) == self._names_msg("ExpressionAttributeNames", size) + + def test_put_item_values_key(self, dynamodb_client_no_validation, table): + # [c02] 255 accepted, 256 and 257 refused with the size suffix. + ok = ":" + "a" * 254 + code, _ = _error( + put(dynamodb_client_no_validation, table, f"#v = {ok}", ExpressionAttributeValues={ok: {"S": "x"}}) + ) + assert code == "ConditionalCheckFailedException" + for size in (256, 257): + key = ":" + "a" * (size - 1) + msg = _validation( + put( + dynamodb_client_no_validation, + table, + f"#v = {key}", + ExpressionAttributeValues={key: {"S": "x"}}, + ) + ) + assert _after_prefix(msg) == self._names_msg("ExpressionAttributeValues", size) + + def test_put_item_key_length_is_bytes_and_checked_before_syntax( + self, dynamodb_client_no_validation, table + ): + # [c03] 127 two-byte characters: 255 bytes, passes the length check and + # fails key syntax; [c04] 128: 257 bytes; [c05] 258 bytes with an + # invalid character reports the length. + key = "#" + "\u00e9" * 127 + msg = _validation( + put(dynamodb_client_no_validation, table, "#v = :v", ExpressionAttributeNames={"#v": "v", key: "w"}) + ) + assert _after_prefix(msg) == ( + f'ExpressionAttributeNames contains invalid key: Syntax error; key: "{key}"' + ) + key = "#" + "\u00e9" * 128 + msg = _validation( + put(dynamodb_client_no_validation, table, "#v = :v", ExpressionAttributeNames={"#v": "v", key: "w"}) + ) + assert _after_prefix(msg) == self._names_msg("ExpressionAttributeNames", 257) + key = "#" + "a" * 255 + "-x" + msg = _validation( + put(dynamodb_client_no_validation, table, "#v = :v", ExpressionAttributeNames={"#v": "v", key: "w"}) + ) + assert _after_prefix(msg) == self._names_msg("ExpressionAttributeNames", 258) + + def test_put_item_too_long_key_beats_the_expression_checks( + self, dynamodb_client_no_validation, table + ): + # [c06] unreferenced, [c07] with a 300-term chain, [c08] with a syntax + # error, [c09] with an undefined name, [c10] with an unused value. + key = "#" + "w" * 255 + names = {"#v": "v", key: "w"} + expected = self._names_msg("ExpressionAttributeNames", 256) + for condition, extra in [ + ("#v = :v", {}), + (chain("#v = :v", 300), {}), + ("#v = = :v", {}), + ("#x = :v", {}), + ("#v = :v", {"ExpressionAttributeValues": {":v": {"S": "x"}, ":u": {"S": "y"}}}), + ]: + msg = _validation( + put(dynamodb_client_no_validation, table, condition, ExpressionAttributeNames=names, **extra) + ) + assert _after_prefix(msg) == expected, condition + + def test_scan_query_update_keys(self, dynamodb_client_no_validation, table): + # [c12] Scan names, [s10] Scan values, [s11] Query values, [s12] Query + # names: no prefix. [c13] UpdateItem names: prefix on the service. + nkey = "#" + "w" * 255 + vkey = ":" + "w" * 255 + assert _validation( + scan(dynamodb_client_no_validation, table, f"{nkey} = :v", ExpressionAttributeNames={nkey: "w"}) + ) == self._names_msg("ExpressionAttributeNames", 256) + assert _validation( + scan(dynamodb_client_no_validation, table, f"#v = {vkey}", ExpressionAttributeValues={vkey: {"S": "x"}}) + ) == self._names_msg("ExpressionAttributeValues", 256) + assert _validation( + query( + dynamodb_client_no_validation, + table, + "pk = :p", + FilterExpression=f"#v = {vkey}", + ExpressionAttributeValues={":p": {"S": "p1"}, vkey: {"S": "x"}}, + ) + ) == self._names_msg("ExpressionAttributeValues", 256) + assert _validation( + query( + dynamodb_client_no_validation, + table, + "pk = :v", + ExpressionAttributeNames={nkey: "w"}, + ) + ) == self._names_msg("ExpressionAttributeNames", 256) + msg = _validation( + lambda: dynamodb_client_no_validation.update_item( + TableName=table, + Key={"pk": {"S": "absent"}, "sk": {"S": "absent"}}, + UpdateExpression=f"SET {nkey} = :v", + ExpressionAttributeNames={nkey: "w"}, + ExpressionAttributeValues=VALUES, + ) + ) + assert _after_prefix(msg) == self._names_msg("ExpressionAttributeNames", 256) + + +class TestSyntaxErrorWording: + """Token and window for a repeated or misplaced operator or keyword.""" + + @pytest.mark.parametrize( + "case, expr, token, near", + [ + ("d01", "#v = = :v", "=", "= = :v"), + ("d02", "attribute_not_exists(pk) OR #v = = :v", "=", "= = :v"), + ("d13", "attribute_exists(pk) AND #v = :v AND #v = = :v", "=", "= = :v"), + ("d03", "= #v = :v", "=", "= #v"), + ("d04", "#v = :v =", "=", ":v ="), + ("d05", "#v = :v AND", "", "AND"), + ("d06", "#v = :v OR", "", "OR"), + ("d07", "#v = :v AND AND #v = :v", "AND", "AND AND #v"), + ("d08", "#v = :v OR OR #v = :v", "OR", "OR OR #v"), + ("d09", "AND #v = :v", "AND", "AND #v"), + ("d10", "#v < > :v", ">", "< > :v"), + ("d11", "#v = :v #v = :v", "#v", ":v #v ="), + ("d14", "#v <> <> :v", "<>", "<> <> :v"), + ("d15", "#v >= >= :v", ">=", ">= >= :v"), + ("d16", "#v = = = :v", "=", "= = ="), + ("d17", "#v==:v", "=", "==:v"), + ("d18", "#v =\t= :v", "=", "=\t= :v"), + ("d19", "#v = :v AND NOT", "", "NOT"), + ("d29", "=", "=", "="), + ("d30", "= =", "=", "= ="), + ("lower-case and", "#v = :v and and #v = :v", "and", "and and #v"), + ], + ) + def test_put_item_condition(self, dynamodb_client, table, case, expr, token, near): + msg = _validation(put(dynamodb_client, table, expr)) + assert _after_prefix(msg) == syntax("ConditionExpression", token, near), case + + def test_put_item_long_tokens(self, dynamodb_client, table): + # [d12] + msg = _validation( + put( + dynamodb_client, + table, + "#longname = = :longvalue", + ExpressionAttributeNames={"#longname": "v"}, + ExpressionAttributeValues={":longvalue": {"S": "x"}}, + ) + ) + assert _after_prefix(msg) == syntax("ConditionExpression", "=", "= = :longvalue") + + def test_scan_filter(self, dynamodb_client, table): + # [d20], [d21]: no prefix on Scan. + assert _validation(scan(dynamodb_client, table, "#v = = :v")) == syntax( + "FilterExpression", "=", "= = :v" + ) + assert _validation(scan(dynamodb_client, table, "#v = :v AND")) == syntax( + "FilterExpression", "", "AND" + ) + # Measured 2026-10-25: a lower-case keyword is shown as written. + assert _validation(scan(dynamodb_client, table, "#v = :v and and #v = :v")) == syntax( + "FilterExpression", "and", "and and #v" + ) + + def test_fault_at_the_first_token_keeps_leading_whitespace(self, dynamodb_client, table): + # Measured 2026-10-25: the window starts at the start of the expression. + for expr, token, near in [ + (" = #v = :v", "=", " = #v"), + ("\t= #v = :v", "=", "\t= #v"), + (" AND #v = :v", "AND", " AND #v"), + ("= #v = :v", "=", "= #v"), + ]: + assert _validation(scan(dynamodb_client, table, expr)) == syntax( + "FilterExpression", token, near + ), expr + assert _validation(query(dynamodb_client, table, " = pk = :v")) == syntax( + "KeyConditionExpression", "=", " = pk" + ) + msg = _validation( + lambda: dynamodb_client.get_item( + TableName=table, + Key={"pk": {"S": "p1"}, "sk": {"S": "s1"}}, + ProjectionExpression=" , #v", + ExpressionAttributeNames=NAMES, + ) + ) + assert msg == syntax("ProjectionExpression", ",", " , #v") + + def test_query_key_condition(self, dynamodb_client, table): + # [d22], [d23] + assert _validation(query(dynamodb_client, table, "pk = = :v")) == syntax( + "KeyConditionExpression", "=", "= = :v" + ) + assert _validation(query(dynamodb_client, table, "pk = :v AND")) == syntax( + "KeyConditionExpression", "", "AND" + ) + # Measured 2026-10-25: a lower-case keyword in a key condition. + lower = query( + dynamodb_client, + table, + "pk = :p and and sk = :s", + ExpressionAttributeValues={":p": {"S": "p1"}, ":s": {"S": "a"}, ":v": {"S": "x"}}, + ) + assert _validation(lower) == syntax("KeyConditionExpression", "and", "and and sk") + + @pytest.mark.parametrize( + "case, expr, token, near", + [ + ("d24", "SET #v = = :v", "=", "= = :v"), + ("d25", "SET #v = :v + + :v", "+", "+ + :v"), + ("d26", "SET #v = :v,", "", ","), + ("b07", "SET #v = :v + :v + :v", "+", ":v + :v"), + ("u08", "SET #v = :v +", "", "+"), + ("u10", "SET #v = :v REMOVE", "", "REMOVE"), + ("u11", "SET = :v", "=", "SET = :v"), + ("u12", "SET #v :v", ":v", "#v :v"), + ], + ) + def test_update_expression(self, dynamodb_client, table, case, expr, token, near): + msg = _validation( + lambda: dynamodb_client.update_item( + TableName=table, + Key={"pk": {"S": "absent"}, "sk": {"S": "absent"}}, + UpdateExpression=expr, + ExpressionAttributeNames=NAMES, + ExpressionAttributeValues=VALUES, + ) + ) + assert _after_prefix(msg) == syntax("UpdateExpression", token, near), case + + def test_projection_expression(self, dynamodb_client, table): + # [d27], [d28] + for expr, token, near in [("#v, , #v", ",", ", , #v"), ("#v,", "", ",")]: + msg = _validation( + lambda expr=expr: dynamodb_client.get_item( + TableName=table, + Key={"pk": {"S": "p1"}, "sk": {"S": "s1"}}, + ProjectionExpression=expr, + ExpressionAttributeNames=NAMES, + ) + ) + assert msg == syntax("ProjectionExpression", token, near), expr + + def test_syntax_error_beats_the_other_expression_checks(self, dynamodb_client, table): + # [e06] undefined name, [e07] undefined value, [e08] unused value, + # [e09] unused name, [e10] operand type. + rows = [ + ("#w = = :v", ":v", {}), + ("#v = = :u", ":u", {}), + ("#v = = :v", ":v", {"ExpressionAttributeValues": {":v": {"S": "x"}, ":u": {"S": "y"}}}), + ("#v = = :v", ":v", {"ExpressionAttributeNames": {"#v": "v", "#x": "x"}}), + ( + "begins_with(#v, :n) AND #v = = :v", + ":v", + {"ExpressionAttributeValues": {":v": {"S": "x"}, ":n": {"N": "1"}}}, + ), + ] + for condition, last, extra in rows: + msg = _validation(put(dynamodb_client, table, condition, **extra)) + assert _after_prefix(msg) == syntax("ConditionExpression", "=", f"= = {last}"), condition diff --git a/tests/test_legacy_filter_operator_count.py b/tests/test_legacy_filter_operator_count.py new file mode 100644 index 000000000..c42ab0a34 --- /dev/null +++ b/tests/test_legacy_filter_operator_count.py @@ -0,0 +1,84 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""The 300-operator cap does not apply to the legacy condition parameters. + +ScanFilter, QueryFilter and Expected are not expressions, and Amazon DynamoDB +accepts 160 conditions in each: a Scan with a 160-condition ScanFilter and a +Query with a 160-condition QueryFilter return normally, and a PutItem with a +160-condition Expected is evaluated (measured 2026-10-25 in us-east-1, each +request twice). ExtendDB desugars these parameters into expressions, which +must not be counted. + +Dual-target against Amazon DynamoDB and extenddb. +""" + +from __future__ import annotations + +import pytest +from botocore.exceptions import ClientError + +from conftest import scoped_table + +CONDITIONS = 160 + + +def _eq_conditions(n: int) -> dict: + return { + f"a{i}": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]} + for i in range(n) + } + + +@pytest.fixture(scope="module") +def table(dynamodb_client): + 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: + dynamodb_client.put_item( + TableName=name, + Item={"pk": {"S": "p1"}, "sk": {"S": "s1"}, "v": {"S": "x"}}, + ) + yield name + + +def test_scan_filter_with_160_conditions_is_not_counted(dynamodb_client, table): + # Measured 2026-10-25: OK, Count 0, ScannedCount 1. + out = dynamodb_client.scan( + TableName=table, Limit=1, ScanFilter=_eq_conditions(CONDITIONS) + ) + assert out["Count"] == 0 + assert out["ScannedCount"] == 1 + + +def test_query_filter_with_160_conditions_is_not_counted(dynamodb_client, table): + # Measured 2026-10-25: OK, Count 0. + out = dynamodb_client.query( + TableName=table, + Limit=1, + KeyConditions={ + "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "p1"}]} + }, + QueryFilter=_eq_conditions(CONDITIONS), + ) + assert out["Count"] == 0 + + +def test_expected_with_160_conditions_is_evaluated(dynamodb_client, table): + # Measured 2026-10-25: ConditionalCheckFailedException, the item has none of the + # compared attributes. + with pytest.raises(ClientError) as exc_info: + dynamodb_client.put_item( + TableName=table, + Item={"pk": {"S": "absent"}, "sk": {"S": "absent"}}, + Expected={f"a{i}": {"Value": {"S": "x"}} for i in range(CONDITIONS)}, + ) + assert exc_info.value.response["Error"]["Code"] == "ConditionalCheckFailedException"