From 7791fa7a9c642a438ac0a6bc83de83483506fbbd Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:03:48 +0100 Subject: [PATCH 1/5] fix(format): preserve compiler-compatible SQL --- .../src/sql_lint/_helpers/formatter.rs | 77 +++++ .../src/sql_lint/tests/test_sql_lint.rs | 6 + .../cli/commands/_helpers/lint/selection.py | 111 ++++++- .../cli/commands/main/project/_format.py | 2 +- src/sqlbuild/lint/_helpers/headers.py | 4 + src/sqlbuild/lint/_helpers/native.py | 68 ++++- src/sqlbuild/lint/_helpers/native_format.py | 64 +++- src/sqlbuild/lint/constants.py | 1 + src/sqlbuild/lint/main/run_format.py | 44 ++- src/sqlbuild/lint/models.py | 8 + .../sqlbuild/cli/commands/main/_test_types.py | 41 +++ .../src/sqlbuild/cli/commands/main/helpers.py | 36 +++ .../sqlbuild/cli/commands/main/test_format.py | 280 ++++++++++++++++++ .../sqlbuild/lint/_helpers/test_headers.py | 8 + 14 files changed, 720 insertions(+), 30 deletions(-) diff --git a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs index bf432bf0a1..7a5074115b 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs @@ -119,6 +119,7 @@ fn format_once(neutral_sql: &str, context: &FormatOnceContext<'_>) -> Result) -> Result, +) -> Result { + let authored_count = context + .original_tokens + .windows(2) + .filter(|tokens| { + token_text(context.original_sql, &tokens[0]).as_deref() == Some("FROM") + && token_text(context.original_sql, &tokens[1]).as_deref() == Some("VALUES") + }) + .count(); + if authored_count == 0 { + return Ok(formatted); + } + let formatted_tokens = context + .dialect + .tokenize(&formatted) + .map_err(|error| error.to_string())?; + let mut removals: Vec<(usize, usize)> = Vec::new(); + let mut search_start = 0_usize; + for _ in 0..authored_count { + let Some(from_index) = + (search_start..formatted_tokens.len().saturating_sub(1)).find(|&index| { + token_text(&formatted, &formatted_tokens[index]).as_deref() == Some("FROM") + && (token_text(&formatted, &formatted_tokens[index + 1]).as_deref() + == Some("VALUES") + || (index + 2 < formatted_tokens.len() + && token_text(&formatted, &formatted_tokens[index + 1]).as_deref() + == Some(OPEN_PARENTHESIS) + && token_text(&formatted, &formatted_tokens[index + 2]).as_deref() + == Some("VALUES"))) + }) + else { + return Err(UNSUPPORTED_SQL_FAILURE.to_string()); + }; + if token_text(&formatted, &formatted_tokens[from_index + 1]).as_deref() == Some("VALUES") { + search_start = from_index + 2; + continue; + } + let wrapper_index = from_index + 1; + let mut depth = 0_i32; + let mut close_index: Option = None; + for (index, token) in formatted_tokens.iter().enumerate().skip(wrapper_index) { + match token_text(&formatted, token).as_deref() { + Some(OPEN_PARENTHESIS) => depth += 1, + Some(CLOSE_PARENTHESIS) => { + depth -= 1; + if depth == 0 { + close_index = Some(index); + break; + } + } + _ => {} + } + } + let close_index = close_index.ok_or_else(|| UNSUPPORTED_SQL_FAILURE.to_string())?; + let open = &formatted_tokens[wrapper_index]; + let close = &formatted_tokens[close_index]; + removals.push((open.span.start, open.span.end)); + removals.push((close.span.start, close.span.end)); + search_start = close_index + 1; + } + removals.sort_unstable(); + for (start, end) in removals.into_iter().rev() { + let start = char_to_byte(&formatted, start)?; + let end = char_to_byte(&formatted, end)?; + formatted.replace_range(start..end, ""); + } + Ok(formatted) +} + +fn token_text(sql: &str, token: &Token) -> Option { + char_slice(sql, token.span.start, token.span.end).map(|value| value.to_ascii_uppercase()) +} + fn comment_insertion( formatted: &str, formatted_tokens: &[Token], diff --git a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_sql_lint.rs b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_sql_lint.rs index d8b4dddba5..71155cae67 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_sql_lint.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_sql_lint.rs @@ -1893,6 +1893,12 @@ fn given_format_cases_when_formatting_then_output_matches() -> Result<(), String expected_sql: "SELECT\n a\nFROM items -- retained\n", expected_changed: true, }, + test_types::FormatTestCase { + description: "unparenthesized values relation remains unparenthesized", + sql: "select column1::varchar as customer_key, column2::integer as order_count from values ('c1', 1), ('c2', 2)", + expected_sql: "SELECT\n CAST(column1 AS VARCHAR) AS customer_key,\n CAST(column2 AS INT) AS order_count\nFROM VALUES ('c1', 1), ('c2', 2)", + expected_changed: true, + }, ]; for test_case in &test_cases { diff --git a/src/sqlbuild/cli/commands/_helpers/lint/selection.py b/src/sqlbuild/cli/commands/_helpers/lint/selection.py index 8c57ba6923..615f493922 100644 --- a/src/sqlbuild/cli/commands/_helpers/lint/selection.py +++ b/src/sqlbuild/cli/commands/_helpers/lint/selection.py @@ -66,6 +66,37 @@ def resolve_lint_inputs( ) if not select and not exclude: return adapter, None, discovered + combined_paths: frozenset[Path] | None = _resolve_combined_lint_paths( + project_dir=project_dir, + discovered=discovered, + adapter=adapter, + select=select, + exclude=exclude, + ) + if combined_paths is not None: + _validate_selected_paths(paths=combined_paths) + return adapter, combined_paths, discovered + paths: frozenset[Path] = _resolve_model_paths( + project_dir=project_dir, + discovered=discovered, + adapter=adapter, + select=select, + exclude=exclude, + ) + _validate_selected_paths(paths=paths) + return adapter, paths, discovered + + +def _resolve_model_paths( + *, + project_dir: Path, + discovered: DiscoveredProjectInputs, + adapter: BaseAdapter, + select: tuple[str, ...], + exclude: tuple[str, ...], +) -> frozenset[Path]: + """Resolve model selectors to authored model paths.""" + exact_paths: frozenset[Path] | None = _resolve_exact_model_paths( project_dir=project_dir, discovered=discovered, @@ -73,8 +104,7 @@ def resolve_lint_inputs( exclude=exclude, ) if exact_paths is not None: - _validate_selected_paths(paths=exact_paths) - return adapter, exact_paths, discovered + return exact_paths graph: ProjectGraph = build_project_graph(discovered_inputs=discovered, adapter=adapter) selected_keys: frozenset[CompiledObjectKey] = resolve_project_selectors( select=select, @@ -88,13 +118,84 @@ def resolve_lint_inputs( selected_names: frozenset[str] = frozenset( key.name for key in selected_keys if key.resource_type == CompiledResourceType.MODEL ) - paths: frozenset[Path] = frozenset( + return frozenset( (project_dir / model.relative_path).resolve() for model in graph.project.models if model.name in selected_names ) - _validate_selected_paths(paths=paths) - return adapter, paths, discovered + + +def _resolve_combined_lint_paths( + *, + project_dir: Path, + discovered: DiscoveredProjectInputs, + adapter: BaseAdapter, + select: tuple[str, ...], + exclude: tuple[str, ...], +) -> frozenset[Path] | None: + """Resolve default or path-based format scopes before graph selector expansion.""" + + if any(not selector.strip() for selector in (*select, *exclude)): + return None + selected_path_tokens, selected_model_tokens = _partition_lint_selectors(select) + excluded_path_tokens, excluded_model_tokens = _partition_lint_selectors(exclude) + has_path_selector: bool = bool(selected_path_tokens or excluded_path_tokens) + if not has_path_selector and select: + return None + all_paths: frozenset[Path] = frozenset( + _paths_for_prefixes(project_dir=project_dir, prefixes=()) + ) + if select: + selected: set[Path] = set( + _paths_for_prefixes(project_dir=project_dir, prefixes=selected_path_tokens) + if selected_path_tokens + else () + ) + if selected_model_tokens: + selected.update( + _resolve_model_paths( + project_dir=project_dir, + discovered=discovered, + adapter=adapter, + select=selected_model_tokens, + exclude=(), + ) + ) + else: + selected = set(all_paths) + excluded: set[Path] = set( + _paths_for_prefixes(project_dir=project_dir, prefixes=excluded_path_tokens) + if excluded_path_tokens + else () + ) + if excluded_model_tokens: + excluded.update( + _resolve_model_paths( + project_dir=project_dir, + discovered=discovered, + adapter=adapter, + select=excluded_model_tokens, + exclude=(), + ) + ) + return frozenset(selected - excluded) + + +def _partition_lint_selectors( + raw_selectors: tuple[str, ...], +) -> tuple[tuple[str, ...], tuple[str, ...]]: + """Separate valid formatter-root paths from model selectors.""" + + path_tokens: list[str] = [] + model_tokens: list[str] = [] + for raw_selector in raw_selectors: + for token in raw_selector.split(): + prefixes: tuple[str, ...] | None = _lint_path_prefixes(raw_selectors=(token,)) + if prefixes is not None: + path_tokens.extend(prefixes) + else: + model_tokens.append(token) + return tuple(path_tokens), tuple(model_tokens) def _resolve_discovered_adapter( diff --git a/src/sqlbuild/cli/commands/main/project/_format.py b/src/sqlbuild/cli/commands/main/project/_format.py index f42641ef3e..588fcac330 100644 --- a/src/sqlbuild/cli/commands/main/project/_format.py +++ b/src/sqlbuild/cli/commands/main/project/_format.py @@ -114,7 +114,7 @@ def run_format_command( show_formatted=True, formatted_heading="Would format files:" if check or diff else "Formatted files:", ) - return 1 if result.violations or (check and result.formatted_files) else 0 + return 1 if result.faults or (check and result.formatted_files) else 0 def _render_format_diff(*, result: LintRunResult) -> None: diff --git a/src/sqlbuild/lint/_helpers/headers.py b/src/sqlbuild/lint/_helpers/headers.py index 72fe23f732..98131f6426 100644 --- a/src/sqlbuild/lint/_helpers/headers.py +++ b/src/sqlbuild/lint/_helpers/headers.py @@ -63,12 +63,15 @@ def _scan_delimited_regions( """Return line-leading parenthesized regions with one of the requested names.""" spans: list[HeaderSpan] = [] + covered_until: int = 0 match: re.Match[str] for match in _delimited_region_pattern(kinds).finditer(contents): kind: str | None = match.group("kind") if kind is None: continue keyword_start: int = match.start("kind") + if keyword_start < covered_until: + continue span: HeaderSpan | None = _match_header_span( contents=contents, kind=kind, @@ -77,6 +80,7 @@ def _scan_delimited_regions( ) if span is not None: spans.append(span) + covered_until = span.end if first_only: break return tuple(spans) diff --git a/src/sqlbuild/lint/_helpers/native.py b/src/sqlbuild/lint/_helpers/native.py index ac9dcd97c9..553eb60398 100644 --- a/src/sqlbuild/lint/_helpers/native.py +++ b/src/sqlbuild/lint/_helpers/native.py @@ -14,12 +14,14 @@ ) from sqlbuild.compiler.discovery.exceptions import ModelSqlParseError from sqlbuild.lint.constants import ( + CARRIAGE_RETURN_LINE_FEED, CLOSING_PAREN_CHARACTER, DESCRIPTION_HEADER_KINDS, DESCRIPTION_REQUIRED_HEADER_KINDS, HEADER_KIND_FUNCTION, HEADER_KIND_MODEL, IDENTIFIER_SEPARATOR_CHARACTER, + LINE_FEED, RULE_DESCRIPTION_LENGTH, RULE_DESCRIPTION_PRESENT, RULE_HEADER_PARSE, @@ -29,6 +31,7 @@ VIOLATION_SEVERITY_WARNING, ) from sqlbuild.lint.models import HeaderSpan, LintConfig, LintViolation +from sqlbuild.lint.types import LintSeverity _QUOTE_CHARACTERS: frozenset[str] = frozenset({"'", '"'}) _ESCAPE_CHARACTER: str = "\\" @@ -59,6 +62,7 @@ def lint_native_headers( file_path: Path, headers: tuple[HeaderSpan, ...], config: LintConfig, + description_present_severity: LintSeverity = VIOLATION_SEVERITY_FAULT, ) -> tuple[LintViolation, ...]: """Run all native header rules and return their violations.""" @@ -73,6 +77,7 @@ def lint_native_headers( header=header, config=config, line_starts=line_starts, + description_present_severity=description_present_severity, ) ) violations.extend( @@ -100,6 +105,64 @@ def prepare_native_header_cache(*, files: dict[Path, str]) -> None: prepare_model_header_tokens(headers) +def safe_format_files( + *, files: dict[Path, str], config: LintConfig +) -> tuple[dict[Path, str], dict[Path, str]]: + """Return parse-safe files together with every authored newline convention.""" + + safe_files: dict[Path, str] = { + file_path: contents + for file_path, contents in files.items() + if not _header_parse_faults(file_path=file_path, contents=contents, config=config) + } + newline_by_path: dict[Path, str] = { + file_path: CARRIAGE_RETURN_LINE_FEED if CARRIAGE_RETURN_LINE_FEED in contents else LINE_FEED + for file_path, contents in files.items() + } + return safe_files, newline_by_path + + +def reject_unparseable_header_rewrites( + *, + updated: dict[Path, str], + config: LintConfig, + faults: list[LintViolation], +) -> tuple[dict[Path, str], list[LintViolation]]: + """Discard every file rewrite whose resulting header cannot be parsed.""" + + accepted: dict[Path, str] = dict(updated) + for file_path, contents in tuple(updated.items()): + parse_faults: tuple[LintViolation, ...] = _header_parse_faults( + file_path=file_path, + contents=contents, + config=config, + ) + if not parse_faults: + continue + accepted.pop(file_path, None) + faults.extend(parse_faults) + return accepted, faults + + +def _header_parse_faults( + *, file_path: Path, contents: str, config: LintConfig +) -> tuple[LintViolation, ...]: + from sqlbuild.lint._helpers.headers import scan_headers + + headers: tuple[HeaderSpan, ...] = scan_headers(contents=contents) + return tuple( + violation + for violation in lint_native_headers( + contents=contents, + file_path=file_path, + headers=headers, + config=config, + description_present_severity=VIOLATION_SEVERITY_WARNING, + ) + if violation.code == RULE_HEADER_PARSE + ) + + def format_native_headers( *, contents: str, @@ -134,6 +197,7 @@ def _lint_header_values( header: HeaderSpan, config: LintConfig, line_starts: tuple[int, ...], + description_present_severity: LintSeverity, ) -> tuple[LintViolation, ...]: header_text: str = contents[header.start : header.end] try: @@ -180,6 +244,7 @@ def _lint_header_values( code=RULE_DESCRIPTION_PRESENT, message=f"{header.kind}() header requires a description", remediation=f"Add a description to the {header.kind}() header.", + severity=description_present_severity, ) ) if header.kind in DESCRIPTION_HEADER_KINDS and isinstance(effective_description, str): @@ -598,6 +663,7 @@ def _violation_for_header_start( code: str, message: str, remediation: str, + severity: LintSeverity = VIOLATION_SEVERITY_FAULT, ) -> LintViolation: position: tuple[int, int] = _offset_to_position( offset=header.start, line_starts=_line_starts(contents) @@ -608,7 +674,7 @@ def _violation_for_header_start( column=position[1], code=code, message=message, - severity=VIOLATION_SEVERITY_FAULT, + severity=severity, engine="sqlbuild", remediation=remediation, ) diff --git a/src/sqlbuild/lint/_helpers/native_format.py b/src/sqlbuild/lint/_helpers/native_format.py index 24b16dc631..11f665cbd8 100644 --- a/src/sqlbuild/lint/_helpers/native_format.py +++ b/src/sqlbuild/lint/_helpers/native_format.py @@ -10,9 +10,21 @@ import sqlbuild._native as _native from sqlbuild.lint._helpers.headers import lint_body_ranges, scan_headers from sqlbuild.lint._helpers.sqlbuild_tokens import neutralize_interpolation, restore_interpolation -from sqlbuild.lint.constants import CARRIAGE_RETURN_LINE_FEED, LINE_FEED +from sqlbuild.lint.constants import ( + CARRIAGE_RETURN_LINE_FEED, + LINE_FEED, + LINT_ENGINE_NATIVE, + RULE_FORMAT_SAFETY, + VIOLATION_SEVERITY_FAULT, +) from sqlbuild.lint.exceptions import NativeLintError -from sqlbuild.lint.models import HeaderSpan, InterpolationSite, LintConfig +from sqlbuild.lint.models import ( + HeaderSpan, + InterpolationSite, + LintConfig, + LintViolation, + NativeFormatResult, +) _NATIVE_FORMAT_API_VERSION: int = 1 @@ -45,8 +57,8 @@ def with_newline_style(*, contents: str, newline: str) -> str: def format_native_sql_bodies( *, files: dict[Path, str], config: LintConfig, project_dir: Path -) -> dict[Path, str]: - """Format supported authored SQL bodies and preserve unsupported bodies unchanged.""" +) -> NativeFormatResult: + """Format authored SQL bodies without partially rewriting rejected files.""" prepared_by_path: dict[Path, tuple[_PreparedBody, ...]] = {} requests_by_key: dict[tuple[str, str], dict[str, object]] = {} @@ -87,14 +99,31 @@ def format_native_sql_bodies( requests_by_key=requests_by_key ) formatted_files: dict[Path, str] = {} + faults: list[LintViolation] = [] for file_path, contents in sorted(files.items()): updated: str = contents + file_faults: list[LintViolation] = [] for prepared in reversed(prepared_by_path[file_path]): response: dict[str, Any] = response_cache[prepared.cache_key] raw_sql: object = response.get("sql") changed: object = response.get("changed") - if not isinstance(raw_sql, str) or not isinstance(changed, bool): + formatted: object = response.get("formatted") + if ( + not isinstance(raw_sql, str) + or not isinstance(changed, bool) + or not isinstance(formatted, bool) + ): raise NativeLintError("native formatter returned invalid SQL or changed state") + if not formatted: + file_faults.append( + _format_safety_fault( + file_path=file_path, + contents=contents, + body_start=prepared.start, + reason=response.get("reason"), + ) + ) + continue if not changed: continue restored: str = restore_interpolation( @@ -104,9 +133,30 @@ def format_native_sql_bodies( updated = ( f"{updated[: prepared.start]}{restored}{prepared.trailing}{updated[prepared.end :]}" ) - if updated != contents: + if file_faults: + faults.extend(reversed(file_faults)) + elif updated != contents: formatted_files[file_path] = updated - return formatted_files + return NativeFormatResult(formatted_files=formatted_files, faults=tuple(faults)) + + +def _format_safety_fault( + *, file_path: Path, contents: str, body_start: int, reason: object +) -> LintViolation: + line: int = contents.count("\n", 0, body_start) + 1 + previous_newline: int = contents.rfind("\n", 0, body_start) + column: int = body_start - previous_newline + detail: str = reason if isinstance(reason, str) and reason else "unknown formatter rejection" + return LintViolation( + file_path=file_path, + line=line, + column=column, + code=RULE_FORMAT_SAFETY, + message=f"SQL body could not be formatted safely: {detail}", + severity=VIOLATION_SEVERITY_FAULT, + engine=LINT_ENGINE_NATIVE, + remediation="Correct the SQL syntax or leave the file unchanged and report the parser gap.", + ) def _format_responses( diff --git a/src/sqlbuild/lint/constants.py b/src/sqlbuild/lint/constants.py index e39076ac2e..d8706e0989 100644 --- a/src/sqlbuild/lint/constants.py +++ b/src/sqlbuild/lint/constants.py @@ -60,6 +60,7 @@ RULE_LEADING_COMMENT_DESCRIPTION: str = "leading-comment-description" RULE_HEADER_WHITESPACE: str = "header-whitespace" RULE_HEADER_PARSE: str = "header-parse" +RULE_FORMAT_SAFETY: str = "format-safety" PROJECT_CONFIG_FILENAME_KEY: str = "sqlbuild_project.toml" FORMAT_SECTION_KEY: str = "format" diff --git a/src/sqlbuild/lint/main/run_format.py b/src/sqlbuild/lint/main/run_format.py index c1bca1d779..ee62335ab5 100644 --- a/src/sqlbuild/lint/main/run_format.py +++ b/src/sqlbuild/lint/main/run_format.py @@ -12,20 +12,23 @@ format_native_headers, lint_native_headers, prepare_native_header_cache, + reject_unparseable_header_rewrites, + safe_format_files, ) from sqlbuild.lint._helpers.native_format import ( format_native_sql_bodies, - newline_style, with_newline_style, ) from sqlbuild.lint._helpers.project_files import collect_project_files, sort_violations from sqlbuild.lint._helpers.suppressions import apply_suppressions +from sqlbuild.lint.constants import VIOLATION_SEVERITY_WARNING from sqlbuild.lint.models import ( FormatChange, HeaderSpan, LintConfig, LintRunResult, LintViolation, + NativeFormatResult, ) @@ -44,11 +47,9 @@ def run_format( files: dict[Path, str] = collect_project_files( project_dir=project_dir, selected_paths=selected_paths ) - newline_by_path: dict[Path, str] = { - file_path: newline_style(contents=contents) for file_path, contents in files.items() - } - updated_contents: dict[Path, str] = _apply_fixes( - files=files, + safe_files, newline_by_path = safe_format_files(files=files, config=config) + updated_contents, format_faults = _apply_fixes( + files=safe_files, config=config, project_dir=project_dir, discovered_inputs=discovered_inputs, @@ -89,6 +90,7 @@ def run_format( value_renderer=value_renderer, ) ) + violations.extend(format_faults) final_contents: dict[Path, str] = { path: updated_contents.get(path, contents) for path, contents in files.items() } @@ -113,7 +115,7 @@ def _apply_fixes( project_dir: Path, discovered_inputs: DiscoveredProjectInputs | None, fixtures_only: bool, -) -> dict[Path, str]: +) -> tuple[dict[Path, str], list[LintViolation]]: """Return contents after native header and supported SQL body fixes.""" updated: dict[Path, str] = ( @@ -126,7 +128,7 @@ def _apply_fixes( else {} ) if fixtures_only: - return updated + return updated, [] file_path: Path contents: str current_files: dict[Path, str] = { @@ -142,18 +144,27 @@ def _apply_fixes( if native_result[0] != contents: updated[file_path] = native_result[0] if not config.native_enabled: - return updated + return reject_unparseable_header_rewrites( + updated=updated, + config=config, + faults=[], + ) current_files = { file_path: updated.get(file_path, contents) for file_path, contents in files.items() } - updated.update( - format_native_sql_bodies( - files=current_files, - config=config, - project_dir=project_dir, - ) + native_result: NativeFormatResult = format_native_sql_bodies( + files=current_files, + config=config, + project_dir=project_dir, + ) + updated.update(native_result.formatted_files) + for fault in native_result.faults: + updated.pop(fault.file_path, None) + return reject_unparseable_header_rewrites( + updated=updated, + config=config, + faults=list(native_result.faults), ) - return updated def _lint_final_contents( @@ -179,6 +190,7 @@ def _lint_final_contents( file_path=file_path, headers=headers, config=config, + description_present_severity=VIOLATION_SEVERITY_WARNING, ) ) return violations diff --git a/src/sqlbuild/lint/models.py b/src/sqlbuild/lint/models.py index c0079f988a..507afee98b 100644 --- a/src/sqlbuild/lint/models.py +++ b/src/sqlbuild/lint/models.py @@ -95,6 +95,14 @@ class FormatChange: after: str +@dataclass(frozen=True) +class NativeFormatResult: + """Safe SQL body rewrites and files rejected by the formatter boundary.""" + + formatted_files: dict[Path, str] + faults: tuple[LintViolation, ...] + + @dataclass(frozen=True) class LintFixRecord: """One applied or skipped lint repair for human and machine reporting.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 73dec1c303..60dc502ef5 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -159,6 +159,47 @@ class DescriptionFormatIntegrationTestCase: expected_formatted_description: str +@dataclass(frozen=True) +class FormatSafetyIntegrationTestCase: + """One format safety expectation through the real CLI.""" + + description: str + authored_sql: str + expected_fault_code: str + expected_exit_code: int + + +@dataclass(frozen=True) +class FromValuesFormatIntegrationTestCase: + """One dialect expectation for an unparenthesized values relation.""" + + description: str + adapter: str + expected_exit_code: int + expected_literal: str + + +@dataclass(frozen=True) +class FormatScopeIntegrationTestCase: + """Expected CLI outcomes for the format selection contract.""" + + description: str + expected_exclude_exit: int + expected_selected_model_exit: int + expected_path_with_exclude_exit: int + expected_exclude_path_exit: int + + +@dataclass(frozen=True) +class FormatWarningIntegrationTestCase: + """One non-failing format warning expectation.""" + + description: str + expected_exit_code: int + expected_code: str + expected_severity: str + + @dataclass(frozen=True) class ContractCommandIntegrationTestCase: """One target-backed contract CLI expectation.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py index 2d66509057..406315fa55 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py @@ -28,6 +28,42 @@ def write_compile_startup_project(project_dir: Path) -> None: ) +def write_from_values_format_project(*, tmp_path: Path, adapter: str) -> tuple[Path, Path]: + """Write a format project containing an authored values relation.""" + + project_dir: Path = tmp_path / adapter + project_dir.mkdir() + (project_dir / "sqlbuild_project.toml").write_text( + f'name = "orders"\nadapter = "{adapter}"\n\n[rules]\nselect = ["SQBRSQL039"]\n', + encoding="utf-8", + ) + models: Path = project_dir / "models" + models.mkdir() + (models / "customers.sql").write_text( + 'MODEL (description "Customers.");\nSELECT 1 AS customer_key\n', encoding="utf-8" + ) + (models / "orders.sql").write_text( + 'MODEL (description "Orders.");\nSELECT customer_key, 1 AS order_count ' + 'FROM __ref("customers")\n', + encoding="utf-8", + ) + test_file: Path = project_dir / "tests" / "unit" / "test_orders.sql" + test_file.parent.mkdir(parents=True) + test_file.write_text( + 'TEST (name "orders_from_values");\n\n' + "WITH __ref__customers AS (\n" + " SELECT COLUMN1::VARCHAR AS customer_key, COLUMN2::INTEGER AS order_count\n" + " FROM VALUES\n" + " ('c1', 1),\n" + " ('c2', 2)\n" + "),\n" + "__expected__orders AS (SELECT 'c1' AS customer_key, 1 AS order_count)\n" + "SELECT 1\n", + encoding="utf-8", + ) + return project_dir, test_file + + def prepare_contract_project(tmp_path: Path, *, prod_connection_toml: str = "") -> Path: database: Path = tmp_path / "warehouse.duckdb" _ = (tmp_path / "sqlbuild_project.toml").write_text( diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py index 6f9d7b7ebf..1f213b2225 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py @@ -2,14 +2,23 @@ from __future__ import annotations +import json from pathlib import Path import pytest +from _pytest.capture import CaptureResult from sqlbuild.cli.commands.main.entrypoint.entry import main from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( DescriptionFormatIntegrationTestCase, FormatCompileIntegrationTestCase, + FormatSafetyIntegrationTestCase, + FormatScopeIntegrationTestCase, + FormatWarningIntegrationTestCase, + FromValuesFormatIntegrationTestCase, +) +from tests.integration.src.sqlbuild.cli.commands.main.helpers import ( + write_from_values_format_project, ) @@ -303,3 +312,274 @@ def test_given_description_width_when_formatting_then_wrapped_model_compiles( assert f'description "{test_case.expected_formatted_description}"' in model.read_text( encoding="utf-8" ) + + +@pytest.mark.parametrize( + "test_case", + [ + FormatCompileIntegrationTestCase( + description="schema column remains valid and idempotent", + expected_literal="SCHEMA (type VARCHAR)", + ) + ], + ids=lambda case: case.description, +) +def test_given_schema_column_when_formatting_then_header_remains_valid_and_idempotent( + test_case: FormatCompileIntegrationTestCase, + tmp_path: Path, +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "catalog"\nadapter = "duckdb"\n', encoding="utf-8" + ) + model: Path = tmp_path / "models" / "catalog_objects.sql" + model.parent.mkdir() + model.write_text( + "MODEL (\n" + ' description "Catalog objects.",\n' + " materialized table,\n" + " columns (\n" + " object_id (type INTEGER),\n" + " SCHEMA (type VARCHAR),\n" + " ),\n" + ");\n\n" + "select 1 as object_id, 'analytics' as \"SCHEMA\"\n", + encoding="utf-8", + ) + + first_exit: int = main(["--project-dir", str(tmp_path), "format"]) + compile_exit: int = main(["--project-dir", str(tmp_path), "compile", "--no-cache"]) + formatted_once: str = model.read_text(encoding="utf-8") + second_exit: int = main(["--project-dir", str(tmp_path), "format", "--check"]) + + assert first_exit == 0 + assert compile_exit == 0 + assert second_exit == 0 + assert formatted_once == model.read_text(encoding="utf-8") + assert formatted_once.count(test_case.expected_literal) == 1 + + +@pytest.mark.parametrize( + "test_case", + [ + FormatSafetyIntegrationTestCase( + description="unparseable header is untouched", + authored_sql="MODEL (description);\nselect 1 as order_id\n", + expected_fault_code="header-parse", + expected_exit_code=1, + ), + FormatSafetyIntegrationTestCase( + description="unparseable SQL is untouched", + authored_sql='MODEL (description "Orders." );\nselect from\n', + expected_fault_code="format-safety", + expected_exit_code=1, + ), + ], + ids=lambda case: case.description, +) +def test_given_unparseable_file_when_formatting_then_file_is_untouched_and_faults( + test_case: FormatSafetyIntegrationTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" + ) + model: Path = tmp_path / "models" / "orders.sql" + model.parent.mkdir() + model.write_text(test_case.authored_sql, encoding="utf-8") + + exit_code: int = main(["--project-dir", str(tmp_path), "format"]) + + captured: CaptureResult[str] = capsys.readouterr() + assert exit_code == test_case.expected_exit_code + assert test_case.expected_fault_code in captured.out + assert model.read_text(encoding="utf-8") == test_case.authored_sql + + +@pytest.mark.parametrize( + "test_case", + [ + FromValuesFormatIntegrationTestCase( + description="snowflake preserves unparenthesized values relation", + adapter="snowflake", + expected_exit_code=0, + expected_literal="FROM VALUES", + ) + ], + ids=lambda case: case.description, +) +def test_given_snowflake_from_values_when_formatting_then_form_is_preserved( + test_case: FromValuesFormatIntegrationTestCase, + tmp_path: Path, +) -> None: + project_dir, test_file = write_from_values_format_project( + tmp_path=tmp_path, adapter=test_case.adapter + ) + + first_exit: int = main(["--project-dir", str(project_dir), "format"]) + formatted_once: str = test_file.read_text(encoding="utf-8") + second_exit: int = main(["--project-dir", str(project_dir), "format", "--check"]) + + assert first_exit == test_case.expected_exit_code + assert second_exit == test_case.expected_exit_code + assert test_case.expected_literal in formatted_once + assert "FROM (VALUES" not in formatted_once + assert formatted_once == test_file.read_text(encoding="utf-8") + + +@pytest.mark.parametrize( + "test_case", + [ + FromValuesFormatIntegrationTestCase( + description="duckdb values relation remains valid and idempotent", + adapter="duckdb", + expected_exit_code=0, + expected_literal="FROM VALUES", + ) + ], + ids=lambda case: case.description, +) +def test_given_duckdb_from_values_when_formatting_then_project_compiles( + test_case: FromValuesFormatIntegrationTestCase, + tmp_path: Path, +) -> None: + project_dir, test_file = write_from_values_format_project( + tmp_path=tmp_path, adapter=test_case.adapter + ) + + first_exit: int = main(["--project-dir", str(project_dir), "format"]) + compile_exit: int = main(["--project-dir", str(project_dir), "compile", "--no-cache"]) + formatted_once: str = test_file.read_text(encoding="utf-8") + second_exit: int = main(["--project-dir", str(project_dir), "format", "--check"]) + + assert first_exit == test_case.expected_exit_code + assert compile_exit == test_case.expected_exit_code + assert second_exit == test_case.expected_exit_code + assert test_case.expected_literal in formatted_once + assert formatted_once == test_file.read_text(encoding="utf-8") + + +@pytest.mark.parametrize( + "test_case", + [ + FormatScopeIntegrationTestCase( + description="default exclusions and path selectors share format scope", + expected_exclude_exit=1, + expected_selected_model_exit=0, + expected_path_with_exclude_exit=1, + expected_exclude_path_exit=0, + ) + ], + ids=lambda case: case.description, +) +def test_given_format_selectors_when_scoping_then_paths_and_default_exclusions_are_consistent( + test_case: FormatScopeIntegrationTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" + ) + models: Path = tmp_path / "models" + models.mkdir() + (models / "customers.sql").write_text( + "MODEL (materialized table, columns (customer_id (type INTEGER),));\n" + "SELECT\n" + " 1 AS customer_id\n", + encoding="utf-8", + ) + (models / "orders.sql").write_text( + 'MODEL (description "Orders.", materialized table, ' + "columns (order_id (type INTEGER),));\n" + "SELECT\n" + " customer_id AS order_id\n" + 'FROM __ref("customers")\n', + encoding="utf-8", + ) + test_file: Path = tmp_path / "tests" / "unit" / "test_orders.sql" + test_file.parent.mkdir(parents=True) + unformatted_test: str = ( + 'TEST (name "orders_returns_one_row");\n\n' + "WITH __ref__customers AS (\n SELECT 1 AS customer_id\n),\n" + "__expected__orders AS (\n SELECT 1 AS order_id\n)\nSELECT 1\n" + ) + test_file.write_text(unformatted_test, encoding="utf-8") + + exclude_exit: int = main( + ["--project-dir", str(tmp_path), "format", "--check", "--exclude", "customers"] + ) + exclude_output: str = capsys.readouterr().err + selected_model_exit: int = main( + ["--project-dir", str(tmp_path), "format", "--check", "--select", "orders"] + ) + selected_model_output: str = capsys.readouterr().err + path_with_exclude_exit: int = main( + [ + "--project-dir", + str(tmp_path), + "format", + "--check", + "--select", + "path:tests", + "--exclude", + "orders", + ] + ) + path_with_exclude_output: str = capsys.readouterr().err + exclude_path_exit: int = main( + ["--project-dir", str(tmp_path), "format", "--check", "--exclude", "path:tests"] + ) + exclude_path_output: str = capsys.readouterr().err + + assert exclude_exit == test_case.expected_exclude_exit + assert "2 files checked" in exclude_output + assert selected_model_exit == test_case.expected_selected_model_exit + assert "1 files checked" in selected_model_output + assert path_with_exclude_exit == test_case.expected_path_with_exclude_exit + assert "1 files checked" in path_with_exclude_output + assert exclude_path_exit == test_case.expected_exclude_path_exit + assert "2 files checked" in exclude_path_output + assert test_file.read_text(encoding="utf-8") == unformatted_test + + +@pytest.mark.parametrize( + "test_case", + [ + FormatWarningIntegrationTestCase( + description="missing model description warns without failing format", + expected_exit_code=0, + expected_code="description-present", + expected_severity="warning", + ) + ], + ids=lambda case: case.description, +) +def test_given_missing_description_when_formatting_then_warning_does_not_fail( + test_case: FormatWarningIntegrationTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" + ) + model: Path = tmp_path / "models" / "orders.sql" + model.parent.mkdir() + model.write_text("MODEL (materialized table);\nSELECT 1 AS order_id\n", encoding="utf-8") + + write_exit: int = main(["--project-dir", str(tmp_path), "format"]) + _ = capsys.readouterr() + text_exit: int = main(["--project-dir", str(tmp_path), "format", "--check"]) + text_output: str = capsys.readouterr().out + json_exit: int = main(["--project-dir", str(tmp_path), "format", "--check", "--json"]) + payload: dict[str, object] = json.loads(capsys.readouterr().out) + + assert write_exit == test_case.expected_exit_code + assert text_exit == test_case.expected_exit_code + assert f"warning[{test_case.expected_code}]" in text_output + assert json_exit == test_case.expected_exit_code + assert payload["faults"] == 0 + assert payload["warnings"] == 1 + violations: object = payload["violations"] + assert isinstance(violations, list) + assert violations[0]["code"] == test_case.expected_code + assert violations[0]["severity"] == test_case.expected_severity diff --git a/tests/unit/src/sqlbuild/lint/_helpers/test_headers.py b/tests/unit/src/sqlbuild/lint/_helpers/test_headers.py index 0041b4d6d3..a2710cb009 100644 --- a/tests/unit/src/sqlbuild/lint/_helpers/test_headers.py +++ b/tests/unit/src/sqlbuild/lint/_helpers/test_headers.py @@ -54,6 +54,14 @@ ), expected_kinds=("MODEL",), ), + ScanHeadersTestCase( + description="ignores header keyword used as a nested column name", + contents=( + "MODEL (\n columns (\n object_id (type INTEGER),\n" + " SCHEMA (type VARCHAR(100)),\n ),\n);\nSELECT 1 AS object_id\n" + ), + expected_kinds=("MODEL",), + ), ScanHeadersTestCase( description="ignores header-like text inside comments", contents=( From 2af4b31623329a427ea256e9e03f84c0e573295a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:20:59 +0100 Subject: [PATCH 2/5] fix(format): preserve mixed values relations --- .../src/sql_lint/_helpers/formatter.rs | 67 +++++++++++-------- .../src/sql_lint/constants.rs | 1 + .../src/sql_lint/tests.rs | 2 + .../sql_lint/tests/test_values_formatter.rs | 53 +++++++++++++++ .../sqlbuild/cli/commands/main/_test_types.py | 11 +++ .../src/sqlbuild/cli/commands/main/helpers.py | 12 ++++ .../sqlbuild/cli/commands/main/test_format.py | 41 ++++++++++++ 7 files changed, 159 insertions(+), 28 deletions(-) create mode 100644 crates/sqlbuild-rules-native/src/sql_lint/tests/test_values_formatter.rs diff --git a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs index 7a5074115b..0e10aca3bb 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs @@ -6,6 +6,7 @@ use rayon::iter::{IntoParallelIterator, ParallelIterator}; use crate::sql_lint::constants::{ CAST_TYPE_SEPARATOR_KEYWORD, CLOSE_PARENTHESIS, LINT_API_VERSION, OPEN_PARENTHESIS, + VALUES_RELATION_PREFIX_TOKEN_COUNT, }; use crate::sql_lint::models::{FormatRequest, FormatResponse}; @@ -158,43 +159,52 @@ fn restore_unparenthesized_from_values( mut formatted: String, context: &FormatOnceContext<'_>, ) -> Result { - let authored_count = context + let authored_parenthesized: Vec = context .original_tokens - .windows(2) - .filter(|tokens| { - token_text(context.original_sql, &tokens[0]).as_deref() == Some("FROM") - && token_text(context.original_sql, &tokens[1]).as_deref() == Some("VALUES") + .iter() + .enumerate() + .filter_map(|(index, token)| { + (token_text(context.original_sql, token).as_deref() == Some("VALUES")).then(|| { + index > 0 + && token_text(context.original_sql, &context.original_tokens[index - 1]) + .as_deref() + == Some(OPEN_PARENTHESIS) + }) }) - .count(); - if authored_count == 0 { + .collect(); + if authored_parenthesized.is_empty() { return Ok(formatted); } let formatted_tokens = context .dialect .tokenize(&formatted) .map_err(|error| error.to_string())?; + let formatted_values: Vec = formatted_tokens + .iter() + .enumerate() + .filter_map(|(index, token)| { + (token_text(&formatted, token).as_deref() == Some("VALUES")).then_some(index) + }) + .collect(); + if authored_parenthesized.len() != formatted_values.len() { + return Ok(formatted); + } let mut removals: Vec<(usize, usize)> = Vec::new(); - let mut search_start = 0_usize; - for _ in 0..authored_count { - let Some(from_index) = - (search_start..formatted_tokens.len().saturating_sub(1)).find(|&index| { - token_text(&formatted, &formatted_tokens[index]).as_deref() == Some("FROM") - && (token_text(&formatted, &formatted_tokens[index + 1]).as_deref() - == Some("VALUES") - || (index + 2 < formatted_tokens.len() - && token_text(&formatted, &formatted_tokens[index + 1]).as_deref() - == Some(OPEN_PARENTHESIS) - && token_text(&formatted, &formatted_tokens[index + 2]).as_deref() - == Some("VALUES"))) - }) - else { - return Err(UNSUPPORTED_SQL_FAILURE.to_string()); - }; - if token_text(&formatted, &formatted_tokens[from_index + 1]).as_deref() == Some("VALUES") { - search_start = from_index + 2; + for (was_parenthesized, values_index) in + authored_parenthesized.into_iter().zip(formatted_values) + { + if was_parenthesized || values_index < VALUES_RELATION_PREFIX_TOKEN_COUNT { + continue; + } + let wrapper_index = values_index - 1; + let relation_index = values_index - 2; + let relation_keyword = token_text(&formatted, &formatted_tokens[relation_index]); + if token_text(&formatted, &formatted_tokens[wrapper_index]).as_deref() + != Some(OPEN_PARENTHESIS) + || !matches!(relation_keyword.as_deref(), Some("FROM" | "JOIN")) + { continue; } - let wrapper_index = from_index + 1; let mut depth = 0_i32; let mut close_index: Option = None; for (index, token) in formatted_tokens.iter().enumerate().skip(wrapper_index) { @@ -210,12 +220,13 @@ fn restore_unparenthesized_from_values( _ => {} } } - let close_index = close_index.ok_or_else(|| UNSUPPORTED_SQL_FAILURE.to_string())?; + let Some(close_index) = close_index else { + return Ok(formatted); + }; let open = &formatted_tokens[wrapper_index]; let close = &formatted_tokens[close_index]; removals.push((open.span.start, open.span.end)); removals.push((close.span.start, close.span.end)); - search_start = close_index + 1; } removals.sort_unstable(); for (start, end) in removals.into_iter().rev() { diff --git a/crates/sqlbuild-rules-native/src/sql_lint/constants.rs b/crates/sqlbuild-rules-native/src/sql_lint/constants.rs index da36d20ba1..539928f25d 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/constants.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/constants.rs @@ -2,3 +2,4 @@ pub(crate) const CAST_TYPE_SEPARATOR_KEYWORD: &str = "AS"; pub(crate) const CLOSE_PARENTHESIS: &str = ")"; pub(crate) const LINT_API_VERSION: u32 = 1; pub(crate) const OPEN_PARENTHESIS: &str = "("; +pub(crate) const VALUES_RELATION_PREFIX_TOKEN_COUNT: usize = 2; diff --git a/crates/sqlbuild-rules-native/src/sql_lint/tests.rs b/crates/sqlbuild-rules-native/src/sql_lint/tests.rs index 8bf146e4e8..086e1c5203 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/tests.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/tests.rs @@ -8,3 +8,5 @@ mod helpers; mod sql_lint; #[path = "tests/test_types.rs"] mod test_types; +#[path = "tests/test_values_formatter.rs"] +mod values_formatter; diff --git a/crates/sqlbuild-rules-native/src/sql_lint/tests/test_values_formatter.rs b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_values_formatter.rs new file mode 100644 index 0000000000..b71fe80394 --- /dev/null +++ b/crates/sqlbuild-rules-native/src/sql_lint/tests/test_values_formatter.rs @@ -0,0 +1,53 @@ +use serde_json::{Value, json}; + +use crate::sql_lint::main::formatter::format_json; +use crate::sql_lint::tests::test_types; + +#[test] +fn given_mixed_values_relations_when_formatting_then_each_authored_form_is_preserved() +-> Result<(), String> { + let test_cases = [ + test_types::FormatTestCase { + description: "mixed values relations preserve parenthesized relation first", + sql: "SELECT * FROM (VALUES (1, 2)) AS first_values(a, b) UNION ALL SELECT * FROM VALUES (3, 4)", + expected_sql: "SELECT\n *\nFROM (VALUES (1, 2)) AS first_values(a, b)\nUNION ALL\nSELECT\n *\nFROM VALUES (3, 4)", + expected_changed: true, + }, + test_types::FormatTestCase { + description: "mixed values relations preserve unparenthesized relation first", + sql: "SELECT * FROM VALUES (1, 2) UNION ALL SELECT * FROM (VALUES (3, 4)) AS second_values(a, b)", + expected_sql: "SELECT\n *\nFROM VALUES (1, 2)\nUNION ALL\nSELECT\n *\nFROM (VALUES (3, 4)) AS second_values(a, b)", + expected_changed: true, + }, + test_types::FormatTestCase { + description: "mixed values relations inside joined subquery preserve authored forms", + sql: "SELECT base.id FROM items AS base JOIN (SELECT * FROM (VALUES (1)) AS first_values(id) UNION ALL SELECT * FROM VALUES (2)) AS mixed ON base.id = mixed.id", + expected_sql: "SELECT\n base.id\nFROM items AS base\nJOIN (\n SELECT\n *\n FROM (VALUES (1)) AS first_values(id)\n UNION ALL\n SELECT\n *\n FROM VALUES (2)\n) AS mixed\n ON base.id = mixed.id", + expected_changed: true, + }, + test_types::FormatTestCase { + description: "values text in string and comment is ignored", + sql: "-- VALUES and FROM VALUES are examples\nSELECT 'VALUES' AS label FROM (VALUES (1)) AS first_values(id) UNION ALL SELECT * FROM VALUES (2)", + expected_sql: "-- VALUES and FROM VALUES are examples\nSELECT\n 'VALUES' AS label\nFROM (VALUES (1)) AS first_values(id)\nUNION ALL\nSELECT\n *\nFROM VALUES (2)", + expected_changed: true, + }, + ]; + + for test_case in &test_cases { + let response = format_json( + &json!({"version": 1, "sql": test_case.sql, "dialect": "snowflake"}).to_string(), + )?; + let payload: Value = serde_json::from_str(&response).map_err(|error| error.to_string())?; + assert_eq!( + payload["sql"], test_case.expected_sql, + "{}", + test_case.description + ); + assert_eq!( + payload["changed"], test_case.expected_changed, + "{}", + test_case.description + ); + } + Ok(()) +} diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 60dc502ef5..ca73b11ea4 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -179,6 +179,17 @@ class FromValuesFormatIntegrationTestCase: expected_literal: str +@dataclass(frozen=True) +class MixedFromValuesFormatIntegrationTestCase: + """One positional values-relation preservation expectation.""" + + description: str + authored_query: str + expected_parenthesized_literal: str + expected_unparenthesized_literal: str + expected_exit_code: int + + @dataclass(frozen=True) class FormatScopeIntegrationTestCase: """Expected CLI outcomes for the format selection contract.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py index 406315fa55..9f080e40e5 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/helpers.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/helpers.py @@ -64,6 +64,18 @@ def write_from_values_format_project(*, tmp_path: Path, adapter: str) -> tuple[P return project_dir, test_file +def write_snowflake_format_test(*, tmp_path: Path, test_sql: str) -> tuple[Path, Path]: + """Write a minimal Snowflake-dialect project for real CLI formatting.""" + + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "products"\nadapter = "snowflake"\n', encoding="utf-8" + ) + test_file: Path = tmp_path / "tests" / "unit" / "test_products.sql" + test_file.parent.mkdir(parents=True) + test_file.write_text(test_sql, encoding="utf-8") + return tmp_path, test_file + + def prepare_contract_project(tmp_path: Path, *, prod_connection_toml: str = "") -> Path: database: Path = tmp_path / "warehouse.duckdb" _ = (tmp_path / "sqlbuild_project.toml").write_text( diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py index 1f213b2225..0bd00f2c58 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py @@ -16,9 +16,11 @@ FormatScopeIntegrationTestCase, FormatWarningIntegrationTestCase, FromValuesFormatIntegrationTestCase, + MixedFromValuesFormatIntegrationTestCase, ) from tests.integration.src.sqlbuild.cli.commands.main.helpers import ( write_from_values_format_project, + write_snowflake_format_test, ) @@ -459,6 +461,45 @@ def test_given_duckdb_from_values_when_formatting_then_project_compiles( assert formatted_once == test_file.read_text(encoding="utf-8") +@pytest.mark.parametrize( + "test_case", + [ + MixedFromValuesFormatIntegrationTestCase( + description="mixed values forms retain positional authorship through real CLI", + authored_query=( + 'TEST (name "mixed_product_values");\n\n' + "-- VALUES inside this comment is not a relation.\n" + "SELECT 'VALUES' AS label FROM (VALUES (1)) AS first_values(id)\n" + "UNION ALL\n" + "SELECT 'second' AS label FROM VALUES (2)\n" + ), + expected_parenthesized_literal="FROM (VALUES (1)) AS first_values(id)", + expected_unparenthesized_literal="FROM VALUES (2)", + expected_exit_code=0, + ) + ], + ids=lambda case: case.description, +) +def test_given_mixed_values_forms_when_formatting_then_each_relation_preserves_authorship( + test_case: MixedFromValuesFormatIntegrationTestCase, + tmp_path: Path, +) -> None: + project_dir, test_file = write_snowflake_format_test( + tmp_path=tmp_path, + test_sql=test_case.authored_query, + ) + + first_exit: int = main(["--project-dir", str(project_dir), "format"]) + formatted_once: str = test_file.read_text(encoding="utf-8") + second_exit: int = main(["--project-dir", str(project_dir), "format", "--check"]) + + assert first_exit == test_case.expected_exit_code + assert second_exit == test_case.expected_exit_code + assert test_case.expected_parenthesized_literal in formatted_once + assert test_case.expected_unparenthesized_literal in formatted_once + assert formatted_once == test_file.read_text(encoding="utf-8") + + @pytest.mark.parametrize( "test_case", [ From cee5a0e1c95e2f97408b7aa8ddac87c8d0f57de6 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:41:44 +0100 Subject: [PATCH 3/5] fix(format): preserve skipped bodies and fixture stability --- .../_helpers/sql_tests/fixture_formatting.py | 68 ++++++++++ src/sqlbuild/lint/_helpers/native_format.py | 50 +------- src/sqlbuild/lint/constants.py | 1 - src/sqlbuild/lint/main/run_format.py | 9 +- src/sqlbuild/lint/models.py | 8 -- .../sqlbuild/cli/commands/main/_test_types.py | 19 +++ .../sqlbuild/cli/commands/main/test_format.py | 117 +++++++++++++++++- tests/unit/src/sqlbuild/lint/test_run_lint.py | 4 +- 8 files changed, 208 insertions(+), 68 deletions(-) diff --git a/src/sqlbuild/compiler/planner/_helpers/sql_tests/fixture_formatting.py b/src/sqlbuild/compiler/planner/_helpers/sql_tests/fixture_formatting.py index 4c72ed67ae..2952ce407d 100644 --- a/src/sqlbuild/compiler/planner/_helpers/sql_tests/fixture_formatting.py +++ b/src/sqlbuild/compiler/planner/_helpers/sql_tests/fixture_formatting.py @@ -56,6 +56,14 @@ re.IGNORECASE, ) _SELECT_LINE_PATTERN: re.Pattern[str] = re.compile(r"^\s*SELECT\b(?P.*)$", re.IGNORECASE) +_EMPTY_TYPED_NULL_QUERY_PATTERN: re.Pattern[str] = re.compile( + r"^\s*SELECT\s+(?P.+?)\s+WHERE\s+FALSE\s*$", + re.IGNORECASE | re.DOTALL, +) +_SQL_QUOTE_CHARACTERS: frozenset[str] = frozenset({"'", '"', "`"}) +_OPEN_PARENTHESIS_CHARACTER: str = "(" +_CLOSE_PARENTHESIS_CHARACTER: str = ")" +_COMMA_CHARACTER: str = "," def format_redundant_fixture_nulls( @@ -215,6 +223,8 @@ def _remove_redundant_typed_null_lines(*, sql: str, relation: FixtureRelationMet metadata_by_name: dict[str, FixtureColumnMetadata] = { column.name.casefold(): column for column in relation.columns } + if _is_empty_typed_null_query(sql=sql, metadata_by_name=metadata_by_name): + return "SELECT * FROM __empty_fixture()" lines: list[str] = sql.splitlines() typed_null_indexes: set[int] = set() remove_indexes: set[int] = set() @@ -256,6 +266,64 @@ def _remove_redundant_typed_null_lines(*, sql: str, relation: FixtureRelationMet return candidate +def _is_empty_typed_null_query( + *, sql: str, metadata_by_name: dict[str, FixtureColumnMetadata] +) -> bool: + """Return whether an empty fixture projects only known typed-null columns.""" + + query_match: re.Match[str] | None = _EMPTY_TYPED_NULL_QUERY_PATTERN.fullmatch(sql) + if query_match is None: + return False + projections: tuple[str, ...] = _split_top_level_expressions( + value=query_match.group("projection") + ) + if not projections: + return False + for projection in projections: + typed_null_match: re.Match[str] | None = _TYPED_NULL_LINE_PATTERN.fullmatch( + projection.strip() + ) + if typed_null_match is None: + return False + alias: str = typed_null_match.group("alias").strip('"`') + column: FixtureColumnMetadata | None = metadata_by_name.get(alias.casefold()) + if column is None or column.type is None: + return False + return True + + +def _split_top_level_expressions(*, value: str) -> tuple[str, ...]: + """Split a projection list without splitting commas inside types or quoted values.""" + + expressions: list[str] = [] + start: int = 0 + depth: int = 0 + quote: str | None = None + index: int = 0 + while index < len(value): + character: str = value[index] + if quote is not None: + if character == quote: + if index + 1 < len(value) and value[index + 1] == quote: + index += 2 + continue + quote = None + index += 1 + continue + if character in _SQL_QUOTE_CHARACTERS: + quote = character + elif character == _OPEN_PARENTHESIS_CHARACTER: + depth += 1 + elif character == _CLOSE_PARENTHESIS_CHARACTER: + depth = max(0, depth - 1) + elif character == _COMMA_CHARACTER and depth == 0: + expressions.append(value[start:index]) + start = index + 1 + index += 1 + expressions.append(value[start:]) + return tuple(expression for expression in expressions if expression.strip()) + + def _projection_aliases(*, lines: list[str]) -> tuple[str, ...] | None: aliases: list[str] = [] saw_select: bool = False diff --git a/src/sqlbuild/lint/_helpers/native_format.py b/src/sqlbuild/lint/_helpers/native_format.py index 11f665cbd8..0dc110b5de 100644 --- a/src/sqlbuild/lint/_helpers/native_format.py +++ b/src/sqlbuild/lint/_helpers/native_format.py @@ -13,18 +13,9 @@ from sqlbuild.lint.constants import ( CARRIAGE_RETURN_LINE_FEED, LINE_FEED, - LINT_ENGINE_NATIVE, - RULE_FORMAT_SAFETY, - VIOLATION_SEVERITY_FAULT, ) from sqlbuild.lint.exceptions import NativeLintError -from sqlbuild.lint.models import ( - HeaderSpan, - InterpolationSite, - LintConfig, - LintViolation, - NativeFormatResult, -) +from sqlbuild.lint.models import HeaderSpan, InterpolationSite, LintConfig _NATIVE_FORMAT_API_VERSION: int = 1 @@ -57,8 +48,8 @@ def with_newline_style(*, contents: str, newline: str) -> str: def format_native_sql_bodies( *, files: dict[Path, str], config: LintConfig, project_dir: Path -) -> NativeFormatResult: - """Format authored SQL bodies without partially rewriting rejected files.""" +) -> dict[Path, str]: + """Format supported SQL bodies while leaving declined bodies unchanged.""" prepared_by_path: dict[Path, tuple[_PreparedBody, ...]] = {} requests_by_key: dict[tuple[str, str], dict[str, object]] = {} @@ -99,10 +90,8 @@ def format_native_sql_bodies( requests_by_key=requests_by_key ) formatted_files: dict[Path, str] = {} - faults: list[LintViolation] = [] for file_path, contents in sorted(files.items()): updated: str = contents - file_faults: list[LintViolation] = [] for prepared in reversed(prepared_by_path[file_path]): response: dict[str, Any] = response_cache[prepared.cache_key] raw_sql: object = response.get("sql") @@ -115,14 +104,6 @@ def format_native_sql_bodies( ): raise NativeLintError("native formatter returned invalid SQL or changed state") if not formatted: - file_faults.append( - _format_safety_fault( - file_path=file_path, - contents=contents, - body_start=prepared.start, - reason=response.get("reason"), - ) - ) continue if not changed: continue @@ -133,30 +114,9 @@ def format_native_sql_bodies( updated = ( f"{updated[: prepared.start]}{restored}{prepared.trailing}{updated[prepared.end :]}" ) - if file_faults: - faults.extend(reversed(file_faults)) - elif updated != contents: + if updated != contents: formatted_files[file_path] = updated - return NativeFormatResult(formatted_files=formatted_files, faults=tuple(faults)) - - -def _format_safety_fault( - *, file_path: Path, contents: str, body_start: int, reason: object -) -> LintViolation: - line: int = contents.count("\n", 0, body_start) + 1 - previous_newline: int = contents.rfind("\n", 0, body_start) - column: int = body_start - previous_newline - detail: str = reason if isinstance(reason, str) and reason else "unknown formatter rejection" - return LintViolation( - file_path=file_path, - line=line, - column=column, - code=RULE_FORMAT_SAFETY, - message=f"SQL body could not be formatted safely: {detail}", - severity=VIOLATION_SEVERITY_FAULT, - engine=LINT_ENGINE_NATIVE, - remediation="Correct the SQL syntax or leave the file unchanged and report the parser gap.", - ) + return formatted_files def _format_responses( diff --git a/src/sqlbuild/lint/constants.py b/src/sqlbuild/lint/constants.py index d8706e0989..e39076ac2e 100644 --- a/src/sqlbuild/lint/constants.py +++ b/src/sqlbuild/lint/constants.py @@ -60,7 +60,6 @@ RULE_LEADING_COMMENT_DESCRIPTION: str = "leading-comment-description" RULE_HEADER_WHITESPACE: str = "header-whitespace" RULE_HEADER_PARSE: str = "header-parse" -RULE_FORMAT_SAFETY: str = "format-safety" PROJECT_CONFIG_FILENAME_KEY: str = "sqlbuild_project.toml" FORMAT_SECTION_KEY: str = "format" diff --git a/src/sqlbuild/lint/main/run_format.py b/src/sqlbuild/lint/main/run_format.py index ee62335ab5..d79cd611a8 100644 --- a/src/sqlbuild/lint/main/run_format.py +++ b/src/sqlbuild/lint/main/run_format.py @@ -28,7 +28,6 @@ LintConfig, LintRunResult, LintViolation, - NativeFormatResult, ) @@ -152,18 +151,16 @@ def _apply_fixes( current_files = { file_path: updated.get(file_path, contents) for file_path, contents in files.items() } - native_result: NativeFormatResult = format_native_sql_bodies( + native_formatted: dict[Path, str] = format_native_sql_bodies( files=current_files, config=config, project_dir=project_dir, ) - updated.update(native_result.formatted_files) - for fault in native_result.faults: - updated.pop(fault.file_path, None) + updated.update(native_formatted) return reject_unparseable_header_rewrites( updated=updated, config=config, - faults=list(native_result.faults), + faults=[], ) diff --git a/src/sqlbuild/lint/models.py b/src/sqlbuild/lint/models.py index 507afee98b..c0079f988a 100644 --- a/src/sqlbuild/lint/models.py +++ b/src/sqlbuild/lint/models.py @@ -95,14 +95,6 @@ class FormatChange: after: str -@dataclass(frozen=True) -class NativeFormatResult: - """Safe SQL body rewrites and files rejected by the formatter boundary.""" - - formatted_files: dict[Path, str] - faults: tuple[LintViolation, ...] - - @dataclass(frozen=True) class LintFixRecord: """One applied or skipped lint repair for human and machine reporting.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index ca73b11ea4..4471df3df5 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -169,6 +169,25 @@ class FormatSafetyIntegrationTestCase: expected_exit_code: int +@dataclass(frozen=True) +class FormatterDeclineIntegrationTestCase: + """One expected native formatter decline through the real CLI.""" + + description: str + authored_body: str + expected_exit_code: int + + +@dataclass(frozen=True) +class TypedNullFormatIntegrationTestCase: + """One single-pass typed-null fixture formatting expectation.""" + + description: str + fixture_projection: str + expected_literal: str + expected_exit_code: int + + @dataclass(frozen=True) class FromValuesFormatIntegrationTestCase: """One dialect expectation for an unparenthesized values relation.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py index 0bd00f2c58..4f90d72714 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py @@ -14,9 +14,11 @@ FormatCompileIntegrationTestCase, FormatSafetyIntegrationTestCase, FormatScopeIntegrationTestCase, + FormatterDeclineIntegrationTestCase, FormatWarningIntegrationTestCase, FromValuesFormatIntegrationTestCase, MixedFromValuesFormatIntegrationTestCase, + TypedNullFormatIntegrationTestCase, ) from tests.integration.src.sqlbuild.cli.commands.main.helpers import ( write_from_values_format_project, @@ -369,12 +371,6 @@ def test_given_schema_column_when_formatting_then_header_remains_valid_and_idemp expected_fault_code="header-parse", expected_exit_code=1, ), - FormatSafetyIntegrationTestCase( - description="unparseable SQL is untouched", - authored_sql='MODEL (description "Orders." );\nselect from\n', - expected_fault_code="format-safety", - expected_exit_code=1, - ), ], ids=lambda case: case.description, ) @@ -398,6 +394,115 @@ def test_given_unparseable_file_when_formatting_then_file_is_untouched_and_fault assert model.read_text(encoding="utf-8") == test_case.authored_sql +@pytest.mark.parametrize( + "test_case", + [ + FormatterDeclineIntegrationTestCase( + description="unsupported SQL body is skipped while header formats", + authored_body="select from\n", + expected_exit_code=0, + ), + FormatterDeclineIntegrationTestCase( + description="comment attachment decline is skipped while header formats", + authored_body="select order_id order_key /* keep */ from orders\n", + expected_exit_code=0, + ), + ], + ids=lambda case: case.description, +) +def test_given_native_formatter_decline_when_formatting_then_header_formats_and_body_is_unchanged( + test_case: FormatterDeclineIntegrationTestCase, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "orders"\nadapter = "duckdb"\n', encoding="utf-8" + ) + model: Path = tmp_path / "models" / "orders.sql" + model.parent.mkdir() + model.write_text( + "MODEL (\n" + ' description "Orders.", \n' + " materialized table,\n" + ");\n\n" + f"{test_case.authored_body}", + encoding="utf-8", + ) + + first_exit: int = main(["--project-dir", str(tmp_path), "format"]) + first_output: str = capsys.readouterr().out + formatted_once: str = model.read_text(encoding="utf-8") + second_exit: int = main(["--project-dir", str(tmp_path), "format", "--check"]) + second_output: str = capsys.readouterr().out + + assert first_exit == test_case.expected_exit_code + assert second_exit == test_case.expected_exit_code + assert ' description "Orders.",\n' in formatted_once + assert test_case.authored_body in formatted_once + assert "FAULT=0 WARN=0" in first_output + assert "FAULT=0 WARN=0" in second_output + assert formatted_once == model.read_text(encoding="utf-8") + + +@pytest.mark.parametrize( + "test_case", + [ + TypedNullFormatIntegrationTestCase( + description="inline colon casts reach empty fixture fixed point in one pass", + fixture_projection=("NULL::VARCHAR AS customer_key, NULL::BIGINT AS order_count"), + expected_literal="__EMPTY_FIXTURE()", + expected_exit_code=0, + ) + ], + ids=lambda case: case.description, +) +def test_given_inline_typed_null_fixture_when_formatting_then_one_pass_is_idempotent( + test_case: TypedNullFormatIntegrationTestCase, + tmp_path: Path, +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "customers"\nadapter = "duckdb"\n', encoding="utf-8" + ) + model: Path = tmp_path / "models" / "customers.sql" + model.parent.mkdir() + model.write_text( + "MODEL (\n" + ' description "Customers.",\n' + " contract enforced,\n" + " columns (\n" + " customer_key (type VARCHAR),\n" + " order_count (type BIGINT),\n" + " ),\n" + ");\n\n" + "SELECT 'c1' AS customer_key, 1 AS order_count\n", + encoding="utf-8", + ) + test_file: Path = tmp_path / "tests" / "unit" / "test_customers.sql" + test_file.parent.mkdir(parents=True) + test_file.write_text( + "TEST();\n\n" + "WITH __ref__customers AS (\n" + f" SELECT {test_case.fixture_projection}\n" + " WHERE FALSE\n" + "),\n" + "__expected__customers AS (\n" + " SELECT 'c1' AS customer_key, 1 AS order_count\n" + ")\n" + "SELECT 1\n", + encoding="utf-8", + ) + + first_exit: int = main(["--project-dir", str(tmp_path), "format"]) + formatted_once: str = test_file.read_text(encoding="utf-8") + second_exit: int = main(["--project-dir", str(tmp_path), "format", "--check"]) + + assert first_exit == test_case.expected_exit_code + assert second_exit == test_case.expected_exit_code + assert test_case.expected_literal in formatted_once + assert "CAST(NULL" not in formatted_once + assert formatted_once == test_file.read_text(encoding="utf-8") + + @pytest.mark.parametrize( "test_case", [ diff --git a/tests/unit/src/sqlbuild/lint/test_run_lint.py b/tests/unit/src/sqlbuild/lint/test_run_lint.py index 95d626b25b..40838593e9 100644 --- a/tests/unit/src/sqlbuild/lint/test_run_lint.py +++ b/tests/unit/src/sqlbuild/lint/test_run_lint.py @@ -90,10 +90,10 @@ def test_given_synthetic_project_when_linting_then_results_match_expected( expected_formatted_count=1, ), FormatProjectTestCase( - description="unfixable faults remain after formatting", + description="missing description remains a non-failing format warning", files={"models/no_description.sql": NO_DESCRIPTION_MODEL}, expected_written_fragments={}, - expected_fault_codes=("description-present",), + expected_fault_codes=(), expected_formatted_count=1, ), FormatProjectTestCase( From 032cc4a3c0a18232b26ebc494649cea95a3382f0 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:56:19 +0100 Subject: [PATCH 4/5] fix(format): stabilize canonical fixture rewrites --- .../discovery/main/_parse_sql_test_file.py | 14 ++++ .../_helpers/sql_tests/fixture_formatting.py | 10 ++- src/sqlbuild/lint/main/run_format.py | 20 ++++++ .../sqlbuild/cli/commands/main/_test_types.py | 10 +++ .../sqlbuild/cli/commands/main/test_format.py | 64 +++++++++++++++++++ 5 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 src/sqlbuild/compiler/discovery/main/_parse_sql_test_file.py diff --git a/src/sqlbuild/compiler/discovery/main/_parse_sql_test_file.py b/src/sqlbuild/compiler/discovery/main/_parse_sql_test_file.py new file mode 100644 index 0000000000..6c366a9231 --- /dev/null +++ b/src/sqlbuild/compiler/discovery/main/_parse_sql_test_file.py @@ -0,0 +1,14 @@ +"""Public discovery entry point for parsing in-memory SQL test contents.""" + +from __future__ import annotations + +from pathlib import Path + +from sqlbuild.compiler.discovery._helpers.sql.tests import parse_sql_test_file as _parse +from sqlbuild.compiler.discovery.models import DiscoveredSqlTestBlock + + +def parse_sql_test_file(*, contents: str, file_path: Path) -> tuple[DiscoveredSqlTestBlock, ...]: + """Parse current SQL test contents into ordered test blocks.""" + + return _parse(contents=contents, file_path=file_path) diff --git a/src/sqlbuild/compiler/planner/_helpers/sql_tests/fixture_formatting.py b/src/sqlbuild/compiler/planner/_helpers/sql_tests/fixture_formatting.py index 2952ce407d..3f6b5e3da4 100644 --- a/src/sqlbuild/compiler/planner/_helpers/sql_tests/fixture_formatting.py +++ b/src/sqlbuild/compiler/planner/_helpers/sql_tests/fixture_formatting.py @@ -15,6 +15,7 @@ from sqlbuild.compiler.compile.models import CompileSqlTestCte from sqlbuild.compiler.compile.types import CompiledResourceType, SqlTestMode from sqlbuild.compiler.discovery.main._model_schema_columns import parse_schema_columns +from sqlbuild.compiler.discovery.main._parse_sql_test_file import parse_sql_test_file from sqlbuild.compiler.discovery.models import ( DiscoveredProjectInputs, DiscoveredSqlModelFile, @@ -86,8 +87,15 @@ def format_redundant_fixture_nulls( contents: str | None = updated.get(file_path, files.get(file_path)) if contents is None: continue + try: + current_blocks: tuple[DiscoveredSqlTestBlock, ...] = parse_sql_test_file( + contents=contents, + file_path=file_path, + ) + except (OSError, UnicodeError, ValueError, SyntaxError): + continue block: DiscoveredSqlTestBlock - for block in test_file.blocks: + for block in current_blocks: fixed_block: str = _format_test_block( block=block, file_label=str(test_file.relative_path), diff --git a/src/sqlbuild/lint/main/run_format.py b/src/sqlbuild/lint/main/run_format.py index d79cd611a8..0693b86fa8 100644 --- a/src/sqlbuild/lint/main/run_format.py +++ b/src/sqlbuild/lint/main/run_format.py @@ -157,6 +157,26 @@ def _apply_fixes( project_dir=project_dir, ) updated.update(native_formatted) + post_native_files: dict[Path, str] = { + file_path: updated.get(file_path, contents) for file_path, contents in files.items() + } + fixture_formatted: dict[Path, str] = ( + FixtureNullAutofix.apply( + files=post_native_files, + project_dir=project_dir, + discovered_inputs=discovered_inputs, + ) + if discovered_inputs is not None + else {} + ) + if fixture_formatted: + final_native: dict[Path, str] = format_native_sql_bodies( + files=fixture_formatted, + config=config, + project_dir=project_dir, + ) + updated.update(fixture_formatted) + updated.update(final_native) return reject_unparseable_header_rewrites( updated=updated, config=config, diff --git a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py index 4471df3df5..bb5568f98a 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/_test_types.py @@ -188,6 +188,16 @@ class TypedNullFormatIntegrationTestCase: expected_exit_code: int +@dataclass(frozen=True) +class CanonicalFixtureFormatIntegrationTestCase: + """One post-native fixture simplification expectation.""" + + description: str + expected_retained_literal: str + expected_removed_literal: str + expected_exit_code: int + + @dataclass(frozen=True) class FromValuesFormatIntegrationTestCase: """One dialect expectation for an unparenthesized values relation.""" diff --git a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py index 4f90d72714..a257ae8ca3 100644 --- a/tests/integration/src/sqlbuild/cli/commands/main/test_format.py +++ b/tests/integration/src/sqlbuild/cli/commands/main/test_format.py @@ -10,6 +10,7 @@ from sqlbuild.cli.commands.main.entrypoint.entry import main from tests.integration.src.sqlbuild.cli.commands.main._test_types import ( + CanonicalFixtureFormatIntegrationTestCase, DescriptionFormatIntegrationTestCase, FormatCompileIntegrationTestCase, FormatSafetyIntegrationTestCase, @@ -503,6 +504,69 @@ def test_given_inline_typed_null_fixture_when_formatting_then_one_pass_is_idempo assert formatted_once == test_file.read_text(encoding="utf-8") +@pytest.mark.parametrize( + "test_case", + [ + CanonicalFixtureFormatIntegrationTestCase( + description="post-native fixture simplification reaches fixed point in one pass", + expected_retained_literal="CAST(COLUMN2 AS TEXT) AS CLUSTER_ID", + expected_removed_literal="is_archived", + expected_exit_code=0, + ) + ], + ids=lambda case: case.description, +) +def test_given_redundant_typed_null_after_multi_projection_when_formatting_then_is_idempotent( + test_case: CanonicalFixtureFormatIntegrationTestCase, + tmp_path: Path, +) -> None: + (tmp_path / "sqlbuild_project.toml").write_text( + 'name = "customer_segments"\nadapter = "duckdb"\n', encoding="utf-8" + ) + model: Path = tmp_path / "models" / "customer_clusters.sql" + model.parent.mkdir() + model.write_text( + "MODEL (\n" + ' description "Customer clusters.",\n' + " contract enforced,\n" + " columns (\n" + " customer_key (type VARCHAR),\n" + " cluster_id (type VARCHAR),\n" + " is_archived (type BOOLEAN),\n" + " ),\n" + ");\n\n" + "SELECT 'c1' AS customer_key, 'k1' AS cluster_id, FALSE AS is_archived\n", + encoding="utf-8", + ) + test_file: Path = tmp_path / "tests" / "unit" / "test_customer_clusters.sql" + test_file.parent.mkdir(parents=True) + test_file.write_text( + "TEST();\n\n" + "WITH __ref__customer_clusters AS (\n" + " SELECT COLUMN1::VARCHAR AS CUSTOMER_KEY, COLUMN2::VARCHAR AS CLUSTER_ID,\n" + " NULL::BOOLEAN AS is_archived\n" + " FROM VALUES\n" + " ('c1', 'k1')\n" + "),\n" + "__expected__customer_clusters AS (\n" + " SELECT 'c1' AS customer_key, 'k1' AS cluster_id, FALSE AS is_archived\n" + ")\n" + "SELECT 1\n", + encoding="utf-8", + ) + + first_exit: int = main(["--project-dir", str(tmp_path), "format"]) + formatted_once: str = test_file.read_text(encoding="utf-8") + second_exit: int = main(["--project-dir", str(tmp_path), "format", "--check"]) + fixture_section: str = formatted_once.split("__expected__customer_clusters", maxsplit=1)[0] + + assert first_exit == test_case.expected_exit_code + assert second_exit == test_case.expected_exit_code + assert test_case.expected_retained_literal in fixture_section + assert test_case.expected_removed_literal not in fixture_section + assert formatted_once == test_file.read_text(encoding="utf-8") + + @pytest.mark.parametrize( "test_case", [ From 86aa95f3b2067665c5505c9c7af22131520bd63b Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Wed, 23 Sep 2026 14:40:31 +0100 Subject: [PATCH 5/5] fix(format): satisfy clippy in values restoration --- .../src/sql_lint/_helpers/formatter.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs index 0e10aca3bb..ab39f208af 100644 --- a/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs +++ b/crates/sqlbuild-rules-native/src/sql_lint/_helpers/formatter.rs @@ -163,13 +163,11 @@ fn restore_unparenthesized_from_values( .original_tokens .iter() .enumerate() - .filter_map(|(index, token)| { - (token_text(context.original_sql, token).as_deref() == Some("VALUES")).then(|| { - index > 0 - && token_text(context.original_sql, &context.original_tokens[index - 1]) - .as_deref() - == Some(OPEN_PARENTHESIS) - }) + .filter(|(_, token)| token_text(context.original_sql, token).as_deref() == Some("VALUES")) + .map(|(index, _)| { + index > 0 + && token_text(context.original_sql, &context.original_tokens[index - 1]).as_deref() + == Some(OPEN_PARENTHESIS) }) .collect(); if authored_parenthesized.is_empty() {