diff --git a/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs b/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs index c9c2b6481..88ff51b15 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/expressions/calls.rs @@ -167,7 +167,9 @@ pub(crate) fn convert_function(payload: &Value, site: Site<'_>) -> Result concatenate(parts, &construct, site), + ("CONCAT", parts) if !parts.is_empty() => { + concatenate(&parts.iter().collect::>(), &construct, site) + } ("CONCAT_WS", parts) => concatenate_with_separator(parts, &construct, site), ("CONTAINS", parts) => text_contains(parts, &construct, site), ("EDITDISTANCE", parts) => edit_distance(parts, &construct, site), diff --git a/crates/sqlquack-rules/src/conversion/_helpers/expressions/strings.rs b/crates/sqlquack-rules/src/conversion/_helpers/expressions/strings.rs index a7a17f148..2b88e4181 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/expressions/strings.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/expressions/strings.rs @@ -243,8 +243,8 @@ pub(crate) fn concat_operator(payload: &Value, site: Site<'_>) -> Result, ) -> Result { diff --git a/crates/sqlquack-rules/src/conversion/_helpers/support/depth.rs b/crates/sqlquack-rules/src/conversion/_helpers/support/depth.rs new file mode 100644 index 000000000..7901efca7 --- /dev/null +++ b/crates/sqlquack-rules/src/conversion/_helpers/support/depth.rs @@ -0,0 +1,37 @@ +//! The bound on how deep a converted tree may nest, checked before conversion recurses into it. + +use serde_json::Value; +use sqlquack_core::refusals::main::missing::missing; +use sqlquack_core::refusals::models::{Refusal, RefusalCategory}; +use sqlquack_syntax::{Expression, ExpressionWalk}; + +use crate::conversion::_helpers::support::tree::node_construct; +use crate::conversion::constants::MAX_TREE_DEPTH; + +/// Nothing, or a budget refusal when `tree` nests deeper than conversion can recurse safely. +pub(crate) fn within_depth(tree: &Expression) -> Result<(), Refusal> { + let depth = tree.tree_depth(); + match depth > MAX_TREE_DEPTH { + false => Ok(()), + true => Err(Refusal { + construct: None, + location: None, + category: RefusalCategory::ConversionBudget, + reason: format!( + "the statement nests {depth} levels deep, beyond the {MAX_TREE_DEPTH} conversion \ + accepts; each operand of a chain such as a + b + c counts as one level" + ), + }), + } +} + +/// `tree` as the JSON conversion walks, once its depth is within the bound. +pub(crate) fn positioned(tree: &Expression) -> Result { + within_depth(tree)?; + serde_json::to_value(tree).map_err(|error| { + missing( + node_construct("statement"), + format!("the tree does not serialise: {error}"), + ) + }) +} diff --git a/crates/sqlquack-rules/src/conversion/_helpers/support/mod.rs b/crates/sqlquack-rules/src/conversion/_helpers/support/mod.rs index f1a92c3ef..8880519bb 100644 --- a/crates/sqlquack-rules/src/conversion/_helpers/support/mod.rs +++ b/crates/sqlquack-rules/src/conversion/_helpers/support/mod.rs @@ -1,6 +1,7 @@ pub(crate) mod collecting; pub(crate) mod comments; pub(crate) mod declared; +pub(crate) mod depth; pub(crate) mod divergence; pub(crate) mod failure; pub(crate) mod locations; diff --git a/crates/sqlquack-rules/src/conversion/constants.rs b/crates/sqlquack-rules/src/conversion/constants.rs index fbea3862d..c30f845d0 100644 --- a/crates/sqlquack-rules/src/conversion/constants.rs +++ b/crates/sqlquack-rules/src/conversion/constants.rs @@ -108,6 +108,12 @@ pub(crate) const HIERARCHY_WALK: &str = "SQLQUACK$WALK"; pub(crate) const HIERARCHY_PRIOR: &str = "SQLQUACK$PRIOR"; /// The depth past which a CONNECT BY walk raises rather than grow without end; never truncates. pub(crate) const HIERARCHY_MAX_DEPTH: u32 = 10_000; +/// How deep a tree may nest for conversion, which recurses once per level, flat chains included. +#[cfg(not(target_arch = "wasm32"))] +pub(crate) const MAX_TREE_DEPTH: usize = 512; +/// The same on WebAssembly, whose stack is far smaller. +#[cfg(target_arch = "wasm32")] +pub(crate) const MAX_TREE_DEPTH: usize = 128; /// The tag of the raise past `HIERARCHY_MAX_DEPTH`. pub(crate) const HIERARCHY_DEPTH_TAG: &str = "connect_by_depth"; /// Tree nodes holding a query, refused in a hierarchical query's projection and ordering. diff --git a/crates/sqlquack-rules/src/conversion/main/collect_refusals.rs b/crates/sqlquack-rules/src/conversion/main/collect_refusals.rs index c7236bb88..601f0129f 100644 --- a/crates/sqlquack-rules/src/conversion/main/collect_refusals.rs +++ b/crates/sqlquack-rules/src/conversion/main/collect_refusals.rs @@ -8,6 +8,7 @@ use sqlquack_syntax::Expression; use crate::conversion::_helpers::queries::query::convert_query; use crate::conversion::_helpers::support::collecting::{Collector, collected}; +use crate::conversion::_helpers::support::depth::within_depth; use crate::conversion::_helpers::support::locations::Locator; use crate::conversion::_helpers::support::scope::{Context, OrderTrack}; use crate::conversion::_helpers::support::tree::{session_tree, strip_positions}; @@ -22,6 +23,12 @@ pub fn collect_refusals( statement: &Expression, (schema, options, lines): (&Schema, &ConversionOptions, &SourceLines), ) -> CollectedRefusals { + if let Err(refusal) = within_depth(statement) { + return CollectedRefusals { + refusals: vec![refusal], + ..CollectedRefusals::default() + }; + } let converted = serde_json::to_value(statement) .map(|positioned| collected_conversion(&positioned, (schema, options, lines))); let mut guessed = false; diff --git a/crates/sqlquack-rules/src/conversion/main/convert_function_body.rs b/crates/sqlquack-rules/src/conversion/main/convert_function_body.rs index 50de162c8..bef66e1e1 100644 --- a/crates/sqlquack-rules/src/conversion/main/convert_function_body.rs +++ b/crates/sqlquack-rules/src/conversion/main/convert_function_body.rs @@ -9,6 +9,7 @@ use crate::conversion::_helpers::queries::function_body::{ check_body, converted_body, held_types, parameters_joined, refused, require_declared_types, selected_columns, }; +use crate::conversion::_helpers::support::depth::positioned; use crate::conversion::_helpers::support::locations::Locator; use crate::conversion::_helpers::support::scope::{Context, OrderTrack}; use crate::conversion::_helpers::support::tree::session_tree; @@ -26,8 +27,7 @@ pub fn convert_function_body( if spec.language != FunctionLanguage::Sql { return Err(refused("only SQL function bodies convert")); } - let positioned = serde_json::to_value(query) - .map_err(|error| refused(&format!("the tree does not serialise: {error}")))?; + let positioned = positioned(query)?; let tree = session_tree(&positioned, options.quoted_identifiers_ignore_case) .map_err(|reason| refused(&reason))?; let locator = Locator::new(&tree, &positioned, lines); diff --git a/crates/sqlquack-rules/src/conversion/main/convert_statement.rs b/crates/sqlquack-rules/src/conversion/main/convert_statement.rs index 4363ca30a..09a1cf518 100644 --- a/crates/sqlquack-rules/src/conversion/main/convert_statement.rs +++ b/crates/sqlquack-rules/src/conversion/main/convert_statement.rs @@ -16,6 +16,7 @@ use crate::conversion::_helpers::queries::function_body::{ column_types, function_stub, override_stub, }; use crate::conversion::_helpers::queries::query::convert_query; +use crate::conversion::_helpers::support::depth::positioned; use crate::conversion::_helpers::support::locations::Locator; use crate::conversion::_helpers::support::scope::{Context, OrderTrack}; use crate::conversion::_helpers::support::tree::{node_construct, session_tree}; @@ -31,12 +32,7 @@ pub fn convert_statement( (schema, options, lines): (&Schema, &ConversionOptions, &SourceLines), log: ProvenanceLog, ) -> Result { - let positioned = serde_json::to_value(statement).map_err(|error| { - missing( - node_construct("statement"), - format!("the tree does not serialise: {error}"), - ) - })?; + let positioned = positioned(statement)?; let tree = session_tree(&positioned, options.quoted_identifiers_ignore_case) .map_err(|reason| outside_conditions(node_construct("statement"), reason))?; let locator = Locator::new(&tree, &positioned, lines); diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index c71a8f0b7..3f52bec9d 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -6,8 +6,9 @@ use crate::helpers::{ use crate::test_types::{ ColumnNameTestCase, ConvertDiagnosticTestCase, ConvertOutcome, ConvertTestCase, ConvertedRunTestCase, CountedCallsTestCase, CrossingTestCase, DedicatedOverrideTestCase, - FailureKindTestCase, OutputFormatTestCase, OverrideFailureTestCase, OverrideTestCase, - PolicyFreeTestCase, PythonRefusalTestCase, PythonRunTestCase, RunErrorTestCase, ValueTestCase, + DeepChainTestCase, FailureKindTestCase, OutputFormatTestCase, OverrideFailureTestCase, + OverrideTestCase, PolicyFreeTestCase, PythonRefusalTestCase, PythonRunTestCase, + RunErrorTestCase, ValueTestCase, }; use sha2::{Digest, Sha256}; @@ -1537,3 +1538,71 @@ fn given_numbers_beyond_38_digits_when_running_then_duckdb_raises_a_numeric_rang ); } } + +#[test] +fn given_a_long_flat_chain_when_converting_then_it_converts_or_is_refused_within_budget() { + let test_cases = [ + DeepChainTestCase { + description: "a long chain of additions is refused for its depth, not a stack overflow", + operator: " + ", + operand: "x", + terms: 3_000, + expected_exit: Some(1), + expected_mention: "SQ0006", + }, + DeepChainTestCase { + description: "a chain far past the depth limit is refused before conversion recurses", + operator: " - ", + operand: "x", + terms: 50_000, + expected_exit: Some(1), + expected_mention: "SQ0006", + }, + DeepChainTestCase { + description: "a long chain of || is refused for its depth, without quadratic memory", + operator: " || ", + operand: "s", + terms: 3_000, + expected_exit: Some(1), + expected_mention: "SQ0006", + }, + DeepChainTestCase { + description: "a || chain within the depth limit still converts", + operator: " || ", + operand: "s", + terms: 400, + expected_exit: Some(0), + expected_mention: "done, converted", + }, + DeepChainTestCase { + description: "a long AND chain, which the parser balances, still converts", + operator: " AND ", + operand: "b", + terms: 3_000, + expected_exit: Some(0), + expected_mention: "done, converted", + }, + ]; + for test_case in test_cases { + let sql = format!( + "SELECT {} AS y FROM (SELECT 1 AS x, 'a' AS s, TRUE AS b)", + vec![test_case.operand; test_case.terms].join(test_case.operator) + ); + let started = std::time::Instant::now(); + + let outcome = run_convert(&sql, &["--builtin-policy"], test_case.expected_mention); + + assert_eq!( + (outcome.exit_code, outcome.stderr_mentions), + (test_case.expected_exit, true), + "{}", + test_case.description + ); + assert!( + started.elapsed() < std::time::Duration::from_secs(20), + "{}: took {:?}", + test_case.description, + started.elapsed() + ); + } +} diff --git a/crates/sqlquack/tests/cli/test_types.rs b/crates/sqlquack/tests/cli/test_types.rs index b12faffb7..18007da69 100644 --- a/crates/sqlquack/tests/cli/test_types.rs +++ b/crates/sqlquack/tests/cli/test_types.rs @@ -267,3 +267,14 @@ pub(crate) struct TranslationStatementTestCase { /// One construct its statement must list as used. pub(crate) expected_use: &'static str, } + +/// A flat chain of `terms` copies of an operand joined by `operator`, and how converting it ends. +pub(crate) struct DeepChainTestCase { + pub(crate) description: &'static str, + pub(crate) operator: &'static str, + pub(crate) operand: &'static str, + pub(crate) terms: usize, + pub(crate) expected_exit: Option, + /// Text standard error must hold, such as the refusal's code. + pub(crate) expected_mention: &'static str, +} diff --git a/tests/unit/python/src/sqlquack/api/main/_test_types.py b/tests/unit/python/src/sqlquack/api/main/_test_types.py index 4ec27e595..b8cc4d383 100644 --- a/tests/unit/python/src/sqlquack/api/main/_test_types.py +++ b/tests/unit/python/src/sqlquack/api/main/_test_types.py @@ -49,3 +49,11 @@ class TypedDiagnosticTestCase: options: ConvertOptions expected_identity: tuple[str, str, str] expected_span: Span + + +@dataclass(frozen=True) +class DeepChainTestCase: + description: str + sql: str + expected_converted: bool + expected_codes: tuple[str, ...] diff --git a/tests/unit/python/src/sqlquack/api/main/test_convert.py b/tests/unit/python/src/sqlquack/api/main/test_convert.py index c3eff3cf6..44c8a5df9 100644 --- a/tests/unit/python/src/sqlquack/api/main/test_convert.py +++ b/tests/unit/python/src/sqlquack/api/main/test_convert.py @@ -10,6 +10,7 @@ from sqlquack import Conversion, ConvertOptions, Diagnostic from tests.unit.python.src.sqlquack.api.main._test_types import ( CommandLineTestCase, + DeepChainTestCase, FailureTestCase, TypedDiagnosticTestCase, ) @@ -138,3 +139,37 @@ def test_given_input_the_command_line_exits_2_on_when_converting_then_sqlquack_e monkeypatch.chdir(tmp_path) with pytest.raises(sqlquack.SqlQuackError, match=test_case.expected_message): sqlquack.convert(sql=test_case.sql, options=test_case.options) + + +@pytest.mark.parametrize( + "test_case", + ( + DeepChainTestCase( + description="a long chain of additions is refused, not a crash of the interpreter", + sql="SELECT " + " + ".join(["x"] * 3_000) + " AS y FROM (SELECT 1 AS x)", + expected_converted=False, + expected_codes=("SQ0006",), + ), + DeepChainTestCase( + description="a long chain of || is refused, not a crash of the interpreter", + sql="SELECT " + " || ".join(["s"] * 3_000) + " AS y FROM (SELECT 'a' AS s)", + expected_converted=False, + expected_codes=("SQ0006",), + ), + DeepChainTestCase( + description="a || chain within the depth limit converts", + sql="SELECT " + " || ".join(["s"] * 400) + " AS y FROM (SELECT 'a' AS s)", + expected_converted=True, + expected_codes=(), + ), + ), + ids=lambda case: case.description, +) +def test_given_a_long_flat_chain_when_converting_then_the_interpreter_survives( + test_case: DeepChainTestCase, +) -> None: + conversion: Conversion = sqlquack.convert( + sql=test_case.sql, options=ConvertOptions(builtin_policy=True) + ) + assert conversion.converted is test_case.expected_converted + assert tuple(found.code for found in conversion.diagnostics) == test_case.expected_codes