From eb370a39aa5f4ddb0266ff4923d58d48ec95c1fe Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sat, 24 Oct 2026 23:36:41 +0000 Subject: [PATCH 01/12] fix(query): check Query values against the key schema and the number rules A Query with a KeyConditionExpression did not check a key condition value's type against the key attribute's AttributeDefinitions type, and did not check that an N value in ExpressionAttributeValues is a number. The storage layer compared the value's inner text with the stored key and ignored the type tag, so {"N": "bob"} on an S partition key read the bob partition while authorization bound no dynamodb:LeadingKeys value to it, and a sort key value of another type answered InternalServerError on SQLite. Add the two checks the service applies before it reads a row, on the expression path (crates/engine/src/query_key_values.rs): - an N value in ExpressionAttributeValues that is not a number answers "The parameter cannot be converted to a numeric value: " (the overflow and 38-digit texts for those cases; an empty value answers the message with the colon). The check covers the whole map, including NS members and values nested in L and M, whether the key condition, the filter or nothing reads them, with the walk PutItem and UpdateItem already use. It runs at request time after the Limit and expression parameter mix checks and before the table lookup, which moves those two checks ahead of the table lookup as well. The server runs the same request-time checks ahead of the IAM decision, so a principal under any LeadingKeys fence form gets ValidationException and no items; - a key condition value whose type is not the key attribute's type answers "One or more parameter values were invalid: Condition parameter type does not match schema type", for the partition key, the sort key under every comparison operator, both BETWEEN bounds, the begins_with prefix and multi-attribute index keys, after the key schema, filter, projection, unused value and Select checks and before the ExclusiveStartKey check. When a compared placeholder is undefined the check returns without a verdict and the undefined placeholder is reported. Measured on the service on 2026-10-24 (113 requests, each sent twice). The legacy KeyConditions path is unchanged; its checks land with #507. Scan is unchanged. Unit tests at the checks and a dual-target wire test file, tests/test_query_key_value_types.py, which fails on main. --- crates/core/src/validation/mod.rs | 10 +- crates/engine/src/lib.rs | 1 + crates/engine/src/query.rs | 59 +-- crates/engine/src/query_key_values.rs | 224 +++++++++ crates/engine/src/query_key_values/tests.rs | 395 ++++++++++++++++ crates/server/src/request_helpers.rs | 14 + tests/test_query_key_value_types.py | 489 ++++++++++++++++++++ 7 files changed, 1156 insertions(+), 36 deletions(-) create mode 100644 crates/engine/src/query_key_values.rs create mode 100644 crates/engine/src/query_key_values/tests.rs create mode 100644 tests/test_query_key_value_types.py diff --git a/crates/core/src/validation/mod.rs b/crates/core/src/validation/mod.rs index c144b46b6..02e24e618 100755 --- a/crates/core/src/validation/mod.rs +++ b/crates/core/src/validation/mod.rs @@ -1903,7 +1903,15 @@ fn validate_lsi_requires_range_key(input: &CreateTableInput) -> Result<(), Dynam Ok(()) } -fn validate_attribute_number(value: &AttributeValue) -> Result<(), DynamoDbError> { +/// Validate every number inside one attribute value (`N`, `NS` members, and +/// the values nested in `L` and `M`) against the `DynamoDB` number rules, with +/// the bare numeric message. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` for the first number that is +/// not a number, has more than 38 significant digits, or is out of range. +pub fn validate_attribute_number(value: &AttributeValue) -> Result<(), DynamoDbError> { match value { AttributeValue::N(n) => { number::validate_and_normalize_number(n)?; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 74083fbfa..d24b47286 100755 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -27,6 +27,7 @@ mod legacy_filter; mod list_tables; mod put_item; mod query; +pub mod query_key_values; mod read_helpers; mod request_limits; mod scan; diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index f45526af8..97e07ead2 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -71,6 +71,14 @@ pub async fn handle_query( )?; let input: QueryInput = serde_json::from_value(body).map_err(crate::deserialize_error)?; + // Request-time checks in the service's order: Limit, the expression and + // non-expression parameter mix, then every number in + // ExpressionAttributeValues (measured 2026-10-24: a Limit of 0 and a + // parameter mix are reported ahead of an N value that is not a number, and + // that value is reported ahead of a missing table or index). The server + // runs the same function ahead of the IAM decision; see `query_key_values`. + crate::query_key_values::validate_query_request_values(&input)?; + // P118: Fetch key_info first so we can use table_id for index lookup. let key_info = ctx .table_key_info(&input.table_name) @@ -137,14 +145,7 @@ pub async fn handle_query( )?; } - // Validate Limit >= 1 (REQ-QUERY-001) - if let Some(limit) = input.limit - && limit < 1 - { - return Err(DynamoDbError::ValidationException( - "1 validation error detected: Value at 'Limit' failed to satisfy constraint: Member must have value greater than or equal to 1".to_owned(), - )); - } + // Limit >= 1 is checked at request time in validate_query_request_values. // For index queries, build a key_info that reflects the index's key schema // so the storage layer uses the correct SK column for the index table. @@ -166,35 +167,10 @@ pub async fn handle_query( key_info.clone() }; - // --- Legacy vs expression mutual exclusivity checks --- - let kce_present = input.key_condition_expression.is_some(); - let has_kc = input.key_conditions.as_ref().is_some_and(|m| !m.is_empty()); - let has_fe = input.filter_expression.is_some(); - let has_qf = input.query_filter.as_ref().is_some_and(|m| !m.is_empty()); - let has_pe = input.projection_expression.is_some(); - let has_atg = input - .attributes_to_get - .as_ref() - .is_some_and(|a| !a.is_empty()); + // The legacy versus expression mutual exclusivity check runs at request + // time in validate_query_request_values. let has_cond_op = input.conditional_operator.is_some(); - // KeyConditions legitimately combines with FilterExpression and - // ProjectionExpression, so it joins the mixing check only when - // KeyConditionExpression is also present. - extenddb_core::validation::validate_no_expression_param_mixing( - &[ - ("AttributesToGet", has_atg), - ("QueryFilter", has_qf), - ("ConditionalOperator", has_cond_op), - ("KeyConditions", has_kc && kce_present), - ], - &[ - ("ProjectionExpression", has_pe), - ("FilterExpression", has_fe), - ("KeyConditionExpression", kce_present), - ], - )?; - // An explicitly empty KeyConditionExpression is rejected up front, before // the expression-parameter-usage checks (matching real DynamoDB). reject_empty_key_condition_expression(input.key_condition_expression.as_deref())?; @@ -471,6 +447,19 @@ pub async fn handle_query( )?; } + // A key condition value whose type is not the key attribute's type is + // refused after the key schema, filter, projection, unused value and + // Select checks and before the ExclusiveStartKey check (measured + // 2026-10-24). The legacy KeyConditions path makes its own checks when it + // desugars. + if legacy_kc_maps.is_none() { + crate::query_key_values::validate_key_condition_types( + &key_condition, + effective_maps, + &query_key_info.attribute_definitions, + )?; + } + // Validate ExclusiveStartKey matches the key schema if let Some(ref start_key) = input.exclusive_start_key { validate_query_exclusive_start_key(start_key, &key_info, index_info.as_ref())?; diff --git a/crates/engine/src/query_key_values.rs b/crates/engine/src/query_key_values.rs new file mode 100644 index 000000000..e49968648 --- /dev/null +++ b/crates/engine/src/query_key_values.rs @@ -0,0 +1,224 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! Checks on the values a `Query` carries, as the service applies them before +//! any row is read. +//! +//! Measured on 2026-10-24 (tables with S/S, S/N and B/B keys, a GSI and an +//! LSI): +//! +//! 1. an `N` value in `ExpressionAttributeValues` that is not a number answers +//! `The parameter cannot be converted to a numeric value: ` (or the +//! overflow, underflow and 38-digit messages). Every value in the map is +//! checked, whether the key condition, the filter or nothing reads it. The +//! check runs at request time, after the `Limit` check and the expression +//! and non-expression parameter mix check, and before the table is looked +//! up; see [`validate_query_request_values`]; +//! 2. a key condition value whose type is not the key attribute's +//! `AttributeDefinitions` type answers `One or more parameter values were +//! invalid: Condition parameter type does not match schema type`, on the +//! partition and the sort key, on a table, a GSI and an LSI, for `=`, the +//! ordering operators, `BETWEEN` and `begins_with`. The check runs after the +//! key schema, filter, projection, unused value and `Select` checks and +//! before the `ExclusiveStartKey` check; see +//! [`validate_key_condition_types`]. +//! +//! Without these checks a backend compares the value's inner text with the +//! stored key and ignores the type tag, so `{"N": "bob"}` on an S key reads the +//! `bob` partition, while authorization binds no `dynamodb:LeadingKeys` value +//! to a number it cannot parse. + +use std::collections::HashMap; + +use extenddb_core::error::DynamoDbError; +use extenddb_core::expression::{ + Expr, ExpressionMaps, KeyCondition, PathElement, SortKeyCondition, +}; +use extenddb_core::types::{AttributeDefinition, AttributeValue, QueryInput, ScalarAttributeType}; + +/// The service's text for a key condition value of the wrong type. +pub const KEY_CONDITION_TYPE_MISMATCH: &str = "One or more parameter values were invalid: Condition parameter type does not match schema type"; + +/// The service's text for a `Limit` below 1 on `Query`. +pub const LIMIT_BELOW_ONE: &str = "1 validation error detected: Value at 'Limit' failed to satisfy constraint: Member must have value greater than or equal to 1"; + +/// The bare numeric message the number validator uses for an empty `N`. +const EMPTY_NUMBER: &str = "The parameter cannot be converted to a numeric value"; + +/// The `Query` checks that run on the request alone, before the table is +/// looked up, in the service's order: `Limit`, the expression and +/// non-expression parameter mix, then every number in +/// `ExpressionAttributeValues`. +/// +/// The server runs the same function ahead of the IAM decision, so a principal +/// whose `dynamodb:LeadingKeys` fence cannot bind a number that does not parse +/// gets the request's `ValidationException` rather than +/// `AccessDeniedException`. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn validate_query_request_values(input: &QueryInput) -> Result<(), DynamoDbError> { + if let Some(limit) = input.limit + && limit < 1 + { + return Err(DynamoDbError::ValidationException( + LIMIT_BELOW_ONE.to_owned(), + )); + } + + let kce_present = input.key_condition_expression.is_some(); + let has_kc = input.key_conditions.as_ref().is_some_and(|m| !m.is_empty()); + let has_qf = input.query_filter.as_ref().is_some_and(|m| !m.is_empty()); + let has_atg = input + .attributes_to_get + .as_ref() + .is_some_and(|a| !a.is_empty()); + // KeyConditions combines with FilterExpression and ProjectionExpression, so + // it joins the mix check only when KeyConditionExpression is also present. + extenddb_core::validation::validate_no_expression_param_mixing( + &[ + ("AttributesToGet", has_atg), + ("QueryFilter", has_qf), + ("ConditionalOperator", input.conditional_operator.is_some()), + ("KeyConditions", has_kc && kce_present), + ], + &[ + ( + "ProjectionExpression", + input.projection_expression.is_some(), + ), + ("FilterExpression", input.filter_expression.is_some()), + ("KeyConditionExpression", kce_present), + ], + )?; + + if let Some(values) = &input.expression_attribute_values { + validate_expression_attribute_value_numbers(values)?; + } + Ok(()) +} + +/// Refuse the first `N` value in `ExpressionAttributeValues` (including `NS` +/// members and values nested in `L` and `M`) that is not a number, with the +/// bare numeric message: no `ExpressionAttributeValues contains invalid value` +/// wrapper, which the service uses on `UpdateItem` and not on `Query`. The +/// placeholders are visited in sorted order so the reported value is +/// deterministic when several are invalid. An empty `N` answers the message +/// with the colon and nothing after it, as the service does on `Query`. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with the service's text. +pub fn validate_expression_attribute_value_numbers( + values: &HashMap, +) -> Result<(), DynamoDbError> { + let mut keys: Vec<&String> = values.keys().collect(); + keys.sort(); + for key in keys { + match extenddb_core::validation::validate_attribute_number(&values[key]) { + Ok(()) => {} + Err(DynamoDbError::ValidationException(msg)) if msg == EMPTY_NUMBER => { + return Err(DynamoDbError::ValidationException(format!( + "{EMPTY_NUMBER}: " + ))); + } + Err(other) => return Err(other), + } + } + Ok(()) +} + +/// Refuse a key condition value whose type is not the key's +/// `AttributeDefinitions` type. +/// +/// Every value the condition compares is checked: the partition key value(s), +/// the sort key value (or both `BETWEEN` bounds, or the `begins_with` prefix) +/// and the extra equality conditions of a multi-attribute key schema. A path +/// that does not resolve to an attribute name, a value that is not a +/// placeholder, and a key attribute with no definition are left to the checks +/// that already refuse them. When any compared placeholder is undefined the +/// whole check is skipped: the service reports the undefined placeholder +/// first, and the storage layer does so here. +/// +/// # Errors +/// +/// Returns `DynamoDbError::ValidationException` with +/// [`KEY_CONDITION_TYPE_MISMATCH`]. +pub fn validate_key_condition_types( + key_condition: &KeyCondition, + maps: &ExpressionMaps, + attribute_definitions: &[AttributeDefinition], +) -> Result<(), DynamoDbError> { + let mut pairs: Vec<(&[PathElement], &Expr)> = + vec![(&key_condition.pk_path, &key_condition.pk_value)]; + for (path, value) in &key_condition.extra_pk_conditions { + pairs.push((path, value)); + } + match &key_condition.sk_condition { + Some(SortKeyCondition::Compare { path, value, .. }) => pairs.push((path, value)), + Some(SortKeyCondition::Between { path, low, high }) => { + pairs.push((path, low)); + pairs.push((path, high)); + } + Some(SortKeyCondition::BeginsWith { path, prefix }) => pairs.push((path, prefix)), + None => {} + } + for (path, value) in &key_condition.extra_sk_conditions { + pairs.push((path, value)); + } + + let any_undefined = pairs.iter().any( + |(_, value)| matches!(value, Expr::Placeholder(name) if !maps.values.contains_key(name)), + ); + if any_undefined { + return Ok(()); + } + + for (path, value) in pairs { + let Some(attr_name) = resolve_attr_name(path, &maps.names) else { + continue; + }; + let Some(expected) = attribute_definitions + .iter() + .find(|ad| ad.attribute_name == attr_name) + .map(|ad| ad.attribute_type) + else { + continue; + }; + let Expr::Placeholder(name) = value else { + continue; + }; + let Some(av) = maps.values.get(name) else { + continue; + }; + if !type_matches(expected, av) { + return Err(DynamoDbError::ValidationException( + KEY_CONDITION_TYPE_MISMATCH.to_owned(), + )); + } + } + Ok(()) +} + +fn type_matches(expected: ScalarAttributeType, value: &AttributeValue) -> bool { + matches!( + (expected, value), + (ScalarAttributeType::S, AttributeValue::S(_)) + | (ScalarAttributeType::N, AttributeValue::N(_)) + | (ScalarAttributeType::B, AttributeValue::B(_)) + ) +} + +/// The top-level attribute name of a key path, through `#name` references. +fn resolve_attr_name(path: &[PathElement], names: &HashMap) -> Option { + match path.first()? { + PathElement::Attribute(name) => match name.strip_prefix('#') { + Some(reference) => names.get(reference).cloned(), + None => Some(name.clone()), + }, + PathElement::Index(_) => None, + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/engine/src/query_key_values/tests.rs b/crates/engine/src/query_key_values/tests.rs new file mode 100644 index 000000000..6130dd875 --- /dev/null +++ b/crates/engine/src/query_key_values/tests.rs @@ -0,0 +1,395 @@ +// SPDX-License-Identifier: Apache-2.0 + +use std::collections::HashMap; + +use extenddb_core::error::DynamoDbError; +use extenddb_core::expression::{ + ExpressionMaps, KeyCondition, parse_key_condition, tokenize_with_limit, +}; +use extenddb_core::types::{AttributeDefinition, AttributeValue, QueryInput, ScalarAttributeType}; + +use super::{ + KEY_CONDITION_TYPE_MISMATCH, LIMIT_BELOW_ONE, validate_expression_attribute_value_numbers, + validate_key_condition_types, validate_query_request_values, +}; + +fn defs(pairs: &[(&str, ScalarAttributeType)]) -> Vec { + pairs + .iter() + .map(|(name, ty)| AttributeDefinition { + attribute_name: (*name).to_owned(), + attribute_type: *ty, + }) + .collect() +} + +fn parse(kce: &str) -> KeyCondition { + let tokens = tokenize_with_limit(kce, 1000).expect("tokenize"); + parse_key_condition(&tokens).expect("parse") +} + +fn maps(names: &[(&str, &str)], values: &[(&str, AttributeValue)]) -> ExpressionMaps { + ExpressionMaps::new( + names + .iter() + .map(|(k, v)| ((*k).to_owned(), (*v).to_owned())) + .collect(), + values + .iter() + .map(|(k, v)| ((*k).to_owned(), v.clone())) + .collect(), + ) +} + +fn s(v: &str) -> AttributeValue { + AttributeValue::S(v.to_owned()) +} + +fn n(v: &str) -> AttributeValue { + AttributeValue::N(v.to_owned()) +} + +fn message(result: Result<(), DynamoDbError>) -> Option { + match result { + Ok(()) => None, + Err(DynamoDbError::ValidationException(msg)) => Some(msg), + Err(other) => panic!("unexpected error {other:?}"), + } +} + +const SS: &[(&str, ScalarAttributeType)] = &[ + ("pk", ScalarAttributeType::S), + ("sk", ScalarAttributeType::S), +]; + +#[test] +fn partition_value_of_another_type_is_refused() { + let kc = parse("pk = :u"); + for wrong in [ + n("5"), + AttributeValue::B(vec![1]), + AttributeValue::Bool(true), + AttributeValue::Null, + ] { + let m = maps(&[], &[("u", wrong)]); + assert_eq!( + message(validate_key_condition_types(&kc, &m, &defs(SS))).as_deref(), + Some(KEY_CONDITION_TYPE_MISMATCH) + ); + } +} + +#[test] +fn matching_types_pass() { + let kc = parse("pk = :u AND sk BETWEEN :a AND :b"); + let m = maps(&[], &[("u", s("x")), ("a", s("a")), ("b", s("b"))]); + assert!(validate_key_condition_types(&kc, &m, &defs(SS)).is_ok()); + + let kc = parse("pk = :u AND begins_with(sk, :p)"); + let m = maps(&[], &[("u", s("x")), ("p", s("a"))]); + assert!(validate_key_condition_types(&kc, &m, &defs(SS)).is_ok()); + + let n_sk = defs(&[ + ("pk", ScalarAttributeType::S), + ("sk", ScalarAttributeType::N), + ]); + let kc = parse("pk = :u AND sk > :v"); + let m = maps(&[], &[("u", s("x")), ("v", n("5"))]); + assert!(validate_key_condition_types(&kc, &m, &n_sk).is_ok()); +} + +#[test] +fn sort_key_value_of_another_type_is_refused_for_every_operator() { + for kce in [ + "pk = :u AND sk = :v", + "pk = :u AND sk < :v", + "pk = :u AND sk <= :v", + "pk = :u AND sk > :v", + "pk = :u AND sk >= :v", + "pk = :u AND :v <= sk", + "pk = :u AND begins_with(sk, :v)", + ] { + let kc = parse(kce); + let m = maps(&[], &[("u", s("x")), ("v", n("5"))]); + assert_eq!( + message(validate_key_condition_types(&kc, &m, &defs(SS))).as_deref(), + Some(KEY_CONDITION_TYPE_MISMATCH), + "{kce}" + ); + } +} + +#[test] +fn either_between_bound_of_another_type_is_refused() { + let kc = parse("pk = :u AND sk BETWEEN :a AND :b"); + for (a, b) in [(n("1"), s("b")), (s("a"), n("2")), (n("1"), n("2"))] { + let m = maps(&[], &[("u", s("x")), ("a", a), ("b", b)]); + assert_eq!( + message(validate_key_condition_types(&kc, &m, &defs(SS))).as_deref(), + Some(KEY_CONDITION_TYPE_MISMATCH) + ); + } +} + +#[test] +fn string_on_a_number_or_binary_key_is_refused() { + let n_sk = defs(&[ + ("pk", ScalarAttributeType::S), + ("sk", ScalarAttributeType::N), + ]); + let kc = parse("pk = :u AND sk = :v"); + let m = maps(&[], &[("u", s("x")), ("v", s("5"))]); + assert_eq!( + message(validate_key_condition_types(&kc, &m, &n_sk)).as_deref(), + Some(KEY_CONDITION_TYPE_MISMATCH) + ); + + let b_pk = defs(&[ + ("bpk", ScalarAttributeType::B), + ("bsk", ScalarAttributeType::B), + ]); + let kc = parse("bpk = :u"); + for wrong in [s("x"), n("5")] { + let m = maps(&[], &[("u", wrong)]); + assert_eq!( + message(validate_key_condition_types(&kc, &m, &b_pk)).as_deref(), + Some(KEY_CONDITION_TYPE_MISMATCH) + ); + } +} + +#[test] +fn name_reference_resolves_to_the_defined_attribute() { + let kc = parse("#p = :u"); + let m = maps(&[("p", "pk")], &[("u", n("5"))]); + assert_eq!( + message(validate_key_condition_types(&kc, &m, &defs(SS))).as_deref(), + Some(KEY_CONDITION_TYPE_MISMATCH) + ); + let m = maps(&[("p", "pk")], &[("u", s("x"))]); + assert!(validate_key_condition_types(&kc, &m, &defs(SS)).is_ok()); +} + +#[test] +fn index_key_schema_uses_the_index_attribute_definition() { + // A GSI on gpk (S) with a range key gsk (S) on a table whose base sort key is N. + let d = defs(&[ + ("pk", ScalarAttributeType::S), + ("sk", ScalarAttributeType::N), + ("gpk", ScalarAttributeType::S), + ("gsk", ScalarAttributeType::S), + ]); + let kc = parse("gpk = :g AND gsk = :s"); + let m = maps(&[], &[("g", s("x")), ("s", n("5"))]); + assert_eq!( + message(validate_key_condition_types(&kc, &m, &d)).as_deref(), + Some(KEY_CONDITION_TYPE_MISMATCH) + ); + let m = maps(&[], &[("g", s("x")), ("s", s("g"))]); + assert!(validate_key_condition_types(&kc, &m, &d).is_ok()); +} + +#[test] +fn multipart_hash_conditions_are_checked() { + let d = defs(&[ + ("g1", ScalarAttributeType::S), + ("g2", ScalarAttributeType::N), + ("r", ScalarAttributeType::S), + ]); + let mut kc = parse("g1 = :a AND g2 = :b AND r > :c"); + let names = HashMap::new(); + kc.resolve_multipart(&["g1", "g2"], &names) + .expect("resolve"); + assert_eq!(kc.extra_pk_conditions.len(), 1); + let m = maps(&[], &[("a", s("x")), ("b", s("1")), ("c", s("r"))]); + assert_eq!( + message(validate_key_condition_types(&kc, &m, &d)).as_deref(), + Some(KEY_CONDITION_TYPE_MISMATCH) + ); + let m = maps(&[], &[("a", s("x")), ("b", n("1")), ("c", s("r"))]); + assert!(validate_key_condition_types(&kc, &m, &d).is_ok()); +} + +#[test] +fn undefined_placeholder_and_undefined_attribute_are_left_to_other_checks() { + let kc = parse("pk = :u AND sk = :zz"); + let m = maps(&[], &[("u", n("5"))]); + // The partition value is wrong, and :zz is undefined: the service reports + // the undefined placeholder first, so the type check stands aside. + assert!(validate_key_condition_types(&kc, &m, &defs(SS)).is_ok()); + let m = maps(&[], &[("u", s("x"))]); + assert!(validate_key_condition_types(&kc, &m, &defs(SS)).is_ok()); + + // No AttributeDefinition for the attribute: not this check's refusal. + let kc = parse("other = :u"); + let m = maps(&[], &[("u", n("5"))]); + assert!(validate_key_condition_types(&kc, &m, &defs(SS)).is_ok()); +} + +fn query(body: serde_json::Value) -> QueryInput { + serde_json::from_value(body).expect("QueryInput") +} + +#[test] +fn number_values_that_are_not_numbers_are_refused_with_the_bare_message() { + let cases = [ + ( + "bob", + "The parameter cannot be converted to a numeric value: bob", + ), + ( + "1e", + "The parameter cannot be converted to a numeric value: 1e", + ), + ( + "12abc", + "The parameter cannot be converted to a numeric value: 12abc", + ), + ( + " 5", + "The parameter cannot be converted to a numeric value: 5", + ), + ("", "The parameter cannot be converted to a numeric value: "), + ( + "1e200", + "Number overflow. Attempting to store a number with magnitude larger than supported range", + ), + ]; + for (raw, expected) in cases { + let values = HashMap::from([(":u".to_owned(), n(raw))]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some(expected), + "{raw:?}" + ); + } + let values = HashMap::from([(":u".to_owned(), n(&"1".repeat(39)))]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some("Attempting to store more than 38 significant digits in a Number") + ); +} + +#[test] +fn number_values_that_are_numbers_and_other_types_pass() { + let values = HashMap::from([ + (":u".to_owned(), n("5")), + (":v".to_owned(), n("-1.5e3")), + (":w".to_owned(), s("bob")), + (":x".to_owned(), AttributeValue::Bool(true)), + (":y".to_owned(), AttributeValue::L(vec![n("1"), s("bob")])), + ]); + assert!(validate_expression_attribute_value_numbers(&values).is_ok()); +} + +#[test] +fn every_value_in_the_map_is_checked_whatever_reads_it() { + // :n is read by nothing in the key condition: checked all the same. + let values = HashMap::from([(":p".to_owned(), s("x")), (":n".to_owned(), n("bob"))]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob") + ); +} + +#[test] +fn numbers_nested_in_sets_lists_and_maps_are_checked() { + let nested = [ + AttributeValue::NS(["1".to_owned(), "bob".to_owned()].into_iter().collect()), + AttributeValue::L(vec![s("a"), n("bob")]), + AttributeValue::M([("a".to_owned(), n("bob"))].into_iter().collect()), + AttributeValue::M( + [( + "a".to_owned(), + AttributeValue::L(vec![AttributeValue::M( + [("b".to_owned(), n("bob"))].into_iter().collect(), + )]), + )] + .into_iter() + .collect(), + ), + ]; + for value in nested { + let values = HashMap::from([(":p".to_owned(), s("x")), (":n".to_owned(), value.clone())]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob"), + "{value:?}" + ); + } +} + +#[test] +fn first_invalid_number_in_placeholder_order_is_reported() { + let values = HashMap::from([(":b".to_owned(), n("two")), (":a".to_owned(), n("one"))]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some("The parameter cannot be converted to a numeric value: one") + ); +} + +#[test] +fn request_time_checks_run_limit_then_mix_then_numbers() { + let bad_number = serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"N": "bob"}}, + }); + + let mut with_limit = bad_number.clone(); + with_limit["Limit"] = serde_json::json!(0); + assert_eq!( + message(validate_query_request_values(&query(with_limit))).as_deref(), + Some(LIMIT_BELOW_ONE) + ); + + let mut with_mix = bad_number.clone(); + with_mix["KeyConditions"] = serde_json::json!({ + "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]} + }); + assert_eq!( + message(validate_query_request_values(&query(with_mix))).as_deref(), + Some( + "Can not use both expression and non-expression parameters in the same request: \ + Non-expression parameters: {KeyConditions} Expression parameters: {KeyConditionExpression}" + ) + ); + + assert_eq!( + message(validate_query_request_values(&query(bad_number))).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob") + ); + + // A number only a FilterExpression reads, and one nothing reads. + for body in [ + serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :p", + "FilterExpression": "a = :n", + "ExpressionAttributeValues": {":p": {"S": "x"}, ":n": {"N": "bob"}}, + }), + serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": {":p": {"S": "x"}, ":n": {"N": "bob"}}, + }), + ] { + assert_eq!( + message(validate_query_request_values(&query(body))).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob") + ); + } + + // Legacy KeyConditions alone does not join the mix check; a well-formed + // request passes. + let legacy = serde_json::json!({ + "TableName": "t", + "KeyConditions": { + "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]} + }, + "FilterExpression": "a = :n", + "ExpressionAttributeValues": {":n": {"N": "5"}}, + "Limit": 1, + }); + assert!(validate_query_request_values(&query(legacy)).is_ok()); +} diff --git a/crates/server/src/request_helpers.rs b/crates/server/src/request_helpers.rs index e7d12abf9..9f34b2632 100755 --- a/crates/server/src/request_helpers.rs +++ b/crates/server/src/request_helpers.rs @@ -184,6 +184,20 @@ pub(crate) async fn authorize_request( authz_resource::validate_tag_resource_arn_shape(arn)?; } + // A Query's request-time checks (Limit, the expression and non-expression + // parameter mix, every number in ExpressionAttributeValues) run before the + // IAM decision, for every principal. The engine makes the same checks at + // request time; they run here as well because authorization binds no + // dynamodb:LeadingKeys value to a number it cannot parse, and a fence that + // needs the key present (ForAnyValue, a Null guard, the plain operators) + // would otherwise answer AccessDeniedException where ValidationException is + // the request's error. A body the engine cannot deserialize is left to it. + if operation == "Query" + && let Ok(query) = serde_json::from_value::(input.clone()) + { + extenddb_engine::query_key_values::validate_query_request_values(&query)?; + } + // Every other ARN-bearing field (StreamArn, BackupArn, the export TableArn, // a restore SourceTableArn) must parse to the resource kind the operation // takes, or the request is refused with ValidationException before the IAM diff --git a/tests/test_query_key_value_types.py b/tests/test_query_key_value_types.py new file mode 100644 index 000000000..02ee3b317 --- /dev/null +++ b/tests/test_query_key_value_types.py @@ -0,0 +1,489 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Query key condition values checked against the key schema. + +A ``KeyConditionExpression`` value is refused before any row is read when an +``N`` value is not a number (``The parameter cannot be converted to a numeric +value: ``) or when a key condition value's type is not the key +attribute's ``AttributeDefinitions`` type (``One or more parameter values were +invalid: Condition parameter type does not match schema type``). Every expected +answer below was measured against the service on 2026-10-24 (15:10 UTC and +21:00 UTC) on tables with S/S, S/N and B/B keys, a GSI and an LSI. The rows +also pin where the two checks fall against the other Query checks: the numeric +check after Limit and the expression parameter mix check and before the table +lookup; the type check after the key schema, filter, projection, unused value +and Select checks and before the ExclusiveStartKey check. + +Dual-target: runs against the service and against extenddb with the same +assertions. The LeadingKeys class needs the extenddb management API and is +skipped against the service. + +Run: + EXTENDDB_TEST_ENDPOINT=https://localhost:18443 \\ + EXTENDDB_ADMIN_USER=admin EXTENDDB_ADMIN_PASSWORD= \\ + pytest tests/test_query_key_value_types.py -v +""" + +from __future__ import annotations + +import os +import uuid +from typing import Any + +import boto3 +import pytest +from botocore.config import Config as BotoConfig +from botocore.exceptions import ClientError + +from conftest import scoped_table, wait_for_active + +NUMERIC = "The parameter cannot be converted to a numeric value: " +TYPE_MISMATCH = ( + "One or more parameter values were invalid: " + "Condition parameter type does not match schema type" +) + +S = {"AttributeType": "S"} + + +def _err(exc_info) -> tuple[str, str]: + err = exc_info.value.response["Error"] + return err["Code"], err["Message"] + + +@pytest.fixture(scope="module") +def ss_table(dynamodb_client): + """pk S, sk S; LSI lsi1 on lsk S; GSI gsi1 on gpk S (hash only).""" + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", **S}, + {"AttributeName": "sk", **S}, + {"AttributeName": "lsk", **S}, + {"AttributeName": "gpk", **S}, + ], + key_schema=[ + {"AttributeName": "pk", "KeyType": "HASH"}, + {"AttributeName": "sk", "KeyType": "RANGE"}, + ], + LocalSecondaryIndexes=[{ + "IndexName": "lsi1", + "KeySchema": [ + {"AttributeName": "pk", "KeyType": "HASH"}, + {"AttributeName": "lsk", "KeyType": "RANGE"}, + ], + "Projection": {"ProjectionType": "ALL"}, + }], + GlobalSecondaryIndexes=[{ + "IndexName": "gsi1", + "KeySchema": [{"AttributeName": "gpk", "KeyType": "HASH"}], + "Projection": {"ProjectionType": "ALL"}, + }], + ) as name: + for pk in ("alice", "bob"): + dynamodb_client.put_item(TableName=name, Item={ + "pk": {"S": pk}, "sk": {"S": "1"}, "lsk": {"S": "l"}, "gpk": {"S": pk}}) + yield name + + +@pytest.fixture(scope="module") +def ns_table(dynamodb_client): + """pk S, sk N; GSI gsi1 on gpk S / gsk S.""" + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", **S}, + {"AttributeName": "sk", "AttributeType": "N"}, + {"AttributeName": "gpk", **S}, + {"AttributeName": "gsk", **S}, + ], + key_schema=[ + {"AttributeName": "pk", "KeyType": "HASH"}, + {"AttributeName": "sk", "KeyType": "RANGE"}, + ], + GlobalSecondaryIndexes=[{ + "IndexName": "gsi1", + "KeySchema": [ + {"AttributeName": "gpk", "KeyType": "HASH"}, + {"AttributeName": "gsk", "KeyType": "RANGE"}, + ], + "Projection": {"ProjectionType": "ALL"}, + }], + ) as name: + for pk in ("alice", "bob"): + dynamodb_client.put_item(TableName=name, Item={ + "pk": {"S": pk}, "sk": {"N": "5"}, "gpk": {"S": pk}, "gsk": {"S": "g"}}) + yield name + + +@pytest.fixture(scope="module") +def bb_table(dynamodb_client): + """bpk B, bsk B.""" + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "bpk", "AttributeType": "B"}, + {"AttributeName": "bsk", "AttributeType": "B"}, + ], + key_schema=[ + {"AttributeName": "bpk", "KeyType": "HASH"}, + {"AttributeName": "bsk", "KeyType": "RANGE"}, + ], + ) as name: + dynamodb_client.put_item(TableName=name, Item={"bpk": {"B": b"bob"}, "bsk": {"B": b"x"}}) + yield name + + +NB = {"N": "bob"} +N5 = {"N": "5"} +SX = {"S": "x"} +B_BOB = {"B": b"bob"} +MISSING = "no-such-table-kv-types" +LEGACY_PK = {"pk": {"ComparisonOperator": "EQ", "AttributeValueList": [SX]}} +L_NB = {"L": [{"S": "a"}, NB]} +M_NB = {"M": {"a": NB}} +NS_NB = {"NS": ["1", "bob"]} + +LIMIT_MSG = ( + "1 validation error detected: Value at 'Limit' failed to satisfy constraint: " + "Member must have value greater than or equal to 1" +) +MIX_MSG = ( + "Can not use both expression and non-expression parameters in the same request: " + "Non-expression parameters: {KeyConditions} Expression parameters: {KeyConditionExpression}" +) +OVERFLOW_MSG = ( + "Number overflow. Attempting to store a number with magnitude larger than supported range" +) +DIGITS_MSG = "Attempting to store more than 38 significant digits in a Number" +MISSED_PK = "Query condition missed key schema element: pk" +MISSED_GPK = "Query condition missed key schema element: gpk" +RESERVED_KCE = ( + "Invalid KeyConditionExpression: Attribute name is a reserved keyword; reserved keyword: other" +) +NO_INDEX = "The table does not have the specified index: nope" +UNUSED_X = "Value provided in ExpressionAttributeValues unused in expressions: keys: {:x}" +GSI_CONSISTENT = "Consistent reads are not supported on global secondary indexes" +FILTER_BEGINS_WITH_N = ( + "Invalid FilterExpression: Incorrect operand type for operator or function; " + "operator or function: begins_with, operand type: N" +) +PROJECTION_OVERLAP = ( + "Invalid ProjectionExpression: Two document paths overlap with each other; " + "must remove or rewrite one of these paths; path one: [a], path two: [a, b]" +) +FILTER_RESERVED = ( + "Invalid FilterExpression: Attribute name is a reserved keyword; reserved keyword: name" +) +SELECT_PROJECTION = ( + "1 validation error detected: Cannot specify the ProjectionExpression when choosing to " + "get ALL_ATTRIBUTES" +) +V = "ValidationException" +RNF = "ResourceNotFoundException" + + +def kce(expr, values, **extra): + return {"KeyConditionExpression": expr, "ExpressionAttributeValues": values, **extra} + + +# (row, table, Query arguments, expected error code, expected message). +# Tables: ss (pk S, sk S, LSI lsi1 on lsk S, GSI gsi1 on gpk S), ns (pk S, sk N, +# GSI gsi1 on gpk S / gsk S), bb (bpk B, bsk B), missing (no such table). +ROWS = [ + # The numeric check against the other Query checks. + ("a1 missing table", "missing", kce("pk = :u", {":u": NB}), V, NUMERIC + "bob"), + ("a2 unknown IndexName", "ss", kce("pk = :u", {":u": NB}, IndexName="nope"), V, NUMERIC + "bob"), + ("a3 undefined placeholder", "ss", kce("pk = :u AND sk = :zz", {":u": NB}), V, NUMERIC + "bob"), + ("a4 unused value", "ss", kce("pk = :u", {":u": NB, ":x": SX}), V, NUMERIC + "bob"), + ("a5 syntax error", "ss", kce("pk = :u AND", {":u": NB}), V, NUMERIC + "bob"), + ("a6 Limit 0", "ss", kce("pk = :u", {":u": NB}, Limit=0), V, LIMIT_MSG), + ("a7 N bob only in FilterExpression", "ss", + kce("pk = :p", {":p": SX, ":n": NB}, FilterExpression="a = :n"), V, NUMERIC + "bob"), + ("a8 N bob unused", "ss", kce("pk = :p", {":p": SX, ":n": NB}), V, NUMERIC + "bob"), + ("a9 N bob in filter beside pk = N 5", "ss", + kce("pk = :p", {":p": N5, ":n": NB}, FilterExpression="a = :n"), V, NUMERIC + "bob"), + ("a10 ConsistentRead on GSI", "ss", + kce("gpk = :u", {":u": NB}, IndexName="gsi1", ConsistentRead=True), V, NUMERIC + "bob"), + ("a11 N empty", "ss", kce("pk = :u", {":u": {"N": ""}}), V, NUMERIC), + ("a12 N 1e", "ss", kce("pk = :u", {":u": {"N": "1e"}}), V, NUMERIC + "1e"), + ("a13 N 12abc", "ss", kce("pk = :u", {":u": {"N": "12abc"}}), V, NUMERIC + "12abc"), + ("a14 sk only", "ss", kce("sk = :u", {":u": NB}), V, NUMERIC + "bob"), + ("a15 KeyConditions and KeyConditionExpression", "ss", + kce("pk = :u", {":u": NB}, KeyConditions=LEGACY_PK), V, MIX_MSG), + ("a16 BETWEEN bounds reversed", "ss", + kce("pk = :u AND sk BETWEEN :a AND :b", {":u": NB, ":a": {"S": "b"}, ":b": {"S": "a"}}), + V, NUMERIC + "bob"), + ("a17 nested path", "ss", kce("pk.x = :u", {":u": NB}), V, NUMERIC + "bob"), + ("a18 pk >", "ss", kce("pk > :u", {":u": NB}), V, NUMERIC + "bob"), + ("a19 ExclusiveStartKey bad", "ss", + kce("pk = :u", {":u": NB}, ExclusiveStartKey={"zz": SX}), V, NUMERIC + "bob"), + ("a20 pk = B AND sk = N bob", "ss", + kce("pk = :u AND sk = :s", {":u": B_BOB, ":s": NB}), V, NUMERIC + "bob"), + ("a21 pk = S AND sk = N bob", "ss", + kce("pk = :u AND sk = :s", {":u": SX, ":s": NB}), V, NUMERIC + "bob"), + ("a22 N 1e200", "ss", kce("pk = :u", {":u": {"N": "1e200"}}), V, OVERFLOW_MSG), + ("a23 N 39 digits", "ss", kce("pk = :u", {":u": {"N": "1" * 39}}), V, DIGITS_MSG), + ("a24 Select ALL_ATTRIBUTES with ProjectionExpression", "ss", + kce("pk = :u", {":u": NB}, Select="ALL_ATTRIBUTES", ProjectionExpression="a"), + V, NUMERIC + "bob"), + ("a25 reserved word in filter", "ss", + kce("pk = :u", {":u": NB, ":n": SX}, FilterExpression="name = :n"), V, NUMERIC + "bob"), + ("a26 undefined name in filter", "ss", + kce("pk = :u", {":u": NB, ":n": SX}, FilterExpression="#n = :n"), V, NUMERIC + "bob"), + ("a27 two sort key conditions", "ss", + kce("pk = :u AND sk = :s AND sk > :t", {":u": NB, ":s": N5, ":t": {"N": "6"}}), + V, NUMERIC + "bob"), + ("a28 sk = N bob with unused value", "ss", + kce("pk = :u AND sk = :s", {":u": SX, ":s": NB, ":x": SX}), V, NUMERIC + "bob"), + ("a29 ProjectionExpression overlap", "ss", + kce("pk = :u", {":u": NB}, ProjectionExpression="a, a.b"), V, NUMERIC + "bob"), + ("a30 N space 5", "ss", kce("pk = :u", {":u": {"N": " 5"}}), V, NUMERIC + " 5"), + # The type check against the other key condition checks. + ("b1 sk = N 5 only", "ss", kce("sk = :s", {":s": N5}), V, MISSED_PK), + ("b2 reserved word in key condition", "ss", + kce("pk = :u AND other = :s", {":u": SX, ":s": N5}), V, RESERVED_KCE), + ("b4 BETWEEN N bounds on S sk", "ss", + kce("pk = :u AND sk BETWEEN :a AND :b", {":u": SX, ":a": {"N": "1"}, ":b": {"N": "2"}}), + V, TYPE_MISMATCH), + ("b6 sk > N 5", "ss", kce("pk = :u AND sk > :s", {":u": SX, ":s": N5}), V, TYPE_MISMATCH), + ("b7 sk < B", "ss", kce("pk = :u AND sk < :s", {":u": SX, ":s": B_BOB}), V, TYPE_MISMATCH), + ("b9 unknown IndexName", "ss", kce("pk = :u", {":u": N5}, IndexName="nope"), V, NO_INDEX), + ("b10 Limit 0", "ss", kce("pk = :u", {":u": N5}, Limit=0), V, LIMIT_MSG), + ("b11 unused value", "ss", kce("pk = :u", {":u": N5, ":x": SX}), V, UNUSED_X), + ("b13 missing table", "missing", kce("pk = :u", {":u": N5}), RNF, "Requested resource not found"), + ("b14 GSI gpk = N 5", "ss", kce("gpk = :g", {":g": N5}, IndexName="gsi1"), V, TYPE_MISMATCH), + ("b15 LSI lsk = N 5", "ss", + kce("pk = :u AND lsk = :l", {":u": SX, ":l": N5}, IndexName="lsi1"), V, TYPE_MISMATCH), + ("b16 GSI with base pk = N 5", "ss", kce("pk = :u", {":u": N5}, IndexName="gsi1"), V, MISSED_GPK), + ("b17 GSI with base pk = S", "ss", kce("pk = :u", {":u": SX}, IndexName="gsi1"), V, MISSED_GPK), + ("b18 N sk = S 5", "ns", kce("pk = :u AND sk = :s", {":u": SX, ":s": {"S": "5"}}), V, TYPE_MISMATCH), + ("b19 N sk BETWEEN S", "ns", + kce("pk = :u AND sk BETWEEN :a AND :b", {":u": SX, ":a": {"S": "1"}, ":b": {"S": "2"}}), + V, TYPE_MISMATCH), + ("b20 N sk begins_with S", "ns", + kce("pk = :u AND begins_with(sk, :b)", {":u": SX, ":b": {"S": "1"}}), V, TYPE_MISMATCH), + ("b22 pk = N 5 AND N sk = S 5", "ns", + kce("pk = :u AND sk = :s", {":u": N5, ":s": {"S": "5"}}), V, TYPE_MISMATCH), + ("b23 N sk = N bob", "ns", kce("pk = :u AND sk = :s", {":u": SX, ":s": NB}), V, NUMERIC + "bob"), + ("b24 B pk = S", "bb", kce("bpk = :u", {":u": SX}), V, TYPE_MISMATCH), + ("b25 B pk = N 5", "bb", kce("bpk = :u", {":u": N5}), V, TYPE_MISMATCH), + ("b26 B pk = N bob", "bb", kce("bpk = :u", {":u": NB}), V, NUMERIC + "bob"), + ("b27 B sk begins_with S", "bb", + kce("bpk = :u AND begins_with(bsk, :b)", {":u": B_BOB, ":b": SX}), V, TYPE_MISMATCH), + ("b28 ConsistentRead on GSI", "ss", + kce("gpk = :g", {":g": N5}, IndexName="gsi1", ConsistentRead=True), V, GSI_CONSISTENT), + ("b29 filter begins_with N", "ss", + kce("pk = :u", {":u": N5, ":n": {"N": "1"}}, FilterExpression="begins_with(a, :n)"), + V, FILTER_BEGINS_WITH_N), + ("b30 ProjectionExpression overlap", "ss", + kce("pk = :u", {":u": N5}, ProjectionExpression="a, a.b"), V, PROJECTION_OVERLAP), + ("b31 ExclusiveStartKey bad", "ss", + kce("pk = :u", {":u": N5}, ExclusiveStartKey={"zz": SX}), V, TYPE_MISMATCH), + ("b32 pk = N 5 AND sk = N bob", "ss", + kce("pk = :u AND sk = :s", {":u": N5, ":s": NB}), V, NUMERIC + "bob"), + ("b33 pk = B AND sk = N 5", "ss", + kce("pk = :u AND sk = :s", {":u": B_BOB, ":s": N5}), V, TYPE_MISMATCH), + ("b35 pk = NULL", "ss", kce("pk = :u", {":u": {"NULL": True}}), V, TYPE_MISMATCH), + ("b36 pk = SS", "ss", kce("pk = :u", {":u": {"SS": ["a"]}}), V, TYPE_MISMATCH), + ("b37 pk = L", "ss", kce("pk = :u", {":u": {"L": [SX]}}), V, TYPE_MISMATCH), + ("b38 pk = M", "ss", kce("pk = :u", {":u": {"M": {"a": SX}}}), V, TYPE_MISMATCH), + ("b41 reserved word in filter", "ss", + kce("pk = :u", {":u": N5, ":n": SX}, FilterExpression="name = :n"), V, FILTER_RESERVED), + ("b42 Select ALL_ATTRIBUTES with ProjectionExpression", "ss", + kce("pk = :u", {":u": N5}, Select="ALL_ATTRIBUTES", ProjectionExpression="a"), + V, SELECT_PROJECTION), + ("b43 non-key second clause", "ss", + kce("pk = :u AND other = :s", {":u": N5, ":s": SX}), V, RESERVED_KCE), + ("b45 pk = N 5 through #p", "ss", + kce("#p = :u", {":u": N5}, ExpressionAttributeNames={"#p": "pk"}), V, TYPE_MISMATCH), + ("b46 reversed operands", "ss", + kce("pk = :u AND :s <= sk", {":u": SX, ":s": N5}), V, TYPE_MISMATCH), + ("b47 GSI gsk = N 5", "ns", + kce("gpk = :g AND gsk = :s", {":g": SX, ":s": N5}, IndexName="gsi1"), V, TYPE_MISMATCH), + # Nested N values, values no expression reads, and the Limit and mix checks + # against the table and index lookups (measured 2026-10-24 21:06 UTC). + ("c1 L with N bob in filter", "ss", + kce("pk = :p", {":p": SX, ":l": L_NB}, FilterExpression="a = :l"), V, NUMERIC + "bob"), + ("c2 L with N bob unused", "ss", kce("pk = :p", {":p": SX, ":l": L_NB}), V, NUMERIC + "bob"), + ("c3 M with N bob in filter", "ss", + kce("pk = :p", {":p": SX, ":m": M_NB}, FilterExpression="a = :m"), V, NUMERIC + "bob"), + ("c4 M with N bob unused", "ss", kce("pk = :p", {":p": SX, ":m": M_NB}), V, NUMERIC + "bob"), + ("c5 NS with bob in filter", "ss", + kce("pk = :p", {":p": SX, ":n": NS_NB}, FilterExpression="a = :n"), V, NUMERIC + "bob"), + ("c6 NS with bob unused", "ss", kce("pk = :p", {":p": SX, ":n": NS_NB}), V, NUMERIC + "bob"), + ("c7 L with N empty in filter", "ss", + kce("pk = :p", {":p": SX, ":l": {"L": [{"N": ""}]}}, FilterExpression="a = :l"), V, NUMERIC), + ("c8 M in L in M with N bob", "ss", + kce("pk = :p", {":p": SX, ":m": {"M": {"a": {"L": [M_NB]}}}}), V, NUMERIC + "bob"), + ("c9 N bob unused on a missing table", "missing", + kce("pk = :p", {":p": SX, ":n": NB}), V, NUMERIC + "bob"), + ("c12 N bob with no key condition", "ss", + {"ExpressionAttributeValues": {":n": NB}}, V, NUMERIC + "bob"), + ("c13 N bob beside legacy KeyConditions", "ss", + {"KeyConditions": LEGACY_PK, "ExpressionAttributeValues": {":n": NB}}, V, NUMERIC + "bob"), + ("d1 Limit 0 on a missing table", "missing", kce("pk = :p", {":p": SX}, Limit=0), V, LIMIT_MSG), + ("d2 Limit 0 with unknown IndexName", "ss", + kce("pk = :p", {":p": SX}, IndexName="nope", Limit=0), V, LIMIT_MSG), + ("d3 Limit 0 with ConsistentRead on GSI", "ss", + kce("gpk = :p", {":p": SX}, IndexName="gsi1", ConsistentRead=True, Limit=0), V, LIMIT_MSG), + ("d4 KeyConditions and KeyConditionExpression on a missing table", "missing", + kce("pk = :p", {":p": SX}, KeyConditions=LEGACY_PK), V, MIX_MSG), + ("d5 Limit 0 with KeyConditions and KeyConditionExpression", "ss", + kce("pk = :p", {":p": SX}, Limit=0, KeyConditions=LEGACY_PK), V, LIMIT_MSG), + ("d6 Limit 0 with N bob and an undefined placeholder", "ss", + kce("pk = :p AND sk = :zz", {":p": SX, ":n": NB}, Limit=0), V, LIMIT_MSG), + # The other value types the earlier record covers. + ("pk = BOOL", "ss", kce("pk = :u", {":u": {"BOOL": True}}), V, TYPE_MISMATCH), + ("pk = N bob AND pk > S alice", "ss", + kce("pk = :u AND pk > :v", {":u": NB, ":v": {"S": "alice"}}), V, NUMERIC + "bob"), +] + + +class TestKeyConditionValueTypes: + """Each row: a Query and the service's answer.""" + + @pytest.fixture + def tables(self, ss_table, ns_table, bb_table): + return {"ss": ss_table, "ns": ns_table, "bb": bb_table, "missing": MISSING} + + @pytest.mark.parametrize("row,table,kwargs,code,expected", ROWS, ids=[r[0] for r in ROWS]) + def test_rows(self, dynamodb_client_no_validation, tables, row, table, kwargs, code, expected): + with pytest.raises(ClientError) as exc_info: + dynamodb_client_no_validation.query(TableName=tables[table], **kwargs) + assert _err(exc_info) == (code, expected) + + def test_matching_types_read_the_named_partition(self, dynamodb_client, ss_table): + resp = dynamodb_client.query( + TableName=ss_table, KeyConditionExpression="pk = :u", + ExpressionAttributeValues={":u": {"S": "bob"}}) + assert [it["pk"]["S"] for it in resp["Items"]] == ["bob"] + + def test_control_matching_types_on_every_table(self, dynamodb_client, ss_table, ns_table, bb_table): + assert dynamodb_client.query( + TableName=ss_table, KeyConditionExpression="pk = :u AND sk = :s", + ExpressionAttributeValues={":u": {"S": "nope"}, ":s": SX})["Count"] == 0 + assert dynamodb_client.query( + TableName=ns_table, KeyConditionExpression="pk = :u AND sk > :s", + ExpressionAttributeValues={":u": {"S": "bob"}, ":s": {"N": "1"}})["Count"] == 1 + assert dynamodb_client.query( + TableName=bb_table, KeyConditionExpression="bpk = :u", + ExpressionAttributeValues={":u": B_BOB})["Count"] == 1 + + +class TestLeadingKeysWithAnInvalidKeyValue: + """A principal under each LeadingKeys fence form gets ValidationException and + no items for a Query whose ExpressionAttributeValues carries an N value that + is not a number, whether the key condition, the filter or nothing reads it. + + extenddb only: the fences need users created through the management API.""" + + @pytest.fixture(autouse=True) + def setup(self): + endpoint = os.environ.get("EXTENDDB_TEST_ENDPOINT", "").strip() + admin_user = os.environ.get("EXTENDDB_ADMIN_USER", "").strip() + admin_pass = os.environ.get("EXTENDDB_ADMIN_PASSWORD", "").strip() + if not endpoint: + pytest.skip("LeadingKeys fences need the extenddb management API") + if not admin_user or not admin_pass: + pytest.fail("MISCONFIGURED: EXTENDDB_ADMIN_USER and EXTENDDB_ADMIN_PASSWORD are required") + from management_helpers import ManagementClient + + self.endpoint = endpoint + self.region = os.environ.get("AWS_DEFAULT_REGION", "us-east-1") + self.mgmt = ManagementClient(endpoint, admin_user, admin_pass) + self.account_id = f"{uuid.uuid4().int % 10**12:012d}" + resp = self.mgmt.create_account(self.account_id, f"kv-test-{self.account_id}") + assert resp.status_code == 201, resp.text + self._users: list[str] = [] + yield + for u in self._users: + try: + self.mgmt.delete_user(self.account_id, u) + except Exception: + pass + self.mgmt.delete_account(self.account_id) + + def _client(self, policy: dict | None) -> Any: + name = f"kv-{uuid.uuid4().hex[:8]}" + resp = self.mgmt.create_user(self.account_id, name, "TestPass123!") + assert resp.status_code == 201, resp.text + self._users.append(name) + resp = self.mgmt.create_access_key(self.account_id, name) + assert resp.status_code == 201, resp.text + creds = resp.json() + if policy is not None: + resp = self.mgmt.put_user_policy(self.account_id, name, "p", policy) + assert resp.status_code == 204, resp.text + return boto3.client( + "dynamodb", endpoint_url=self.endpoint, region_name=self.region, + aws_access_key_id=creds["access_key_id"], + aws_secret_access_key=creds["secret_access_key"], + verify=False, + config=BotoConfig(retries={"max_attempts": 0}, parameter_validation=False)) + + def _fence(self, condition: dict, table: str) -> dict: + arn = f"arn:aws:dynamodb:{self.region}:{self.account_id}:table/{table}" + return { + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Action": "dynamodb:*", + "Resource": [arn, arn + "/index/*"], + "Condition": condition, + }], + } + + def test_every_fence_form_gets_validation_exception_and_no_items(self): + owner = self._client({ + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": "dynamodb:*", "Resource": "*"}], + }) + table = f"kv-{uuid.uuid4().hex[:8]}" + owner.create_table( + TableName=table, + AttributeDefinitions=[{"AttributeName": "pk", **S}], + KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], + BillingMode="PAY_PER_REQUEST") + wait_for_active(owner, table) + for pk in ("alice", "bob"): + owner.put_item(TableName=table, Item={"pk": {"S": pk}}) + + fences = { + "ForAllValues": {"ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, + "ForAnyValue": {"ForAnyValue:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, + "Null false": {"Null": {"dynamodb:LeadingKeys": "false"}, + "ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, + "StringEquals": {"StringEquals": {"dynamodb:LeadingKeys": "alice"}}, + "StringLike": {"StringLike": {"dynamodb:LeadingKeys": "ali*"}}, + "StringNotEquals": {"StringNotEquals": {"dynamodb:LeadingKeys": "bob"}}, + } + # The reported shape, and an N value that only the filter reads or + # nothing reads beside a key value the fence would bind. + requests = { + "pk = N bob": {"TableName": table, "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": NB}}, + "pk = S alice, N bob in filter": { + "TableName": table, "KeyConditionExpression": "pk = :u", + "FilterExpression": "a = :n", + "ExpressionAttributeValues": {":u": {"S": "alice"}, ":n": NB}}, + "pk = S bob, N bob unused": { + "TableName": table, "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"S": "bob"}, ":n": NB}}, + } + failures = [] + clients = {label: self._client(self._fence(condition, table)) + for label, condition in fences.items()} + clients["owner"] = owner + for shape, request in requests.items(): + for label, client in clients.items(): + try: + resp = client.query(**request) + failures.append(f"{shape} / {label}: {resp['Count']} items returned") + except ClientError as exc: + err = exc.response["Error"] + if (err["Code"], err["Message"]) != ("ValidationException", NUMERIC + "bob"): + failures.append(f"{shape} / {label}: {err['Code']} {err['Message']}") + owner.delete_table(TableName=table) + assert not failures, "\n".join(failures) From f0017ce90ef654ac7957ca1e8f62a8b56b61491d Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 25 Oct 2026 00:35:19 +0000 Subject: [PATCH 02/12] fix(query): run the request-time checks on the raw body, enums in the service's order The server ran its pre-IAM checks only when the body deserialized as QueryInput, and the engine checked the ReturnConsumedCapacity and Select enums before them, so a Query carrying an invalid Select and an N value that is not a number answered the Select message, or AccessDeniedException for a principal whose LeadingKeys fence needs a value. The checks now read the raw body in the service's order, measured 2026-10-24: Limit, the expression and non-expression parameter mix, ReturnConsumedCapacity, every number in ExpressionAttributeValues, then Select. The engine and the server run the same function. --- crates/engine/src/query.rs | 41 ++------- crates/engine/src/query_key_values.rs | 95 +++++++++++++++------ crates/engine/src/query_key_values/tests.rs | 66 +++++++++++--- crates/server/src/request_helpers.rs | 22 ++--- tests/test_query_key_value_types.py | 35 ++++++-- 5 files changed, 173 insertions(+), 86 deletions(-) diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index 97e07ead2..a4e633073 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -43,42 +43,15 @@ pub async fn handle_query( body: Value, ctx: &OperationContext, ) -> Result { - // Pre-scanned before typed deserialization, sequentially: Query stops at - // the FIRST invalid enum, checking ReturnConsumedCapacity ahead of Select. - // Measured 2026-08-24 (us-east-1): both invalid answers "1 validation - // error detected" naming returnConsumedCapacity alone. (Scan is the - // opposite on both counts: it aggregates, select-first; see handle_scan.) - crate::validate_enum_fields( - &body, - &[crate::EnumField { - json_name: "ReturnConsumedCapacity", - valid: &["INDEXES", "TOTAL", "NONE"], - clause: crate::EnumClause::Named("returnConsumedCapacity"), - }], - )?; - crate::validate_enum_fields( - &body, - &[crate::EnumField { - json_name: "Select", - valid: &[ - "SPECIFIC_ATTRIBUTES", - "COUNT", - "ALL_ATTRIBUTES", - "ALL_PROJECTED_ATTRIBUTES", - ], - clause: crate::EnumClause::Named("select"), - }], - )?; + // Request-time checks on the raw body, in the service's order: Limit, the + // expression and non-expression parameter mix, the ReturnConsumedCapacity + // enum, every number in ExpressionAttributeValues, the Select enum. They + // run before typed deserialization so a field that does not deserialize + // does not stop the ones before it. The server runs the same function + // ahead of the IAM decision; see `query_key_values`. + crate::query_key_values::validate_query_request_body(&body)?; let input: QueryInput = serde_json::from_value(body).map_err(crate::deserialize_error)?; - // Request-time checks in the service's order: Limit, the expression and - // non-expression parameter mix, then every number in - // ExpressionAttributeValues (measured 2026-10-24: a Limit of 0 and a - // parameter mix are reported ahead of an N value that is not a number, and - // that value is reported ahead of a missing table or index). The server - // runs the same function ahead of the IAM decision; see `query_key_values`. - crate::query_key_values::validate_query_request_values(&input)?; - // P118: Fetch key_info first so we can use table_id for index lookup. let key_info = ctx .table_key_info(&input.table_name) diff --git a/crates/engine/src/query_key_values.rs b/crates/engine/src/query_key_values.rs index e49968648..7bfb356ba 100644 --- a/crates/engine/src/query_key_values.rs +++ b/crates/engine/src/query_key_values.rs @@ -12,7 +12,7 @@ //! checked, whether the key condition, the filter or nothing reads it. The //! check runs at request time, after the `Limit` check and the expression //! and non-expression parameter mix check, and before the table is looked -//! up; see [`validate_query_request_values`]; +//! up; see [`validate_query_request_body`]; //! 2. a key condition value whose type is not the key attribute's //! `AttributeDefinitions` type answers `One or more parameter values were //! invalid: Condition parameter type does not match schema type`, on the @@ -33,7 +33,8 @@ use extenddb_core::error::DynamoDbError; use extenddb_core::expression::{ Expr, ExpressionMaps, KeyCondition, PathElement, SortKeyCondition, }; -use extenddb_core::types::{AttributeDefinition, AttributeValue, QueryInput, ScalarAttributeType}; +use extenddb_core::types::{AttributeDefinition, AttributeValue, ScalarAttributeType}; +use serde_json::Value; /// The service's text for a key condition value of the wrong type. pub const KEY_CONDITION_TYPE_MISMATCH: &str = "One or more parameter values were invalid: Condition parameter type does not match schema type"; @@ -44,10 +45,18 @@ pub const LIMIT_BELOW_ONE: &str = "1 validation error detected: Value at 'Limit' /// The bare numeric message the number validator uses for an empty `N`. const EMPTY_NUMBER: &str = "The parameter cannot be converted to a numeric value"; -/// The `Query` checks that run on the request alone, before the table is -/// looked up, in the service's order: `Limit`, the expression and -/// non-expression parameter mix, then every number in -/// `ExpressionAttributeValues`. +/// The `Query` checks that run on the request body alone, before the body is +/// deserialized and before the table is looked up, in the service's order: +/// `Limit`, the expression and non-expression parameter mix, the +/// `ReturnConsumedCapacity` enum, every number in `ExpressionAttributeValues`, +/// then the `Select` enum (measured 2026-10-24: a `Limit` of 0 and a parameter +/// mix are reported ahead of an invalid enum value of either field, an invalid +/// `ReturnConsumedCapacity` ahead of an `N` value that is not a number, and +/// that value ahead of an invalid `Select`). +/// +/// The checks read the raw body, so a field that does not deserialize (an +/// invalid `Select`, for one) does not stop the ones before it. A field of the +/// wrong JSON shape is left to the deserializer, which runs next. /// /// The server runs the same function ahead of the IAM decision, so a principal /// whose `dynamodb:LeadingKeys` fence cannot bind a number that does not parse @@ -57,8 +66,8 @@ const EMPTY_NUMBER: &str = "The parameter cannot be converted to a numeric value /// # Errors /// /// Returns `DynamoDbError::ValidationException` with the service's text. -pub fn validate_query_request_values(input: &QueryInput) -> Result<(), DynamoDbError> { - if let Some(limit) = input.limit +pub fn validate_query_request_body(body: &Value) -> Result<(), DynamoDbError> { + if let Some(limit) = body.get("Limit").and_then(Value::as_i64) && limit < 1 { return Err(DynamoDbError::ValidationException( @@ -66,36 +75,74 @@ pub fn validate_query_request_values(input: &QueryInput) -> Result<(), DynamoDbE )); } - let kce_present = input.key_condition_expression.is_some(); - let has_kc = input.key_conditions.as_ref().is_some_and(|m| !m.is_empty()); - let has_qf = input.query_filter.as_ref().is_some_and(|m| !m.is_empty()); - let has_atg = input - .attributes_to_get - .as_ref() - .is_some_and(|a| !a.is_empty()); + let kce_present = present(body, "KeyConditionExpression"); // KeyConditions combines with FilterExpression and ProjectionExpression, so // it joins the mix check only when KeyConditionExpression is also present. extenddb_core::validation::validate_no_expression_param_mixing( &[ - ("AttributesToGet", has_atg), - ("QueryFilter", has_qf), - ("ConditionalOperator", input.conditional_operator.is_some()), - ("KeyConditions", has_kc && kce_present), + ("AttributesToGet", non_empty(body, "AttributesToGet")), + ("QueryFilter", non_empty(body, "QueryFilter")), + ("ConditionalOperator", present(body, "ConditionalOperator")), + ( + "KeyConditions", + non_empty(body, "KeyConditions") && kce_present, + ), ], &[ ( "ProjectionExpression", - input.projection_expression.is_some(), + present(body, "ProjectionExpression"), ), - ("FilterExpression", input.filter_expression.is_some()), + ("FilterExpression", present(body, "FilterExpression")), ("KeyConditionExpression", kce_present), ], )?; - if let Some(values) = &input.expression_attribute_values { - validate_expression_attribute_value_numbers(values)?; + // Query stops at the first invalid enum, ReturnConsumedCapacity ahead of + // Select (measured 2026-08-24); the numbers sit between the two. + crate::validate_enum_fields( + body, + &[crate::EnumField { + json_name: "ReturnConsumedCapacity", + valid: &["INDEXES", "TOTAL", "NONE"], + clause: crate::EnumClause::Named("returnConsumedCapacity"), + }], + )?; + + if let Some(raw) = body.get("ExpressionAttributeValues") + && let Ok(values) = serde_json::from_value::>(raw.clone()) + { + validate_expression_attribute_value_numbers(&values)?; + } + + crate::validate_enum_fields( + body, + &[crate::EnumField { + json_name: "Select", + valid: &[ + "SPECIFIC_ATTRIBUTES", + "COUNT", + "ALL_ATTRIBUTES", + "ALL_PROJECTED_ATTRIBUTES", + ], + clause: crate::EnumClause::Named("select"), + }], + ) +} + +/// Whether `field` is in the body with a value other than `null`, as a typed +/// `Option` field would deserialize to `Some`. +fn present(body: &Value, field: &str) -> bool { + body.get(field).is_some_and(|v| !v.is_null()) +} + +/// Whether `field` is a non-empty JSON object or array in the body. +fn non_empty(body: &Value, field: &str) -> bool { + match body.get(field) { + Some(Value::Object(map)) => !map.is_empty(), + Some(Value::Array(items)) => !items.is_empty(), + _ => false, } - Ok(()) } /// Refuse the first `N` value in `ExpressionAttributeValues` (including `NS` diff --git a/crates/engine/src/query_key_values/tests.rs b/crates/engine/src/query_key_values/tests.rs index 6130dd875..d98e66f05 100644 --- a/crates/engine/src/query_key_values/tests.rs +++ b/crates/engine/src/query_key_values/tests.rs @@ -6,11 +6,11 @@ use extenddb_core::error::DynamoDbError; use extenddb_core::expression::{ ExpressionMaps, KeyCondition, parse_key_condition, tokenize_with_limit, }; -use extenddb_core::types::{AttributeDefinition, AttributeValue, QueryInput, ScalarAttributeType}; +use extenddb_core::types::{AttributeDefinition, AttributeValue, ScalarAttributeType}; use super::{ KEY_CONDITION_TYPE_MISMATCH, LIMIT_BELOW_ONE, validate_expression_attribute_value_numbers, - validate_key_condition_types, validate_query_request_values, + validate_key_condition_types, validate_query_request_body, }; fn defs(pairs: &[(&str, ScalarAttributeType)]) -> Vec { @@ -226,10 +226,6 @@ fn undefined_placeholder_and_undefined_attribute_are_left_to_other_checks() { assert!(validate_key_condition_types(&kc, &m, &defs(SS)).is_ok()); } -fn query(body: serde_json::Value) -> QueryInput { - serde_json::from_value(body).expect("QueryInput") -} - #[test] fn number_values_that_are_not_numbers_are_refused_with_the_bare_message() { let cases = [ @@ -339,7 +335,7 @@ fn request_time_checks_run_limit_then_mix_then_numbers() { let mut with_limit = bad_number.clone(); with_limit["Limit"] = serde_json::json!(0); assert_eq!( - message(validate_query_request_values(&query(with_limit))).as_deref(), + message(validate_query_request_body(&with_limit)).as_deref(), Some(LIMIT_BELOW_ONE) ); @@ -348,7 +344,7 @@ fn request_time_checks_run_limit_then_mix_then_numbers() { "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]} }); assert_eq!( - message(validate_query_request_values(&query(with_mix))).as_deref(), + message(validate_query_request_body(&with_mix)).as_deref(), Some( "Can not use both expression and non-expression parameters in the same request: \ Non-expression parameters: {KeyConditions} Expression parameters: {KeyConditionExpression}" @@ -356,7 +352,7 @@ fn request_time_checks_run_limit_then_mix_then_numbers() { ); assert_eq!( - message(validate_query_request_values(&query(bad_number))).as_deref(), + message(validate_query_request_body(&bad_number)).as_deref(), Some("The parameter cannot be converted to a numeric value: bob") ); @@ -375,7 +371,7 @@ fn request_time_checks_run_limit_then_mix_then_numbers() { }), ] { assert_eq!( - message(validate_query_request_values(&query(body))).as_deref(), + message(validate_query_request_body(&body)).as_deref(), Some("The parameter cannot be converted to a numeric value: bob") ); } @@ -391,5 +387,53 @@ fn request_time_checks_run_limit_then_mix_then_numbers() { "ExpressionAttributeValues": {":n": {"N": "5"}}, "Limit": 1, }); - assert!(validate_query_request_values(&query(legacy)).is_ok()); + assert!(validate_query_request_body(&legacy).is_ok()); +} + +#[test] +fn enum_checks_sit_on_either_side_of_the_numbers() { + // Measured 2026-10-24: an invalid ReturnConsumedCapacity is reported ahead + // of an N value that is not a number, and that value ahead of an invalid + // Select; a Limit of 0 and a parameter mix ahead of either enum. + let bad_number = serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"N": "bob"}}, + }); + let mut rcc = bad_number.clone(); + rcc["ReturnConsumedCapacity"] = serde_json::json!("bad"); + assert_eq!( + message(validate_query_request_body(&rcc)).as_deref(), + Some( + "1 validation error detected: Value 'bad' at 'returnConsumedCapacity' failed to \ + satisfy constraint: Member must satisfy enum value set: [INDEXES, TOTAL, NONE]" + ) + ); + let mut select = bad_number.clone(); + select["Select"] = serde_json::json!("bad"); + assert_eq!( + message(validate_query_request_body(&select)).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob") + ); + for field in ["ReturnConsumedCapacity", "Select"] { + let mut limit = bad_number.clone(); + limit[field] = serde_json::json!("bad"); + limit["Limit"] = serde_json::json!(0); + assert_eq!( + message(validate_query_request_body(&limit)).as_deref(), + Some(LIMIT_BELOW_ONE), + "{field}" + ); + } + // With the numbers valid, an invalid Select is still reported. + let mut only_select = serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"S": "bob"}}, + }); + only_select["Select"] = serde_json::json!("bad"); + assert!( + message(validate_query_request_body(&only_select)) + .is_some_and(|m| m.contains("at 'select' failed to satisfy constraint")) + ); } diff --git a/crates/server/src/request_helpers.rs b/crates/server/src/request_helpers.rs index 9f34b2632..82c694729 100755 --- a/crates/server/src/request_helpers.rs +++ b/crates/server/src/request_helpers.rs @@ -185,17 +185,17 @@ pub(crate) async fn authorize_request( } // A Query's request-time checks (Limit, the expression and non-expression - // parameter mix, every number in ExpressionAttributeValues) run before the - // IAM decision, for every principal. The engine makes the same checks at - // request time; they run here as well because authorization binds no - // dynamodb:LeadingKeys value to a number it cannot parse, and a fence that - // needs the key present (ForAnyValue, a Null guard, the plain operators) - // would otherwise answer AccessDeniedException where ValidationException is - // the request's error. A body the engine cannot deserialize is left to it. - if operation == "Query" - && let Ok(query) = serde_json::from_value::(input.clone()) - { - extenddb_engine::query_key_values::validate_query_request_values(&query)?; + // parameter mix, the ReturnConsumedCapacity and Select enums, every number + // in ExpressionAttributeValues) run before the IAM decision, for every + // principal. The engine makes the same checks at request time; they run + // here as well because authorization binds no dynamodb:LeadingKeys value + // to a number it cannot parse, and a fence that needs the key present + // (ForAnyValue, a Null guard, the plain operators) would otherwise answer + // AccessDeniedException where ValidationException is the request's error. + // The checks read the raw body, so a field that does not deserialize does + // not skip them. + if operation == "Query" { + extenddb_engine::query_key_values::validate_query_request_body(input)?; } // Every other ARN-bearing field (StreamArn, BackupArn, the export TableArn, diff --git a/tests/test_query_key_value_types.py b/tests/test_query_key_value_types.py index 02ee3b317..f01033488 100644 --- a/tests/test_query_key_value_types.py +++ b/tests/test_query_key_value_types.py @@ -8,12 +8,13 @@ value: ``) or when a key condition value's type is not the key attribute's ``AttributeDefinitions`` type (``One or more parameter values were invalid: Condition parameter type does not match schema type``). Every expected -answer below was measured against the service on 2026-10-24 (15:10 UTC and -21:00 UTC) on tables with S/S, S/N and B/B keys, a GSI and an LSI. The rows -also pin where the two checks fall against the other Query checks: the numeric -check after Limit and the expression parameter mix check and before the table -lookup; the type check after the key schema, filter, projection, unused value -and Select checks and before the ExclusiveStartKey check. +answer below was measured against the service on 2026-10-24 (15:10, 21:00, +21:06 and 23:42 UTC) on tables with S/S, S/N and B/B keys, a GSI and an LSI. +The rows also pin where the two checks fall against the other Query checks: +the numeric check after Limit, the expression parameter mix check and the +ReturnConsumedCapacity enum, and before the Select enum and the table lookup; +the type check after the key schema, filter, projection, unused value and +Select checks and before the ExclusiveStartKey check. Dual-target: runs against the service and against extenddb with the same assertions. The LeadingKeys class needs the extenddb management API and is @@ -180,6 +181,10 @@ def bb_table(dynamodb_client): "1 validation error detected: Cannot specify the ProjectionExpression when choosing to " "get ALL_ATTRIBUTES" ) +RCC_MSG = ( + "1 validation error detected: Value 'bad' at 'returnConsumedCapacity' failed to satisfy " + "constraint: Member must satisfy enum value set: [INDEXES, TOTAL, NONE]" +) V = "ValidationException" RNF = "ResourceNotFoundException" @@ -334,6 +339,21 @@ def kce(expr, values, **extra): kce("pk = :p", {":p": SX}, Limit=0, KeyConditions=LEGACY_PK), V, LIMIT_MSG), ("d6 Limit 0 with N bob and an undefined placeholder", "ss", kce("pk = :p AND sk = :zz", {":p": SX, ":n": NB}, Limit=0), V, LIMIT_MSG), + # The two enum checks against the numeric check, Limit and the mix. + ("f1 invalid Select with N bob", "ss", kce("pk = :u", {":u": NB}, Select="bad"), V, NUMERIC + "bob"), + ("f2 invalid ReturnConsumedCapacity with N bob", "ss", + kce("pk = :u", {":u": NB}, ReturnConsumedCapacity="bad"), V, RCC_MSG), + ("f3 invalid Select with Limit 0", "ss", + kce("pk = :u", {":u": {"S": "alice"}}, Select="bad", Limit=0), V, LIMIT_MSG), + ("f4 invalid ReturnConsumedCapacity with Limit 0", "ss", + kce("pk = :u", {":u": {"S": "alice"}}, ReturnConsumedCapacity="bad", Limit=0), V, LIMIT_MSG), + ("f5 invalid Select with KeyConditions and KeyConditionExpression", "ss", + kce("pk = :u", {":u": {"S": "alice"}}, Select="bad", + KeyConditions={"pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "alice"}]}}), + V, MIX_MSG), + ("f6 Limit -1 with N bob", "ss", kce("pk = :u", {":u": NB}, Limit=-1), V, LIMIT_MSG), + ("f7 Limit 0 with a type mismatch and N bob", "ss", + kce("pk = :u AND sk = :s", {":u": N5, ":s": NB}, Limit=0), V, LIMIT_MSG), # The other value types the earlier record covers. ("pk = BOOL", "ss", kce("pk = :u", {":u": {"BOOL": True}}), V, TYPE_MISMATCH), ("pk = N bob AND pk > S alice", "ss", @@ -471,6 +491,9 @@ def test_every_fence_form_gets_validation_exception_and_no_items(self): "pk = S bob, N bob unused": { "TableName": table, "KeyConditionExpression": "pk = :u", "ExpressionAttributeValues": {":u": {"S": "bob"}, ":n": NB}}, + "pk = N bob, invalid Select": { + "TableName": table, "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": NB}, "Select": "bad"}, } failures = [] clients = {label: self._client(self._fence(condition, table)) From e4f660d620f8ccb7eefc7a1cbadd8d738f9410af Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 25 Oct 2026 00:35:43 +0000 Subject: [PATCH 03/12] docs(query): name the request-time function in the comments that point at it --- crates/engine/src/query.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index a4e633073..9b3e6d7e8 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -118,7 +118,7 @@ pub async fn handle_query( )?; } - // Limit >= 1 is checked at request time in validate_query_request_values. + // Limit >= 1 is checked at request time in validate_query_request_body. // For index queries, build a key_info that reflects the index's key schema // so the storage layer uses the correct SK column for the index table. @@ -141,7 +141,7 @@ pub async fn handle_query( }; // The legacy versus expression mutual exclusivity check runs at request - // time in validate_query_request_values. + // time in validate_query_request_body. let has_cond_op = input.conditional_operator.is_some(); // An explicitly empty KeyConditionExpression is rejected up front, before From 4a4e92f4d4ac6a31921645ef896e4cff8fdc14a9 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sat, 24 Oct 2026 02:22:31 +0000 Subject: [PATCH 04/12] fix(cassandra): pass the bare index name in IndexNotFound so the Query sentence matches the other backends --- crates/storage-cassandra/src/data/index.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/storage-cassandra/src/data/index.rs b/crates/storage-cassandra/src/data/index.rs index 4ebd72dac..3aa353ac6 100644 --- a/crates/storage-cassandra/src/data/index.rs +++ b/crates/storage-cassandra/src/data/index.rs @@ -147,7 +147,7 @@ pub async fn fetch_index_by_name( let row = rows .into_iter() .next() - .ok_or_else(|| StorageError::IndexNotFound(format!("Index {index_name} not found")))?; + .ok_or_else(|| StorageError::IndexNotFound(index_name.to_owned()))?; let index_id: String = get_column(&row, "index_id", "fetch_index_by_name")?; let index_type: String = get_column(&row, "index_type", "fetch_index_by_name")?; From 03caeed76d08f716fedca6a88319253a8bc36560 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 25 Oct 2026 11:44:43 +0000 Subject: [PATCH 05/12] fix(query): accept Unicode decimal digits in the number check and report a shape fault ahead of the request-time checks The request-time number check refused an N value written with a Unicode decimal digit (fullwidth, Arabic-Indic, Devanagari) that the service accepts as the digit it stands for, so a Query that returns rows on the service answered ValidationException here. The check now folds every general category Nd digit to ASCII before the number rules run and echoes the value as the request wrote it when the rules refuse it; the Nd zero code points of Unicode 15.0 live in a module of their own. The fold applies to the check only: the handler and the server's key binding read the value as written. The write paths are unchanged. The checks run on the raw body before deserialization, so a field of the wrong JSON type elsewhere in the body (a string ExpressionAttributeValues beside a Limit of 0, a string ExpressionAttributeNames beside a malformed N, a numeric Select) answered the check's ValidationException where the service and the previous release answered SerializationException. When a check fails, the body is now deserialized as the typed input and a SerializationException from that is returned instead; a ValidationException the deserializer raises does not preempt the check, and a body that passes every check is not deserialized twice. The server's call before dispatch goes through the same function. Measured against the service on 2026-10-25 (11:31 UTC) on tables with S/N and S/S keys, each request twice. Unit tests cover the digit table, the folded check and the shape-fault order; the wire file gains the measured rows for both. --- crates/engine/src/decimal_digits.rs | 122 ++++++++++++++++ crates/engine/src/lib.rs | 1 + crates/engine/src/query_key_values.rs | 88 ++++++++++-- crates/engine/src/query_key_values/tests.rs | 133 +++++++++++++++++ tests/test_query_key_value_types.py | 152 +++++++++++++++++++- 5 files changed, 481 insertions(+), 15 deletions(-) create mode 100644 crates/engine/src/decimal_digits.rs diff --git a/crates/engine/src/decimal_digits.rs b/crates/engine/src/decimal_digits.rs new file mode 100644 index 000000000..7717634dd --- /dev/null +++ b/crates/engine/src/decimal_digits.rs @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! Unicode decimal digits (general category `Nd`) folded to ASCII for the +//! request-time number check. +//! +//! Measured 2026-10-25 (us-east-1, each request twice): the service reads a +//! fullwidth, Arabic-Indic or Devanagari digit in an `N` value as the digit it +//! stands for (`{"N": "\u{ff11}"}` in a `FilterExpression` answers OK; thirty +//! nine Devanagari ones answer `Attempting to store more than 38 significant +//! digits in a Number`), and refuses a superscript (`\u{b2}`), a vulgar +//! fraction (`\u{bd}`), a fullwidth minus (`\u{ff0d}`) and a fullwidth full +//! stop (`\u{ff0e}`) with `The parameter cannot be converted to a numeric +//! value: `. The accepted characters are exactly those of category `Nd`; +//! the refused ones are `No` or `Pd` and `Po`. +//! +//! Every `Nd` run in Unicode 15.0 is ten consecutive code points starting at +//! its zero, so the table below holds the 68 zeros (generated with Python's +//! `unicodedata`, Unicode 15.0.0) and a digit's value is its offset from the +//! run's zero. + +use std::borrow::Cow; + +/// The zero of every `Nd` run in Unicode 15.0, ascending. +const ND_ZEROS: [u32; 68] = [ + 0x0030, 0x0660, 0x06F0, 0x07C0, 0x0966, 0x09E6, 0x0A66, 0x0AE6, 0x0B66, 0x0BE6, 0x0C66, 0x0CE6, + 0x0D66, 0x0DE6, 0x0E50, 0x0ED0, 0x0F20, 0x1040, 0x1090, 0x17E0, 0x1810, 0x1946, 0x19D0, 0x1A80, + 0x1A90, 0x1B50, 0x1BB0, 0x1C40, 0x1C50, 0xA620, 0xA8D0, 0xA900, 0xA9D0, 0xA9F0, 0xAA50, 0xABF0, + 0xFF10, 0x104A0, 0x10D30, 0x11066, 0x110F0, 0x11136, 0x111D0, 0x112F0, 0x11450, 0x114D0, + 0x11650, 0x116C0, 0x11730, 0x118E0, 0x11950, 0x11C50, 0x11D50, 0x11DA0, 0x11F50, 0x16A60, + 0x16AC0, 0x16B50, 0x1D7CE, 0x1D7D8, 0x1D7E2, 0x1D7EC, 0x1D7F6, 0x1E140, 0x1E2F0, 0x1E4F0, + 0x1E950, 0x1FBF0, +]; + +/// The ASCII digit `c` stands for when `c` is a Unicode decimal digit +/// (general category `Nd`), or `None` for every other character. +pub(crate) fn ascii_digit(c: char) -> Option { + let cp = u32::from(c); + // The zero of the run that could hold `cp`: the last zero at or below it. + let run = ND_ZEROS + .partition_point(|&zero| zero <= cp) + .checked_sub(1)?; + let offset = cp - ND_ZEROS[run]; + u8::try_from(offset) + .ok() + .filter(|offset| *offset < 10) + .map(|offset| char::from(b'0' + offset)) +} + +/// `s` with every Unicode decimal digit replaced by the ASCII digit it stands +/// for. Borrows `s` when it has no such digit outside ASCII. +pub(crate) fn fold_decimal_digits(s: &str) -> Cow<'_, str> { + if s.is_ascii() || !s.chars().any(|c| !c.is_ascii() && ascii_digit(c).is_some()) { + return Cow::Borrowed(s); + } + Cow::Owned(s.chars().map(|c| ascii_digit(c).unwrap_or(c)).collect()) +} + +#[cfg(test)] +mod tests { + use super::{ND_ZEROS, ascii_digit, fold_decimal_digits}; + + #[test] + fn measured_digits_fold_to_their_ascii_value() { + // Fullwidth one, Arabic-Indic one, Devanagari one (X545-1 K1 to K3), + // fullwidth seven (H545-2 C11). + assert_eq!(ascii_digit('\u{ff11}'), Some('1')); + assert_eq!(ascii_digit('\u{661}'), Some('1')); + assert_eq!(ascii_digit('\u{967}'), Some('1')); + assert_eq!(ascii_digit('\u{ff17}'), Some('7')); + assert_eq!(ascii_digit('0'), Some('0')); + assert_eq!(ascii_digit('9'), Some('9')); + } + + #[test] + fn measured_controls_are_not_digits() { + // Superscript two, vulgar half, fullwidth minus, fullwidth full stop + // (H545-2 C13 to C16): refused by the service. + for c in ['\u{b2}', '\u{bd}', '\u{ff0d}', '\u{ff0e}'] { + assert_eq!(ascii_digit(c), None, "{c:?}"); + } + // Neighbours of a run and non-digit characters. + assert_eq!(ascii_digit('\u{ff10}'), Some('0')); + assert_eq!(ascii_digit('\u{ff19}'), Some('9')); + assert_eq!(ascii_digit('\u{ff1a}'), None); + assert_eq!(ascii_digit('\u{ff0f}'), None); + assert_eq!(ascii_digit('a'), None); + assert_eq!(ascii_digit('.'), None); + assert_eq!(ascii_digit('e'), None); + assert_eq!(ascii_digit('\u{1FBF9}'), Some('9')); + assert_eq!(ascii_digit('\u{1FBFA}'), None); + assert_eq!(ascii_digit(char::MAX), None); + assert_eq!(ascii_digit('\u{0}'), None); + } + + #[test] + fn zeros_are_ascending_and_distinct_runs() { + for pair in ND_ZEROS.windows(2) { + assert!(pair[0] + 10 <= pair[1], "{pair:?}"); + } + } + + #[test] + fn folding_keeps_every_other_character() { + assert_eq!(fold_decimal_digits("\u{ff11}"), "1"); + assert_eq!(fold_decimal_digits("1\u{ff12}"), "12"); + assert_eq!(fold_decimal_digits("1e\u{ff12}"), "1e2"); + assert_eq!(fold_decimal_digits(&"\u{967}".repeat(39)), "1".repeat(39)); + assert_eq!(fold_decimal_digits("\u{ff0d}1"), "\u{ff0d}1"); + assert_eq!(fold_decimal_digits("1\u{ff0e}5"), "1\u{ff0e}5"); + assert_eq!(fold_decimal_digits("\u{b2}"), "\u{b2}"); + assert_eq!(fold_decimal_digits("-1.5e3"), "-1.5e3"); + assert_eq!(fold_decimal_digits(""), ""); + assert!(matches!( + fold_decimal_digits("bob"), + std::borrow::Cow::Borrowed(_) + )); + assert!(matches!( + fold_decimal_digits("\u{ff0d}"), + std::borrow::Cow::Borrowed(_) + )); + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index d24b47286..2cb47da7f 100755 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -12,6 +12,7 @@ mod batch_get_item; mod batch_write_item; pub mod capacity_helpers; mod create_table; +mod decimal_digits; mod delete_item; mod delete_table; mod describe_endpoints; diff --git a/crates/engine/src/query_key_values.rs b/crates/engine/src/query_key_values.rs index 7bfb356ba..68e47b657 100644 --- a/crates/engine/src/query_key_values.rs +++ b/crates/engine/src/query_key_values.rs @@ -33,7 +33,8 @@ use extenddb_core::error::DynamoDbError; use extenddb_core::expression::{ Expr, ExpressionMaps, KeyCondition, PathElement, SortKeyCondition, }; -use extenddb_core::types::{AttributeDefinition, AttributeValue, ScalarAttributeType}; +use extenddb_core::types::{AttributeDefinition, AttributeValue, QueryInput, ScalarAttributeType}; +use serde::de::DeserializeOwned; use serde_json::Value; /// The service's text for a key condition value of the wrong type. @@ -55,18 +56,54 @@ const EMPTY_NUMBER: &str = "The parameter cannot be converted to a numeric value /// that value ahead of an invalid `Select`). /// /// The checks read the raw body, so a field that does not deserialize (an -/// invalid `Select`, for one) does not stop the ones before it. A field of the -/// wrong JSON shape is left to the deserializer, which runs next. +/// invalid `Select`, for one) does not stop the ones before it. When a check +/// fails, the body is deserialized as a [`QueryInput`], and a +/// `SerializationException` from that (a field of the wrong JSON shape +/// anywhere in the body) is the answer instead, as the service reports a shape +/// fault ahead of every validation (measured 2026-10-25: a `Limit` of 0 with a +/// string `ExpressionAttributeValues`, a string `ExpressionAttributeNames` with +/// an `N` value that is not a number, and a `Limit` of 0 with `Select: 5` all +/// answer `SerializationException`). A `ValidationException` the deserializer +/// raises (an empty set, a duplicate, an invalid `AttributeValue`) does not +/// preempt the check. A body that passes every check is not deserialized here. /// /// The server runs the same function ahead of the IAM decision, so a principal /// whose `dynamodb:LeadingKeys` fence cannot bind a number that does not parse /// gets the request's `ValidationException` rather than -/// `AccessDeniedException`. +/// `AccessDeniedException`. That call goes through this function too, so it +/// answers the shape fault first as well. /// /// # Errors /// -/// Returns `DynamoDbError::ValidationException` with the service's text. +/// Returns `DynamoDbError::ValidationException` with the service's text, or +/// `DynamoDbError::SerializationException` for a body of the wrong shape. pub fn validate_query_request_body(body: &Value) -> Result<(), DynamoDbError> { + query_request_checks(body).map_err(|check| shape_fault_first::(body, check)) +} + +/// When a request-time check has failed with `check`, the deserializer's +/// `SerializationException` for `body` if there is one, else `check`. The two +/// enum fields stay in the body: an enum value that is a string but not a +/// member deserializes to a `ValidationException`, which this ignores, and one +/// of another JSON type is a shape fault the service reports ahead of the +/// checks (measured 2026-10-25: `Select: 5` with a `Limit` of 0 answers +/// `SerializationException: NUMBER_VALUE cannot be converted to String`). +pub(crate) fn shape_fault_first( + body: &Value, + check: DynamoDbError, +) -> DynamoDbError { + match serde_json::from_value::(body.clone()) { + Err(e) => match crate::deserialize_error(e) { + shape @ DynamoDbError::SerializationException(_) => shape, + _ => check, + }, + Ok(_) => check, + } +} + +/// The `Query` checks in the service's order; see +/// [`validate_query_request_body`]. +fn query_request_checks(body: &Value) -> Result<(), DynamoDbError> { if let Some(limit) = body.get("Limit").and_then(Value::as_i64) && limit < 1 { @@ -153,6 +190,12 @@ fn non_empty(body: &Value, field: &str) -> bool { /// deterministic when several are invalid. An empty `N` answers the message /// with the colon and nothing after it, as the service does on `Query`. /// +/// A Unicode decimal digit (general category `Nd`) counts as the digit it +/// stands for, as the service reads it (measured 2026-10-25: a fullwidth or +/// Arabic-Indic digit is accepted, thirty nine Devanagari ones answer the 38 +/// digit message); see [`crate::decimal_digits`]. The message echoes the value +/// as the request wrote it. +/// /// # Errors /// /// Returns `DynamoDbError::ValidationException` with the service's text. @@ -162,19 +205,36 @@ pub fn validate_expression_attribute_value_numbers( let mut keys: Vec<&String> = values.keys().collect(); keys.sort(); for key in keys { - match extenddb_core::validation::validate_attribute_number(&values[key]) { - Ok(()) => {} - Err(DynamoDbError::ValidationException(msg)) if msg == EMPTY_NUMBER => { - return Err(DynamoDbError::ValidationException(format!( - "{EMPTY_NUMBER}: " - ))); - } - Err(other) => return Err(other), - } + validate_value_numbers(&values[key])?; } Ok(()) } +/// Every number inside one attribute value (`N`, `NS` members, and the values +/// nested in `L` and `M`), with Unicode decimal digits folded for the check. +fn validate_value_numbers(value: &AttributeValue) -> Result<(), DynamoDbError> { + match value { + AttributeValue::N(n) => validate_number_text(n), + AttributeValue::NS(set) => set.iter().try_for_each(|n| validate_number_text(n)), + AttributeValue::L(list) => list.iter().try_for_each(validate_value_numbers), + AttributeValue::M(map) => map.values().try_for_each(validate_value_numbers), + _ => Ok(()), + } +} + +/// One number text against the number rules, with the service's `Query` +/// message for an empty value and the original text echoed for a malformed one. +fn validate_number_text(text: &str) -> Result<(), DynamoDbError> { + let folded = crate::decimal_digits::fold_decimal_digits(text); + match extenddb_core::validation::number::validate_and_normalize_number(&folded) { + Ok(_) => Ok(()), + Err(DynamoDbError::ValidationException(msg)) if msg.starts_with(EMPTY_NUMBER) => Err( + DynamoDbError::ValidationException(format!("{EMPTY_NUMBER}: {text}")), + ), + Err(other) => Err(other), + } +} + /// Refuse a key condition value whose type is not the key's /// `AttributeDefinitions` type. /// diff --git a/crates/engine/src/query_key_values/tests.rs b/crates/engine/src/query_key_values/tests.rs index d98e66f05..e3a03dc2f 100644 --- a/crates/engine/src/query_key_values/tests.rs +++ b/crates/engine/src/query_key_values/tests.rs @@ -437,3 +437,136 @@ fn enum_checks_sit_on_either_side_of_the_numbers() { .is_some_and(|m| m.contains("at 'select' failed to satisfy constraint")) ); } + +#[test] +fn unicode_decimal_digits_count_as_the_digits_they_stand_for() { + // Measured 2026-10-25 (X545-1 K1 to K6, F1 to F6, U1 to U6; H545-2 C11, + // C12, C17): a fullwidth, Arabic-Indic or Devanagari digit is accepted as a + // key condition, filter or unused value; thirty nine Devanagari ones answer + // the 38 digit message; `1\u{ff12}` and `1e\u{ff12}` are accepted. + for raw in [ + "\u{ff11}", + "\u{661}", + "\u{967}", + "1\u{ff12}", + "1e\u{ff12}", + "\u{ff17}", + ] { + let values = HashMap::from([(":u".to_owned(), n(raw))]); + assert!( + validate_expression_attribute_value_numbers(&values).is_ok(), + "{raw:?}" + ); + } + let values = HashMap::from([(":u".to_owned(), n(&"\u{967}".repeat(39)))]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some("Attempting to store more than 38 significant digits in a Number") + ); + // A fullwidth digit as an NS member (H545-2 C18, X545-1 NS1). + let values = HashMap::from([( + ":u".to_owned(), + AttributeValue::NS(["\u{ff11}".to_owned()].into_iter().collect()), + )]); + assert!(validate_expression_attribute_value_numbers(&values).is_ok()); +} + +#[test] +fn characters_that_are_not_decimal_digits_are_refused_with_the_text_as_sent() { + // Measured 2026-10-25 (X545-1 K7 to K10; H545-2 C13 to C16): a superscript, + // a vulgar fraction, a fullwidth minus and a fullwidth full stop are refused + // and the message echoes the value as the request wrote it. + for raw in ["\u{b2}", "\u{bd}", "\u{ff0d}1", "1\u{ff0e}5", "\u{ff11}bob"] { + let values = HashMap::from([(":u".to_owned(), n(raw))]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some(format!("The parameter cannot be converted to a numeric value: {raw}").as_str()), + "{raw:?}" + ); + } +} + +#[test] +fn a_shape_fault_in_another_field_is_reported_ahead_of_the_checks() { + // Measured 2026-10-25 (X545-1 S1 to S6, S8): a field of the wrong JSON type + // anywhere in the body answers SerializationException even when a Limit, + // mix, enum or number check would fail. + let shape_faults = [ + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "ExpressionAttributeValues": "x", + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "ExpressionAttributeNames": "x", + "ExpressionAttributeValues": {":p": {"N": "bob"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": "1", "ExpressionAttributeValues": {":p": {"N": "bob"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "AttributesToGet": {"a": 1}, "ProjectionExpression": "a", + "ExpressionAttributeValues": {":p": {"S": "x"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "QueryFilter": [1], "FilterExpression": "a = :p", + "ExpressionAttributeValues": {":p": {"S": "x"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Select": 5, "ExpressionAttributeValues": {":p": {"N": "bob"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "Select": 5, "ExpressionAttributeValues": {":p": {"S": "x"}}, + }), + ]; + for body in shape_faults { + assert!( + matches!( + validate_query_request_body(&body), + Err(DynamoDbError::SerializationException(_)) + ), + "{body}" + ); + } + + // A string enum value that is not a member is the checks' own answer, not a + // shape fault (X545-1 S7 and the 2026-10-24 enum rows). + let mut select = serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}}, + }); + select["Select"] = serde_json::json!("bad"); + assert_eq!( + message(validate_query_request_body(&select)).as_deref(), + Some(LIMIT_BELOW_ONE) + ); + select["ReturnConsumedCapacity"] = serde_json::json!("bad"); + select["Limit"] = serde_json::json!(1); + assert!( + message(validate_query_request_body(&select)) + .is_some_and(|m| m.contains("at 'returnConsumedCapacity' failed to satisfy constraint")) + ); + + // A ValidationException the deserializer raises does not preempt a check: + // an empty NS beside a Limit of 0 answers the Limit text. + let empty_set = serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}, ":s": {"NS": []}}, + }); + assert_eq!( + message(validate_query_request_body(&empty_set)).as_deref(), + Some(LIMIT_BELOW_ONE) + ); + + // A body that passes every check is left to the handler's deserializer. + let shape_fault_only = serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": "x", + }); + assert!(validate_query_request_body(&shape_fault_only).is_ok()); +} diff --git a/tests/test_query_key_value_types.py b/tests/test_query_key_value_types.py index f01033488..cb5c802ea 100644 --- a/tests/test_query_key_value_types.py +++ b/tests/test_query_key_value_types.py @@ -9,7 +9,10 @@ attribute's ``AttributeDefinitions`` type (``One or more parameter values were invalid: Condition parameter type does not match schema type``). Every expected answer below was measured against the service on 2026-10-24 (15:10, 21:00, -21:06 and 23:42 UTC) on tables with S/S, S/N and B/B keys, a GSI and an LSI. +21:06 and 23:42 UTC) and 2026-10-25 (11:31 UTC) on tables with S/S, S/N and +B/B keys, a GSI and an LSI. A Unicode decimal digit in an N value counts as +the digit it stands for; a field of the wrong JSON type anywhere in the body +answers SerializationException ahead of every check. The rows also pin where the two checks fall against the other Query checks: the numeric check after Limit, the expression parameter mix check and the ReturnConsumedCapacity enum, and before the Select enum and the table lookup; @@ -115,6 +118,10 @@ def ns_table(dynamodb_client): for pk in ("alice", "bob"): dynamodb_client.put_item(TableName=name, Item={ "pk": {"S": pk}, "sk": {"N": "5"}, "gpk": {"S": pk}, "gsk": {"S": "g"}}) + # The item the Unicode digit rows read: sk N 1 and a N 1 in ASCII. + dynamodb_client.put_item(TableName=name, Item={ + "pk": {"S": "carol"}, "sk": {"N": "1"}, "a": {"N": "1"}, + "gpk": {"S": "carol"}, "gsk": {"S": "g"}}) yield name @@ -165,6 +172,14 @@ def bb_table(dynamodb_client): ) NO_INDEX = "The table does not have the specified index: nope" UNUSED_X = "Value provided in ExpressionAttributeValues unused in expressions: keys: {:x}" +UNUSED_F = "Value provided in ExpressionAttributeValues unused in expressions: keys: {:f}" +# Unicode decimal digits (general category Nd) and characters that are not. +FW_ONE = "\uff11" # fullwidth digit one +AR_ONE = "\u0661" # Arabic-Indic digit one +DEVA_ONE = "\u0967" # Devanagari digit one +# Superscript two, vulgar half, fullwidth minus then 1, 1 fullwidth full stop 5. +NOT_DIGITS = ["\u00b2", "\u00bd", "\uff0d1", "1\uff0e5"] +CAROL = {"S": "carol"} GSI_CONSISTENT = "Consistent reads are not supported on global secondary indexes" FILTER_BEGINS_WITH_N = ( "Invalid FilterExpression: Incorrect operand type for operator or function; " @@ -354,6 +369,35 @@ def kce(expr, values, **extra): ("f6 Limit -1 with N bob", "ss", kce("pk = :u", {":u": NB}, Limit=-1), V, LIMIT_MSG), ("f7 Limit 0 with a type mismatch and N bob", "ss", kce("pk = :u AND sk = :s", {":u": N5, ":s": NB}, Limit=0), V, LIMIT_MSG), + # Unicode decimal digits are read as the digits they stand for; other + # characters are refused with the value echoed as sent (measured + # 2026-10-25 11:31 UTC on an S/N table). + ("g1 39 Devanagari ones as sk", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": DEVA_ONE * 39}}), V, DIGITS_MSG), + ("g2 39 Devanagari ones in filter", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": "1"}, ":f": {"N": DEVA_ONE * 39}}, + FilterExpression="a = :f"), V, DIGITS_MSG), + ("g3 39 Devanagari ones unused", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": "1"}, ":f": {"N": DEVA_ONE * 39}}), + V, DIGITS_MSG), + *[(f"g4 {raw!r} as sk", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": raw}}), V, NUMERIC + raw) + for raw in NOT_DIGITS], + *[(f"g5 {raw!r} in filter", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": "1"}, ":f": {"N": raw}}, + FilterExpression="a = :f"), V, NUMERIC + raw) + for raw in NOT_DIGITS], + *[(f"g6 {raw!r} unused", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": "1"}, ":f": {"N": raw}}), + V, NUMERIC + raw) + for raw in NOT_DIGITS], + *[(f"g7 {raw!r} unused passes the number check", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": "1"}, ":f": {"N": raw}}), + V, UNUSED_F) + for raw in (FW_ONE, AR_ONE, DEVA_ONE, "1\uff12", "1e\uff12")], + ("g8 NS with a fullwidth digit unused", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": "1"}, ":f": {"NS": [FW_ONE]}}), + V, UNUSED_F), # The other value types the earlier record covers. ("pk = BOOL", "ss", kce("pk = :u", {":u": {"BOOL": True}}), V, TYPE_MISMATCH), ("pk = N bob AND pk > S alice", "ss", @@ -392,6 +436,112 @@ def test_control_matching_types_on_every_table(self, dynamodb_client, ss_table, ExpressionAttributeValues={":u": B_BOB})["Count"] == 1 +class TestUnicodeDecimalDigitsAreRead: + """A Unicode decimal digit in an N value is the digit it stands for: as a + key condition value it reads the item stored with the ASCII digit, as a + filter value it matches the ASCII attribute (measured 2026-10-25 11:31 UTC + on an S/N table holding sk N 1 and a N 1).""" + + DIGITS = [FW_ONE, AR_ONE, DEVA_ONE] + DIGIT_IDS = ["fullwidth", "arabic-indic", "devanagari"] + + @pytest.mark.parametrize("digit", DIGITS, ids=DIGIT_IDS) + def test_key_condition_value_reads_the_ascii_item(self, dynamodb_client, ns_table, digit): + resp = dynamodb_client.query( + TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", + ExpressionAttributeValues={":u": CAROL, ":s": {"N": digit}}) + assert resp["Count"] == 1 + assert resp["Items"][0]["sk"] == {"N": "1"} + + @pytest.mark.parametrize("digit", DIGITS, ids=DIGIT_IDS) + def test_filter_value_matches_the_ascii_attribute(self, dynamodb_client, ns_table, digit): + resp = dynamodb_client.query( + TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", + FilterExpression="a = :f", + ExpressionAttributeValues={":u": CAROL, ":s": {"N": "1"}, ":f": {"N": digit}}) + assert resp["Count"] == 1 + assert resp["Items"][0]["a"] == {"N": "1"} + + @pytest.mark.parametrize("raw", ["1\uff12", "1e\uff12"], ids=["1-fullwidth-2", "1e-fullwidth-2"]) + def test_mixed_digits_are_one_number(self, dynamodb_client, ns_table, raw): + # Read as 12 and 1e2: accepted, matching nothing stored. + as_key = dynamodb_client.query( + TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", + ExpressionAttributeValues={":u": CAROL, ":s": {"N": raw}}) + assert (as_key["Count"], as_key["ScannedCount"]) == (0, 0) + in_filter = dynamodb_client.query( + TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", + FilterExpression="a = :f", + ExpressionAttributeValues={":u": CAROL, ":s": {"N": "1"}, ":f": {"N": raw}}) + assert (in_filter["Count"], in_filter["ScannedCount"]) == (0, 1) + + def test_fullwidth_digit_in_a_number_set_passes_the_check(self, dynamodb_client, ns_table): + resp = dynamodb_client.query( + TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", + FilterExpression="contains(ns, :f)", + ExpressionAttributeValues={":u": CAROL, ":s": {"N": "1"}, ":f": {"NS": [FW_ONE]}}) + assert (resp["Count"], resp["ScannedCount"]) == (0, 1) + + +def _raw_query(client, body: dict): + """POST ``body`` as the Query JSON exactly as written, signed with the + client's credentials, so a field of the wrong JSON type reaches the server + (boto3 would refuse or reshape it).""" + import json + + from botocore.awsrequest import AWSRequest + + req = AWSRequest(method="POST", url=client.meta.endpoint_url, data=json.dumps(body), + headers={"Content-Type": "application/x-amz-json-1.0", + "X-Amz-Target": "DynamoDB_20120810.Query"}) + client._request_signer.sign("Query", req) + resp = client._endpoint.http_session.send(req.prepare()) + return resp.status_code, json.loads(resp.content.decode("utf-8", "replace") or "{}") + + +SHAPE_FAULTS = [ + ("Limit 0 with ExpressionAttributeValues string", + {"KeyConditionExpression": "pk = :p", "Limit": 0, "ExpressionAttributeValues": "x"}), + ("ExpressionAttributeNames string with N bob", + {"KeyConditionExpression": "pk = :p", "ExpressionAttributeNames": "x", + "ExpressionAttributeValues": {":p": NB}}), + ("Limit string with N bob", + {"KeyConditionExpression": "pk = :p", "Limit": "1", "ExpressionAttributeValues": {":p": NB}}), + ("AttributesToGet object with ProjectionExpression", + {"KeyConditionExpression": "pk = :p", "AttributesToGet": {"a": 1}, "ProjectionExpression": "a", + "ExpressionAttributeValues": {":p": SX}}), + ("QueryFilter array with FilterExpression", + {"KeyConditionExpression": "pk = :p", "QueryFilter": [1], "FilterExpression": "a = :p", + "ExpressionAttributeValues": {":p": SX}}), + ("Select number with N bob", + {"KeyConditionExpression": "pk = :p", "Select": 5, "ExpressionAttributeValues": {":p": NB}}), + ("Limit 0 with Select number", + {"KeyConditionExpression": "pk = :p", "Limit": 0, "Select": 5, + "ExpressionAttributeValues": {":p": SX}}), +] + + +class TestShapeFaultsAreReportedFirst: + """A field of the wrong JSON type anywhere in the body answers + SerializationException ahead of the Limit, parameter mix, enum and number + checks (measured 2026-10-25 11:31 UTC). The message texts differ between + the service and extenddb, so only the error type is asserted.""" + + @pytest.mark.parametrize("row,body", SHAPE_FAULTS, ids=[r[0] for r in SHAPE_FAULTS]) + def test_rows(self, dynamodb_client, ss_table, row, body): + status, payload = _raw_query(dynamodb_client, {"TableName": ss_table, **body}) + assert status == 400 + assert payload.get("__type", "").endswith("SerializationException"), payload + + def test_control_without_a_shape_fault_answers_the_check(self, dynamodb_client, ss_table): + status, payload = _raw_query(dynamodb_client, { + "TableName": ss_table, "KeyConditionExpression": "pk = :p", "Limit": 0, + "ExpressionAttributeValues": {":p": NB}}) + assert status == 400 + assert payload.get("__type", "").endswith("ValidationException") + assert payload.get("message", payload.get("Message")) == LIMIT_MSG + + class TestLeadingKeysWithAnInvalidKeyValue: """A principal under each LeadingKeys fence form gets ValidationException and no items for a Query whose ExpressionAttributeValues carries an N value that From bde92d3f11930294c8da437aacabc66ac1b33f04 Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 25 Oct 2026 13:46:22 +0000 Subject: [PATCH 06/12] fix(query): leave an unknown type descriptor to the request-time checks, and assert only acceptance for Unicode digits The shape-fault pass returned the deserializer's SerializationException for an AttributeValue with an unknown type descriptor, so Limit 0 beside {"X": "1"} changed class. The service answers the Limit ValidationException for that body (measured 2026-10-25), and the check's answer is returned again. The Unicode digit tests asserted that a key condition or filter written with a Unicode digit reads the item stored with the ASCII digit. This change makes the request-time check accept such a value; it does not change how the handler reads it, so the tests assert only that the value is accepted. A new test checks that a partition key value written with Unicode digits returns no item of a partition a ForAllValues LeadingKeys fence keeps the caller from. --- crates/engine/src/decimal_digits.rs | 13 ++-- crates/engine/src/query_key_values.rs | 12 ++++ crates/engine/src/query_key_values/tests.rs | 23 +++++-- tests/test_query_key_value_types.py | 70 ++++++++++++++------- 4 files changed, 84 insertions(+), 34 deletions(-) diff --git a/crates/engine/src/decimal_digits.rs b/crates/engine/src/decimal_digits.rs index 7717634dd..8aeb24130 100644 --- a/crates/engine/src/decimal_digits.rs +++ b/crates/engine/src/decimal_digits.rs @@ -17,6 +17,11 @@ //! its zero, so the table below holds the 68 zeros (generated with Python's //! `unicodedata`, Unicode 15.0.0) and a digit's value is its offset from the //! run's zero. +//! +//! The fold applies to the check only. The handler and the server's +//! `dynamodb:LeadingKeys` binding read the value as the request wrote it, so +//! both see the same text; reading a Unicode digit as the digit has to change +//! both together. use std::borrow::Cow; @@ -61,8 +66,8 @@ mod tests { #[test] fn measured_digits_fold_to_their_ascii_value() { - // Fullwidth one, Arabic-Indic one, Devanagari one (X545-1 K1 to K3), - // fullwidth seven (H545-2 C11). + // Fullwidth one, Arabic-Indic one, Devanagari one and fullwidth seven, + // measured 2026-10-25. assert_eq!(ascii_digit('\u{ff11}'), Some('1')); assert_eq!(ascii_digit('\u{661}'), Some('1')); assert_eq!(ascii_digit('\u{967}'), Some('1')); @@ -73,8 +78,8 @@ mod tests { #[test] fn measured_controls_are_not_digits() { - // Superscript two, vulgar half, fullwidth minus, fullwidth full stop - // (H545-2 C13 to C16): refused by the service. + // Superscript two, vulgar half, fullwidth minus, fullwidth full stop: + // refused by the service (measured 2026-10-25). for c in ['\u{b2}', '\u{bd}', '\u{ff0d}', '\u{ff0e}'] { assert_eq!(ascii_digit(c), None, "{c:?}"); } diff --git a/crates/engine/src/query_key_values.rs b/crates/engine/src/query_key_values.rs index 68e47b657..54886bf96 100644 --- a/crates/engine/src/query_key_values.rs +++ b/crates/engine/src/query_key_values.rs @@ -88,12 +88,20 @@ pub fn validate_query_request_body(body: &Value) -> Result<(), DynamoDbError> { /// of another JSON type is a shape fault the service reports ahead of the /// checks (measured 2026-10-25: `Select: 5` with a `Limit` of 0 answers /// `SerializationException: NUMBER_VALUE cannot be converted to String`). +/// +/// An AttributeValue with an unknown type descriptor is not a shape fault to +/// the service: `Limit: 0` beside `{"X": "1"}` answers the `Limit` +/// `ValidationException` (measured 2026-10-25), so the deserializer's +/// `SerializationException` for that message does not preempt the check. pub(crate) fn shape_fault_first( body: &Value, check: DynamoDbError, ) -> DynamoDbError { match serde_json::from_value::(body.clone()) { Err(e) => match crate::deserialize_error(e) { + DynamoDbError::SerializationException(msg) if msg.contains(UNKNOWN_TYPE_DESCRIPTOR) => { + check + } shape @ DynamoDbError::SerializationException(_) => shape, _ => check, }, @@ -101,6 +109,10 @@ pub(crate) fn shape_fault_first( } } +/// The deserializer's text for an AttributeValue whose type descriptor is not +/// one of the service's. +const UNKNOWN_TYPE_DESCRIPTOR: &str = "unknown AttributeValue type descriptor"; + /// The `Query` checks in the service's order; see /// [`validate_query_request_body`]. fn query_request_checks(body: &Value) -> Result<(), DynamoDbError> { diff --git a/crates/engine/src/query_key_values/tests.rs b/crates/engine/src/query_key_values/tests.rs index e3a03dc2f..e9958fe59 100644 --- a/crates/engine/src/query_key_values/tests.rs +++ b/crates/engine/src/query_key_values/tests.rs @@ -440,8 +440,7 @@ fn enum_checks_sit_on_either_side_of_the_numbers() { #[test] fn unicode_decimal_digits_count_as_the_digits_they_stand_for() { - // Measured 2026-10-25 (X545-1 K1 to K6, F1 to F6, U1 to U6; H545-2 C11, - // C12, C17): a fullwidth, Arabic-Indic or Devanagari digit is accepted as a + // Measured 2026-10-25: a fullwidth, Arabic-Indic or Devanagari digit is accepted as a // key condition, filter or unused value; thirty nine Devanagari ones answer // the 38 digit message; `1\u{ff12}` and `1e\u{ff12}` are accepted. for raw in [ @@ -463,7 +462,7 @@ fn unicode_decimal_digits_count_as_the_digits_they_stand_for() { message(validate_expression_attribute_value_numbers(&values)).as_deref(), Some("Attempting to store more than 38 significant digits in a Number") ); - // A fullwidth digit as an NS member (H545-2 C18, X545-1 NS1). + // A fullwidth digit as an NS member. let values = HashMap::from([( ":u".to_owned(), AttributeValue::NS(["\u{ff11}".to_owned()].into_iter().collect()), @@ -473,7 +472,7 @@ fn unicode_decimal_digits_count_as_the_digits_they_stand_for() { #[test] fn characters_that_are_not_decimal_digits_are_refused_with_the_text_as_sent() { - // Measured 2026-10-25 (X545-1 K7 to K10; H545-2 C13 to C16): a superscript, + // Measured 2026-10-25: a superscript, // a vulgar fraction, a fullwidth minus and a fullwidth full stop are refused // and the message echoes the value as the request wrote it. for raw in ["\u{b2}", "\u{bd}", "\u{ff0d}1", "1\u{ff0e}5", "\u{ff11}bob"] { @@ -488,7 +487,7 @@ fn characters_that_are_not_decimal_digits_are_refused_with_the_text_as_sent() { #[test] fn a_shape_fault_in_another_field_is_reported_ahead_of_the_checks() { - // Measured 2026-10-25 (X545-1 S1 to S6, S8): a field of the wrong JSON type + // Measured 2026-10-25: a field of the wrong JSON type // anywhere in the body answers SerializationException even when a Limit, // mix, enum or number check would fail. let shape_faults = [ @@ -535,7 +534,7 @@ fn a_shape_fault_in_another_field_is_reported_ahead_of_the_checks() { } // A string enum value that is not a member is the checks' own answer, not a - // shape fault (X545-1 S7 and the 2026-10-24 enum rows). + // shape fault. let mut select = serde_json::json!({ "TableName": "t", "KeyConditionExpression": "pk = :p", "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}}, @@ -563,6 +562,18 @@ fn a_shape_fault_in_another_field_is_reported_ahead_of_the_checks() { Some(LIMIT_BELOW_ONE) ); + // Neither does an unknown type descriptor, which the deserializer reports + // as a SerializationException: the service answers the Limit text + // (measured 2026-10-25). + let unknown_type = serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}, ":a": {"X": "1"}}, + }); + assert_eq!( + message(validate_query_request_body(&unknown_type)).as_deref(), + Some(LIMIT_BELOW_ONE) + ); + // A body that passes every check is left to the handler's deserializer. let shape_fault_only = serde_json::json!({ "TableName": "t", "KeyConditionExpression": "pk = :p", diff --git a/tests/test_query_key_value_types.py b/tests/test_query_key_value_types.py index cb5c802ea..c336b6acc 100644 --- a/tests/test_query_key_value_types.py +++ b/tests/test_query_key_value_types.py @@ -436,44 +436,31 @@ def test_control_matching_types_on_every_table(self, dynamodb_client, ss_table, ExpressionAttributeValues={":u": B_BOB})["Count"] == 1 -class TestUnicodeDecimalDigitsAreRead: - """A Unicode decimal digit in an N value is the digit it stands for: as a - key condition value it reads the item stored with the ASCII digit, as a - filter value it matches the ASCII attribute (measured 2026-10-25 11:31 UTC - on an S/N table holding sk N 1 and a N 1).""" +class TestUnicodeDecimalDigitsPassTheCheck: + """A Unicode decimal digit in an N value passes the request-time number + check: the service reads it as the digit it stands for (measured 2026-10-25 + 11:31 UTC). Reading such a value as the digit is not part of this change, + so only the acceptance is asserted, in the positions where the request + reaches no numeric key comparison.""" DIGITS = [FW_ONE, AR_ONE, DEVA_ONE] DIGIT_IDS = ["fullwidth", "arabic-indic", "devanagari"] @pytest.mark.parametrize("digit", DIGITS, ids=DIGIT_IDS) - def test_key_condition_value_reads_the_ascii_item(self, dynamodb_client, ns_table, digit): - resp = dynamodb_client.query( - TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", - ExpressionAttributeValues={":u": CAROL, ":s": {"N": digit}}) - assert resp["Count"] == 1 - assert resp["Items"][0]["sk"] == {"N": "1"} - - @pytest.mark.parametrize("digit", DIGITS, ids=DIGIT_IDS) - def test_filter_value_matches_the_ascii_attribute(self, dynamodb_client, ns_table, digit): + def test_filter_value_is_accepted(self, dynamodb_client, ns_table, digit): resp = dynamodb_client.query( TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", FilterExpression="a = :f", ExpressionAttributeValues={":u": CAROL, ":s": {"N": "1"}, ":f": {"N": digit}}) - assert resp["Count"] == 1 - assert resp["Items"][0]["a"] == {"N": "1"} + assert resp["ScannedCount"] == 1 @pytest.mark.parametrize("raw", ["1\uff12", "1e\uff12"], ids=["1-fullwidth-2", "1e-fullwidth-2"]) - def test_mixed_digits_are_one_number(self, dynamodb_client, ns_table, raw): - # Read as 12 and 1e2: accepted, matching nothing stored. - as_key = dynamodb_client.query( - TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", - ExpressionAttributeValues={":u": CAROL, ":s": {"N": raw}}) - assert (as_key["Count"], as_key["ScannedCount"]) == (0, 0) - in_filter = dynamodb_client.query( + def test_mixed_digits_in_the_filter_are_accepted(self, dynamodb_client, ns_table, raw): + resp = dynamodb_client.query( TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", FilterExpression="a = :f", ExpressionAttributeValues={":u": CAROL, ":s": {"N": "1"}, ":f": {"N": raw}}) - assert (in_filter["Count"], in_filter["ScannedCount"]) == (0, 1) + assert (resp["Count"], resp["ScannedCount"]) == (0, 1) def test_fullwidth_digit_in_a_number_set_passes_the_check(self, dynamodb_client, ns_table): resp = dynamodb_client.query( @@ -660,3 +647,38 @@ def test_every_fence_form_gets_validation_exception_and_no_items(self): failures.append(f"{shape} / {label}: {err['Code']} {err['Message']}") owner.delete_table(TableName=table) assert not failures, "\n".join(failures) + + def test_a_unicode_digit_key_value_reads_no_fenced_partition(self): + """A partition key value written with Unicode digits is not bound to a + LeadingKeys value, so it must not read a partition the fence keeps the + principal from. The service's answer for a fenced principal cannot be + measured here; the test asserts only that no item of partition 20 is + returned, whatever the answer is.""" + owner = self._client({ + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": "dynamodb:*", "Resource": "*"}], + }) + table = f"kv-{uuid.uuid4().hex[:8]}" + owner.create_table( + TableName=table, + AttributeDefinitions=[{"AttributeName": "pk", "AttributeType": "N"}], + KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], + BillingMode="PAY_PER_REQUEST") + wait_for_active(owner, table) + for pk in ("10", "20"): + owner.put_item(TableName=table, Item={"pk": {"N": pk}, "v": {"S": "p" + pk}}) + fenced = self._client(self._fence( + {"ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["10"]}}, table)) + leaks = [] + for label, value in (("fullwidth", "\uff12\uff10"), ("arabic-indic", "\u0662\u0660"), + ("devanagari", "\u0968\u0966")): + try: + resp = fenced.query( + TableName=table, KeyConditionExpression="pk = :p", + ExpressionAttributeValues={":p": {"N": value}}) + except ClientError: + continue + if any(item.get("v") == {"S": "p20"} for item in resp["Items"]): + leaks.append(label) + owner.delete_table(TableName=table) + assert not leaks, leaks From 3a633fd13a637f796f8bf27667ea83c58dca2bef Mon Sep 17 00:00:00 2001 From: Lee Hannigan Date: Sun, 25 Oct 2026 16:01:04 +0000 Subject: [PATCH 07/12] test(query): say which Limit text the service uses beside an unknown type descriptor --- crates/engine/src/query_key_values/tests.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/query_key_values/tests.rs b/crates/engine/src/query_key_values/tests.rs index e9958fe59..f1ba2d3e6 100644 --- a/crates/engine/src/query_key_values/tests.rs +++ b/crates/engine/src/query_key_values/tests.rs @@ -563,8 +563,9 @@ fn a_shape_fault_in_another_field_is_reported_ahead_of_the_checks() { ); // Neither does an unknown type descriptor, which the deserializer reports - // as a SerializationException: the service answers the Limit text - // (measured 2026-10-25). + // as a SerializationException: the service answers the Limit + // ValidationException (measured 2026-10-25), in its older + // `Value '0' at 'limit'` form; ExtendDB answers its usual Limit text. let unknown_type = serde_json::json!({ "TableName": "t", "KeyConditionExpression": "pk = :p", "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}, ":a": {"X": "1"}}, From 44b7b18028b250d27169d4395200de64d5006b2b Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:29:33 +0000 Subject: [PATCH 08/12] test(query): pass the RANGE attributes to resolve_multipart Main changes resolve_multipart to take the HASH and the RANGE attribute names (#429). The multi-attribute HASH test now gives r as the RANGE attribute, as its attribute definitions do. --- crates/engine/src/query_key_values/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/engine/src/query_key_values/tests.rs b/crates/engine/src/query_key_values/tests.rs index f1ba2d3e6..68c353943 100644 --- a/crates/engine/src/query_key_values/tests.rs +++ b/crates/engine/src/query_key_values/tests.rs @@ -198,7 +198,7 @@ fn multipart_hash_conditions_are_checked() { ]); let mut kc = parse("g1 = :a AND g2 = :b AND r > :c"); let names = HashMap::new(); - kc.resolve_multipart(&["g1", "g2"], &names) + kc.resolve_multipart(&["g1", "g2"], &["r"], &names) .expect("resolve"); assert_eq!(kc.extra_pk_conditions.len(), 1); let m = maps(&[], &[("a", s("x")), ("b", s("1")), ("c", s("r"))]); From 0b8e97dbc94459d5e142dfc5b1a20f25b965fa01 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 6 Nov 2026 19:56:16 +0000 Subject: [PATCH 09/12] fix(query): order the key value type check and the begins_with refusal as the service does On a single-attribute key schema, the later check of this branch now reports a wrong type in a key condition expression, as the service does. The early check from main still runs there for legacy KeyConditions, which the later check skips. The begins_with refusal now reads the type of the value instead of the type of the sort key. --- crates/engine/src/query.rs | 86 +++++++++------ crates/engine/src/query_begins_with_tests.rs | 107 +++++++++++++++++++ tests/test_query_key_value_types.py | 35 +++++- 3 files changed, 189 insertions(+), 39 deletions(-) create mode 100644 crates/engine/src/query_begins_with_tests.rs diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index 503c8e993..a67ed5f13 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -226,11 +226,9 @@ pub async fn handle_query( // HASH or several RANGE attributes) every clause is assigned by name, every // HASH attribute needs an equality and the RANGE conditions must cover a // leading run of the RANGE attributes with equalities before the last one. - // A wrong-typed value is reported where the service reports it: after those - // structural rules and before the non-key rule on a multi-attribute key - // schema, and ahead of the missing-key check on a single-attribute one - // (measured 2026-10-21: `pk = :n` on an S key with a second clause on a - // non-key attribute reports the type, not the missing key). + // On a multi-attribute key schema the service reports a wrong-typed value + // after those structural rules and before the non-key rule, so the + // classifier runs the type check there. // // On a multi-attribute key schema the classifier refuses a clause on an // attribute outside the key schema (`Query condition missed key schema @@ -240,11 +238,17 @@ pub async fn handle_query( // 2026-10-30 on a GSI with HASH `GSI2PK` and RANGE `GSI2SK`: `GSI2PK = :h // AND PK = :k` answers `Query condition missed key schema element: GSI2SK` // whether `:k` has the table's type for `PK` or not, so the service's type - // check does not see an attribute outside the queried key schema. The - // single-attribute path has no non-key rule yet: a clause on another - // attribute reaches storage, which applies it to the first RANGE - // attribute, so it keeps checking against the table's definitions, which - // refuse a value of the wrong type before storage does. + // check does not see an attribute outside the queried key schema. + // + // On a single-attribute key schema the service reports a wrong-typed value + // after the missing-key, filter, projection, unused-value and Select + // checks (measured 2026-11-06). The classifier does not run the type check + // there, and `validate_key_condition_types` reports the value below. `pk = + // :n` on an S key with a second clause on a non-key attribute reports the + // type, not a missing key schema element (measured 2026-10-21). This path + // has no non-key rule, so that query also reaches the check below. + // Legacy `KeyConditions` do not go through the check below, so the + // classifier still checks their values, against the table's definitions. let multipart = extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema); let key_schema_definitions: Vec = query_key_info .attribute_definitions @@ -262,16 +266,19 @@ pub async fn handle_query( } else { &query_key_info.attribute_definitions }; + let check_types_here = multipart || legacy_kc_maps.is_some(); key_condition.resolve_key_schema_with( &query_key_info.key_schema, &effective_maps.names, - |path, value| check_key_value_type(path, value, type_definitions, effective_maps), - )?; - refuse_begins_with_on_number( - &key_condition, - &query_key_info.attribute_definitions, - effective_maps, + |path, value| { + if check_types_here { + check_key_value_type(path, value, type_definitions, effective_maps) + } else { + Ok(()) + } + }, )?; + refuse_begins_with_on_number(&key_condition, effective_maps)?; // On a single-attribute key schema, validate that the partition key is // actually referenced in the condition. The classifier has already required // an equality on every HASH attribute of a multi-attribute one. @@ -633,30 +640,35 @@ fn check_key_value_type( } } -/// Refuse `begins_with` on a number key attribute (measured 2026-10-21: -/// `Invalid KeyConditionExpression: Incorrect operand type for operator or -/// function; operator or function: begins_with, operand type: N`). +/// Refuse a `begins_with` key condition whose value is not an S or a B, with +/// the value's type in the text: `Invalid KeyConditionExpression: Incorrect +/// operand type for operator or function; operator or function: begins_with, +/// operand type: N`. +/// +/// Measured on the service 2026-11-06: the value's type decides the refusal, +/// not the sort key's type. An N or a BOOL value is refused on an S, an N and +/// a B sort key alike. An S or a B value on a sort key of another type gets +/// the type mismatch from the type check instead. The refusal comes before the +/// unused-value, projection overlap and Select checks and before a wrong-typed +/// partition key value. A placeholder that has no value passes here, and the +/// existing checks report it. fn refuse_begins_with_on_number( key_condition: &extenddb_core::expression::KeyCondition, - attribute_definitions: &[extenddb_core::types::AttributeDefinition], maps: &ExpressionMaps, ) -> Result<(), DynamoDbError> { - use extenddb_core::expression::SortKeyCondition; - use extenddb_core::types::ScalarAttributeType; + use extenddb_core::expression::{Expr, SortKeyCondition}; - if let Some(SortKeyCondition::BeginsWith { path, .. }) = &key_condition.sk_condition - && let Some(attr) = resolve_path_attr_name(path, &maps.names) - && attribute_definitions - .iter() - .any(|d| d.attribute_name == attr && d.attribute_type == ScalarAttributeType::N) - { - return Err(DynamoDbError::ValidationException( - "Invalid KeyConditionExpression: Incorrect operand type for operator or function; \ - operator or function: begins_with, operand type: N" - .to_owned(), - )); - } - Ok(()) + let Some(SortKeyCondition::BeginsWith { path, prefix }) = &key_condition.sk_condition else { + return Ok(()); + }; + // The operand check of the filter, applied to the key condition's call. + let call = Expr::Function { + name: "begins_with".to_owned(), + args: vec![Expr::Path(path.clone()), prefix.clone()], + }; + extenddb_core::expression::validate_begins_with_operands(&call, maps).map_err(|e| { + crate::expression_helpers::prefix_expression_error(e, ExpressionKind::KeyCondition) + }) } /// Validate that all `#name` references in an expression AST are defined. @@ -760,3 +772,7 @@ mod empty_key_condition_tests { assert!(reject_empty_key_condition_expression(Some("pk = :v")).is_ok()); } } + +#[cfg(test)] +#[path = "query_begins_with_tests.rs"] +mod begins_with_tests; diff --git a/crates/engine/src/query_begins_with_tests.rs b/crates/engine/src/query_begins_with_tests.rs new file mode 100644 index 000000000..0a97577bd --- /dev/null +++ b/crates/engine/src/query_begins_with_tests.rs @@ -0,0 +1,107 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Tests for [`refuse_begins_with_on_number`]: the type of the `begins_with` +//! value decides the refusal, not the sort key's type (measured 2026-11-06). + +use std::collections::HashMap; + +use extenddb_core::error::DynamoDbError; +use extenddb_core::expression::{ + ExpressionMaps, KeyCondition, parse_key_condition, tokenize_with_limit, +}; +use extenddb_core::types::{AttributeDefinition, AttributeValue, ScalarAttributeType}; + +use super::refuse_begins_with_on_number; +use crate::query_key_values::{KEY_CONDITION_TYPE_MISMATCH, validate_key_condition_types}; + +const OPERAND_TYPE: &str = "Invalid KeyConditionExpression: Incorrect operand type for operator or \ + function; operator or function: begins_with, operand type: "; + +const SORT_KEY_TYPES: [ScalarAttributeType; 3] = [ + ScalarAttributeType::S, + ScalarAttributeType::N, + ScalarAttributeType::B, +]; + +fn parse(kce: &str) -> KeyCondition { + let tokens = tokenize_with_limit(kce, 1000).expect("tokenize"); + parse_key_condition(&tokens).expect("parse") +} + +fn values(pairs: Vec<(&str, AttributeValue)>) -> ExpressionMaps { + ExpressionMaps::new( + HashMap::new(), + pairs.into_iter().map(|(k, v)| (k.to_owned(), v)).collect(), + ) +} + +fn message(result: Result<(), DynamoDbError>) -> Option { + match result { + Ok(()) => None, + Err(DynamoDbError::ValidationException(msg)) => Some(msg), + Err(other) => panic!("unexpected error {other:?}"), + } +} + +/// The answer to `pk = :u AND begins_with(sk, :b)` with an S partition key and +/// a sort key of `sk_type`: the refusal, then the type check that +/// `handle_query` runs later. `None` when both pass. +fn answer(sk_type: ScalarAttributeType, value: AttributeValue) -> Option { + let key_condition = parse("pk = :u AND begins_with(sk, :b)"); + let maps = values(vec![("u", AttributeValue::S("x".to_owned())), ("b", value)]); + let definitions = [ + AttributeDefinition { + attribute_name: "pk".to_owned(), + attribute_type: ScalarAttributeType::S, + }, + AttributeDefinition { + attribute_name: "sk".to_owned(), + attribute_type: sk_type, + }, + ]; + message( + refuse_begins_with_on_number(&key_condition, &maps) + .and_then(|()| validate_key_condition_types(&key_condition, &maps, &definitions)), + ) +} + +#[test] +fn number_and_bool_values_are_refused_with_their_type_on_every_sort_key_type() { + for sk_type in SORT_KEY_TYPES { + for (value, type_code) in [ + (AttributeValue::N("1".to_owned()), "N"), + (AttributeValue::Bool(true), "BOOL"), + ] { + assert_eq!( + answer(sk_type, value), + Some(format!("{OPERAND_TYPE}{type_code}")), + "{sk_type:?} sort key" + ); + } + } +} + +#[test] +fn string_and_binary_values_pass_to_the_type_check_on_every_sort_key_type() { + for sk_type in SORT_KEY_TYPES { + for (value, value_type) in [ + (AttributeValue::S("1".to_owned()), ScalarAttributeType::S), + (AttributeValue::B(vec![1]), ScalarAttributeType::B), + ] { + let expected = (value_type != sk_type).then(|| KEY_CONDITION_TYPE_MISMATCH.to_owned()); + assert_eq!( + answer(sk_type, value), + expected, + "{value_type:?} value on {sk_type:?} sort key" + ); + } + } +} + +#[test] +fn a_placeholder_without_a_value_passes() { + let key_condition = parse("pk = :u AND begins_with(sk, :zz)"); + let maps = values(vec![("u", AttributeValue::S("x".to_owned()))]); + assert!(refuse_begins_with_on_number(&key_condition, &maps).is_ok()); +} diff --git a/tests/test_query_key_value_types.py b/tests/test_query_key_value_types.py index c336b6acc..16f2d1546 100644 --- a/tests/test_query_key_value_types.py +++ b/tests/test_query_key_value_types.py @@ -9,10 +9,11 @@ attribute's ``AttributeDefinitions`` type (``One or more parameter values were invalid: Condition parameter type does not match schema type``). Every expected answer below was measured against the service on 2026-10-24 (15:10, 21:00, -21:06 and 23:42 UTC) and 2026-10-25 (11:31 UTC) on tables with S/S, S/N and -B/B keys, a GSI and an LSI. A Unicode decimal digit in an N value counts as -the digit it stands for; a field of the wrong JSON type anywhere in the body -answers SerializationException ahead of every check. +21:06 and 23:42 UTC), 2026-10-25 (11:31 UTC) and 2026-11-06 (the begins_with +rows after b20) on tables with S/S, S/N and B/B keys, a GSI and an LSI. A +Unicode decimal digit in an N value counts as the digit it stands for; a field +of the wrong JSON type anywhere in the body answers SerializationException +ahead of every check. The rows also pin where the two checks fall against the other Query checks: the numeric check after Limit, the expression parameter mix check and the ReturnConsumedCapacity enum, and before the Select enum and the table lookup; @@ -185,6 +186,10 @@ def bb_table(dynamodb_client): "Invalid FilterExpression: Incorrect operand type for operator or function; " "operator or function: begins_with, operand type: N" ) +BEGINS_WITH_KCE = ( + "Invalid KeyConditionExpression: Incorrect operand type for operator or function; " + "operator or function: begins_with, operand type: " +) PROJECTION_OVERLAP = ( "Invalid ProjectionExpression: Two document paths overlap with each other; " "must remove or rewrite one of these paths; path one: [a], path two: [a, b]" @@ -284,6 +289,28 @@ def kce(expr, values, **extra): V, TYPE_MISMATCH), ("b20 N sk begins_with S", "ns", kce("pk = :u AND begins_with(sk, :b)", {":u": SX, ":b": {"S": "1"}}), V, TYPE_MISMATCH), + # begins_with is refused by the type of its value, on every sort key type, + # ahead of the unused value check; an S or B value on a sort key of another + # type gets the type mismatch (measured on Amazon DynamoDB in us-east-1 on + # 6 Nov 2026). + ("q1 S sk begins_with N", "ss", + kce("pk = :u AND begins_with(sk, :n)", {":u": SX, ":n": {"N": "1"}}), V, BEGINS_WITH_KCE + "N"), + ("q2 N sk begins_with B", "ns", + kce("pk = :u AND begins_with(sk, :b)", {":u": SX, ":b": B_BOB}), V, TYPE_MISMATCH), + ("q3 B sk begins_with N", "bb", + kce("bpk = :u AND begins_with(bsk, :n)", {":u": B_BOB, ":n": {"N": "1"}}), + V, BEGINS_WITH_KCE + "N"), + ("q4 N sk begins_with BOOL", "ns", + kce("pk = :u AND begins_with(sk, :t)", {":u": SX, ":t": {"BOOL": True}}), + V, BEGINS_WITH_KCE + "BOOL"), + ("q5 S sk begins_with N with unused value", "ss", + kce("pk = :u AND begins_with(sk, :n)", {":u": SX, ":n": {"N": "1"}, ":x": SX}), + V, BEGINS_WITH_KCE + "N"), + ("p2 N sk begins_with N", "ns", + kce("pk = :u AND begins_with(sk, :n)", {":u": SX, ":n": {"N": "1"}}), V, BEGINS_WITH_KCE + "N"), + ("p4 N sk begins_with N with unused value", "ns", + kce("pk = :u AND begins_with(sk, :n)", {":u": SX, ":n": {"N": "1"}, ":x": SX}), + V, BEGINS_WITH_KCE + "N"), ("b22 pk = N 5 AND N sk = S 5", "ns", kce("pk = :u AND sk = :s", {":u": N5, ":s": {"S": "5"}}), V, TYPE_MISMATCH), ("b23 N sk = N bob", "ns", kce("pk = :u AND sk = :s", {":u": SX, ":s": NB}), V, NUMERIC + "bob"), From 7b0924f5369dd816215358a5626a5aebd35ed797 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Fri, 6 Nov 2026 20:54:23 +0000 Subject: [PATCH 10/12] test(query): pin begins_with on a non-key attribute and correct the order comments Two new rows send begins_with with an N value on an attribute that is not in the key schema. One row uses a table with an S sort key and the other uses a table with an N sort key. The service refuses both requests with the begins_with operand type message. The comments in handle_query now say that the classifier checks the values of legacy KeyConditions. They also say which attribute definitions it uses for each kind of key schema. The doc of resolve_key_schema_with now says where the service reports a wrong type on a single-attribute key schema (measured 2026-11-06). --- crates/core/src/expression/key_condition.rs | 9 ++++++--- crates/engine/src/query.rs | 8 +++++--- tests/test_query_key_value_types.py | 12 ++++++++---- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/crates/core/src/expression/key_condition.rs b/crates/core/src/expression/key_condition.rs index edaa1f8e5..4d461356e 100755 --- a/crates/core/src/expression/key_condition.rs +++ b/crates/core/src/expression/key_condition.rs @@ -381,9 +381,12 @@ impl KeyCondition { /// [`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. + /// schema `value_check` runs on every clause first, and then + /// [`Self::resolve_pk_sk`] picks the partition key clause. On a + /// single-attribute key schema the service reports a value of the wrong + /// type after the missing-key, filter, projection, unused-value and Select + /// checks (measured 2026-11-06), so the engine passes a `value_check` that + /// checks nothing there unless the request uses legacy `KeyConditions`. /// /// The engine reads a `Query` key condition through this, and so does /// authorization when it finds the partition key value for diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index a67ed5f13..5dd85edfe 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -248,7 +248,9 @@ pub async fn handle_query( // type, not a missing key schema element (measured 2026-10-21). This path // has no non-key rule, so that query also reaches the check below. // Legacy `KeyConditions` do not go through the check below, so the - // classifier still checks their values, against the table's definitions. + // classifier still checks their values: against the key schema's + // definitions on a multi-attribute key schema and against the table's on a + // single-attribute one. let multipart = extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema); let key_schema_definitions: Vec = query_key_info .attribute_definitions @@ -447,8 +449,8 @@ pub async fn handle_query( // A key condition value whose type is not the key attribute's type is // refused after the key schema, filter, projection, unused value and // Select checks and before the ExclusiveStartKey check (measured - // 2026-10-24). The legacy KeyConditions path makes its own checks when it - // desugars. + // 2026-10-24). The classifier above checked the values of legacy + // `KeyConditions`. if legacy_kc_maps.is_none() { crate::query_key_values::validate_key_condition_types( &key_condition, diff --git a/tests/test_query_key_value_types.py b/tests/test_query_key_value_types.py index 16f2d1546..6db90cbd3 100644 --- a/tests/test_query_key_value_types.py +++ b/tests/test_query_key_value_types.py @@ -289,10 +289,10 @@ def kce(expr, values, **extra): V, TYPE_MISMATCH), ("b20 N sk begins_with S", "ns", kce("pk = :u AND begins_with(sk, :b)", {":u": SX, ":b": {"S": "1"}}), V, TYPE_MISMATCH), - # begins_with is refused by the type of its value, on every sort key type, - # ahead of the unused value check; an S or B value on a sort key of another - # type gets the type mismatch (measured on Amazon DynamoDB in us-east-1 on - # 6 Nov 2026). + # begins_with is refused by the type of its value, on every sort key type + # and on an attribute outside the key schema, ahead of the unused value + # check; an S or B value on a sort key of another type gets the type + # mismatch (measured on Amazon DynamoDB in us-east-1 on 6 Nov 2026). ("q1 S sk begins_with N", "ss", kce("pk = :u AND begins_with(sk, :n)", {":u": SX, ":n": {"N": "1"}}), V, BEGINS_WITH_KCE + "N"), ("q2 N sk begins_with B", "ns", @@ -311,6 +311,10 @@ def kce(expr, values, **extra): ("p4 N sk begins_with N with unused value", "ns", kce("pk = :u AND begins_with(sk, :n)", {":u": SX, ":n": {"N": "1"}, ":x": SX}), V, BEGINS_WITH_KCE + "N"), + ("m2 non-key begins_with N, S sk", "ss", + kce("pk = :u AND begins_with(zz, :n)", {":u": SX, ":n": {"N": "1"}}), V, BEGINS_WITH_KCE + "N"), + ("m3 non-key begins_with N, N sk", "ns", + kce("pk = :u AND begins_with(zz, :n)", {":u": SX, ":n": {"N": "1"}}), V, BEGINS_WITH_KCE + "N"), ("b22 pk = N 5 AND N sk = S 5", "ns", kce("pk = :u AND sk = :s", {":u": N5, ":s": {"S": "5"}}), V, TYPE_MISMATCH), ("b23 N sk = N bob", "ns", kce("pk = :u AND sk = :s", {":u": SX, ":s": NB}), V, NUMERIC + "bob"), From cb2df0b1ee41bfd8bc00453f44ed35fdcad35905 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Wed, 11 Nov 2026 12:00:29 +0000 Subject: [PATCH 11/12] fix(query): check each value on its own and answer in the service's order - Report an invalid ReturnConsumedCapacity ahead of the expression and non-expression parameter mix. - Check the numbers in each ExpressionAttributeValues entry on its own. A value that does not deserialize no longer turns the check off for the others, in the handler and before authorization. - Beside a value the service cannot read as one type, answer in the service's order. Such a value is a null, or holds no known type descriptor, more than one, or an unknown one beside a known one. Select, ReturnConsumedCapacity and Limit come together, with the Limit text that names the value. The parameter mix comes next, then the first value with a fault and its placeholder. A value the service cannot read comes ahead of a number fault in another value. - In the shape fault check, replace only the parts of such a value that the service cannot read. A field of the wrong JSON type in the rest of the value still answers SerializationException. - Count a decimal digit only below U+10000, as the service does. The check refuses a digit from U+104A0 up. - Echo a refused number with each decimal digit folded to ASCII. - Add rows for legacy KeyConditions and for a multi-attribute key schema beside a non-key clause. Skip the second type check where the classifier checked the values. - Delete the LeadingKeys test tables when a call raises. - Give the checks for values the service cannot read their own module and tests. Move the request order unit tests and the shape fault and LeadingKeys Python tests to files of their own. --- crates/engine/src/decimal_digits.rs | 59 +-- crates/engine/src/query.rs | 20 +- crates/engine/src/query_gate_tests.rs | 145 ++++++++ crates/engine/src/query_key_values.rs | 301 +++++++++++----- .../query_key_values/request_order_tests.rs | 279 +++++++++++++++ crates/engine/src/query_key_values/tests.rs | 303 +++++----------- .../engine/src/query_key_values/unreadable.rs | 208 +++++++++++ .../src/query_key_values/unreadable_tests.rs | 301 ++++++++++++++++ tests/test_query_key_value_types.py | 337 ++++++------------ tests/test_query_leading_keys_values.py | 182 ++++++++++ tests/test_query_value_shapes.py | 174 +++++++++ 11 files changed, 1757 insertions(+), 552 deletions(-) create mode 100644 crates/engine/src/query_gate_tests.rs create mode 100644 crates/engine/src/query_key_values/request_order_tests.rs create mode 100644 crates/engine/src/query_key_values/unreadable.rs create mode 100644 crates/engine/src/query_key_values/unreadable_tests.rs create mode 100644 tests/test_query_leading_keys_values.py create mode 100644 tests/test_query_value_shapes.py diff --git a/crates/engine/src/decimal_digits.rs b/crates/engine/src/decimal_digits.rs index 8aeb24130..60e86a4ee 100644 --- a/crates/engine/src/decimal_digits.rs +++ b/crates/engine/src/decimal_digits.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 -//! Unicode decimal digits (general category `Nd`) folded to ASCII for the -//! request-time number check. +//! Unicode decimal digits (general category `Nd`) in the Basic Multilingual +//! Plane, folded to ASCII for the request-time number check. //! //! Measured 2026-10-25 (us-east-1, each request twice): the service reads a //! fullwidth, Arabic-Indic or Devanagari digit in an `N` value as the digit it @@ -10,34 +10,37 @@ //! digits in a Number`), and refuses a superscript (`\u{b2}`), a vulgar //! fraction (`\u{bd}`), a fullwidth minus (`\u{ff0d}`) and a fullwidth full //! stop (`\u{ff0e}`) with `The parameter cannot be converted to a numeric -//! value: `. The accepted characters are exactly those of category `Nd`; -//! the refused ones are `No` or `Pd` and `Po`. +//! value: `. The refused ones are `No` or `Pd` and `Po`. +//! +//! The service accepts a category `Nd` digit only below U+10000 (measured +//! 2026-11-11, each request twice): digit one of each of the 37 `Nd` runs up +//! to U+FF10 is accepted as a `FilterExpression` value, and digit one of each +//! of the 31 runs from U+104A0 up answers `The parameter cannot be converted to +//! a numeric value: `. //! //! Every `Nd` run in Unicode 15.0 is ten consecutive code points starting at -//! its zero, so the table below holds the 68 zeros (generated with Python's -//! `unicodedata`, Unicode 15.0.0) and a digit's value is its offset from the -//! run's zero. +//! its zero, so the table below holds the 37 zeros below U+10000 (generated +//! with Python's `unicodedata`, Unicode 15.0.0) and a digit's value is its +//! offset from the run's zero. //! -//! The fold applies to the check only. The handler and the server's -//! `dynamodb:LeadingKeys` binding read the value as the request wrote it, so -//! both see the same text; reading a Unicode digit as the digit has to change -//! both together. +//! The fold applies to the check and the text of its refusal only. The handler +//! and the server's `dynamodb:LeadingKeys` binding read the value as the +//! request wrote it, so both see the same text; reading a Unicode digit as the +//! digit has to change both together. use std::borrow::Cow; -/// The zero of every `Nd` run in Unicode 15.0, ascending. -const ND_ZEROS: [u32; 68] = [ +/// The zero of every `Nd` run in Unicode 15.0 below U+10000, ascending. +const ND_ZEROS: [u32; 37] = [ 0x0030, 0x0660, 0x06F0, 0x07C0, 0x0966, 0x09E6, 0x0A66, 0x0AE6, 0x0B66, 0x0BE6, 0x0C66, 0x0CE6, 0x0D66, 0x0DE6, 0x0E50, 0x0ED0, 0x0F20, 0x1040, 0x1090, 0x17E0, 0x1810, 0x1946, 0x19D0, 0x1A80, 0x1A90, 0x1B50, 0x1BB0, 0x1C40, 0x1C50, 0xA620, 0xA8D0, 0xA900, 0xA9D0, 0xA9F0, 0xAA50, 0xABF0, - 0xFF10, 0x104A0, 0x10D30, 0x11066, 0x110F0, 0x11136, 0x111D0, 0x112F0, 0x11450, 0x114D0, - 0x11650, 0x116C0, 0x11730, 0x118E0, 0x11950, 0x11C50, 0x11D50, 0x11DA0, 0x11F50, 0x16A60, - 0x16AC0, 0x16B50, 0x1D7CE, 0x1D7D8, 0x1D7E2, 0x1D7EC, 0x1D7F6, 0x1E140, 0x1E2F0, 0x1E4F0, - 0x1E950, 0x1FBF0, + 0xFF10, ]; /// The ASCII digit `c` stands for when `c` is a Unicode decimal digit -/// (general category `Nd`), or `None` for every other character. +/// (general category `Nd`) below U+10000, or `None` for every other +/// character. pub(crate) fn ascii_digit(c: char) -> Option { let cp = u32::from(c); // The zero of the run that could hold `cp`: the last zero at or below it. @@ -91,12 +94,28 @@ mod tests { assert_eq!(ascii_digit('a'), None); assert_eq!(ascii_digit('.'), None); assert_eq!(ascii_digit('e'), None); - assert_eq!(ascii_digit('\u{1FBF9}'), Some('9')); - assert_eq!(ascii_digit('\u{1FBFA}'), None); assert_eq!(ascii_digit(char::MAX), None); assert_eq!(ascii_digit('\u{0}'), None); } + #[test] + fn digits_from_u_104a0_up_are_not_folded() { + // Measured 2026-11-11: digit one of each run from U+104A0 up is refused, + // and digit one of the last run below U+10000 is accepted. + for c in [ + '\u{104A1}', + '\u{11067}', + '\u{1D7CF}', + '\u{1D7F7}', + '\u{1E951}', + '\u{1FBF1}', + ] { + assert_eq!(ascii_digit(c), None, "{c:?}"); + } + assert_eq!(ascii_digit('\u{ABF1}'), Some('1')); + assert_eq!(fold_decimal_digits("\u{ff11}\u{1D7CF}"), "1\u{1D7CF}"); + } + #[test] fn zeros_are_ascending_and_distinct_runs() { for pair in ND_ZEROS.windows(2) { diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index 5dd85edfe..76fedd5ea 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -48,8 +48,8 @@ pub async fn handle_query( ctx: &OperationContext, ) -> Result { // Request-time checks on the raw body, in the service's order: Limit, the - // expression and non-expression parameter mix, the ReturnConsumedCapacity - // enum, every number in ExpressionAttributeValues, the Select enum. They + // ReturnConsumedCapacity enum, the expression and non-expression parameter + // mix, every number in ExpressionAttributeValues, the Select enum. They // run before typed deserialization so a field that does not deserialize // does not stop the ones before it. The server runs the same function // ahead of the IAM decision; see `query_key_values`. @@ -247,10 +247,10 @@ pub async fn handle_query( // :n` on an S key with a second clause on a non-key attribute reports the // type, not a missing key schema element (measured 2026-10-21). This path // has no non-key rule, so that query also reaches the check below. - // Legacy `KeyConditions` do not go through the check below, so the - // classifier still checks their values: against the key schema's - // definitions on a multi-attribute key schema and against the table's on a - // single-attribute one. + // Legacy `KeyConditions` are checked by the classifier: against the key + // schema's definitions on a multi-attribute key schema and against the + // table's on a single-attribute one. The check below runs only where the + // classifier did not check (`!check_types_here`). let multipart = extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema); let key_schema_definitions: Vec = query_key_info .attribute_definitions @@ -450,8 +450,8 @@ pub async fn handle_query( // refused after the key schema, filter, projection, unused value and // Select checks and before the ExclusiveStartKey check (measured // 2026-10-24). The classifier above checked the values of legacy - // `KeyConditions`. - if legacy_kc_maps.is_none() { + // `KeyConditions` and of every multi-attribute key schema. + if !check_types_here { crate::query_key_values::validate_key_condition_types( &key_condition, effective_maps, @@ -778,3 +778,7 @@ mod empty_key_condition_tests { #[cfg(test)] #[path = "query_begins_with_tests.rs"] mod begins_with_tests; + +#[cfg(test)] +#[path = "query_gate_tests.rs"] +mod gate_tests; diff --git a/crates/engine/src/query_gate_tests.rs b/crates/engine/src/query_gate_tests.rs new file mode 100644 index 000000000..c39f5c1b0 --- /dev/null +++ b/crates/engine/src/query_gate_tests.rs @@ -0,0 +1,145 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! Tests for the split of the key condition type check in [`handle_query`]: +//! the classifier checks the values of legacy `KeyConditions` and of a +//! multi-attribute key schema, and `validate_key_condition_types` checks the +//! others after the Select check. The multi-attribute row fails when the +//! classifier stops checking a multi-attribute key schema. The legacy row pins +//! only that legacy values are type-checked: with the legacy term of the split +//! dropped, `validate_key_condition_types` answers the same text. The expected +//! answers were measured on the service in us-east-1 on 2026-11-11, the +//! multi-attribute ones on a GSI. + +use std::sync::Arc; + +use serde_json::{Value, json}; + +use extenddb_core::error::DynamoDbError; +use extenddb_core::limits::LimitsConfig; +use extenddb_core::types::{ + AttributeDefinition, KeySchemaElement, KeyType, ScalarAttributeType, TableKeyInfo, +}; +use extenddb_storage_sqlite::SqliteEngine; + +use super::handle_query; +use crate::query_key_values::KEY_CONDITION_TYPE_MISMATCH; +use crate::{AuthorizedKeyInfo, BackupRuntime, OperationContext}; + +/// A context whose authorization snapshot holds table `kvt` with `key_schema` +/// and `definitions`, so the request reads no catalog. Every request here is +/// refused before a row is read, so the store stays empty. The key schema is +/// the one the query reads, so a multi-attribute one stands for a GSI's. +async fn context( + key_schema: &[(&str, KeyType)], + definitions: &[(&str, ScalarAttributeType)], +) -> OperationContext { + let storage = SqliteEngine::new(":memory:", 1, "us-east-1", 409_600) + .await + .expect("an in-memory database opens"); + let key_schema: Vec = key_schema + .iter() + .map(|(name, key_type)| KeySchemaElement { + attribute_name: (*name).to_owned(), + key_type: *key_type, + }) + .collect(); + let info = TableKeyInfo { + table_name: "kvt".to_owned(), + account_id: "000000000000".to_owned(), + table_id: "kvt-id".to_owned(), + key_schema: key_schema.clone(), + base_key_schema: key_schema, + attribute_definitions: definitions + .iter() + .map(|(name, attribute_type)| AttributeDefinition { + attribute_name: (*name).to_owned(), + attribute_type: *attribute_type, + }) + .collect(), + has_lsi: false, + global_secondary_indexes: Vec::new(), + local_secondary_indexes: Vec::new(), + stream_specification: None, + vector_indexes: Vec::new(), + }; + let mut authorized_key_info = AuthorizedKeyInfo::new(); + authorized_key_info.insert("kvt".to_owned(), Some(info)); + OperationContext { + storage: Arc::new(storage), + backup: Arc::new(BackupRuntime::new(1)), + limits: Arc::new(LimitsConfig::default()), + region: Arc::from("us-east-1"), + account_id: Arc::from("000000000000"), + import_paths: Arc::from(Vec::new()), + export_paths: Arc::from(Vec::new()), + authorized_key_info, + auth_cache: extenddb_auth::AuthCacheRegistry::default(), + table_key_info_lookup: None, + item_authorizer: None, + } +} + +/// The `ValidationException` text `body` answers. +async fn refusal(ctx: &OperationContext, body: Value) -> String { + match handle_query(body, ctx).await { + Err(DynamoDbError::ValidationException(msg)) => msg, + Err(other) => panic!("unexpected error {other:?}"), + Ok(_) => panic!("the request was accepted"), + } +} + +#[tokio::test] +async fn legacy_key_conditions_value_of_another_type_is_refused() { + // `KeyConditions` with `{"N": "5"}` for the S partition key. + let ctx = context( + &[("pk", KeyType::Hash), ("sk", KeyType::Range)], + &[ + ("pk", ScalarAttributeType::S), + ("sk", ScalarAttributeType::S), + ], + ) + .await; + let body = json!({ + "TableName": "kvt", + "KeyConditions": { + "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"N": "5"}]}, + }, + }); + assert_eq!(refusal(&ctx, body).await, KEY_CONDITION_TYPE_MISMATCH); +} + +#[tokio::test] +async fn multi_attribute_value_of_another_type_is_refused_before_the_non_key_rule() { + // HASH `h1` (S) and `h2` (N), RANGE `r` (S): an S value for `h2` beside a + // clause on the non-key attribute `z` answers the type, and with an N + // value the clause on `z` answers the missing RANGE attribute. + let ctx = context( + &[ + ("h1", KeyType::Hash), + ("h2", KeyType::Hash), + ("r", KeyType::Range), + ], + &[ + ("h1", ScalarAttributeType::S), + ("h2", ScalarAttributeType::N), + ("r", ScalarAttributeType::S), + ], + ) + .await; + let query = |h2: Value| { + json!({ + "TableName": "kvt", + "KeyConditionExpression": "h1 = :a AND h2 = :b AND z = :c", + "ExpressionAttributeValues": {":a": {"S": "x"}, ":b": h2, ":c": {"S": "x"}}, + }) + }; + assert_eq!( + refusal(&ctx, query(json!({"S": "1"}))).await, + KEY_CONDITION_TYPE_MISMATCH + ); + assert_eq!( + refusal(&ctx, query(json!({"N": "1"}))).await, + "Query condition missed key schema element: r" + ); +} diff --git a/crates/engine/src/query_key_values.rs b/crates/engine/src/query_key_values.rs index 54886bf96..69fbb6ce1 100644 --- a/crates/engine/src/query_key_values.rs +++ b/crates/engine/src/query_key_values.rs @@ -4,15 +4,16 @@ //! any row is read. //! //! Measured on 2026-10-24 (tables with S/S, S/N and B/B keys, a GSI and an -//! LSI): +//! LSI) and on 2026-11-11: //! //! 1. an `N` value in `ExpressionAttributeValues` that is not a number answers //! `The parameter cannot be converted to a numeric value: ` (or the //! overflow, underflow and 38-digit messages). Every value in the map is //! checked, whether the key condition, the filter or nothing reads it. The -//! check runs at request time, after the `Limit` check and the expression -//! and non-expression parameter mix check, and before the table is looked -//! up; see [`validate_query_request_body`]; +//! check runs at request time, after the `Limit` check, the +//! `ReturnConsumedCapacity` enum check and the expression and +//! non-expression parameter mix check, and before the table is looked up; +//! see [`validate_query_request_body`]; //! 2. a key condition value whose type is not the key attribute's //! `AttributeDefinitions` type answers `One or more parameter values were //! invalid: Condition parameter type does not match schema type`, on the @@ -35,7 +36,11 @@ use extenddb_core::expression::{ }; use extenddb_core::types::{AttributeDefinition, AttributeValue, QueryInput, ScalarAttributeType}; use serde::de::DeserializeOwned; -use serde_json::Value; +use serde_json::{Map, Value}; + +mod unreadable; + +use unreadable::{readable_for_shape_check, unreadable, unreadable_value_checks}; /// The service's text for a key condition value of the wrong type. pub const KEY_CONDITION_TYPE_MISMATCH: &str = "One or more parameter values were invalid: Condition parameter type does not match schema type"; @@ -46,14 +51,38 @@ pub const LIMIT_BELOW_ONE: &str = "1 validation error detected: Value at 'Limit' /// The bare numeric message the number validator uses for an empty `N`. const EMPTY_NUMBER: &str = "The parameter cannot be converted to a numeric value"; +/// The service's texts for a value it cannot read as one type. +const NULL_VALUE: &str = "The supplied Item contains a null AttributeValue"; +const EMPTY_VALUE: &str = + "Supplied AttributeValue is empty, must contain exactly one of the supported datatypes"; +const SEVERAL_TYPES: &str = "Supplied AttributeValue has more than one datatypes set, must contain exactly one of the supported datatypes"; + +/// The type descriptors of an AttributeValue. +const TYPE_DESCRIPTORS: [&str; 10] = ["S", "N", "B", "SS", "NS", "BS", "M", "L", "NULL", "BOOL"]; + +/// The members of the two enums the request-time checks read. +const RETURN_CONSUMED_CAPACITY: &[&str] = &["INDEXES", "TOTAL", "NONE"]; +const SELECT: &[&str] = &[ + "SPECIFIC_ATTRIBUTES", + "COUNT", + "ALL_ATTRIBUTES", + "ALL_PROJECTED_ATTRIBUTES", +]; + /// The `Query` checks that run on the request body alone, before the body is /// deserialized and before the table is looked up, in the service's order: -/// `Limit`, the expression and non-expression parameter mix, the -/// `ReturnConsumedCapacity` enum, every number in `ExpressionAttributeValues`, -/// then the `Select` enum (measured 2026-10-24: a `Limit` of 0 and a parameter -/// mix are reported ahead of an invalid enum value of either field, an invalid -/// `ReturnConsumedCapacity` ahead of an `N` value that is not a number, and -/// that value ahead of an invalid `Select`). +/// `Limit`, the `ReturnConsumedCapacity` enum, the expression and +/// non-expression parameter mix, every number in `ExpressionAttributeValues`, +/// then the `Select` enum (measured 2026-10-24 and 2026-11-11: a `Limit` of 0 +/// is reported ahead of an invalid enum value of either field and of a +/// parameter mix, an invalid `ReturnConsumedCapacity` ahead of a mix with +/// `AttributesToGet`, `QueryFilter`, `ConditionalOperator` or `KeyConditions` +/// and ahead of an `N` value that is not a number, a mix ahead of that value, +/// and that value ahead of an invalid `Select`). +/// +/// A request whose `ExpressionAttributeValues` holds a value the service +/// cannot read as one type takes another order; see +/// `unreadable_value_checks`. /// /// The checks read the raw body, so a field that does not deserialize (an /// invalid `Select`, for one) does not stop the ones before it. When a check @@ -65,7 +94,11 @@ const EMPTY_NUMBER: &str = "The parameter cannot be converted to a numeric value /// an `N` value that is not a number, and a `Limit` of 0 with `Select: 5` all /// answer `SerializationException`). A `ValidationException` the deserializer /// raises (an empty set, a duplicate, an invalid `AttributeValue`) does not -/// preempt the check. A body that passes every check is not deserialized here. +/// preempt the check. In a value the service cannot read as one type, only the +/// parts it cannot read are replaced for that deserialization, so a JSON-type +/// fault in the rest of the value is still a shape fault; see +/// `readable_for_shape_check`. A body that passes every check is not +/// deserialized here. /// /// The server runs the same function ahead of the IAM decision, so a principal /// whose `dynamodb:LeadingKeys` fence cannot bind a number that does not parse @@ -78,7 +111,8 @@ const EMPTY_NUMBER: &str = "The parameter cannot be converted to a numeric value /// Returns `DynamoDbError::ValidationException` with the service's text, or /// `DynamoDbError::SerializationException` for a body of the wrong shape. pub fn validate_query_request_body(body: &Value) -> Result<(), DynamoDbError> { - query_request_checks(body).map_err(|check| shape_fault_first::(body, check)) + query_request_checks(body) + .map_err(|check| shape_fault_first::(&readable_for_shape_check(body), check)) } /// When a request-time check has failed with `check`, the deserializer's @@ -116,6 +150,15 @@ const UNKNOWN_TYPE_DESCRIPTOR: &str = "unknown AttributeValue type descriptor"; /// The `Query` checks in the service's order; see /// [`validate_query_request_body`]. fn query_request_checks(body: &Value) -> Result<(), DynamoDbError> { + let values = body + .get("ExpressionAttributeValues") + .and_then(Value::as_object); + if let Some(values) = values + && values.values().any(unreadable) + { + return unreadable_value_checks(body, values); + } + if let Some(limit) = body.get("Limit").and_then(Value::as_i64) && limit < 1 { @@ -124,6 +167,36 @@ fn query_request_checks(body: &Value) -> Result<(), DynamoDbError> { )); } + // Query stops at the first invalid enum: ReturnConsumedCapacity ahead of + // the parameter mix, Select after the numbers (measured 2026-08-24 and + // 2026-11-11). + crate::validate_enum_fields( + body, + &[crate::EnumField { + json_name: "ReturnConsumedCapacity", + valid: RETURN_CONSUMED_CAPACITY, + clause: crate::EnumClause::Named("returnConsumedCapacity"), + }], + )?; + + validate_no_parameter_mix(body)?; + + if let Some(values) = values { + validate_expression_attribute_value_numbers(values)?; + } + + crate::validate_enum_fields( + body, + &[crate::EnumField { + json_name: "Select", + valid: SELECT, + clause: crate::EnumClause::Named("select"), + }], + ) +} + +/// The expression and non-expression parameter mix check. +fn validate_no_parameter_mix(body: &Value) -> Result<(), DynamoDbError> { let kce_present = present(body, "KeyConditionExpression"); // KeyConditions combines with FilterExpression and ProjectionExpression, so // it joins the mix check only when KeyConditionExpression is also present. @@ -145,37 +218,6 @@ fn query_request_checks(body: &Value) -> Result<(), DynamoDbError> { ("FilterExpression", present(body, "FilterExpression")), ("KeyConditionExpression", kce_present), ], - )?; - - // Query stops at the first invalid enum, ReturnConsumedCapacity ahead of - // Select (measured 2026-08-24); the numbers sit between the two. - crate::validate_enum_fields( - body, - &[crate::EnumField { - json_name: "ReturnConsumedCapacity", - valid: &["INDEXES", "TOTAL", "NONE"], - clause: crate::EnumClause::Named("returnConsumedCapacity"), - }], - )?; - - if let Some(raw) = body.get("ExpressionAttributeValues") - && let Ok(values) = serde_json::from_value::>(raw.clone()) - { - validate_expression_attribute_value_numbers(&values)?; - } - - crate::validate_enum_fields( - body, - &[crate::EnumField { - json_name: "Select", - valid: &[ - "SPECIFIC_ATTRIBUTES", - "COUNT", - "ALL_ATTRIBUTES", - "ALL_PROJECTED_ATTRIBUTES", - ], - clause: crate::EnumClause::Named("select"), - }], ) } @@ -194,56 +236,139 @@ fn non_empty(body: &Value, field: &str) -> bool { } } +/// The entries of `values` by placeholder, so the reported value is +/// deterministic when several are invalid. +fn sorted_entries(values: &Map) -> Vec<(&String, &Value)> { + let mut entries: Vec<(&String, &Value)> = values.iter().collect(); + entries.sort_by_key(|(key, _)| *key); + entries +} + +/// The first fault inside one value, in document order for `L` items and in +/// key order for `M` members. +enum Fault<'a> { + /// A null, with the member name when it is the value of an `M` member. + Null(Option<&'a str>), + /// No type descriptor the service knows. + Empty, + /// More than one type descriptor. + SeveralTypes, + /// An `N` text or `NS` member that is not a number, as the request wrote it. + NotANumber(&'a str), + /// A number the number rules refuse for its magnitude or precision, with + /// their text. + Refused(String), +} + +impl Fault<'_> { + /// The request-time number check's text: the value echoed with each + /// decimal digit folded to ASCII (measured 2026-11-11: `{"N": "\u{ff11}bob"}` + /// answers `The parameter cannot be converted to a numeric value: 1bob`, + /// `\u{661}\u{662}x` answers `12x`), and the colon kept after an empty one. + fn request_time_text(self) -> String { + match self { + Fault::NotANumber(text) => format!( + "{EMPTY_NUMBER}: {}", + crate::decimal_digits::fold_decimal_digits(text) + ), + other => other.reason(), + } + } + + /// The text [`unreadable_value_checks`] wraps: a number echoed as written + /// and no colon after an empty one (measured 2026-11-11). + fn reason(self) -> String { + match self { + Fault::Null(None) => NULL_VALUE.to_owned(), + Fault::Null(Some(member)) => format!("{NULL_VALUE}: {member} is null"), + Fault::Empty => EMPTY_VALUE.to_owned(), + Fault::SeveralTypes => SEVERAL_TYPES.to_owned(), + Fault::NotANumber("") => EMPTY_NUMBER.to_owned(), + Fault::NotANumber(text) => format!("{EMPTY_NUMBER}: {text}"), + Fault::Refused(msg) => msg, + } + } +} + +/// The first fault in `value` (`member` is its name when it is the value of an +/// `M` member): a null, no type descriptor or more than one, then the numbers +/// in `N`, `NS` and every value an `L` or `M` holds. `L` items are visited in +/// document order and `M` members in key order: this build of `serde_json` +/// keeps an object's members sorted by key. A value of another JSON type, and +/// an `N` or `NS` payload of the wrong JSON type, are shape faults the +/// deserializer reports. +fn first_fault<'a>(value: &'a Value, member: Option<&'a str>) -> Option> { + let Value::Object(map) = value else { + return value.is_null().then_some(Fault::Null(member)); + }; + let mut known = map + .iter() + .filter(|(key, _)| TYPE_DESCRIPTORS.contains(&key.as_str())); + let (descriptor, payload) = match (known.next(), known.next()) { + (None, _) => return Some(Fault::Empty), + (Some(_), Some(_)) => return Some(Fault::SeveralTypes), + (Some(entry), None) => entry, + }; + match (descriptor.as_str(), payload) { + ("N", Value::String(text)) => number_fault(text), + ("NS", Value::Array(members)) => members + .iter() + .filter_map(Value::as_str) + .find_map(number_fault), + ("L", Value::Array(items)) => items.iter().find_map(|item| first_fault(item, None)), + ("M", Value::Object(members)) => members + .iter() + .find_map(|(name, inner)| first_fault(inner, Some(name))), + _ => None, + } +} + +/// `text` against the number rules, with Unicode decimal digits folded for the +/// check; see [`crate::decimal_digits`]. +fn number_fault(text: &str) -> Option> { + let folded = crate::decimal_digits::fold_decimal_digits(text); + match extenddb_core::validation::number::validate_and_normalize_number(&folded) { + Ok(_) => None, + Err(DynamoDbError::ValidationException(msg)) if msg.starts_with(EMPTY_NUMBER) => { + Some(Fault::NotANumber(text)) + } + Err(DynamoDbError::ValidationException(msg)) => Some(Fault::Refused(msg)), + Err(other) => Some(Fault::Refused(other.to_string())), + } +} + /// Refuse the first `N` value in `ExpressionAttributeValues` (including `NS` /// members and values nested in `L` and `M`) that is not a number, with the /// bare numeric message: no `ExpressionAttributeValues contains invalid value` -/// wrapper, which the service uses on `UpdateItem` and not on `Query`. The -/// placeholders are visited in sorted order so the reported value is -/// deterministic when several are invalid. An empty `N` answers the message -/// with the colon and nothing after it, as the service does on `Query`. +/// wrapper, which the service uses on `UpdateItem` and on `Query` only for a +/// value it cannot read as one type. The placeholders are visited in sorted +/// order and each value on its own, so a value of the wrong JSON shape does +/// not stop the check of another (the service picks among several invalid +/// values by an order of its own: measured 2026-11-11, `:a` and `:b` with +/// numbers that are not numbers named either over ten requests). An empty `N` +/// answers the message with the colon and nothing after it, as the service +/// does on `Query`. /// -/// A Unicode decimal digit (general category `Nd`) counts as the digit it -/// stands for, as the service reads it (measured 2026-10-25: a fullwidth or -/// Arabic-Indic digit is accepted, thirty nine Devanagari ones answer the 38 -/// digit message); see [`crate::decimal_digits`]. The message echoes the value -/// as the request wrote it. +/// A Unicode decimal digit in the Basic Multilingual Plane counts as the +/// digit it stands for, as the service reads it (measured 2026-10-25: a +/// fullwidth or Arabic-Indic digit is accepted, thirty nine Devanagari ones +/// answer the 38 digit message); see [`crate::decimal_digits`]. The message +/// echoes the value with each such digit folded to ASCII. /// /// # Errors /// /// Returns `DynamoDbError::ValidationException` with the service's text. -pub fn validate_expression_attribute_value_numbers( - values: &HashMap, +fn validate_expression_attribute_value_numbers( + values: &Map, ) -> Result<(), DynamoDbError> { - let mut keys: Vec<&String> = values.keys().collect(); - keys.sort(); - for key in keys { - validate_value_numbers(&values[key])?; - } - Ok(()) -} - -/// Every number inside one attribute value (`N`, `NS` members, and the values -/// nested in `L` and `M`), with Unicode decimal digits folded for the check. -fn validate_value_numbers(value: &AttributeValue) -> Result<(), DynamoDbError> { - match value { - AttributeValue::N(n) => validate_number_text(n), - AttributeValue::NS(set) => set.iter().try_for_each(|n| validate_number_text(n)), - AttributeValue::L(list) => list.iter().try_for_each(validate_value_numbers), - AttributeValue::M(map) => map.values().try_for_each(validate_value_numbers), - _ => Ok(()), - } -} - -/// One number text against the number rules, with the service's `Query` -/// message for an empty value and the original text echoed for a malformed one. -fn validate_number_text(text: &str) -> Result<(), DynamoDbError> { - let folded = crate::decimal_digits::fold_decimal_digits(text); - match extenddb_core::validation::number::validate_and_normalize_number(&folded) { - Ok(_) => Ok(()), - Err(DynamoDbError::ValidationException(msg)) if msg.starts_with(EMPTY_NUMBER) => Err( - DynamoDbError::ValidationException(format!("{EMPTY_NUMBER}: {text}")), - ), - Err(other) => Err(other), + match sorted_entries(values) + .into_iter() + .find_map(|(_, value)| first_fault(value, None)) + { + Some(fault) => Err(DynamoDbError::ValidationException( + fault.request_time_text(), + )), + None => Ok(()), } } @@ -341,3 +466,9 @@ fn resolve_attr_name(path: &[PathElement], names: &HashMap) -> O #[cfg(test)] mod tests; + +#[cfg(test)] +mod request_order_tests; + +#[cfg(test)] +mod unreadable_tests; diff --git a/crates/engine/src/query_key_values/request_order_tests.rs b/crates/engine/src/query_key_values/request_order_tests.rs new file mode 100644 index 000000000..c83d769ae --- /dev/null +++ b/crates/engine/src/query_key_values/request_order_tests.rs @@ -0,0 +1,279 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! Tests for the order of the request-time `Query` checks, and for a shape +//! fault reported ahead of them. + +use extenddb_core::error::DynamoDbError; +use serde_json::json; + +use super::tests::message; +use super::{LIMIT_BELOW_ONE, validate_query_request_body}; + +#[test] +fn request_time_checks_run_limit_then_mix_then_numbers() { + let bad_number = serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"N": "bob"}}, + }); + + let mut with_limit = bad_number.clone(); + with_limit["Limit"] = serde_json::json!(0); + assert_eq!( + message(validate_query_request_body(&with_limit)).as_deref(), + Some(LIMIT_BELOW_ONE) + ); + + let mut with_mix = bad_number.clone(); + with_mix["KeyConditions"] = serde_json::json!({ + "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]} + }); + assert_eq!( + message(validate_query_request_body(&with_mix)).as_deref(), + Some( + "Can not use both expression and non-expression parameters in the same request: \ + Non-expression parameters: {KeyConditions} Expression parameters: {KeyConditionExpression}" + ) + ); + + assert_eq!( + message(validate_query_request_body(&bad_number)).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob") + ); + + // A number only a FilterExpression reads, and one nothing reads. + for body in [ + serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :p", + "FilterExpression": "a = :n", + "ExpressionAttributeValues": {":p": {"S": "x"}, ":n": {"N": "bob"}}, + }), + serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": {":p": {"S": "x"}, ":n": {"N": "bob"}}, + }), + ] { + assert_eq!( + message(validate_query_request_body(&body)).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob") + ); + } + + // Legacy KeyConditions alone does not join the mix check; a well-formed + // request passes. + let legacy = serde_json::json!({ + "TableName": "t", + "KeyConditions": { + "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]} + }, + "FilterExpression": "a = :n", + "ExpressionAttributeValues": {":n": {"N": "5"}}, + "Limit": 1, + }); + assert!(validate_query_request_body(&legacy).is_ok()); +} + +#[test] +fn enum_checks_sit_on_either_side_of_the_numbers() { + // Measured 2026-10-24 and 2026-11-11: an invalid ReturnConsumedCapacity is + // reported ahead of an N value that is not a number, and that value ahead + // of an invalid Select; a Limit of 0 ahead of either enum. + let bad_number = serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"N": "bob"}}, + }); + let mut rcc = bad_number.clone(); + rcc["ReturnConsumedCapacity"] = serde_json::json!("bad"); + assert_eq!( + message(validate_query_request_body(&rcc)).as_deref(), + Some( + "1 validation error detected: Value 'bad' at 'returnConsumedCapacity' failed to \ + satisfy constraint: Member must satisfy enum value set: [INDEXES, TOTAL, NONE]" + ) + ); + let mut select = bad_number.clone(); + select["Select"] = serde_json::json!("bad"); + assert_eq!( + message(validate_query_request_body(&select)).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob") + ); + for field in ["ReturnConsumedCapacity", "Select"] { + let mut limit = bad_number.clone(); + limit[field] = serde_json::json!("bad"); + limit["Limit"] = serde_json::json!(0); + assert_eq!( + message(validate_query_request_body(&limit)).as_deref(), + Some(LIMIT_BELOW_ONE), + "{field}" + ); + } + // With the numbers valid, an invalid Select is still reported. + let mut only_select = serde_json::json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"S": "bob"}}, + }); + only_select["Select"] = serde_json::json!("bad"); + assert!( + message(validate_query_request_body(&only_select)) + .is_some_and(|m| m.contains("at 'select' failed to satisfy constraint")) + ); +} + +const RCC_MSG: &str = "1 validation error detected: Value 'bad' at 'returnConsumedCapacity' \ + failed to satisfy constraint: Member must satisfy enum value set: \ + [INDEXES, TOTAL, NONE]"; + +#[test] +fn return_consumed_capacity_is_reported_ahead_of_the_parameter_mix() { + // Measured 2026-11-11 with each non-expression parameter beside + // KeyConditionExpression: an invalid ReturnConsumedCapacity is reported + // ahead of the mix, and the mix ahead of an N value that is not a number + // and of an invalid Select. + for (name, value) in [ + ("AttributesToGet", json!(["a"])), + ( + "QueryFilter", + json!({"a": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]}}), + ), + ("ConditionalOperator", json!("AND")), + ( + "KeyConditions", + json!({"pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]}}), + ), + ] { + let mut body = json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"N": "bob"}}, + "Select": "bad", + }); + body[name] = value; + let mix = format!( + "Can not use both expression and non-expression parameters in the same request: \ + Non-expression parameters: {{{name}}} Expression parameters: \ + {{KeyConditionExpression}}" + ); + assert_eq!( + message(validate_query_request_body(&body)).as_deref(), + Some(mix.as_str()), + "{name}" + ); + body["ReturnConsumedCapacity"] = json!("bad"); + assert_eq!( + message(validate_query_request_body(&body)).as_deref(), + Some(RCC_MSG), + "{name}" + ); + body["Limit"] = json!(0); + assert_eq!( + message(validate_query_request_body(&body)).as_deref(), + Some(LIMIT_BELOW_ONE), + "{name}" + ); + } +} + +#[test] +fn a_shape_fault_in_another_field_is_reported_ahead_of_the_checks() { + // Measured 2026-10-25: a field of the wrong JSON type + // anywhere in the body answers SerializationException even when a Limit, + // mix, enum or number check would fail. + let shape_faults = [ + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "ExpressionAttributeValues": "x", + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "ExpressionAttributeNames": "x", + "ExpressionAttributeValues": {":p": {"N": "bob"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": "1", "ExpressionAttributeValues": {":p": {"N": "bob"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "AttributesToGet": {"a": 1}, "ProjectionExpression": "a", + "ExpressionAttributeValues": {":p": {"S": "x"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "QueryFilter": [1], "FilterExpression": "a = :p", + "ExpressionAttributeValues": {":p": {"S": "x"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Select": 5, "ExpressionAttributeValues": {":p": {"N": "bob"}}, + }), + serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "Select": 5, "ExpressionAttributeValues": {":p": {"S": "x"}}, + }), + ]; + for body in shape_faults { + assert!( + matches!( + validate_query_request_body(&body), + Err(DynamoDbError::SerializationException(_)) + ), + "{body}" + ); + } + + // A string enum value that is not a member is the checks' own answer, not a + // shape fault. + let mut select = serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}}, + }); + select["Select"] = serde_json::json!("bad"); + assert_eq!( + message(validate_query_request_body(&select)).as_deref(), + Some(LIMIT_BELOW_ONE) + ); + select["ReturnConsumedCapacity"] = serde_json::json!("bad"); + select["Limit"] = serde_json::json!(1); + assert!( + message(validate_query_request_body(&select)) + .is_some_and(|m| m.contains("at 'returnConsumedCapacity' failed to satisfy constraint")) + ); + + // A ValidationException the deserializer raises does not preempt a check: + // an empty NS beside a Limit of 0 answers the Limit text. + let empty_set = serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}, ":s": {"NS": []}}, + }); + assert_eq!( + message(validate_query_request_body(&empty_set)).as_deref(), + Some(LIMIT_BELOW_ONE) + ); + + // Neither does an unknown type descriptor, which the deserializer reports + // as a SerializationException: the service answers the Limit + // ValidationException in the form that names the value (measured + // 2026-10-25 and 2026-11-11). + let unknown_type = serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}, ":a": {"X": "1"}}, + }); + assert_eq!( + message(validate_query_request_body(&unknown_type)).as_deref(), + Some( + "1 validation error detected: Value '0' at 'limit' failed to satisfy constraint: \ + Member must have value greater than or equal to 1" + ) + ); + + // A body that passes every check is left to the handler's deserializer. + let shape_fault_only = serde_json::json!({ + "TableName": "t", "KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": "x", + }); + assert!(validate_query_request_body(&shape_fault_only).is_ok()); +} diff --git a/crates/engine/src/query_key_values/tests.rs b/crates/engine/src/query_key_values/tests.rs index 68c353943..2a2bce4b4 100644 --- a/crates/engine/src/query_key_values/tests.rs +++ b/crates/engine/src/query_key_values/tests.rs @@ -7,12 +7,23 @@ use extenddb_core::expression::{ ExpressionMaps, KeyCondition, parse_key_condition, tokenize_with_limit, }; use extenddb_core::types::{AttributeDefinition, AttributeValue, ScalarAttributeType}; +use serde_json::{Value, json}; use super::{ - KEY_CONDITION_TYPE_MISMATCH, LIMIT_BELOW_ONE, validate_expression_attribute_value_numbers, - validate_key_condition_types, validate_query_request_body, + KEY_CONDITION_TYPE_MISMATCH, validate_key_condition_types, validate_query_request_body, }; +/// The request-time number check on typed values, serialized as a request +/// carries them. +fn validate_expression_attribute_value_numbers( + values: &HashMap, +) -> Result<(), DynamoDbError> { + let Value::Object(raw) = serde_json::to_value(values).expect("the values serialize") else { + panic!("a map serializes to an object"); + }; + super::validate_expression_attribute_value_numbers(&raw) +} + fn defs(pairs: &[(&str, ScalarAttributeType)]) -> Vec { pairs .iter() @@ -49,7 +60,7 @@ fn n(v: &str) -> AttributeValue { AttributeValue::N(v.to_owned()) } -fn message(result: Result<(), DynamoDbError>) -> Option { +pub(super) fn message(result: Result<(), DynamoDbError>) -> Option { match result { Ok(()) => None, Err(DynamoDbError::ValidationException(msg)) => Some(msg), @@ -324,120 +335,6 @@ fn first_invalid_number_in_placeholder_order_is_reported() { ); } -#[test] -fn request_time_checks_run_limit_then_mix_then_numbers() { - let bad_number = serde_json::json!({ - "TableName": "t", - "KeyConditionExpression": "pk = :u", - "ExpressionAttributeValues": {":u": {"N": "bob"}}, - }); - - let mut with_limit = bad_number.clone(); - with_limit["Limit"] = serde_json::json!(0); - assert_eq!( - message(validate_query_request_body(&with_limit)).as_deref(), - Some(LIMIT_BELOW_ONE) - ); - - let mut with_mix = bad_number.clone(); - with_mix["KeyConditions"] = serde_json::json!({ - "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]} - }); - assert_eq!( - message(validate_query_request_body(&with_mix)).as_deref(), - Some( - "Can not use both expression and non-expression parameters in the same request: \ - Non-expression parameters: {KeyConditions} Expression parameters: {KeyConditionExpression}" - ) - ); - - assert_eq!( - message(validate_query_request_body(&bad_number)).as_deref(), - Some("The parameter cannot be converted to a numeric value: bob") - ); - - // A number only a FilterExpression reads, and one nothing reads. - for body in [ - serde_json::json!({ - "TableName": "t", - "KeyConditionExpression": "pk = :p", - "FilterExpression": "a = :n", - "ExpressionAttributeValues": {":p": {"S": "x"}, ":n": {"N": "bob"}}, - }), - serde_json::json!({ - "TableName": "t", - "KeyConditionExpression": "pk = :p", - "ExpressionAttributeValues": {":p": {"S": "x"}, ":n": {"N": "bob"}}, - }), - ] { - assert_eq!( - message(validate_query_request_body(&body)).as_deref(), - Some("The parameter cannot be converted to a numeric value: bob") - ); - } - - // Legacy KeyConditions alone does not join the mix check; a well-formed - // request passes. - let legacy = serde_json::json!({ - "TableName": "t", - "KeyConditions": { - "pk": {"ComparisonOperator": "EQ", "AttributeValueList": [{"S": "x"}]} - }, - "FilterExpression": "a = :n", - "ExpressionAttributeValues": {":n": {"N": "5"}}, - "Limit": 1, - }); - assert!(validate_query_request_body(&legacy).is_ok()); -} - -#[test] -fn enum_checks_sit_on_either_side_of_the_numbers() { - // Measured 2026-10-24: an invalid ReturnConsumedCapacity is reported ahead - // of an N value that is not a number, and that value ahead of an invalid - // Select; a Limit of 0 and a parameter mix ahead of either enum. - let bad_number = serde_json::json!({ - "TableName": "t", - "KeyConditionExpression": "pk = :u", - "ExpressionAttributeValues": {":u": {"N": "bob"}}, - }); - let mut rcc = bad_number.clone(); - rcc["ReturnConsumedCapacity"] = serde_json::json!("bad"); - assert_eq!( - message(validate_query_request_body(&rcc)).as_deref(), - Some( - "1 validation error detected: Value 'bad' at 'returnConsumedCapacity' failed to \ - satisfy constraint: Member must satisfy enum value set: [INDEXES, TOTAL, NONE]" - ) - ); - let mut select = bad_number.clone(); - select["Select"] = serde_json::json!("bad"); - assert_eq!( - message(validate_query_request_body(&select)).as_deref(), - Some("The parameter cannot be converted to a numeric value: bob") - ); - for field in ["ReturnConsumedCapacity", "Select"] { - let mut limit = bad_number.clone(); - limit[field] = serde_json::json!("bad"); - limit["Limit"] = serde_json::json!(0); - assert_eq!( - message(validate_query_request_body(&limit)).as_deref(), - Some(LIMIT_BELOW_ONE), - "{field}" - ); - } - // With the numbers valid, an invalid Select is still reported. - let mut only_select = serde_json::json!({ - "TableName": "t", - "KeyConditionExpression": "pk = :u", - "ExpressionAttributeValues": {":u": {"S": "bob"}}, - }); - only_select["Select"] = serde_json::json!("bad"); - assert!( - message(validate_query_request_body(&only_select)) - .is_some_and(|m| m.contains("at 'select' failed to satisfy constraint")) - ); -} - #[test] fn unicode_decimal_digits_count_as_the_digits_they_stand_for() { // Measured 2026-10-25: a fullwidth, Arabic-Indic or Devanagari digit is accepted as a @@ -475,7 +372,7 @@ fn characters_that_are_not_decimal_digits_are_refused_with_the_text_as_sent() { // Measured 2026-10-25: a superscript, // a vulgar fraction, a fullwidth minus and a fullwidth full stop are refused // and the message echoes the value as the request wrote it. - for raw in ["\u{b2}", "\u{bd}", "\u{ff0d}1", "1\u{ff0e}5", "\u{ff11}bob"] { + for raw in ["\u{b2}", "\u{bd}", "\u{ff0d}1", "1\u{ff0e}5"] { let values = HashMap::from([(":u".to_owned(), n(raw))]); assert_eq!( message(validate_expression_attribute_value_numbers(&values)).as_deref(), @@ -486,99 +383,93 @@ fn characters_that_are_not_decimal_digits_are_refused_with_the_text_as_sent() { } #[test] -fn a_shape_fault_in_another_field_is_reported_ahead_of_the_checks() { - // Measured 2026-10-25: a field of the wrong JSON type - // anywhere in the body answers SerializationException even when a Limit, - // mix, enum or number check would fail. - let shape_faults = [ - serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "Limit": 0, "ExpressionAttributeValues": "x", - }), - serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "ExpressionAttributeNames": "x", - "ExpressionAttributeValues": {":p": {"N": "bob"}}, - }), - serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "Limit": "1", "ExpressionAttributeValues": {":p": {"N": "bob"}}, - }), - serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "AttributesToGet": {"a": 1}, "ProjectionExpression": "a", - "ExpressionAttributeValues": {":p": {"S": "x"}}, - }), - serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "QueryFilter": [1], "FilterExpression": "a = :p", - "ExpressionAttributeValues": {":p": {"S": "x"}}, - }), - serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "Select": 5, "ExpressionAttributeValues": {":p": {"N": "bob"}}, - }), - serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "Limit": 0, "Select": 5, "ExpressionAttributeValues": {":p": {"S": "x"}}, - }), - ]; - for body in shape_faults { - assert!( - matches!( - validate_query_request_body(&body), - Err(DynamoDbError::SerializationException(_)) - ), - "{body}" +fn a_refused_value_is_echoed_with_each_decimal_digit_folded_to_ascii() { + // Measured 2026-11-11: the service folds every decimal digit below U+10000 + // in the echoed value, and a digit from U+104A0 up is refused and echoed + // as sent. + for (raw, echo) in [ + ("\u{ff11}bob", "1bob"), + ("\u{661}\u{662}x", "12x"), + ("bob\u{ff11}", "bob1"), + ("\u{967}\u{968}a\u{969}", "12a3"), + ("\u{ff11}\u{1d7cf}", "1\u{1d7cf}"), + ("\u{1d7cf}", "\u{1d7cf}"), + ("\u{104a1}", "\u{104a1}"), + ] { + let values = HashMap::from([(":u".to_owned(), n(raw))]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some(format!("The parameter cannot be converted to a numeric value: {echo}").as_str()), + "{raw:?}" ); } +} - // A string enum value that is not a member is the checks' own answer, not a - // shape fault. - let mut select = serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}}, - }); - select["Select"] = serde_json::json!("bad"); - assert_eq!( - message(validate_query_request_body(&select)).as_deref(), - Some(LIMIT_BELOW_ONE) - ); - select["ReturnConsumedCapacity"] = serde_json::json!("bad"); - select["Limit"] = serde_json::json!(1); - assert!( - message(validate_query_request_body(&select)) - .is_some_and(|m| m.contains("at 'returnConsumedCapacity' failed to satisfy constraint")) - ); +#[test] +fn decimal_digits_from_u_104a0_up_are_not_numbers() { + // Measured 2026-11-11: digit one of each of the 31 decimal digit runs from + // U+104A0 up is refused as a filter value, and digit one of the last run + // below U+10000 is accepted. + for raw in [ + "\u{104a1}", + "\u{11067}", + "\u{1d7cf}", + "\u{1e951}", + "\u{1fbf1}", + ] { + let values = HashMap::from([(":u".to_owned(), n(raw))]); + assert_eq!( + message(validate_expression_attribute_value_numbers(&values)).as_deref(), + Some(format!("The parameter cannot be converted to a numeric value: {raw}").as_str()), + "{raw:?}" + ); + } + let values = HashMap::from([(":u".to_owned(), n("\u{abf1}"))]); + assert!(validate_expression_attribute_value_numbers(&values).is_ok()); +} - // A ValidationException the deserializer raises does not preempt a check: - // an empty NS beside a Limit of 0 answers the Limit text. - let empty_set = serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}, ":s": {"NS": []}}, +/// A Query body with `values` as its `ExpressionAttributeValues` and `extra` +/// beside them. +pub(super) fn body_with(values: Value, extra: Value) -> Value { + let mut body = json!({ + "TableName": "t", + "KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": values, }); - assert_eq!( - message(validate_query_request_body(&empty_set)).as_deref(), - Some(LIMIT_BELOW_ONE) - ); + if let (Value::Object(body), Value::Object(extra)) = (&mut body, extra) { + body.extend(extra); + } + body +} - // Neither does an unknown type descriptor, which the deserializer reports - // as a SerializationException: the service answers the Limit - // ValidationException (measured 2026-10-25), in its older - // `Value '0' at 'limit'` form; ExtendDB answers its usual Limit text. - let unknown_type = serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "Limit": 0, "ExpressionAttributeValues": {":p": {"S": "x"}, ":a": {"X": "1"}}, - }); - assert_eq!( - message(validate_query_request_body(&unknown_type)).as_deref(), - Some(LIMIT_BELOW_ONE) +#[test] +fn a_value_that_does_not_deserialize_does_not_stop_the_number_check_of_another() { + // Measured 2026-11-11: beside `:u = {"N": "bob"}`, an empty set, a + // duplicate set member or a NULL of false answered either that value's + // fault or the numeric fault over ten requests each, and a B that is not + // base64 answered SerializationException. + for other in [ + json!({"NS": []}), + json!({"SS": ["a", "a"]}), + json!({"NULL": false}), + json!({"L": [{"NS": []}]}), + ] { + let body = body_with( + json!({":p": {"S": "x"}, ":u": {"N": "bob"}, ":z": other}), + json!({}), + ); + assert_eq!( + message(validate_query_request_body(&body)).as_deref(), + Some("The parameter cannot be converted to a numeric value: bob"), + "{other}" + ); + } + let body = body_with( + json!({":p": {"S": "x"}, ":u": {"N": "bob"}, ":z": {"B": "!!"}}), + json!({}), ); - - // A body that passes every check is left to the handler's deserializer. - let shape_fault_only = serde_json::json!({ - "TableName": "t", "KeyConditionExpression": "pk = :p", - "ExpressionAttributeValues": "x", - }); - assert!(validate_query_request_body(&shape_fault_only).is_ok()); + assert!(matches!( + validate_query_request_body(&body), + Err(DynamoDbError::SerializationException(_)) + )); } diff --git a/crates/engine/src/query_key_values/unreadable.rs b/crates/engine/src/query_key_values/unreadable.rs new file mode 100644 index 000000000..cd6ec2e9c --- /dev/null +++ b/crates/engine/src/query_key_values/unreadable.rs @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! `ExpressionAttributeValues` entries the service cannot read as one type: a +//! null, no type descriptor it knows, more than one, or one it does not know +//! beside one it does, in the value or in any value an `L` or `M` holds. A +//! `Query` with such an entry takes its own order of checks (see +//! [`unreadable_value_checks`]), and the shape fault check reads the entry with +//! those parts replaced (see [`readable_for_shape_check`]). + +use std::borrow::Cow; + +use extenddb_core::error::DynamoDbError; +use serde_json::{Map, Value}; + +use super::{ + RETURN_CONSUMED_CAPACITY, SELECT, TYPE_DESCRIPTORS, first_fault, sorted_entries, + validate_no_parameter_mix, +}; + +/// Whether the service cannot read `value` as exactly one AttributeValue type: +/// a null, no type descriptor it knows, more than one, or one it does not know +/// beside one it does, in the value or in any value an `L` or `M` holds. A +/// value of another JSON type is a shape fault instead. +pub(super) fn unreadable(value: &Value) -> bool { + match value { + Value::Null => true, + Value::Object(map) => { + let mut known = map + .iter() + .filter(|(key, _)| TYPE_DESCRIPTORS.contains(&key.as_str())); + match (known.next(), known.next()) { + (Some((key, inner)), None) => { + map.len() > 1 + || match (key.as_str(), inner) { + ("L", Value::Array(items)) => items.iter().any(unreadable), + ("M", Value::Object(members)) => members.values().any(unreadable), + _ => false, + } + } + _ => true, + } + } + _ => false, + } +} + +/// `body` for the shape fault check. When its `ExpressionAttributeValues` +/// holds a value the service cannot read as one type, each part the service +/// cannot read is replaced (see [`make_readable`]) and the rest of the value +/// stays, so the deserializer still reports a JSON-type fault in it. Measured +/// 2026-11-11: `{"L": [null, "x"]}`, `{"L": [{"X": "1"}, {"N": 5}]}` and +/// `{"L": [null, {"B": "!!"}]}`, and `{"S": 5, "X": "1"}` and `{"S": 5, "N": +/// "1"}` beside a `Limit` of 0, all answer `SerializationException`. +pub(super) fn readable_for_shape_check(body: &Value) -> Cow<'_, Value> { + match body + .get("ExpressionAttributeValues") + .and_then(Value::as_object) + { + Some(values) if values.values().any(unreadable) => { + let mut body = body.clone(); + if let Some(Value::Object(values)) = body.get_mut("ExpressionAttributeValues") { + values.values_mut().for_each(make_readable); + } + Cow::Owned(body) + } + _ => Cow::Borrowed(body), + } +} + +/// Replace each part of `value` the service cannot read as one type, in the +/// value and in the values an `L` or `M` holds. A null, or an object with no +/// type descriptor the service knows, becomes `{"NULL": true}`. An unknown +/// descriptor beside a known one is dropped. An object with several known +/// descriptors becomes an `L` of one value per descriptor, so the deserializer +/// still reads each payload (measured 2026-11-11: `{"L": [{"S": "a", "N": +/// "1"}, "x"]}` and `{"S": "a", "N": 1}` answer `SerializationException`). A +/// value of another JSON type is left for the deserializer to report. +fn make_readable(value: &mut Value) { + let replacement = match value { + Value::Null => Some(null_value()), + Value::Object(map) => { + map.retain(|key, _| TYPE_DESCRIPTORS.contains(&key.as_str())); + if map.len() > 1 { + let mut parts: Vec = std::mem::take(map) + .into_iter() + .map(|entry| Value::Object(Map::from_iter([entry]))) + .collect(); + parts.iter_mut().for_each(make_readable); + Some(Value::Object(Map::from_iter([( + "L".to_owned(), + Value::Array(parts), + )]))) + } else if let Some((key, payload)) = map.iter_mut().next() { + match (key.as_str(), payload) { + ("L", Value::Array(items)) => items.iter_mut().for_each(make_readable), + ("M", Value::Object(members)) => members.values_mut().for_each(make_readable), + _ => {} + } + None + } else { + Some(null_value()) + } + } + _ => None, + }; + if let Some(replacement) = replacement { + *value = replacement; + } +} + +/// `{"NULL": true}`, a value the deserializer reads. +fn null_value() -> Value { + Value::Object(Map::from_iter([("NULL".to_owned(), Value::Bool(true))])) +} + +/// The checks for a request whose `ExpressionAttributeValues` holds a value +/// the service cannot read as one type. Measured 2026-11-11, the service then +/// answers in this order: +/// +/// 1. an invalid `Select`, an invalid `ReturnConsumedCapacity` and a `Limit` +/// below 1, reported together in that order, with the `Limit` text naming +/// the value: `2 validation errors detected: Value 'bad' at +/// 'returnConsumedCapacity' failed to satisfy constraint: ...; Value '0' at +/// 'limit' failed to satisfy constraint: Member must have value greater +/// than or equal to 1`; +/// 2. the expression and non-expression parameter mix; +/// 3. the first fault inside a value, in document order for `L` items and in +/// key order for `M` members, as `ExpressionAttributeValues contains +/// invalid value: for key `: a null (`The supplied +/// Item contains a null AttributeValue`, with `: is null` when it +/// is the value of an `M` member), no type descriptor, more than one, or an +/// `N` that is not a number. Such an `N` is echoed as written, and an empty +/// one has no colon after the message. No `M` with more than one member was +/// measured. +/// +/// When several values have a fault, the service names a value it cannot read +/// as one type ahead of a number fault in another value. `:u` with `{"N": +/// "bob"}` beside `:z` with `{"X": "1"}` names `:z` in 28 of 28 requests, and +/// twelve other shapes of `:z` beside that `:u` name `:z` too. Five shapes of +/// `:a` beside that `:u` name `:a`. So the values the service cannot read are +/// visited first, then the others, each group in placeholder order. Three +/// measured requests name another value than this order does. `:a` and `:b` +/// with numbers that are not numbers beside `:z` with `{"X": "1"}` name `:b`. +/// `:a` with `{"X": "1"}` beside `:b` with a null names `:b`, and `:a` with a +/// null beside `:b` with `{"X": "1"}` names `:b`. This order names `:z`, `:a` +/// and `:a` for them. +pub(super) fn unreadable_value_checks( + body: &Value, + values: &Map, +) -> Result<(), DynamoDbError> { + let mut violations: Vec = [ + enum_violation(body, "Select", "select", SELECT), + enum_violation( + body, + "ReturnConsumedCapacity", + "returnConsumedCapacity", + RETURN_CONSUMED_CAPACITY, + ), + ] + .into_iter() + .flatten() + .collect(); + if let Some(limit) = body.get("Limit").and_then(Value::as_i64) + && limit < 1 + { + violations.push(format!( + "Value '{limit}' at 'limit' failed to satisfy constraint: Member must have value \ + greater than or equal to 1" + )); + } + if !violations.is_empty() { + let count = violations.len(); + return Err(DynamoDbError::ValidationException(format!( + "{count} validation error{} detected: {}", + if count == 1 { "" } else { "s" }, + violations.join("; ") + ))); + } + + validate_no_parameter_mix(body)?; + + // The values the service cannot read first; the sort is stable, so each + // group keeps the placeholder order. + let mut entries = sorted_entries(values); + entries.sort_by_key(|(_, value)| !unreadable(value)); + for (key, value) in entries { + if let Some(fault) = first_fault(value, None) { + return Err(DynamoDbError::ValidationException(format!( + "ExpressionAttributeValues contains invalid value: {} for key {key}", + fault.reason() + ))); + } + } + Ok(()) +} + +/// The enum violation clause for `field` when the body holds a string that is +/// not one of `valid`. +fn enum_violation(body: &Value, field: &str, api_name: &str, valid: &[&str]) -> Option { + let value = body.get(field)?.as_str()?; + (!valid.contains(&value)).then(|| { + format!( + "Value '{value}' at '{api_name}' failed to satisfy constraint: Member must satisfy \ + enum value set: [{}]", + valid.join(", ") + ) + }) +} diff --git a/crates/engine/src/query_key_values/unreadable_tests.rs b/crates/engine/src/query_key_values/unreadable_tests.rs new file mode 100644 index 000000000..33f227062 --- /dev/null +++ b/crates/engine/src/query_key_values/unreadable_tests.rs @@ -0,0 +1,301 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! Tests for a `Query` whose `ExpressionAttributeValues` holds a value the +//! service cannot read as one type. + +use extenddb_core::error::DynamoDbError; +use serde_json::{Value, json}; + +use super::tests::{body_with, message}; +use super::validate_query_request_body; + +const INVALID_VALUE: &str = "ExpressionAttributeValues contains invalid value: "; +const EMPTY: &str = + "Supplied AttributeValue is empty, must contain exactly one of the supported datatypes"; + +#[test] +fn a_value_the_service_cannot_read_as_one_type_is_named_with_its_placeholder() { + // Measured 2026-11-11, each value alone beside `:p = {"S": "x"}`. + let several = "Supplied AttributeValue has more than one datatypes set, must contain \ + exactly one of the supported datatypes"; + let null = "The supplied Item contains a null AttributeValue"; + for (value, reason) in [ + (json!({"X": "1"}), EMPTY.to_owned()), + (Value::Null, null.to_owned()), + (json!({}), EMPTY.to_owned()), + (json!({"S": "a", "N": "1"}), several.to_owned()), + (json!({"L": [{"X": "1"}]}), EMPTY.to_owned()), + (json!({"L": [null]}), null.to_owned()), + (json!({"L": [{}]}), EMPTY.to_owned()), + (json!({"L": [{"S": "a", "N": "1"}]}), several.to_owned()), + (json!({"L": [{"S": "a"}, {"X": "1"}]}), EMPTY.to_owned()), + (json!({"M": {"k": {"X": "1"}}}), EMPTY.to_owned()), + (json!({"M": {"k": {}}}), EMPTY.to_owned()), + (json!({"M": {"k": null}}), format!("{null}: k is null")), + ( + json!({"M": {"k": {"M": {"j": null}}}}), + format!("{null}: j is null"), + ), + (json!({"M": {"k": {"L": [null]}}}), null.to_owned()), + ( + json!({"L": [{"M": {"k": null}}]}), + format!("{null}: k is null"), + ), + // The first fault in document order. + ( + json!({"L": [{"N": "bob"}, {"X": "1"}]}), + "The parameter cannot be converted to a numeric value: bob".to_owned(), + ), + (json!({"L": [{"X": "1"}, {"N": "bob"}]}), EMPTY.to_owned()), + ] { + let body = body_with(json!({":p": {"S": "x"}, ":z": value}), json!({})); + assert_eq!( + message(validate_query_request_body(&body)), + Some(format!("{INVALID_VALUE}{reason} for key :z")), + "{value}" + ); + } +} + +#[test] +fn numbers_beside_a_value_the_service_cannot_read_are_wrapped_and_echoed_as_sent() { + // Measured 2026-11-11 beside `:a = {"S": "a", "X": "1"}`, which the service + // reads as an S: the numeric texts are wrapped with the placeholder, the + // value is echoed without folding its digits, an empty N has no colon, and + // a decimal digit below U+10000 still counts as a digit. + let numeric = "The parameter cannot be converted to a numeric value"; + for (value, reason) in [ + (json!({"N": "bob"}), format!("{numeric}: bob")), + (json!({"N": ""}), numeric.to_owned()), + (json!({"NS": ["1", "bob"]}), format!("{numeric}: bob")), + (json!({"L": [{"N": "bob"}]}), format!("{numeric}: bob")), + ( + json!({"N": "\u{ff11}bob"}), + format!("{numeric}: \u{ff11}bob"), + ), + (json!({"N": "\u{1d7cf}"}), format!("{numeric}: \u{1d7cf}")), + ( + json!({"N": "1e200"}), + "Number overflow. Attempting to store a number with magnitude larger than \ + supported range" + .to_owned(), + ), + ( + json!({"N": "1".repeat(39)}), + "Attempting to store more than 38 significant digits in a Number".to_owned(), + ), + ] { + let body = body_with( + json!({":p": {"S": "x"}, ":a": {"S": "a", "X": "1"}, ":u": value}), + json!({}), + ); + assert_eq!( + message(validate_query_request_body(&body)), + Some(format!("{INVALID_VALUE}{reason} for key :u")), + "{value}" + ); + } + let body = body_with( + json!({":p": {"S": "x"}, ":a": {"S": "a", "X": "1"}, ":u": {"N": "\u{ff11}"}}), + json!({}), + ); + assert!(validate_query_request_body(&body).is_ok()); +} + +#[test] +fn a_value_the_service_cannot_read_is_named_ahead_of_a_number_fault() { + // Measured 2026-11-11: beside `:u = {"N": "bob"}`, the service names `:z` + // or `:a` when it holds a value it cannot read as one type (28 of 28 + // requests for `:z = {"X": "1"}`), and names `:u` when the other value has + // no fault of its own. + let several = "Supplied AttributeValue has more than one datatypes set, must contain \ + exactly one of the supported datatypes"; + let null = "The supplied Item contains a null AttributeValue"; + let numeric = "The parameter cannot be converted to a numeric value: bob"; + for (value, reason) in [ + (json!({"X": "1"}), EMPTY.to_owned()), + (Value::Null, null.to_owned()), + (json!({}), EMPTY.to_owned()), + (json!({"S": "a", "N": "1"}), several.to_owned()), + (json!({"L": [{"X": "1"}]}), EMPTY.to_owned()), + (json!({"L": [null]}), null.to_owned()), + (json!({"L": [{}]}), EMPTY.to_owned()), + (json!({"L": [{"S": "a", "N": "1"}]}), several.to_owned()), + (json!({"M": {"k": {"X": "1"}}}), EMPTY.to_owned()), + (json!({"M": {"k": null}}), format!("{null}: k is null")), + (json!({"L": [{"S": "a"}, {"X": "1"}]}), EMPTY.to_owned()), + (json!({"L": [{"N": "bob"}, {"X": "1"}]}), numeric.to_owned()), + (json!({"L": [{"X": "1"}, {"N": "bob"}]}), EMPTY.to_owned()), + ] { + let body = body_with( + json!({":p": {"S": "x"}, ":u": {"N": "bob"}, ":z": value}), + json!({}), + ); + assert_eq!( + message(validate_query_request_body(&body)), + Some(format!("{INVALID_VALUE}{reason} for key :z")), + "{value}" + ); + } + for (value, reason) in [ + (json!({"X": "1"}), EMPTY), + (Value::Null, null), + (json!({}), EMPTY), + (json!({"S": "a", "N": "1"}), several), + (json!({"L": [{"X": "1"}]}), EMPTY), + ] { + let body = body_with( + json!({":p": {"S": "x"}, ":a": value, ":u": {"N": "bob"}}), + json!({}), + ); + assert_eq!( + message(validate_query_request_body(&body)), + Some(format!("{INVALID_VALUE}{reason} for key :a")), + "{value}" + ); + } + for values in [ + json!({":p": {"S": "x"}, ":u": {"N": "bob"}, ":z": {"S": "a", "X": "1"}}), + json!({":p": {"S": "x"}, ":a": {"S": "a", "X": "1"}, ":u": {"N": "bob"}}), + ] { + let body = body_with(values.clone(), json!({})); + assert_eq!( + message(validate_query_request_body(&body)), + Some(format!("{INVALID_VALUE}{numeric} for key :u")), + "{values}" + ); + } +} + +#[test] +fn beside_a_value_the_service_cannot_read_the_enums_and_limit_are_reported_together() { + // Measured 2026-11-11 beside `:z = {"X": "1"}`: Select, then + // ReturnConsumedCapacity, then Limit, in one message, ahead of the + // parameter mix; the mix ahead of the values. + let select = "Value 'bad' at 'select' failed to satisfy constraint: Member must satisfy \ + enum value set: [SPECIFIC_ATTRIBUTES, COUNT, ALL_ATTRIBUTES, \ + ALL_PROJECTED_ATTRIBUTES]"; + let rcc = "Value 'bad' at 'returnConsumedCapacity' failed to satisfy constraint: Member \ + must satisfy enum value set: [INDEXES, TOTAL, NONE]"; + let limit = |n: i64| { + format!( + "Value '{n}' at 'limit' failed to satisfy constraint: Member must have value \ + greater than or equal to 1" + ) + }; + let mix = "Can not use both expression and non-expression parameters in the same request: \ + Non-expression parameters: {AttributesToGet} Expression parameters: \ + {KeyConditionExpression}"; + let values = json!({":p": {"S": "x"}, ":u": {"N": "bob"}, ":z": {"X": "1"}}); + for (extra, expected) in [ + ( + json!({"Limit": 0}), + format!("1 validation error detected: {}", limit(0)), + ), + ( + json!({"Limit": -1}), + format!("1 validation error detected: {}", limit(-1)), + ), + ( + json!({"Select": "bad"}), + format!("1 validation error detected: {select}"), + ), + ( + json!({"ReturnConsumedCapacity": "bad"}), + format!("1 validation error detected: {rcc}"), + ), + ( + json!({"Limit": 0, "ReturnConsumedCapacity": "bad"}), + format!("2 validation errors detected: {rcc}; {}", limit(0)), + ), + ( + json!({"Limit": 0, "Select": "bad", "AttributesToGet": ["a"]}), + format!("2 validation errors detected: {select}; {}", limit(0)), + ), + ( + json!({"Limit": 0, "ReturnConsumedCapacity": "bad", "Select": "bad"}), + format!( + "3 validation errors detected: {select}; {rcc}; {}", + limit(0) + ), + ), + (json!({"AttributesToGet": ["a"]}), mix.to_owned()), + ] { + let body = body_with(values.clone(), extra.clone()); + assert_eq!( + message(validate_query_request_body(&body)), + Some(expected), + "{extra}" + ); + } + // A null value takes the same path. + let body = body_with( + json!({":p": {"S": "x"}, ":z": null}), + json!({"Limit": 0, "ReturnConsumedCapacity": "bad"}), + ); + assert_eq!( + message(validate_query_request_body(&body)), + Some(format!("2 validation errors detected: {rcc}; {}", limit(0))) + ); +} + +#[test] +fn a_shape_fault_is_still_reported_ahead_of_a_value_the_service_cannot_read() { + for extra in [ + json!({"Select": 5}), + json!({"ExpressionAttributeNames": "x"}), + ] { + for value in [json!({"X": "1"}), Value::Null] { + let body = body_with(json!({":p": {"S": "x"}, ":z": value}), extra.clone()); + assert!( + matches!( + validate_query_request_body(&body), + Err(DynamoDbError::SerializationException(_)) + ), + "{body}" + ); + } + } +} + +#[test] +fn a_json_type_fault_inside_a_value_the_service_cannot_read_is_a_shape_fault() { + // Measured 2026-11-11: each answers SerializationException. + for (values, extra) in [ + (json!({":z": {"L": [null, "x"]}}), json!({})), + (json!({":z": {"L": [{"X": "1"}, {"N": 5}]}}), json!({})), + (json!({":z": {"S": 5, "X": "1"}}), json!({"Limit": 0})), + (json!({":z": {"L": [null, {"B": "!!"}]}}), json!({})), + (json!({":z": {"S": 5, "N": "1"}}), json!({"Limit": 0})), + (json!({":z": {"L": [{"S": "a", "N": "1"}, "x"]}}), json!({})), + ( + json!({":a": {"S": "a", "N": "1"}, ":b": {"N": 5}}), + json!({}), + ), + (json!({":z": {"S": "a", "N": 1}}), json!({})), + ] { + let mut values = values; + values[":p"] = json!({"S": "x"}); + let body = body_with(values, extra); + assert!( + matches!( + validate_query_request_body(&body), + Err(DynamoDbError::SerializationException(_)) + ), + "{body}" + ); + } + // Two known descriptors with no JSON-type fault: the Limit text. + let body = body_with( + json!({":p": {"S": "x"}, ":z": {"S": "a", "N": "1"}}), + json!({"Limit": 0}), + ); + assert_eq!( + message(validate_query_request_body(&body)), + Some( + "1 validation error detected: Value '0' at 'limit' failed to satisfy constraint: \ + Member must have value greater than or equal to 1" + .to_owned() + ) + ); +} diff --git a/tests/test_query_key_value_types.py b/tests/test_query_key_value_types.py index 6db90cbd3..1d7b13a86 100644 --- a/tests/test_query_key_value_types.py +++ b/tests/test_query_key_value_types.py @@ -9,20 +9,22 @@ attribute's ``AttributeDefinitions`` type (``One or more parameter values were invalid: Condition parameter type does not match schema type``). Every expected answer below was measured against the service on 2026-10-24 (15:10, 21:00, -21:06 and 23:42 UTC), 2026-10-25 (11:31 UTC) and 2026-11-06 (the begins_with -rows after b20) on tables with S/S, S/N and B/B keys, a GSI and an LSI. A -Unicode decimal digit in an N value counts as the digit it stands for; a field -of the wrong JSON type anywhere in the body answers SerializationException -ahead of every check. +21:06 and 23:42 UTC), 2026-10-25 (11:31 UTC), 2026-11-06 (the begins_with +rows after b20) and 2026-11-11 (rows b48 to b50, f8, f9, g9 and g10, and the +digit runs) on tables with S/S, S/N and B/B keys, a GSI, an LSI and a GSI with +two HASH attributes. A Unicode decimal digit below U+10000 in an N value counts +as the digit it stands for. The rows also pin where the two checks fall against the other Query checks: -the numeric check after Limit, the expression parameter mix check and the -ReturnConsumedCapacity enum, and before the Select enum and the table lookup; -the type check after the key schema, filter, projection, unused value and -Select checks and before the ExclusiveStartKey check. +the numeric check after Limit, the ReturnConsumedCapacity enum and the +expression parameter mix check, and before the Select enum and the table +lookup; the type check after the key schema, filter, projection, unused value +and Select checks and before the ExclusiveStartKey check. Dual-target: runs against the service and against extenddb with the same -assertions. The LeadingKeys class needs the extenddb management API and is -skipped against the service. +assertions. test_query_value_shapes.py holds the bodies of the wrong JSON +shape and the values the service cannot read as one type, and +test_query_leading_keys_values.py the LeadingKeys fences, which need the +extenddb management API. Run: EXTENDDB_TEST_ENDPOINT=https://localhost:18443 \\ @@ -32,16 +34,10 @@ from __future__ import annotations -import os -import uuid -from typing import Any - -import boto3 import pytest -from botocore.config import Config as BotoConfig from botocore.exceptions import ClientError -from conftest import scoped_table, wait_for_active +from conftest import scoped_table NUMERIC = "The parameter cannot be converted to a numeric value: " TYPE_MISMATCH = ( @@ -144,6 +140,31 @@ def bb_table(dynamodb_client): yield name +@pytest.fixture(scope="module") +def mp_table(dynamodb_client): + """pk S; GSI mpi with HASH h1 S and h2 N, RANGE r S.""" + with scoped_table( + dynamodb_client, + attribute_definitions=[ + {"AttributeName": "pk", **S}, + {"AttributeName": "h1", **S}, + {"AttributeName": "h2", "AttributeType": "N"}, + {"AttributeName": "r", **S}, + ], + key_schema=[{"AttributeName": "pk", "KeyType": "HASH"}], + GlobalSecondaryIndexes=[{ + "IndexName": "mpi", + "KeySchema": [ + {"AttributeName": "h1", "KeyType": "HASH"}, + {"AttributeName": "h2", "KeyType": "HASH"}, + {"AttributeName": "r", "KeyType": "RANGE"}, + ], + "Projection": {"ProjectionType": "ALL"}, + }], + ) as name: + yield name + + NB = {"N": "bob"} N5 = {"N": "5"} SX = {"S": "x"} @@ -207,6 +228,32 @@ def bb_table(dynamodb_client): ) V = "ValidationException" RNF = "ResourceNotFoundException" +LEGACY_PK_N5 = {"pk": {"ComparisonOperator": "EQ", "AttributeValueList": [N5]}} +MISSED_R = "Query condition missed key schema element: r" +# Digit one of every decimal digit (category Nd) run in Unicode 15.0: the +# service accepts the 36 runs below U+10000 outside ASCII and refuses the 31 +# runs from U+104A0 up (measured 2026-11-11, each twice). +BMP_ONES = [chr(zero + 1) for zero in ( + 0x0660, 0x06F0, 0x07C0, 0x0966, 0x09E6, 0x0A66, 0x0AE6, 0x0B66, 0x0BE6, 0x0C66, 0x0CE6, + 0x0D66, 0x0DE6, 0x0E50, 0x0ED0, 0x0F20, 0x1040, 0x1090, 0x17E0, 0x1810, 0x1946, 0x19D0, + 0x1A80, 0x1A90, 0x1B50, 0x1BB0, 0x1C40, 0x1C50, 0xA620, 0xA8D0, 0xA900, 0xA9D0, 0xA9F0, + 0xAA50, 0xABF0, 0xFF10)] +HIGH_ONES = [chr(zero + 1) for zero in ( + 0x104A0, 0x10D30, 0x11066, 0x110F0, 0x11136, 0x111D0, 0x112F0, 0x11450, 0x114D0, 0x11650, + 0x116C0, 0x11730, 0x118E0, 0x11950, 0x11C50, 0x11D50, 0x11DA0, 0x11F50, 0x16A60, 0x16AC0, + 0x16B50, 0x1D7CE, 0x1D7D8, 0x1D7E2, 0x1D7EC, 0x1D7F6, 0x1E140, 0x1E2F0, 0x1E4F0, 0x1E950, + 0x1FBF0)] +# A refused value is echoed with each decimal digit below U+10000 folded. +FOLDED_ECHOES = [ + ("\uff11bob", "1bob"), ("\u0661\u0662x", "12x"), ("bob\uff11", "bob1"), + ("\u0967\u0968a\u0969", "12a3"), ("\uff11\U0001d7cf", "1\U0001d7cf"), +] +MIXES = { + "AttributesToGet": {"AttributesToGet": ["a"]}, + "QueryFilter": {"QueryFilter": {"a": {"ComparisonOperator": "EQ", "AttributeValueList": [SX]}}}, + "ConditionalOperator": {"ConditionalOperator": "AND"}, + "KeyConditions": {"KeyConditions": LEGACY_PK}, +} def kce(expr, values, **extra): @@ -433,6 +480,36 @@ def kce(expr, values, **extra): ("pk = BOOL", "ss", kce("pk = :u", {":u": {"BOOL": True}}), V, TYPE_MISMATCH), ("pk = N bob AND pk > S alice", "ss", kce("pk = :u AND pk > :v", {":u": NB, ":v": {"S": "alice"}}), V, NUMERIC + "bob"), + # An invalid ReturnConsumedCapacity is reported ahead of each + # non-expression parameter beside KeyConditionExpression, and the mix + # ahead of the numbers (measured 2026-11-11). + *[(f"f8 invalid ReturnConsumedCapacity with {name}", "ss", + kce("pk = :u", {":u": NB}, ReturnConsumedCapacity="bad", **extra), V, RCC_MSG) + for name, extra in MIXES.items()], + *[(f"f9 {name} with N bob", "ss", kce("pk = :u", {":u": NB}, **extra), V, + "Can not use both expression and non-expression parameters in the same request: " + f"Non-expression parameters: {{{name}}} Expression parameters: {{KeyConditionExpression}}") + for name, extra in MIXES.items() if name != "KeyConditions"], + # A refused value is echoed with its decimal digits folded to ASCII, and a + # digit from U+104A0 up is not a digit (measured 2026-11-11). + *[(f"g9 {raw!r} echoed with ASCII digits", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": "1"}, ":f": {"N": raw}}), + V, NUMERIC + echo) + for raw, echo in FOLDED_ECHOES], + *[(f"g10 U+{ord(one):X} in filter", "ns", + kce("pk = :u AND sk = :s", {":u": CAROL, ":s": {"N": "1"}, ":f": {"N": one}}, + FilterExpression="a = :f"), V, NUMERIC + one) + for one in HIGH_ONES], + # Legacy KeyConditions, and a key schema with two HASH attributes, where + # the type is reported ahead of the clause on a non-key attribute + # (measured 2026-11-11). + ("b48 legacy KeyConditions pk EQ N 5", "ss", {"KeyConditions": LEGACY_PK_N5}, V, TYPE_MISMATCH), + ("b49 two HASH attributes, h2 = S beside a non-key clause", "mp", + kce("h1 = :a AND h2 = :b AND z = :c", {":a": SX, ":b": {"S": "1"}, ":c": SX}, + IndexName="mpi"), V, TYPE_MISMATCH), + ("b50 two HASH attributes, h2 = N beside a non-key clause", "mp", + kce("h1 = :a AND h2 = :b AND z = :c", {":a": SX, ":b": {"N": "1"}, ":c": SX}, + IndexName="mpi"), V, MISSED_R), ] @@ -440,8 +517,8 @@ class TestKeyConditionValueTypes: """Each row: a Query and the service's answer.""" @pytest.fixture - def tables(self, ss_table, ns_table, bb_table): - return {"ss": ss_table, "ns": ns_table, "bb": bb_table, "missing": MISSING} + def tables(self, ss_table, ns_table, bb_table, mp_table): + return {"ss": ss_table, "ns": ns_table, "bb": bb_table, "mp": mp_table, "missing": MISSING} @pytest.mark.parametrize("row,table,kwargs,code,expected", ROWS, ids=[r[0] for r in ROWS]) def test_rows(self, dynamodb_client_no_validation, tables, row, table, kwargs, code, expected): @@ -500,216 +577,10 @@ def test_fullwidth_digit_in_a_number_set_passes_the_check(self, dynamodb_client, ExpressionAttributeValues={":u": CAROL, ":s": {"N": "1"}, ":f": {"NS": [FW_ONE]}}) assert (resp["Count"], resp["ScannedCount"]) == (0, 1) - -def _raw_query(client, body: dict): - """POST ``body`` as the Query JSON exactly as written, signed with the - client's credentials, so a field of the wrong JSON type reaches the server - (boto3 would refuse or reshape it).""" - import json - - from botocore.awsrequest import AWSRequest - - req = AWSRequest(method="POST", url=client.meta.endpoint_url, data=json.dumps(body), - headers={"Content-Type": "application/x-amz-json-1.0", - "X-Amz-Target": "DynamoDB_20120810.Query"}) - client._request_signer.sign("Query", req) - resp = client._endpoint.http_session.send(req.prepare()) - return resp.status_code, json.loads(resp.content.decode("utf-8", "replace") or "{}") - - -SHAPE_FAULTS = [ - ("Limit 0 with ExpressionAttributeValues string", - {"KeyConditionExpression": "pk = :p", "Limit": 0, "ExpressionAttributeValues": "x"}), - ("ExpressionAttributeNames string with N bob", - {"KeyConditionExpression": "pk = :p", "ExpressionAttributeNames": "x", - "ExpressionAttributeValues": {":p": NB}}), - ("Limit string with N bob", - {"KeyConditionExpression": "pk = :p", "Limit": "1", "ExpressionAttributeValues": {":p": NB}}), - ("AttributesToGet object with ProjectionExpression", - {"KeyConditionExpression": "pk = :p", "AttributesToGet": {"a": 1}, "ProjectionExpression": "a", - "ExpressionAttributeValues": {":p": SX}}), - ("QueryFilter array with FilterExpression", - {"KeyConditionExpression": "pk = :p", "QueryFilter": [1], "FilterExpression": "a = :p", - "ExpressionAttributeValues": {":p": SX}}), - ("Select number with N bob", - {"KeyConditionExpression": "pk = :p", "Select": 5, "ExpressionAttributeValues": {":p": NB}}), - ("Limit 0 with Select number", - {"KeyConditionExpression": "pk = :p", "Limit": 0, "Select": 5, - "ExpressionAttributeValues": {":p": SX}}), -] - - -class TestShapeFaultsAreReportedFirst: - """A field of the wrong JSON type anywhere in the body answers - SerializationException ahead of the Limit, parameter mix, enum and number - checks (measured 2026-10-25 11:31 UTC). The message texts differ between - the service and extenddb, so only the error type is asserted.""" - - @pytest.mark.parametrize("row,body", SHAPE_FAULTS, ids=[r[0] for r in SHAPE_FAULTS]) - def test_rows(self, dynamodb_client, ss_table, row, body): - status, payload = _raw_query(dynamodb_client, {"TableName": ss_table, **body}) - assert status == 400 - assert payload.get("__type", "").endswith("SerializationException"), payload - - def test_control_without_a_shape_fault_answers_the_check(self, dynamodb_client, ss_table): - status, payload = _raw_query(dynamodb_client, { - "TableName": ss_table, "KeyConditionExpression": "pk = :p", "Limit": 0, - "ExpressionAttributeValues": {":p": NB}}) - assert status == 400 - assert payload.get("__type", "").endswith("ValidationException") - assert payload.get("message", payload.get("Message")) == LIMIT_MSG - - -class TestLeadingKeysWithAnInvalidKeyValue: - """A principal under each LeadingKeys fence form gets ValidationException and - no items for a Query whose ExpressionAttributeValues carries an N value that - is not a number, whether the key condition, the filter or nothing reads it. - - extenddb only: the fences need users created through the management API.""" - - @pytest.fixture(autouse=True) - def setup(self): - endpoint = os.environ.get("EXTENDDB_TEST_ENDPOINT", "").strip() - admin_user = os.environ.get("EXTENDDB_ADMIN_USER", "").strip() - admin_pass = os.environ.get("EXTENDDB_ADMIN_PASSWORD", "").strip() - if not endpoint: - pytest.skip("LeadingKeys fences need the extenddb management API") - if not admin_user or not admin_pass: - pytest.fail("MISCONFIGURED: EXTENDDB_ADMIN_USER and EXTENDDB_ADMIN_PASSWORD are required") - from management_helpers import ManagementClient - - self.endpoint = endpoint - self.region = os.environ.get("AWS_DEFAULT_REGION", "us-east-1") - self.mgmt = ManagementClient(endpoint, admin_user, admin_pass) - self.account_id = f"{uuid.uuid4().int % 10**12:012d}" - resp = self.mgmt.create_account(self.account_id, f"kv-test-{self.account_id}") - assert resp.status_code == 201, resp.text - self._users: list[str] = [] - yield - for u in self._users: - try: - self.mgmt.delete_user(self.account_id, u) - except Exception: - pass - self.mgmt.delete_account(self.account_id) - - def _client(self, policy: dict | None) -> Any: - name = f"kv-{uuid.uuid4().hex[:8]}" - resp = self.mgmt.create_user(self.account_id, name, "TestPass123!") - assert resp.status_code == 201, resp.text - self._users.append(name) - resp = self.mgmt.create_access_key(self.account_id, name) - assert resp.status_code == 201, resp.text - creds = resp.json() - if policy is not None: - resp = self.mgmt.put_user_policy(self.account_id, name, "p", policy) - assert resp.status_code == 204, resp.text - return boto3.client( - "dynamodb", endpoint_url=self.endpoint, region_name=self.region, - aws_access_key_id=creds["access_key_id"], - aws_secret_access_key=creds["secret_access_key"], - verify=False, - config=BotoConfig(retries={"max_attempts": 0}, parameter_validation=False)) - - def _fence(self, condition: dict, table: str) -> dict: - arn = f"arn:aws:dynamodb:{self.region}:{self.account_id}:table/{table}" - return { - "Version": "2012-10-17", - "Statement": [{ - "Effect": "Allow", - "Action": "dynamodb:*", - "Resource": [arn, arn + "/index/*"], - "Condition": condition, - }], - } - - def test_every_fence_form_gets_validation_exception_and_no_items(self): - owner = self._client({ - "Version": "2012-10-17", - "Statement": [{"Effect": "Allow", "Action": "dynamodb:*", "Resource": "*"}], - }) - table = f"kv-{uuid.uuid4().hex[:8]}" - owner.create_table( - TableName=table, - AttributeDefinitions=[{"AttributeName": "pk", **S}], - KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], - BillingMode="PAY_PER_REQUEST") - wait_for_active(owner, table) - for pk in ("alice", "bob"): - owner.put_item(TableName=table, Item={"pk": {"S": pk}}) - - fences = { - "ForAllValues": {"ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, - "ForAnyValue": {"ForAnyValue:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, - "Null false": {"Null": {"dynamodb:LeadingKeys": "false"}, - "ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, - "StringEquals": {"StringEquals": {"dynamodb:LeadingKeys": "alice"}}, - "StringLike": {"StringLike": {"dynamodb:LeadingKeys": "ali*"}}, - "StringNotEquals": {"StringNotEquals": {"dynamodb:LeadingKeys": "bob"}}, - } - # The reported shape, and an N value that only the filter reads or - # nothing reads beside a key value the fence would bind. - requests = { - "pk = N bob": {"TableName": table, "KeyConditionExpression": "pk = :u", - "ExpressionAttributeValues": {":u": NB}}, - "pk = S alice, N bob in filter": { - "TableName": table, "KeyConditionExpression": "pk = :u", - "FilterExpression": "a = :n", - "ExpressionAttributeValues": {":u": {"S": "alice"}, ":n": NB}}, - "pk = S bob, N bob unused": { - "TableName": table, "KeyConditionExpression": "pk = :u", - "ExpressionAttributeValues": {":u": {"S": "bob"}, ":n": NB}}, - "pk = N bob, invalid Select": { - "TableName": table, "KeyConditionExpression": "pk = :u", - "ExpressionAttributeValues": {":u": NB}, "Select": "bad"}, - } - failures = [] - clients = {label: self._client(self._fence(condition, table)) - for label, condition in fences.items()} - clients["owner"] = owner - for shape, request in requests.items(): - for label, client in clients.items(): - try: - resp = client.query(**request) - failures.append(f"{shape} / {label}: {resp['Count']} items returned") - except ClientError as exc: - err = exc.response["Error"] - if (err["Code"], err["Message"]) != ("ValidationException", NUMERIC + "bob"): - failures.append(f"{shape} / {label}: {err['Code']} {err['Message']}") - owner.delete_table(TableName=table) - assert not failures, "\n".join(failures) - - def test_a_unicode_digit_key_value_reads_no_fenced_partition(self): - """A partition key value written with Unicode digits is not bound to a - LeadingKeys value, so it must not read a partition the fence keeps the - principal from. The service's answer for a fenced principal cannot be - measured here; the test asserts only that no item of partition 20 is - returned, whatever the answer is.""" - owner = self._client({ - "Version": "2012-10-17", - "Statement": [{"Effect": "Allow", "Action": "dynamodb:*", "Resource": "*"}], - }) - table = f"kv-{uuid.uuid4().hex[:8]}" - owner.create_table( - TableName=table, - AttributeDefinitions=[{"AttributeName": "pk", "AttributeType": "N"}], - KeySchema=[{"AttributeName": "pk", "KeyType": "HASH"}], - BillingMode="PAY_PER_REQUEST") - wait_for_active(owner, table) - for pk in ("10", "20"): - owner.put_item(TableName=table, Item={"pk": {"N": pk}, "v": {"S": "p" + pk}}) - fenced = self._client(self._fence( - {"ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["10"]}}, table)) - leaks = [] - for label, value in (("fullwidth", "\uff12\uff10"), ("arabic-indic", "\u0662\u0660"), - ("devanagari", "\u0968\u0966")): - try: - resp = fenced.query( - TableName=table, KeyConditionExpression="pk = :p", - ExpressionAttributeValues={":p": {"N": value}}) - except ClientError: - continue - if any(item.get("v") == {"S": "p20"} for item in resp["Items"]): - leaks.append(label) - owner.delete_table(TableName=table) - assert not leaks, leaks + @pytest.mark.parametrize("one", BMP_ONES, ids=[f"U+{ord(one):04X}" for one in BMP_ONES]) + def test_digit_one_of_every_run_below_u_10000_is_accepted(self, dynamodb_client, ns_table, one): + resp = dynamodb_client.query( + TableName=ns_table, KeyConditionExpression="pk = :u AND sk = :s", + FilterExpression="a = :f", + ExpressionAttributeValues={":u": CAROL, ":s": {"N": "1"}, ":f": {"N": one}}) + assert resp["ScannedCount"] == 1 diff --git a/tests/test_query_leading_keys_values.py b/tests/test_query_leading_keys_values.py new file mode 100644 index 000000000..5c0524de2 --- /dev/null +++ b/tests/test_query_leading_keys_values.py @@ -0,0 +1,182 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Query under each dynamodb:LeadingKeys fence form with an N value that is +not a number. The fences need users created through the extenddb management +API, so this file is skipped against the service. + +Run: + EXTENDDB_TEST_ENDPOINT=https://localhost:18443 \\ + EXTENDDB_ADMIN_USER=admin EXTENDDB_ADMIN_PASSWORD= \\ + pytest tests/test_query_leading_keys_values.py -v +""" + +from __future__ import annotations + +import os +import uuid +from typing import Any + +import boto3 +import pytest +from botocore.config import Config as BotoConfig +from botocore.exceptions import ClientError + +from conftest import scoped_table +from test_query_key_value_types import NB, NUMERIC +from test_query_value_shapes import EMPTY_AV, INVALID_VALUE, X1, _raw_query + + +class TestLeadingKeysWithAnInvalidKeyValue: + """A principal under each LeadingKeys fence form gets ValidationException and + no items for a Query whose ExpressionAttributeValues carries an N value that + is not a number, whether the key condition, the filter or nothing reads it. + + extenddb only: the fences need users created through the management API.""" + + @pytest.fixture(autouse=True) + def setup(self): + endpoint = os.environ.get("EXTENDDB_TEST_ENDPOINT", "").strip() + admin_user = os.environ.get("EXTENDDB_ADMIN_USER", "").strip() + admin_pass = os.environ.get("EXTENDDB_ADMIN_PASSWORD", "").strip() + if not endpoint: + pytest.skip("LeadingKeys fences need the extenddb management API") + if not admin_user or not admin_pass: + pytest.fail("MISCONFIGURED: EXTENDDB_ADMIN_USER and EXTENDDB_ADMIN_PASSWORD are required") + from management_helpers import ManagementClient + + self.endpoint = endpoint + self.region = os.environ.get("AWS_DEFAULT_REGION", "us-east-1") + self.mgmt = ManagementClient(endpoint, admin_user, admin_pass) + self.account_id = f"{uuid.uuid4().int % 10**12:012d}" + resp = self.mgmt.create_account(self.account_id, f"kv-test-{self.account_id}") + assert resp.status_code == 201, resp.text + self._users: list[str] = [] + yield + for u in self._users: + try: + self.mgmt.delete_user(self.account_id, u) + except Exception: + pass + self.mgmt.delete_account(self.account_id) + + def _client(self, policy: dict | None) -> Any: + name = f"kv-{uuid.uuid4().hex[:8]}" + resp = self.mgmt.create_user(self.account_id, name, "TestPass123!") + assert resp.status_code == 201, resp.text + self._users.append(name) + resp = self.mgmt.create_access_key(self.account_id, name) + assert resp.status_code == 201, resp.text + creds = resp.json() + if policy is not None: + resp = self.mgmt.put_user_policy(self.account_id, name, "p", policy) + assert resp.status_code == 204, resp.text + return boto3.client( + "dynamodb", endpoint_url=self.endpoint, region_name=self.region, + aws_access_key_id=creds["access_key_id"], + aws_secret_access_key=creds["secret_access_key"], + verify=False, + config=BotoConfig(retries={"max_attempts": 0}, parameter_validation=False)) + + def _fence(self, condition: dict, table: str) -> dict: + arn = f"arn:aws:dynamodb:{self.region}:{self.account_id}:table/{table}" + return { + "Version": "2012-10-17", + "Statement": [{ + "Effect": "Allow", + "Action": "dynamodb:*", + "Resource": [arn, arn + "/index/*"], + "Condition": condition, + }], + } + + def test_every_fence_form_gets_validation_exception_and_no_items(self): + owner = self._client({ + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": "dynamodb:*", "Resource": "*"}], + }) + failures = [] + with scoped_table(owner) as table: + for pk in ("alice", "bob"): + owner.put_item(TableName=table, Item={"pk": {"S": pk}}) + + fences = { + "ForAllValues": {"ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, + "ForAnyValue": {"ForAnyValue:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, + "Null false": {"Null": {"dynamodb:LeadingKeys": "false"}, + "ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["alice"]}}, + "StringEquals": {"StringEquals": {"dynamodb:LeadingKeys": "alice"}}, + "StringLike": {"StringLike": {"dynamodb:LeadingKeys": "ali*"}}, + "StringNotEquals": {"StringNotEquals": {"dynamodb:LeadingKeys": "bob"}}, + } + # The reported shape, and an N value that only the filter reads or + # nothing reads beside a key value the fence would bind. + requests = { + "pk = N bob": {"TableName": table, "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": NB}}, + "pk = S alice, N bob in filter": { + "TableName": table, "KeyConditionExpression": "pk = :u", + "FilterExpression": "a = :n", + "ExpressionAttributeValues": {":u": {"S": "alice"}, ":n": NB}}, + "pk = S bob, N bob unused": { + "TableName": table, "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": {"S": "bob"}, ":n": NB}}, + "pk = N bob, invalid Select": { + "TableName": table, "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": NB}, "Select": "bad"}, + } + clients = {label: self._client(self._fence(condition, table)) + for label, condition in fences.items()} + clients["owner"] = owner + for shape, request in requests.items(): + for label, client in clients.items(): + try: + resp = client.query(**request) + failures.append(f"{shape} / {label}: {resp['Count']} items returned") + except ClientError as exc: + err = exc.response["Error"] + if (err["Code"], err["Message"]) != ("ValidationException", NUMERIC + "bob"): + failures.append(f"{shape} / {label}: {err['Code']} {err['Message']}") + # Beside a value with an unknown type descriptor, which boto3 cannot + # send, every principal gets the ValidationException that names + # :z. The service names :z for the owner (measured 2026-11-11). Its + # answer for a fenced principal was not measured. + body = {"TableName": table, "KeyConditionExpression": "pk = :u", + "ExpressionAttributeValues": {":u": NB, ":z": X1}} + expected = INVALID_VALUE + EMPTY_AV + " for key :z" + for label, client in clients.items(): + status, payload = _raw_query(client, body) + kind = payload.get("__type", "").rsplit("#", 1)[-1] + msg = payload.get("message", payload.get("Message")) + if (status, kind, msg) != (400, "ValidationException", expected): + failures.append(f"N bob beside X / {label}: {status} {kind} {msg}") + assert not failures, "\n".join(failures) + + def test_a_unicode_digit_key_value_reads_no_fenced_partition(self): + """A partition key value written with Unicode digits is not bound to a + LeadingKeys value, so it must not read a partition the fence keeps the + principal from. The service's answer for a fenced principal cannot be + measured here; the test asserts only that no item of partition 20 is + returned, whatever the answer is.""" + owner = self._client({ + "Version": "2012-10-17", + "Statement": [{"Effect": "Allow", "Action": "dynamodb:*", "Resource": "*"}], + }) + leaks = [] + with scoped_table(owner, attribute_definitions=[ + {"AttributeName": "pk", "AttributeType": "N"}]) as table: + for pk in ("10", "20"): + owner.put_item(TableName=table, Item={"pk": {"N": pk}, "v": {"S": "p" + pk}}) + fenced = self._client(self._fence( + {"ForAllValues:StringEquals": {"dynamodb:LeadingKeys": ["10"]}}, table)) + for label, value in (("fullwidth", "\uff12\uff10"), ("arabic-indic", "\u0662\u0660"), + ("devanagari", "\u0968\u0966")): + try: + resp = fenced.query( + TableName=table, KeyConditionExpression="pk = :p", + ExpressionAttributeValues={":p": {"N": value}}) + except ClientError: + continue + if any(item.get("v") == {"S": "p20"} for item in resp["Items"]): + leaks.append(label) + assert not leaks, leaks diff --git a/tests/test_query_value_shapes.py b/tests/test_query_value_shapes.py new file mode 100644 index 000000000..ebf71678c --- /dev/null +++ b/tests/test_query_value_shapes.py @@ -0,0 +1,174 @@ +# Copyright 2026 ExtendDB contributors +# SPDX-License-Identifier: Apache-2.0 + +"""Query request bodies the service does not read as written: a field of the +wrong JSON type, and an ExpressionAttributeValues value the service cannot +read as one type. boto3 refuses or reshapes these bodies, so they go out as +written. Every expected answer below was measured against the service on +2026-10-25 (11:31 UTC) and 2026-11-11. + +Dual-target: runs against the service and against extenddb with the same +assertions. + +Run: + EXTENDDB_TEST_ENDPOINT=https://localhost:18443 \\ + EXTENDDB_ADMIN_USER=admin EXTENDDB_ADMIN_PASSWORD= \\ + pytest tests/test_query_value_shapes.py -v +""" + +from __future__ import annotations + +import pytest + +from test_query_key_value_types import ( # noqa: F401 (ss_table is a fixture) + LIMIT_MSG, + NB, + NUMERIC, + RCC_MSG, + SX, + ss_table, +) + + +def _raw_query(client, body: dict): + """POST ``body`` as the Query JSON exactly as written, signed with the + client's credentials, so a field of the wrong JSON type reaches the server + (boto3 would refuse or reshape it).""" + import json + + from botocore.awsrequest import AWSRequest + + req = AWSRequest(method="POST", url=client.meta.endpoint_url, data=json.dumps(body), + headers={"Content-Type": "application/x-amz-json-1.0", + "X-Amz-Target": "DynamoDB_20120810.Query"}) + client._request_signer.sign("Query", req) + resp = client._endpoint.http_session.send(req.prepare()) + return resp.status_code, json.loads(resp.content.decode("utf-8", "replace") or "{}") + + +SHAPE_FAULTS = [ + ("Limit 0 with ExpressionAttributeValues string", + {"KeyConditionExpression": "pk = :p", "Limit": 0, "ExpressionAttributeValues": "x"}), + ("ExpressionAttributeNames string with N bob", + {"KeyConditionExpression": "pk = :p", "ExpressionAttributeNames": "x", + "ExpressionAttributeValues": {":p": NB}}), + ("Limit string with N bob", + {"KeyConditionExpression": "pk = :p", "Limit": "1", "ExpressionAttributeValues": {":p": NB}}), + ("AttributesToGet object with ProjectionExpression", + {"KeyConditionExpression": "pk = :p", "AttributesToGet": {"a": 1}, "ProjectionExpression": "a", + "ExpressionAttributeValues": {":p": SX}}), + ("QueryFilter array with FilterExpression", + {"KeyConditionExpression": "pk = :p", "QueryFilter": [1], "FilterExpression": "a = :p", + "ExpressionAttributeValues": {":p": SX}}), + ("Select number with N bob", + {"KeyConditionExpression": "pk = :p", "Select": 5, "ExpressionAttributeValues": {":p": NB}}), + ("Limit 0 with Select number", + {"KeyConditionExpression": "pk = :p", "Limit": 0, "Select": 5, + "ExpressionAttributeValues": {":p": SX}}), + # A field of the wrong JSON type inside a value the service cannot read as + # one type (measured 2026-11-11). + ("string beside a null in an L", + {"KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": {":p": SX, ":z": {"L": [None, "x"]}}}), + ("N number beside an unknown descriptor in an L", + {"KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": {":p": SX, ":z": {"L": [{"X": "1"}, {"N": 5}]}}}), + ("S number beside an unknown descriptor with Limit 0", + {"KeyConditionExpression": "pk = :p", "Limit": 0, + "ExpressionAttributeValues": {":p": SX, ":z": {"S": 5, "X": "1"}}}), + ("B not base64 beside a null in an L", + {"KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": {":p": SX, ":z": {"L": [None, {"B": "!!"}]}}}), +] + + +class TestShapeFaultsAreReportedFirst: + """A field of the wrong JSON type anywhere in the body answers + SerializationException ahead of the Limit, parameter mix, enum and number + checks (measured 2026-10-25 11:31 UTC). The message texts differ between + the service and extenddb, so only the error type is asserted.""" + + @pytest.mark.parametrize("row,body", SHAPE_FAULTS, ids=[r[0] for r in SHAPE_FAULTS]) + def test_rows(self, dynamodb_client, ss_table, row, body): + status, payload = _raw_query(dynamodb_client, {"TableName": ss_table, **body}) + assert status == 400 + assert payload.get("__type", "").endswith("SerializationException"), payload + + def test_control_without_a_shape_fault_answers_the_check(self, dynamodb_client, ss_table): + status, payload = _raw_query(dynamodb_client, { + "TableName": ss_table, "KeyConditionExpression": "pk = :p", "Limit": 0, + "ExpressionAttributeValues": {":p": NB}}) + assert status == 400 + assert payload.get("__type", "").endswith("ValidationException") + assert payload.get("message", payload.get("Message")) == LIMIT_MSG + + +INVALID_VALUE = "ExpressionAttributeValues contains invalid value: " +EMPTY_AV = "Supplied AttributeValue is empty, must contain exactly one of the supported datatypes" +NULL_AV = "The supplied Item contains a null AttributeValue" +SEVERAL_AV = ("Supplied AttributeValue has more than one datatypes set, must contain exactly " + "one of the supported datatypes") +X1 = {"X": "1"} +# An S with an unknown descriptor beside it, which the service reads as an S. +S_PLUS_X = {"S": "a", "X": "1"} +LIMIT_0_AT = ("Value '0' at 'limit' failed to satisfy constraint: " + "Member must have value greater than or equal to 1") + +# (row, values beside :p, other fields, the service's message). +UNREADABLE = [ + ("unknown descriptor", {":z": X1}, {}, INVALID_VALUE + EMPTY_AV + " for key :z"), + ("null", {":z": None}, {}, INVALID_VALUE + NULL_AV + " for key :z"), + ("no descriptor", {":z": {}}, {}, INVALID_VALUE + EMPTY_AV + " for key :z"), + ("two descriptors", {":z": {"S": "a", "N": "1"}}, {}, INVALID_VALUE + SEVERAL_AV + " for key :z"), + ("unknown descriptor in an L", {":z": {"L": [X1]}}, {}, INVALID_VALUE + EMPTY_AV + " for key :z"), + ("null M member", {":z": {"M": {"k": None}}}, {}, + INVALID_VALUE + NULL_AV + ": k is null for key :z"), + ("N bob beside S plus X", {":a": S_PLUS_X, ":u": NB}, {}, INVALID_VALUE + NUMERIC + "bob for key :u"), + ("empty N beside S plus X", {":a": S_PLUS_X, ":u": {"N": ""}}, {}, + INVALID_VALUE + "The parameter cannot be converted to a numeric value for key :u"), + ("fullwidth one bob beside S plus X", {":a": S_PLUS_X, ":u": {"N": "\uff11bob"}}, {}, + INVALID_VALUE + NUMERIC + "\uff11bob for key :u"), + ("Limit 0", {":z": X1}, {"Limit": 0}, "1 validation error detected: " + LIMIT_0_AT), + ("Limit 0 beside null", {":z": None}, {"Limit": 0}, "1 validation error detected: " + LIMIT_0_AT), + ("Limit 0 and ReturnConsumedCapacity", {":z": X1}, {"Limit": 0, "ReturnConsumedCapacity": "bad"}, + "2 validation errors detected: " + RCC_MSG.removeprefix("1 validation error detected: ") + + "; " + LIMIT_0_AT), + ("invalid Select", {":z": X1, ":u": NB}, {"Select": "bad"}, + "1 validation error detected: Value 'bad' at 'select' failed to satisfy constraint: Member " + "must satisfy enum value set: [SPECIFIC_ATTRIBUTES, COUNT, ALL_ATTRIBUTES, " + "ALL_PROJECTED_ATTRIBUTES]"), + ("AttributesToGet", {":z": X1, ":u": NB}, {"AttributesToGet": ["a"]}, + "Can not use both expression and non-expression parameters in the same request: " + "Non-expression parameters: {AttributesToGet} Expression parameters: {KeyConditionExpression}"), +] + + +class TestValuesTheServiceCannotReadAsOneType: + """A value in ExpressionAttributeValues that the service cannot read as one + type (a null, no type descriptor it knows, more than one, or an unknown one + beside a known one) is named with its placeholder, and beside such a value + Select, ReturnConsumedCapacity and Limit are reported together, ahead of + the parameter mix (measured 2026-11-11). boto3 cannot send an unknown + descriptor or a null, so the bodies go out as written.""" + + @pytest.mark.parametrize("row,values,extra,expected", UNREADABLE, ids=[r[0] for r in UNREADABLE]) + def test_rows(self, dynamodb_client, ss_table, row, values, extra, expected): + status, payload = _raw_query(dynamodb_client, { + "TableName": ss_table, "KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": {":p": SX, **values}, **extra}) + assert status == 400, payload + assert payload.get("__type", "").endswith("ValidationException"), payload + assert payload.get("message", payload.get("Message")) == expected + + def test_the_value_the_service_cannot_read_is_named_beside_n_bob(self, dynamodb_client, + ss_table): + """A value the service cannot read as one type is named ahead of a + number fault in another value: :z for this body on 28 of 28 requests + (measured 2026-11-11).""" + status, payload = _raw_query(dynamodb_client, { + "TableName": ss_table, "KeyConditionExpression": "pk = :p", + "ExpressionAttributeValues": {":p": SX, ":u": NB, ":z": X1}}) + assert status == 400, payload + assert payload.get("__type", "").endswith("ValidationException"), payload + assert payload.get("message", payload.get("Message")) == ( + INVALID_VALUE + EMPTY_AV + " for key :z") From 54150fe9cf086cbeb9a408630495770e15c65e4d Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 12 Nov 2026 11:40:40 +0000 Subject: [PATCH 12/12] fix(query): share the key value type check and update three comments The comment above the Query request checks in authorize_request said that they run for every principal. They run after the cross-account refusal, so a table ARN in another account gets AccessDeniedException without them. The service runs them first for a body it can read. Main has the same order for every Query check, and the call does not move. The comment now states this. The comment above the key value type gate in handle_query now states the service's order for legacy KeyConditions. For a legacy value of the wrong type on a single-attribute key schema, the service reports the projection, Select, filter, QueryFilter and unused-value checks first. The classifier's check answers ahead of them, as on main. The doc comment of the Query number check is about the ExpressionAttributeValues contains invalid value wrapper. It said that on Query the service adds the wrapper only for a value it cannot read as one type. The service also adds it beside a Limit above 2147483647 (measured 2026-11-12), and the comment now says so. validate_key_condition_types now calls check_key_value_type for each value it compares. This removes type_matches and resolve_attr_name, which repeated check_key_value_type and resolve_path_attr_name. check_key_value_type returns KEY_CONDITION_TYPE_MISMATCH in place of a second copy of the text. No answer changes. validate_attribute_number in the core validation module is private again. An earlier commit of this branch made it public, and nothing outside that file calls it. --- crates/core/src/validation/mod.rs | 10 +---- crates/engine/src/query.rs | 12 ++--- crates/engine/src/query_key_values.rs | 65 ++++++--------------------- crates/server/src/request_helpers.rs | 22 +++++---- 4 files changed, 35 insertions(+), 74 deletions(-) diff --git a/crates/core/src/validation/mod.rs b/crates/core/src/validation/mod.rs index 7d993415e..50fb2a0f3 100755 --- a/crates/core/src/validation/mod.rs +++ b/crates/core/src/validation/mod.rs @@ -2510,15 +2510,7 @@ fn validate_lsi_requires_range_key(input: &CreateTableInput) -> Result<(), Dynam Ok(()) } -/// Validate every number inside one attribute value (`N`, `NS` members, and -/// the values nested in `L` and `M`) against the `DynamoDB` number rules, with -/// the bare numeric message. -/// -/// # Errors -/// -/// Returns `DynamoDbError::ValidationException` for the first number that is -/// not a number, has more than 38 significant digits, or is out of range. -pub fn validate_attribute_number(value: &AttributeValue) -> Result<(), DynamoDbError> { +fn validate_attribute_number(value: &AttributeValue) -> Result<(), DynamoDbError> { match value { AttributeValue::N(n) => { number::validate_and_normalize_number(n)?; diff --git a/crates/engine/src/query.rs b/crates/engine/src/query.rs index 76fedd5ea..4efded87d 100755 --- a/crates/engine/src/query.rs +++ b/crates/engine/src/query.rs @@ -250,7 +250,11 @@ pub async fn handle_query( // Legacy `KeyConditions` are checked by the classifier: against the key // schema's definitions on a multi-attribute key schema and against the // table's on a single-attribute one. The check below runs only where the - // classifier did not check (`!check_types_here`). + // classifier did not check (`!check_types_here`). For a legacy value of + // the wrong type on a single-attribute key schema, the service reports the + // projection, Select, filter, QueryFilter and unused-value checks first + // (measured 2026-11-11 and 2026-11-12), so the classifier's check answers + // ahead of them there, where the service does not. let multipart = extenddb_core::types::is_multipart_key_schema(&query_key_info.key_schema); let key_schema_definitions: Vec = query_key_info .attribute_definitions @@ -601,7 +605,7 @@ fn resolve_path_attr_name( /// `attribute_definitions` is not checked here. On a multi-attribute key /// schema the caller passes the queried key schema's definitions only, and the /// classifier reports a clause on any other attribute. -fn check_key_value_type( +pub(crate) fn check_key_value_type( path: &[PathElement], expr: &extenddb_core::expression::Expr, attribute_definitions: &[extenddb_core::types::AttributeDefinition], @@ -635,9 +639,7 @@ fn check_key_value_type( Ok(()) } else { Err(DynamoDbError::ValidationException( - "One or more parameter values were invalid: Condition parameter type does not match \ - schema type" - .to_owned(), + crate::query_key_values::KEY_CONDITION_TYPE_MISMATCH.to_owned(), )) } } diff --git a/crates/engine/src/query_key_values.rs b/crates/engine/src/query_key_values.rs index 69fbb6ce1..e134dc252 100644 --- a/crates/engine/src/query_key_values.rs +++ b/crates/engine/src/query_key_values.rs @@ -28,13 +28,11 @@ //! `bob` partition, while authorization binds no `dynamodb:LeadingKeys` value //! to a number it cannot parse. -use std::collections::HashMap; - use extenddb_core::error::DynamoDbError; use extenddb_core::expression::{ Expr, ExpressionMaps, KeyCondition, PathElement, SortKeyCondition, }; -use extenddb_core::types::{AttributeDefinition, AttributeValue, QueryInput, ScalarAttributeType}; +use extenddb_core::types::{AttributeDefinition, QueryInput}; use serde::de::DeserializeOwned; use serde_json::{Map, Value}; @@ -340,8 +338,9 @@ fn number_fault(text: &str) -> Option> { /// Refuse the first `N` value in `ExpressionAttributeValues` (including `NS` /// members and values nested in `L` and `M`) that is not a number, with the /// bare numeric message: no `ExpressionAttributeValues contains invalid value` -/// wrapper, which the service uses on `UpdateItem` and on `Query` only for a -/// value it cannot read as one type. The placeholders are visited in sorted +/// wrapper, which the service uses on `UpdateItem`, and on `Query` for a value +/// it cannot read as one type or beside a `Limit` above 2147483647 (measured +/// 2026-11-12). The placeholders are visited in sorted /// order and each value on its own, so a value of the wrong JSON shape does /// not stop the check of another (the service picks among several invalid /// values by an order of its own: measured 2026-11-11, `:a` and `:b` with @@ -377,12 +376,14 @@ fn validate_expression_attribute_value_numbers( /// /// Every value the condition compares is checked: the partition key value(s), /// the sort key value (or both `BETWEEN` bounds, or the `begins_with` prefix) -/// and the extra equality conditions of a multi-attribute key schema. A path -/// that does not resolve to an attribute name, a value that is not a -/// placeholder, and a key attribute with no definition are left to the checks -/// that already refuse them. When any compared placeholder is undefined the -/// whole check is skipped: the service reports the undefined placeholder -/// first, and the storage layer does so here. +/// and the extra equality conditions of a multi-attribute key schema. Each one +/// goes through the check the key schema classifier applies to a clause +/// (`check_key_value_type` in `query.rs`). A path that does not resolve to an +/// attribute name, a value that is not a placeholder, and a key attribute with +/// no definition are left to the checks that already refuse them. When any +/// compared placeholder is undefined the whole check is skipped: the service +/// reports the undefined placeholder first, and the storage layer does so +/// here. /// /// # Errors /// @@ -419,51 +420,11 @@ pub fn validate_key_condition_types( } for (path, value) in pairs { - let Some(attr_name) = resolve_attr_name(path, &maps.names) else { - continue; - }; - let Some(expected) = attribute_definitions - .iter() - .find(|ad| ad.attribute_name == attr_name) - .map(|ad| ad.attribute_type) - else { - continue; - }; - let Expr::Placeholder(name) = value else { - continue; - }; - let Some(av) = maps.values.get(name) else { - continue; - }; - if !type_matches(expected, av) { - return Err(DynamoDbError::ValidationException( - KEY_CONDITION_TYPE_MISMATCH.to_owned(), - )); - } + crate::query::check_key_value_type(path, value, attribute_definitions, maps)?; } Ok(()) } -fn type_matches(expected: ScalarAttributeType, value: &AttributeValue) -> bool { - matches!( - (expected, value), - (ScalarAttributeType::S, AttributeValue::S(_)) - | (ScalarAttributeType::N, AttributeValue::N(_)) - | (ScalarAttributeType::B, AttributeValue::B(_)) - ) -} - -/// The top-level attribute name of a key path, through `#name` references. -fn resolve_attr_name(path: &[PathElement], names: &HashMap) -> Option { - match path.first()? { - PathElement::Attribute(name) => match name.strip_prefix('#') { - Some(reference) => names.get(reference).cloned(), - None => Some(name.clone()), - }, - PathElement::Index(_) => None, - } -} - #[cfg(test)] mod tests; diff --git a/crates/server/src/request_helpers.rs b/crates/server/src/request_helpers.rs index 3d8e36383..a9ae693a1 100755 --- a/crates/server/src/request_helpers.rs +++ b/crates/server/src/request_helpers.rs @@ -174,14 +174,20 @@ pub(crate) async fn authorize_request( // A Query's request-time checks (Limit, the expression and non-expression // parameter mix, the ReturnConsumedCapacity and Select enums, every number - // in ExpressionAttributeValues) run before the IAM decision, for every - // principal. The engine makes the same checks at request time; they run - // here as well because authorization binds no dynamodb:LeadingKeys value - // to a number it cannot parse, and a fence that needs the key present - // (ForAnyValue, a Null guard, the plain operators) would otherwise answer - // AccessDeniedException where ValidationException is the request's error. - // The checks read the raw body, so a field that does not deserialize does - // not skip them. + // in ExpressionAttributeValues) run before the IAM decision, whatever the + // caller's policy. The engine makes the same checks at request time; they + // run here as well because authorization binds no dynamodb:LeadingKeys + // value to a number it cannot parse, and a fence that needs the key + // present (ForAnyValue, a Null guard, the plain operators) would otherwise + // answer AccessDeniedException where ValidationException is the request's + // error. The checks read the raw body, so a field that does not + // deserialize does not skip them. + // + // They run after the cross-account refusal above, so a Query that names a + // table ARN in another account gets AccessDeniedException without them. + // The service runs them first for a body it can read (measured + // 2026-11-12: with a table ARN in another account, `{"N": "bob"}` answers + // the numeric message and a `Limit` of 0 the `Limit` text). if operation == "Query" { extenddb_engine::query_key_values::validate_query_request_body(input)?; }