fix: reject redundant parentheses in KeyConditionExpression and FilterExpression

[hicksy]

What

Extends redundant-parentheses rejection to KeyConditionExpression and FilterExpression, so all three expression contexts behave the same way.

• Redundant-paren detection moves into one shared token-level check. Each parser prefixes the error with its own expression type.
• KeyConditionExpression now rejects forms like ((pk = :v)).
• FilterExpression errors are now labelled Invalid FilterExpression: instead of Invalid ConditionExpression:.
• ConditionExpression behaviour is unchanged. The detection it gained in fix: reject redundant parentheses in condition expressions #71 now lives in the shared check.

Why

Closes #130.

DynamoDB rejects redundant parentheses in every expression with The expression has redundant parentheses;. #71 added that for ConditionExpression only. KeyConditionExpression accepted them, and FilterExpression rejected them under the wrong label. This brings the remaining two contexts in line.

Testing done

• cargo test --workspace passes. Added unit tests covering rejection and acceptance of parenthesised forms across ConditionExpression, FilterExpression and KeyConditionExpression, including the existing condition and transaction condition-check paths.
• cargo fmt --check and cargo clippy --all-targets -- -D warnings are clean.
• Verified against a local server with the AWS SDK: a Query with ((pk = :v)) and a Scan with ((a = :v)) are now rejected with the documented messages, and PutItem/UpdateItem/DeleteItem and transaction condition checks still reject ((a = :v)) with Invalid ConditionExpression: The expression has redundant parentheses;.

Notes

Redundant-paren detection now runs as a single pass at parse entry rather than during the descent. For an expression that is both redundantly parenthesised and otherwise malformed, the redundant-parentheses error may now surface where a different syntax error would have surfaced previously. Both paths still reject with a ValidationException, and no behavioural contract pins the message for doubly-invalid input.

Checklist

• I have read CONTRIBUTING.md
• All tests pass (cargo test --workspace)
• Code is formatted (cargo fmt --check)
• Clippy is clean (cargo clippy --all-targets -- -D warnings, matching CI)
• I have added or updated tests for new functionality
• I have updated documentation if behavior changed (no user-facing docs describe redundant-paren handling; this increases DynamoDB parity)

Breaking changes

Expressions that were previously accepted but are invalid on DynamoDB are now rejected. KeyConditionExpression values with redundant parentheses such as ((pk = :v)) now return a ValidationException instead of being accepted, matching DynamoDB. What filter and condition expressions accept is unchanged; only the FilterExpression error label changes.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache License 2.0 and I agree to the Developer Certificate of Origin (DCO). See CONTRIBUTING.md for details.

[pdf-amzn]

Oops! The comment originally here was intended for PR 133

[hicksy]

Thanks for taking a look!

Before I action anything - this PR (#131) is the redundant-parentheses fix for #130, and I can't see where it touches resolver.rs, validate_begins_with_operands or begins_with.

Those three points all read like the begins_with operand-type work in #133.

Have I got that right, or is there something in the paren change you're pointing at that I've not picked up?

[pdf-amzn]

oh, ugh, I think the scripting I've been using to queue these up for checking botched this. Either that or I just copy-pasted wrong, although I remember having to double check because I thought something funny was going on. Will recheck

[pdf-amzn]

OK, I've given this a proper look. I'm concerned about the O(n^2) behavior of check_redundant_parens, since it scans forward independently for every "(". Can this be done with a one-pass or two-pass algorithm? Otherwise looks good.

FWIW, GenAI gave me this code, the if i + 1 < close looks strange, but maybe it works well enough to prevent out-of-bounds accesses there? I haven't taken the time to think that through properly.

pub fn check_redundant_parens(tokens: &[Token]) -> Result<(), String> {
    // First pass: compute matching ')' index for each '('.
    let mut match_of: Vec<usize> = vec![0; 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] = i;
                }
            }
            _ => {}
        }
    }
    // Second pass: check for redundant parens.
    for (i, token) in tokens.iter().enumerate() {
        if *token == Token::LParen && match_of[i] > 0 {
            let close = match_of[i];
            if i + 1 < close
                && tokens[i + 1] == Token::LParen
                && match_of[i + 1] == close - 1
            {
                return Err("The expression has redundant parentheses;".to_owned());
            }
        }
    }
    Ok(())
}

[hicksy]

Yep, that's better.

Switched check_redundant_parens to two passes (7657f35): one stack pass mapping each ( to its matching ), then a single scan for nested groups.

The per-paren rescan is removed, behaviour and error messages are unchanged, and the existing paren tests still pass.

Also added a short comment on the i + 1 < close line.