fix: reject redundant parentheses in KeyConditionExpression and FilterExpression

crates/core/src/expression/key_condition.rs

@@ -258,6 +258,9 @@ pub enum SortKeyCondition {
 ///
 /// Returns `ValidationException` for syntax errors or unsupported constructs.
 pub fn parse_key_condition(tokens: &[Token]) -> Result<KeyCondition, 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
         && tokens[0] == Token::LParen
@@ -651,6 +654,35 @@ mod tests {
     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

crates/core/src/expression/parser.rs

@@ -42,6 +42,8 @@ pub fn parse_condition_with_depth_limit(
     tokens: &[Token],
     max_depth: usize,
 ) -> Result<Expr, DynamoDbError> {
+    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)?;
@@ -125,36 +127,9 @@ fn parse_primary(
                 "Invalid ConditionExpression: expression nesting depth exceeded",
             ));
         }
-        let start = *pos;
         let expr = parse_or(tokens, pos, depth, max_depth)?;
         *depth -= 1;
         parser_common::expect_token(tokens, pos, &Token::RParen, ")", "ConditionExpression")?;
-        // Reject redundant parentheses: if no AND/OR was consumed between our
-        // parens (the closing ')' is the token right after the inner expression
-        // ended) AND the inner content started with '(', the parens are redundant.
-        // We detect "no AND/OR at this level" by checking that the inner content
-        // is a single parenthesized group: starts with '(' and its match is at
-        // the end (pos - 2, since pos is now past our ')').
-        if tokens[start] == Token::LParen {
-            // Find matching ')' for the inner '('
-            let mut d = 1usize;
-            let mut s = start + 1;
-            while s < tokens.len() && d > 0 {
-                match &tokens[s] {
-                    Token::LParen => d += 1,
-                    Token::RParen => d -= 1,
-                    _ => {}
-                }
-                s += 1;
-            }
-            // s is now one past the matching ')'. If that equals pos - 1
-            // (our closing ')'), the inner parens span the entire content.
-            if s == *pos - 1 {
-                return Err(validation_err(
-                    "Invalid ConditionExpression: The expression has redundant parentheses;",
-                ));
-            }
-        }
         return Ok(expr);
     }
 
@@ -525,4 +500,22 @@ mod tests {
             );
         }
     }
+
+    #[test]
+    fn redundant_parens_use_canonical_message() {
+        for expr in [
+            "((a = :v))",
+            "(((a = :v)))",
+            "((a = :v AND b = :v2))",
+            "((NOT (a = :v)))",
+        ] {
+            let tokens = tokenize(expr).unwrap();
+            let err = parse_condition(&tokens).unwrap_err();
+            assert!(
+                matches!(&err, DynamoDbError::ValidationException(msg)
+                    if msg == "Invalid ConditionExpression: The expression has redundant parentheses;"),
+                "expr {expr}: got {err:?}"
+            );
+        }
+    }
 }

crates/core/src/expression/parser_common.rs

@@ -81,6 +81,43 @@ pub fn expect_token(
     Ok(())
 }
 
+/// Reject redundant parentheses, matching DynamoDB: a parenthesised group whose
+/// entire content is itself a single parenthesised group, such as `((x))`.
+/// Returns the bare error body so each parser can prefix its own expression type.
+pub fn check_redundant_parens(tokens: &[Token]) -> Result<(), String> {
+    // First pass: map each '(' to the index of its matching ')'. Unbalanced
+    // parentheses stay `None` and are left for the grammar parser to report.
+    let mut match_of: Vec<Option<usize>> = vec![None; tokens.len()];
+    let mut stack: Vec<usize> = Vec::new();
+    for (i, token) in tokens.iter().enumerate() {
+        match token {
+            Token::LParen => stack.push(i),
+            Token::RParen => {
+                if let Some(open) = stack.pop() {
+                    match_of[open] = Some(i);
+                }
+            }
+            _ => {}
+        }
+    }
+
+    // Second pass: flag any group whose entire body is a single nested group,
+    // such as `((x))`. `match_of[i]` is `Some` only for matched '(' tokens.
+    for i in 0..tokens.len() {
+        let Some(close) = match_of[i] else {
+            continue;
+        };
+        // A matched close always sits after `i`, so `tokens[i + 1]` is already
+        // in bounds. `i + 1 < close` is a non-empty-group guard, not bounds
+        // protection: it skips empty `()`, which the LParen check below would
+        // reject anyway.
+        if i + 1 < close && tokens[i + 1] == Token::LParen && match_of[i + 1] == Some(close - 1) {
+            return Err("The expression has redundant parentheses;".to_owned());
+        }
+    }
+    Ok(())
+}
+
 fn validation_err(msg: &str) -> DynamoDbError {
     DynamoDbError::ValidationException(format!("Invalid expression: {msg}"))
 }

crates/engine/src/expression_helpers.rs

@@ -148,11 +148,17 @@ pub fn resolve_condition(
 }
 
 /// Prefix an expression error with the expression type, matching DynamoDB's format.
-/// If the error already starts with "Invalid", it's returned as-is.
+///
+/// `FilterExpression` shares the condition parser, so its errors arrive labelled
+/// `ConditionExpression`; those are relabelled to `expr_type`. Errors already
+/// labelled with another expression type, or non-expression validation errors,
+/// are returned unchanged.
 pub fn prefix_expression_error(err: DynamoDbError, expr_type: &str) -> DynamoDbError {
     match err {
         DynamoDbError::ValidationException(msg) => {
-            if msg.starts_with("Invalid ") || msg.starts_with("1 validation") {
+            if let Some(rest) = msg.strip_prefix("Invalid ConditionExpression:") {
+                DynamoDbError::ValidationException(format!("Invalid {expr_type}:{rest}"))
+            } else if msg.starts_with("Invalid ") || msg.starts_with("1 validation") {
                 DynamoDbError::ValidationException(msg)
             } else {
                 DynamoDbError::ValidationException(format!("Invalid {expr_type}: {msg}"))
@@ -161,3 +167,67 @@ pub fn prefix_expression_error(err: DynamoDbError, expr_type: &str) -> DynamoDbE
         other => other,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use extenddb_core::limits::LimitsConfig;
+
+    const CONDITION_REDUNDANT: &str =
+        "Invalid ConditionExpression: The expression has redundant parentheses;";
+
+    #[test]
+    fn condition_redundant_parens_rejected_with_canonical_message() {
+        let limits = LimitsConfig::default();
+        for expr in [
+            "((a = :v))",
+            "(((a = :v)))",
+            "((a = :v AND b = :v2))",
+            "((NOT (a = :v)))",
+        ] {
+            let err = parse_optional_condition(Some(expr), &limits).unwrap_err();
+            assert!(
+                matches!(&err, DynamoDbError::ValidationException(msg) if msg == CONDITION_REDUNDANT),
+                "expr {expr}: got {err:?}"
+            );
+        }
+    }
+
+    #[test]
+    fn condition_valid_parens_accepted() {
+        let limits = LimitsConfig::default();
+        for expr in [
+            "(a = :v)",
+            "(a = :v) AND (b = :v2)",
+            "((a = :v) AND (b = :v2))",
+            "(NOT (a = :v))",
+        ] {
+            assert!(
+                parse_optional_condition(Some(expr), &limits).is_ok(),
+                "expr {expr} should parse"
+            );
+        }
+    }
+
+    #[test]
+    fn filter_redundant_parens_rejected_with_filter_label() {
+        let limits = LimitsConfig::default();
+        let err = parse_optional_filter(Some("((a = :v))"), &limits).unwrap_err();
+        assert!(
+            matches!(&err, DynamoDbError::ValidationException(msg)
+                if msg == "Invalid FilterExpression: The expression has redundant parentheses;"),
+            "got {err:?}"
+        );
+    }
+
+    #[test]
+    fn filter_parser_errors_carry_filter_label() {
+        let limits = LimitsConfig::default();
+        let err = parse_optional_filter(Some("a"), &limits).unwrap_err();
+        assert!(
+            matches!(&err, DynamoDbError::ValidationException(msg)
+                if msg.starts_with("Invalid FilterExpression:")),
+            "got {err:?}"
+        );
+    }
+}

crates/engine/src/transact_write_helpers.rs

@@ -308,6 +308,17 @@ pub(crate) fn validate_no_key_updates(
 mod tests {
     use super::*;
 
+    #[test]
+    fn transact_condition_redundant_parens_rejected_with_canonical_message() {
+        let limits = extenddb_core::limits::LimitsConfig::default();
+        let err = parse_optional_condition(Some("((a = :v))"), &limits).unwrap_err();
+        assert!(
+            matches!(&err, DynamoDbError::ValidationException(msg)
+                if msg == "Invalid ConditionExpression: The expression has redundant parentheses;"),
+            "got {err:?}"
+        );
+    }
+
     #[test]
     fn validate_token_valid() {
         assert!(validate_client_request_token("abc-123").is_ok());