fix(expressions): validate in the service's order, with its messages

crates/core/src/expression/key_condition.rs

@@ -479,12 +479,19 @@ fn parse_key_clause_inner(tokens: &[Token], pos: &mut usize) -> Result<RawClause
         ));
     }
 
-    // begins_with(path, :val)
+    // begins_with(path, :val). The name is matched as written: Amazon
+    // DynamoDB refuses `Begins_With(sk, :v)` with `Invalid function name;
+    // function: Begins_With`, and any other identifier followed by `(` the
+    // same way.
     if let Token::Ident(name) = &tokens[*pos]
-        && name.eq_ignore_ascii_case("begins_with")
         && *pos + 1 < tokens.len()
         && tokens[*pos + 1] == Token::LParen
     {
+        if name != "begins_with" {
+            return Err(validation_err(&format!(
+                "Invalid KeyConditionExpression: Invalid function name; function: {name}"
+            )));
+        }
         *pos += 1; // skip function name
         parser_common::expect_token(tokens, pos, &Token::LParen, "(", "KeyConditionExpression")?;
         let path = parse_key_operand_path(tokens, pos)?;

crates/core/src/expression/mod.rs

@@ -18,6 +18,7 @@ mod projection;
 mod reserved_words;
 mod resolver;
 mod search_condition;
+mod semantics;
 mod tokenizer;
 mod update_evaluator;
 mod update_parser;
@@ -32,13 +33,17 @@ pub use reserved_words::validate_no_reserved_words;
 pub use resolver::{
     ExpressionMaps, collect_key_condition_refs, collect_value_placeholders, resolve_element_name,
     resolve_name_ref, resolve_path, validate_begins_with_operands, validate_expression_param_usage,
-    validate_ordering_operand_types, validate_unused_attributes,
+    validate_ordering_operand_types, validate_refs_defined, validate_unused_attributes,
+    validate_update_paths_disjoint,
 };
 pub use search_condition::{
     MAX_INLINE_FILTER_CONDITIONS, MAX_PARTITION_KEY_CONDITIONS, MAX_SEARCH_CONDITIONS,
     SearchCondition, validate_conditions_against_search_schema,
     validate_search_condition_expression,
 };
+pub use semantics::{validate_condition_semantics, validate_update_semantics};
 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};
+pub use update_parser::{
+    ParsedUpdate, UpdateSection, parse_update, parse_update_from, parse_update_sections,
+};

crates/core/src/expression/parser.rs

@@ -133,18 +133,12 @@ fn parse_primary(
         return Ok(expr);
     }
 
-    // Try function call: ident followed by '('
-    if let Token::Ident(name) = &tokens[*pos] {
-        let fn_name_lower = name.to_ascii_lowercase();
-        if is_function_name(&fn_name_lower)
-            && *pos + 1 < tokens.len()
-            && tokens[*pos + 1] == Token::LParen
-        {
-            return parse_function_call(tokens, pos);
-        }
-    }
-
-    // Operand — then check for comparator, BETWEEN, or IN
+    // Operand (a path, a placeholder, or a function call), then a comparator,
+    // BETWEEN, or IN. A function call with nothing after it is the condition
+    // itself; whether that function may stand alone, whether its name exists,
+    // and how many operands it takes are semantic questions answered after
+    // the grammar pass (see `validate_condition_semantics`), so a syntax error
+    // further along still wins, as on Amazon DynamoDB.
     let left = parse_operand(tokens, pos)?;
 
     if *pos < tokens.len() {
@@ -188,6 +182,10 @@ fn parse_primary(
         }
     }
 
+    if matches!(left, Expr::Function { .. }) {
+        return Ok(left);
+    }
+
     // Bare operand is not a valid condition on its own
     Err(validation_err(
         "Invalid ConditionExpression: expected comparison operator or function call",
@@ -207,12 +205,8 @@ fn parse_operand(tokens: &[Token], pos: &mut usize) -> Result<Expr, DynamoDbErro
             *pos += 1;
             Ok(expr)
         }
-        Token::Ident(name) => {
-            // size(path) is a function that returns a value, usable as an operand
-            if name.eq_ignore_ascii_case("size")
-                && *pos + 1 < tokens.len()
-                && tokens[*pos + 1] == Token::LParen
-            {
+        Token::Ident(_) => {
+            if *pos + 1 < tokens.len() && tokens[*pos + 1] == Token::LParen {
                 return parse_function_call(tokens, pos);
             }
             let elements = parser_common::parse_path(tokens, pos)?;
@@ -230,7 +224,7 @@ fn parse_operand(tokens: &[Token], pos: &mut usize) -> Result<Expr, DynamoDbErro
 
 fn parse_function_call(tokens: &[Token], pos: &mut usize) -> Result<Expr, DynamoDbError> {
     let name = match &tokens[*pos] {
-        Token::Ident(n) => n.to_ascii_lowercase(),
+        Token::Ident(n) => n.clone(),
         _ => {
             return Err(validation_err(
                 "Invalid ConditionExpression: expected function name",
@@ -241,13 +235,16 @@ fn parse_function_call(tokens: &[Token], pos: &mut usize) -> Result<Expr, Dynamo
 
     parser_common::expect_token(tokens, pos, &Token::LParen, "(", "ConditionExpression")?;
 
-    let mut args = Vec::new();
-    if *pos < tokens.len() && tokens[*pos] != Token::RParen {
+    // `f()` is a syntax error on the service, reported at the `)`.
+    if *pos < tokens.len() && tokens[*pos] == Token::RParen {
+        return Err(validation_err(
+            "Invalid ConditionExpression: Syntax error; token: \")\", near: \"()\"",
+        ));
+    }
+    let mut args = vec![parse_operand(tokens, pos)?];
+    while *pos < tokens.len() && tokens[*pos] == Token::Comma {
+        *pos += 1;
         args.push(parse_operand(tokens, pos)?);
-        while *pos < tokens.len() && tokens[*pos] == Token::Comma {
-            *pos += 1;
-            args.push(parse_operand(tokens, pos)?);
-        }
     }
 
     parser_common::expect_token(tokens, pos, &Token::RParen, ")", "ConditionExpression")?;
@@ -267,13 +264,6 @@ fn try_comparator(token: &Token) -> Option<CompareOp> {
     }
 }
 
-fn is_function_name(name: &str) -> bool {
-    matches!(
-        name,
-        "attribute_exists" | "attribute_not_exists" | "attribute_type" | "begins_with" | "contains"
-    )
-}
-
 fn validation_err(msg: &str) -> DynamoDbError {
     DynamoDbError::ValidationException(msg.to_owned())
 }

crates/core/src/expression/projection.rs

@@ -125,13 +125,13 @@ fn prefix_projection_error(err: DynamoDbError) -> DynamoDbError {
 
 /// A path element after `#name` resolution.
 #[derive(PartialEq, Eq)]
-enum ResolvedElement {
+pub(crate) enum ResolvedElement {
     Attr(String),
     Index(usize),
 }
 
 /// Resolve `#name` references in parsed paths, rejecting index-start paths.
-fn resolve_paths(
+pub(crate) fn resolve_paths(
     paths: &[Vec<PathElement>],
     maps: &ExpressionMaps,
 ) -> Result<Vec<Vec<ResolvedElement>>, DynamoDbError> {
@@ -166,11 +166,20 @@ fn resolve_paths(
 /// Reject paths where one is a prefix of another (DynamoDB's overlap rule),
 /// reporting the first offending pair in expression order. Siblings are fine.
 fn check_overlap(resolved: &[Vec<ResolvedElement>]) -> Result<(), DynamoDbError> {
+    check_overlap_for(resolved, super::ExpressionKind::Projection)
+}
+
+/// The overlap rule for any expression kind that names document paths; the
+/// message carries the kind's `Invalid <Kind>:` prefix.
+pub(crate) fn check_overlap_for(
+    resolved: &[Vec<ResolvedElement>],
+    kind: super::ExpressionKind,
+) -> Result<(), DynamoDbError> {
     for i in 0..resolved.len() {
         for j in (i + 1)..resolved.len() {
             if is_prefix(&resolved[i], &resolved[j]) || is_prefix(&resolved[j], &resolved[i]) {
                 return Err(DynamoDbError::ValidationException(format!(
-                    "Invalid ProjectionExpression: Two document paths overlap with each other; \
+                    "Invalid {kind}: Two document paths overlap with each other; \
                      must remove or rewrite one of these paths; path one: {}, path two: {}",
                     render_path(&resolved[i]),
                     render_path(&resolved[j]),

crates/core/src/expression/resolver.rs

@@ -273,33 +273,129 @@ pub fn validate_unused_attributes(
         collect_action_refs(action, &mut used_names, &mut used_values);
     }
 
-    for key in names.keys() {
-        if !used_names.contains(key.strip_prefix('#').unwrap_or(key)) {
-            let display_key = if key.starts_with('#') {
+    // The service lists every unused key in one set (`keys: {:x, :y, :z}`).
+    // Its order is a hash artifact no client can depend on; the keys are
+    // sorted here so the message is the same on every backend and process.
+    let mut unused_names: Vec<String> = names
+        .keys()
+        .filter(|key| !used_names.contains(key.strip_prefix('#').unwrap_or(key)))
+        .map(|key| {
+            if key.starts_with('#') {
                 key.clone()
             } else {
                 format!("#{key}")
-            };
-            return Err(DynamoDbError::ValidationException(format!(
-                "Value provided in ExpressionAttributeNames unused in expressions: keys: {{{display_key}}}"
-            )));
-        }
+            }
+        })
+        .collect();
+    if !unused_names.is_empty() {
+        unused_names.sort();
+        return Err(DynamoDbError::ValidationException(format!(
+            "Value provided in ExpressionAttributeNames unused in expressions: keys: {{{}}}",
+            unused_names.join(", ")
+        )));
     }
-    for key in values.keys() {
-        if !used_values.contains(key.strip_prefix(':').unwrap_or(key)) {
-            let display_key = if key.starts_with(':') {
+    let mut unused_values: Vec<String> = values
+        .keys()
+        .filter(|key| !used_values.contains(key.strip_prefix(':').unwrap_or(key)))
+        .map(|key| {
+            if key.starts_with(':') {
                 key.clone()
             } else {
                 format!(":{key}")
-            };
-            return Err(DynamoDbError::ValidationException(format!(
-                "Value provided in ExpressionAttributeValues unused in expressions: keys: {{{display_key}}}"
-            )));
-        }
+            }
+        })
+        .collect();
+    if !unused_values.is_empty() {
+        unused_values.sort();
+        return Err(DynamoDbError::ValidationException(format!(
+            "Value provided in ExpressionAttributeValues unused in expressions: keys: {{{}}}",
+            unused_values.join(", ")
+        )));
+    }
+    Ok(())
+}
+
+/// Validate that every `#name` and `:value` reference in the expressions is
+/// defined in the supplied maps.
+///
+/// The single-item APIs discover an undefined reference while evaluating the
+/// expression against the item. Transactions validate every item's expressions
+/// before any table is looked up, so this runs the same check on the parsed
+/// expression ahead of time. Messages match the resolver's, without the
+/// `Invalid <Kind>:` prefix, which the caller adds for the expression at hand.
+///
+/// # Errors
+///
+/// Returns `DynamoDbError::ValidationException` naming the first undefined
+/// reference, names before values, in the order the expressions are given.
+pub fn validate_refs_defined(
+    names: &HashMap<String, String>,
+    values: &HashMap<String, AttributeValue>,
+    exprs: &[&super::ast::Expr],
+    update_actions: &[&super::ast::UpdateAction],
+) -> Result<(), DynamoDbError> {
+    let mut used_names = std::collections::HashSet::new();
+    let mut used_values = std::collections::HashSet::new();
+    for expr in exprs {
+        collect_expr_refs(expr, &mut used_names, &mut used_values);
+    }
+    for action in update_actions {
+        collect_action_refs(action, &mut used_names, &mut used_values);
+    }
+    let mut missing_names: Vec<&String> = used_names
+        .iter()
+        .filter(|n| !names.contains_key(*n))
+        .collect();
+    missing_names.sort();
+    if let Some(name) = missing_names.first() {
+        return Err(DynamoDbError::ValidationException(format!(
+            "An expression attribute name used in the document path is not defined; attribute name: #{name}"
+        )));
+    }
+    let mut missing_values: Vec<&String> = used_values
+        .iter()
+        .filter(|v| !values.contains_key(*v))
+        .collect();
+    missing_values.sort();
+    if let Some(value) = missing_values.first() {
+        return Err(DynamoDbError::ValidationException(format!(
+            "An expression attribute value used in expression is not defined; attribute value: :{value}"
+        )));
     }
     Ok(())
 }
 
+/// Reject an update expression whose actions name overlapping document paths.
+///
+/// Amazon DynamoDB refuses two actions whose paths are equal or where one is a
+/// prefix of the other, across all four sections, after `#name` resolution:
+/// `SET a = :v, a = :w`, `SET m = :v, m.x = :w`, `SET a.b = :v REMOVE a`, and
+/// `SET #l[1] = :v, #l[1].x = :w` with `#l` bound to `lst` all fail. Siblings
+/// (`m.x` and `m.y`) are fine. The first offending pair in expression order is
+/// named, rendered as the service renders it: `[lst, [1]]`.
+///
+/// # Errors
+///
+/// Returns `DynamoDbError::ValidationException` with the `Invalid
+/// UpdateExpression:` prefix.
+pub fn validate_update_paths_disjoint(
+    actions: &[super::ast::UpdateAction],
+    maps: &ExpressionMaps,
+) -> Result<(), DynamoDbError> {
+    use super::ast::UpdateAction;
+    let paths: Vec<Vec<PathElement>> = actions
+        .iter()
+        .map(|action| match action {
+            UpdateAction::Set { path, .. }
+            | UpdateAction::Remove { path }
+            | UpdateAction::Add { path, .. }
+            | UpdateAction::Delete { path, .. } => path.clone(),
+        })
+        .collect();
+    let resolved = super::projection::resolve_paths(&paths, maps)?;
+    super::projection::check_overlap_for(&resolved, super::ExpressionKind::Update)
+}
+
 fn collect_expr_refs(
     expr: &super::ast::Expr,
     names: &mut std::collections::HashSet<String>,