From a677aabdf9804e62038dcb7026f0e130b211befb Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Fri, 6 Nov 2026 11:37:52 +0000 Subject: [PATCH] feat: use the built-in policy when there is no policy file Without --policy and without sqlquack.toml in the current directory, convert, function, census, index and the Python API now use the built-in policy, which refuses every known divergence, instead of exiting 2. --builtin-policy still chooses it explicitly. A --policy file that does not exist, or an invalid policy file, is still an error. --- README.md | 2 +- .../src/diagnostics/constants.rs | 2 +- .../src/bindings/_helpers/convert_json.rs | 3 +- .../src/cli/_helpers/census_command.rs | 2 +- crates/sqlquack/src/cli/_helpers/evidence.rs | 28 ++++++---- .../src/cli/_helpers/function_command.rs | 2 +- .../src/cli/_helpers/index_command.rs | 2 +- .../sqlquack/src/cli/main/convert_report.rs | 2 +- crates/sqlquack/src/cli/models.rs | 8 +-- .../tests/census/test_census_binary.rs | 12 ++-- .../tests/cli/fixtures/invalid_policy.toml | 2 + crates/sqlquack/tests/cli/helpers.rs | 16 +++++- crates/sqlquack/tests/cli/test_convert.rs | 55 ++++++++++++++----- crates/sqlquack/tests/cli/test_types.rs | 1 + .../src/sqlquack/api/main/test_convert.py | 15 +++-- website/src/content/docs/docs/cli/convert.mdx | 2 +- .../docs/docs/diagnostics/reference.mdx | 2 +- website/src/content/docs/docs/policy.mdx | 9 +-- website/src/content/docs/docs/quickstart.mdx | 5 +- 19 files changed, 115 insertions(+), 55 deletions(-) create mode 100644 crates/sqlquack/tests/cli/fixtures/invalid_policy.toml diff --git a/README.md b/README.md index 394ecc542..5e694a305 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,7 @@ A `VALUES` list of more than 1,000 rows is refused for now. session parameters yet and convert under `utc-v1`. `convert`, `function`, `census` and `index` read `sqlquack.toml` (or `--policy`); without one they -exit 2 unless `--builtin-policy` asks for the built-in default, which refuses every known divergence. +use the built-in policy, which refuses every known divergence, as `--builtin-policy` asks for. They share the policy flags (`--policy`, `--builtin-policy`, `--strict`, `--utc-session`, `--confirm-declared-offsets`, `--python-runtime` and `--loud-divergences-only`, which relies only on divergences whose converted SQL raises rather than returns another value). Every command with a report takes diff --git a/crates/sqlquack-core/src/diagnostics/constants.rs b/crates/sqlquack-core/src/diagnostics/constants.rs index d150544c6..3f804a21f 100644 --- a/crates/sqlquack-core/src/diagnostics/constants.rs +++ b/crates/sqlquack-core/src/diagnostics/constants.rs @@ -155,7 +155,7 @@ pub const CODES: &[CodeEntry] = &[ help: Some( "the help names the divergence and the fixes that work in the current mode: allow \ it in the policy file, drop --strict or --loud-divergences-only, or confirm its \ - assumption; --builtin-policy refuses every one", + assumption; the built-in policy refuses every one", ), explanation: "The construct at the span converts only by relying on the known \ divergence the label names, where DuckDB may raise or differ from Snowflake as the \ diff --git a/crates/sqlquack-python/src/bindings/_helpers/convert_json.rs b/crates/sqlquack-python/src/bindings/_helpers/convert_json.rs index b265a356f..62089b9d3 100644 --- a/crates/sqlquack-python/src/bindings/_helpers/convert_json.rs +++ b/crates/sqlquack-python/src/bindings/_helpers/convert_json.rs @@ -6,7 +6,6 @@ use sqlquack::cli::main::convert_report::convert_report; use sqlquack::cli::models::{ConvertArguments, OutputArguments, OutputFormat, PolicyArguments}; use sqlquack_core::diagnostics::main::render_diagnostics::render_diagnostics; use sqlquack_core::diagnostics::models::Diagnostic; -use sqlquack_policy::policy::constants::POLICY_FILE_NAME; use crate::bindings::models::{ConvertRequest, Rendered}; @@ -26,7 +25,7 @@ pub(crate) fn convert_json(sql: &str, request: &str, path: &str) -> PyResult divergence_options(policy.as_ref(), &arguments.conversion), diff --git a/crates/sqlquack/src/cli/_helpers/evidence.rs b/crates/sqlquack/src/cli/_helpers/evidence.rs index 287788dc2..fb5f3e4b5 100644 --- a/crates/sqlquack/src/cli/_helpers/evidence.rs +++ b/crates/sqlquack/src/cli/_helpers/evidence.rs @@ -7,7 +7,7 @@ use sqlquack_core::diagnostics::models::Diagnostic; use sqlquack_core::refusals::models::Refusal; use sqlquack_evidence::cases::main::load_cases::load_cases; use sqlquack_evidence::cases::models::LoadedCase; -use sqlquack_policy::policy::constants::{SESSION_TIME_ZONE, TIMEZONE_PARAMETER}; +use sqlquack_policy::policy::constants::{POLICY_FILE_NAME, SESSION_TIME_ZONE, TIMEZONE_PARAMETER}; use sqlquack_policy::policy::main::load_policy::load_policy; use sqlquack_policy::policy::models::{DivergenceFailure, DivergenceMode, Policy}; @@ -32,15 +32,19 @@ pub(crate) fn load_evidence(paths: &EvidencePaths) -> Result { Ok(Evidence { policy, cases }) } -/// The policy at `path`, or none when `builtin` asks for the default; a missing file is an error. -pub(crate) fn chosen_policy(path: &Path, builtin: bool) -> Result, String> { - if builtin { - return Ok(None); - } +/// The `--policy` file, else an existing `sqlquack.toml`, else none (the built-in policy). +pub(crate) fn chosen_policy(path: Option<&Path>, builtin: bool) -> Result, String> { + let default = Path::new(POLICY_FILE_NAME); + let path = match (builtin, path) { + (true, _) => return Ok(None), + (false, Some(path)) => path, + (false, None) if default.is_file() => default, + (false, None) => return Ok(None), + }; if !path.is_file() { return Err(format!( - "no policy file at {}; pass --policy , or --builtin-policy to refuse every known \ - divergence", + "no policy file at {}; pass --policy with an existing file, or leave --policy out to \ + use the built-in policy, which refuses every known divergence", path.display() )); } @@ -123,11 +127,15 @@ fn divergence_help( ) -> String { let Some(policy) = policy else { return format!( - "--builtin-policy refuses every known divergence; pass --policy with a policy file \ + "the built-in policy refuses every known divergence; pass --policy with a policy file \ that allows `{id}`, such as the repository's sqlquack.toml" ); }; - let file = arguments.policy.display(); + let file = arguments + .policy + .as_deref() + .unwrap_or(Path::new(POLICY_FILE_NAME)) + .display(); let Some(divergence) = policy.divergences.get(id) else { return format!( "{file} has no [divergences.{id}]; copy it from the repository's sqlquack.toml" diff --git a/crates/sqlquack/src/cli/_helpers/function_command.rs b/crates/sqlquack/src/cli/_helpers/function_command.rs index eac31d2e6..68a5f172d 100644 --- a/crates/sqlquack/src/cli/_helpers/function_command.rs +++ b/crates/sqlquack/src/cli/_helpers/function_command.rs @@ -104,7 +104,7 @@ fn function_inputs( arguments: &FunctionArguments, ) -> Result<(Option, BTreeMap), String> { let policy = chosen_policy( - &arguments.conversion.policy, + arguments.conversion.policy.as_deref(), arguments.conversion.builtin_policy, )?; Ok((policy, parse_session(&arguments.session)?)) diff --git a/crates/sqlquack/src/cli/_helpers/index_command.rs b/crates/sqlquack/src/cli/_helpers/index_command.rs index 0ccedba70..df00e4a6c 100644 --- a/crates/sqlquack/src/cli/_helpers/index_command.rs +++ b/crates/sqlquack/src/cli/_helpers/index_command.rs @@ -31,7 +31,7 @@ pub(crate) fn index_command( ) -> ExitCode { let _ = writeln!(stderr, "sqlquack index: converting cases and SQL files…"); let policy = match chosen_policy( - &arguments.conversion.policy, + arguments.conversion.policy.as_deref(), arguments.conversion.builtin_policy, ) { Ok(policy) => policy, diff --git a/crates/sqlquack/src/cli/main/convert_report.rs b/crates/sqlquack/src/cli/main/convert_report.rs index 374271984..cb8a39389 100644 --- a/crates/sqlquack/src/cli/main/convert_report.rs +++ b/crates/sqlquack/src/cli/main/convert_report.rs @@ -20,7 +20,7 @@ pub fn convert_report(arguments: &ConvertArguments, sql: &str) -> Result, + /// Use the built-in policy even where a policy file exists: every known divergence is refused. #[arg(long, conflicts_with = "policy")] pub builtin_policy: bool, /// Refuse every known divergence the policy refuses in strict mode. diff --git a/crates/sqlquack/tests/census/test_census_binary.rs b/crates/sqlquack/tests/census/test_census_binary.rs index 11dbd8191..7d16fb5f0 100644 --- a/crates/sqlquack/tests/census/test_census_binary.rs +++ b/crates/sqlquack/tests/census/test_census_binary.rs @@ -68,14 +68,14 @@ fn given_sql_directory_when_running_census_binary_then_reports_on_the_right_stre }, }, BinaryTestCase { - description: "no policy file and no --builtin-policy is a usage error", + description: "without a policy file census runs under the built-in policy", arguments: &["census", "--json"], expected_outcome: BinaryOutcome { - exit_code: Some(2), - files: None, - unresolved_name: None, - completion_reported: false, - slowest_reported: false, + exit_code: Some(0), + files: Some(2), + unresolved_name: Some(2), + completion_reported: true, + slowest_reported: true, names_printed: false, }, }, diff --git a/crates/sqlquack/tests/cli/fixtures/invalid_policy.toml b/crates/sqlquack/tests/cli/fixtures/invalid_policy.toml new file mode 100644 index 000000000..91fc754b8 --- /dev/null +++ b/crates/sqlquack/tests/cli/fixtures/invalid_policy.toml @@ -0,0 +1,2 @@ +[divergences.hash] +default = 3 diff --git a/crates/sqlquack/tests/cli/helpers.rs b/crates/sqlquack/tests/cli/helpers.rs index 56dbcb2a7..c295c7760 100644 --- a/crates/sqlquack/tests/cli/helpers.rs +++ b/crates/sqlquack/tests/cli/helpers.rs @@ -217,10 +217,22 @@ fn converted_value_with_macros(report: &serde_json::Value) -> String { .expect("the converted statement returns a text value") } -/// The exit code of `arguments` run where no policy file is, over a one-statement file. -pub(crate) fn policy_free_exit_code(arguments: &[&str]) -> Option { +/// The exit code of `arguments` beside a copy of the repository's `policy_file`, over two SQL files. +pub(crate) fn policy_free_exit_code(arguments: &[&str], policy_file: Option<&str>) -> Option { let directory = tempfile::tempdir().expect("temporary directory"); std::fs::write(directory.path().join("query.sql"), "SELECT 1 AS total").expect("query written"); + std::fs::write( + directory.path().join("now.sql"), + "SELECT CURRENT_TIMESTAMP() AS t", + ) + .expect("query written"); + policy_file.iter().for_each(|path| { + std::fs::copy( + repository_root().join(path), + directory.path().join("sqlquack.toml"), + ) + .expect("policy copied"); + }); std::fs::write( directory.path().join("schema.json"), "{\"functions\": {\"TOTAL\": {\"language\": \"sql\", \"arguments\": [], \"returns\": \"NUMBER(1,0)\"}}}", diff --git a/crates/sqlquack/tests/cli/test_convert.rs b/crates/sqlquack/tests/cli/test_convert.rs index 6f54b5aa9..473a16997 100644 --- a/crates/sqlquack/tests/cli/test_convert.rs +++ b/crates/sqlquack/tests/cli/test_convert.rs @@ -426,25 +426,28 @@ fn given_quoted_names_when_converted_and_run_on_duckdb_then_output_names_follow_ } #[test] -fn given_no_policy_file_when_converting_or_counting_then_the_command_exits_2() { +fn given_no_policy_file_when_converting_or_counting_then_the_built_in_policy_applies() { let test_cases = [ PolicyFreeTestCase { - description: "convert", + description: "convert runs under the built-in policy", arguments: &["convert", "query.sql"], - expected_exit_code: Some(2), + policy_file: None, + expected_exit_code: Some(0), }, PolicyFreeTestCase { - description: "census", + description: "census runs under the built-in policy", arguments: &["census", "query.sql"], - expected_exit_code: Some(2), + policy_file: None, + expected_exit_code: Some(0), }, PolicyFreeTestCase { - description: "index", + description: "index runs under the built-in policy", arguments: &["index", "query.sql"], - expected_exit_code: Some(2), + policy_file: None, + expected_exit_code: Some(0), }, PolicyFreeTestCase { - description: "function", + description: "function runs under the built-in policy", arguments: &[ "function", "query.sql", @@ -453,17 +456,43 @@ fn given_no_policy_file_when_converting_or_counting_then_the_command_exits_2() { "--schema", "schema.json", ], - expected_exit_code: Some(2), + policy_file: None, + expected_exit_code: Some(0), + }, + PolicyFreeTestCase { + description: "the built-in policy refuses a divergence the repository policy allows", + arguments: &["convert", "now.sql"], + policy_file: None, + expected_exit_code: Some(1), }, PolicyFreeTestCase { - description: "census with --builtin-policy runs", - arguments: &["census", "--builtin-policy", "query.sql"], + description: "a sqlquack.toml in the directory is still read", + arguments: &["convert", "now.sql"], + policy_file: Some("sqlquack.toml"), expected_exit_code: Some(0), }, + PolicyFreeTestCase { + description: "--builtin-policy still refuses where a sqlquack.toml allows it", + arguments: &["convert", "--builtin-policy", "now.sql"], + policy_file: Some("sqlquack.toml"), + expected_exit_code: Some(1), + }, + PolicyFreeTestCase { + description: "a --policy file that does not exist is a usage error", + arguments: &["convert", "--policy", "missing.toml", "query.sql"], + policy_file: None, + expected_exit_code: Some(2), + }, + PolicyFreeTestCase { + description: "an invalid sqlquack.toml is a usage error", + arguments: &["convert", "query.sql"], + policy_file: Some("crates/sqlquack/tests/cli/fixtures/invalid_policy.toml"), + expected_exit_code: Some(2), + }, ]; for test_case in test_cases { assert_eq!( - policy_free_exit_code(test_case.arguments), + policy_free_exit_code(test_case.arguments, test_case.policy_file), test_case.expected_exit_code, "{}", test_case.description @@ -1513,7 +1542,7 @@ fn given_refused_sql_when_converting_then_text_and_json_carry_the_same_coded_dia expected_text: "error[SQ0008]: the conversion needs a known divergence the policy refuses: \ `fn:HASH`\n --> :1:8\n |\n1 | SELECT HASH(1) AS h\n | ^^^^^^^ fn:HASH \ needs known divergence `hash`, which the policy refuses in this mode\n |\n = help: \ - --builtin-policy refuses every known divergence; pass --policy with a policy \ + the built-in policy refuses every known divergence; pass --policy with a policy \ file that allows `hash`, such as the repository's sqlquack.toml\nsqlquack convert: refused, 1 refusal under the built-in default\n", expected_json: ("SQ0008", "divergence_refused", 1, 8), }, diff --git a/crates/sqlquack/tests/cli/test_types.rs b/crates/sqlquack/tests/cli/test_types.rs index 191b9a7bf..da310088e 100644 --- a/crates/sqlquack/tests/cli/test_types.rs +++ b/crates/sqlquack/tests/cli/test_types.rs @@ -126,6 +126,7 @@ pub(crate) struct ColumnNameTestCase { pub(crate) struct PolicyFreeTestCase { pub(crate) description: &'static str, pub(crate) arguments: &'static [&'static str], + pub(crate) policy_file: Option<&'static str>, pub(crate) expected_exit_code: Option, } 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 44c8a5df9..85dec139f 100644 --- a/tests/unit/python/src/sqlquack/api/main/test_convert.py +++ b/tests/unit/python/src/sqlquack/api/main/test_convert.py @@ -31,6 +31,13 @@ options=ConvertOptions(builtin_policy=True, schema="schema.json"), expected_converted=True, ), + CommandLineTestCase( + description="no policy flag and no sqlquack.toml: the built-in policy", + sql="SELECT order_id, price * 2 AS doubled FROM orders", + arguments=("--schema", "schema.json"), + options=ConvertOptions(schema="schema.json"), + expected_converted=True, + ), CommandLineTestCase( description="a query refused for an undeclared relation", sql="SELECT a FROM customers", @@ -113,10 +120,10 @@ def test_given_a_refusal_when_converting_then_the_diagnostic_is_typed( "test_case", ( FailureTestCase( - description="no policy file and no built-in policy, as on the command line", - sql="SELECT 1", - options=None, - expected_message="no policy file at sqlquack.toml", + description="a policy file that does not exist, as on the command line", + sql="SELECT 1 AS one", + options=ConvertOptions(policy="missing.toml"), + expected_message="no policy file at missing.toml", ), FailureTestCase( description="input holding no statement", diff --git a/website/src/content/docs/docs/cli/convert.mdx b/website/src/content/docs/docs/cli/convert.mdx index 2da414bc7..2c28b1d8a 100644 --- a/website/src/content/docs/docs/cli/convert.mdx +++ b/website/src/content/docs/docs/cli/convert.mdx @@ -90,4 +90,4 @@ A refusal in `refusals` carries the same [diagnostic fields](/docs/diagnostics/o |------|---------| | `0` | Converted. | | `1` | Something was refused; the reasons are on standard error. | -| `2` | It could not run: bad arguments, an unreadable file, no policy file, or input without SQL. | +| `2` | It could not run: bad arguments, an unreadable file, a missing `--policy` file or an invalid policy file, or input without SQL. | diff --git a/website/src/content/docs/docs/diagnostics/reference.mdx b/website/src/content/docs/docs/diagnostics/reference.mdx index 9b0f820af..61b308d1f 100644 --- a/website/src/content/docs/docs/diagnostics/reference.mdx +++ b/website/src/content/docs/docs/diagnostics/reference.mdx @@ -128,7 +128,7 @@ echo "SELECT 1 AS one" | sqlquack convert --builtin-policy --session WEEK_START= The construct at the span converts only by relying on the known divergence the label names, where DuckDB may raise or differ from Snowflake as the policy file describes. The policy refuses that divergence in the current mode, so the statement is refused. Allow it in the policy file, use a mode that allows it, or confirm the assumption it rests on; otherwise rewrite the statement without the construct. -**Help:** the help names the divergence and the fixes that work in the current mode: allow it in the policy file, drop --strict or --loud-divergences-only, or confirm its assumption; --builtin-policy refuses every one +**Help:** the help names the divergence and the fixes that work in the current mode: allow it in the policy file, drop --strict or --loud-divergences-only, or confirm its assumption; the built-in policy refuses every one **Example:** diff --git a/website/src/content/docs/docs/policy.mdx b/website/src/content/docs/docs/policy.mdx index 3bace9e65..ff2398c2f 100644 --- a/website/src/content/docs/docs/policy.mdx +++ b/website/src/content/docs/docs/policy.mdx @@ -21,8 +21,9 @@ error[SQ0008]: the conversion needs a known divergence the policy refuses: `fn:H ## Choosing a policy `convert`, `function`, `census` and `index` read `sqlquack.toml` from the current directory, or the -file `--policy` names. Without one they exit 2, unless `--builtin-policy` asks for the built-in -default, which refuses every known divergence. +file `--policy` names. Without one they use the built-in policy, which refuses every known +divergence; `--builtin-policy` chooses it even where a `sqlquack.toml` exists. A `--policy` file +that does not exist, or a policy file that is not valid, is an error. A policy file is complete on its own: the comparator, session profiles and edge-case tables are required, and a divergence the file leaves out is refused. A file holding only one @@ -49,8 +50,8 @@ sqlquack convert: done, converted under utc-v1 | Flag | Effect | |------|--------| -| `--policy ` | The policy file. Default `sqlquack.toml`. | -| `--builtin-policy` | Use the built-in default: every known divergence is refused. | +| `--policy ` | The policy file. Default `sqlquack.toml` in the current directory, or the built-in policy without one. | +| `--builtin-policy` | Use the built-in policy even where a `sqlquack.toml` exists: every known divergence is refused. | | `--strict` | Refuse every divergence the policy refuses in strict mode. | | `--utc-session` | Confirm both sessions run in UTC, so strict mode relies on `session_timezone_utc`. | | `--confirm-declared-offsets` | Confirm the `TIMESTAMP_TZ` offsets the schema declares. | diff --git a/website/src/content/docs/docs/quickstart.mdx b/website/src/content/docs/docs/quickstart.mdx index c647d0e30..c48235429 100644 --- a/website/src/content/docs/docs/quickstart.mdx +++ b/website/src/content/docs/docs/quickstart.mdx @@ -93,8 +93,9 @@ sqlquack convert: done, converted under the built-in default The DuckDB SQL goes to standard output and the progress lines to standard error, so `sqlquack convert revenue.sql … > revenue.duckdb.sql` keeps only the SQL. -`--builtin-policy` refuses every known divergence. Without it, `convert` reads `sqlquack.toml` -from the current directory, or the file `--policy` names. See [Policy and sessions](/docs/policy/). +`convert` reads `sqlquack.toml` from the current directory, or the file `--policy` names. Without +one, or with `--builtin-policy`, it uses the built-in policy, which refuses every known divergence. +See [Policy and sessions](/docs/policy/). ## Next steps