diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 599d5fe..f36173e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,6 +59,121 @@ jobs: exit 1 fi + # The 23 tests under tests/integration/ are deselected by default and, until this job + # existed, ran only on a contributor's own machine. That is not a theoretical gap: this + # feature's live suite repeatedly found bugs no fixture could reach — a `reltuples = -1` + # sentinel that read as "tiny table" and silently suppressed every proposal, redaction + # dismembering `$N` placeholders, a `toplevel` filter that made a hot function-wrapped query + # vanish while leaving confidently-wrong advice in its place, and a workload statement that + # failed on the wire for every default run. Each was invisible to `pytest` and to review. + integration: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: astral-sh/setup-uv@v5 + with: + python-version: "3.12" + # Started with `docker run` rather than a `services:` container, and that is the whole + # reason this step exists in this shape. `pg_stat_statements` must be loaded by the + # postmaster at startup, but a service container cannot be given a command — GitHub's + # `services:` schema has no `command` key, and `options` is passed to `docker create` + # before the image, so the `-c shared_preload_libraries=...` flags cannot be expressed + # there at all. + # + # The first attempt worked around that with `ALTER SYSTEM` plus a restart, and it failed + # on the real runner: `ALTER SYSTEM SET pg_stat_statements.track` is rejected with + # "unrecognized configuration parameter" because that setting does not exist until the + # library is loaded, which is precisely what the restart was meant to achieve. Ordering + # the two calls correctly would need two restarts. Running the container directly is + # simpler, and — more importantly — takes the same flags as + # tests/integration/docker-compose.yml, so the server this job tests against is + # configured from one description rather than two that can drift. + - name: Start Postgres with pg_stat_statements preloaded + run: | + set -euo pipefail + docker run -d --name sqlquality-ci-pg \ + -e POSTGRES_PASSWORD=sqlquality \ + -e POSTGRES_DB=sqlquality_test \ + -p 27432:5432 \ + postgres:16 \ + postgres -c shared_preload_libraries=pg_stat_statements \ + -c pg_stat_statements.track=all + # No health check gates a plain `docker run`, so wait explicitly. Failing here rather + # than letting the suite skip is the point: a server that never came up must not read + # as a pass, which is what the final step enforces. + for _ in $(seq 1 45); do + if docker exec sqlquality-ci-pg pg_isready -U postgres -d sqlquality_test; then + break + fi + sleep 2 + done + docker exec sqlquality-ci-pg pg_isready -U postgres -d sqlquality_test + # Prove both settings took effect before any test runs, so a misconfiguration names + # itself here instead of surfacing as an error deep inside a fixture. + docker exec sqlquality-ci-pg psql -U postgres -d sqlquality_test -tAc \ + "SELECT current_setting('shared_preload_libraries')" | grep -q pg_stat_statements + test "$(docker exec sqlquality-ci-pg psql -U postgres -d sqlquality_test -tAc \ + "SELECT current_setting('pg_stat_statements.track')")" = "all" + # The `postgres` extra, since psycopg is what the live tests connect with. Not + # `--all-extras`: nothing here needs the llm extra. + - run: uv sync --extra postgres + - name: Run the integration suite + env: + # The fixture's own default DSN is this exact string, but it is set explicitly so a + # change to either side is a visible change here rather than a silent switch to + # whatever the fixture happens to default to. + SQLQUALITY_TEST_DSN: postgresql://postgres:sqlquality@127.0.0.1:27432/sqlquality_test + # `--strict-markers` catches a typo'd `integration` marker, which would otherwise + # select nothing. The junit XML is what the next step reads: pytest's own + # machine-readable count, not a regex over text meant for humans. + run: uv run pytest -m integration -q -rs --strict-markers --junitxml=integration.xml + # `pytest` exits 0 on skips, and every one of these tests skips itself when the server is + # unreachable — so a service that never became ready, a wrong port, or a marker typo would + # otherwise produce a green run that executed nothing at all. Same discipline as the + # `no-extras` job above, which refuses a skip for the same reason: an invariant that is + # only ever asserted by a test that did not run is not asserted. + # + # `if: always()` so a failing suite still reports *why* — a run that skipped everything + # and a run that genuinely failed need different fixes. + - name: Fail if the integration suite skipped or ran nothing + if: always() + run: | + uv run python - <<'PY' + import pathlib + import sys + import xml.etree.ElementTree as ET + + report = pathlib.Path("integration.xml") + if not report.exists(): + # Reachable because this step is `if: always()`: the pytest step can die before + # writing a report. Say so plainly rather than raising a traceback over it. + sys.exit( + "::error::integration.xml was never written, so the previous step did not " + "get as far as running tests. Its own log has the reason." + ) + root = ET.parse(report).getroot() + suites = root.findall("testsuite") or ([root] if root.tag == "testsuite" else []) + if not suites: + sys.exit("::error::integration.xml has no testsuite element; pytest wrote nothing") + total = sum(int(s.get("tests", 0)) for s in suites) + skipped = sum(int(s.get("skipped", 0)) for s in suites) + failures = sum(int(s.get("failures", 0)) + int(s.get("errors", 0)) for s in suites) + executed = total - skipped + print(f"collected={total} executed={executed} skipped={skipped} failed={failures}") + if skipped: + sys.exit( + f"::error::{skipped} integration test(s) skipped. These tests skip themselves " + "when no Postgres answers, so a skip here means the service container never " + "became ready, the published port does not match SQLQUALITY_TEST_DSN, or the " + "server is not the one this suite expects. A skip must not read as a pass." + ) + if executed == 0: + sys.exit( + "::error::zero integration tests ran. Either the `integration` marker no " + "longer selects them or the package collected nothing." + ) + PY + # `uv sync` installs what uv.lock pins, so every job above tests exactly one point in the # dependency ranges pyproject declares. A user running `pip install sqlquality` gets the # *newest* release satisfying those ranges instead — and that difference has already shipped diff --git a/CHANGELOG.md b/CHANGELOG.md index ec7762e..dc35a79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,12 +88,50 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- CI now runs the 23 live-Postgres integration tests, in a job with a `postgres:16` service + container that has `pg_stat_statements` preloaded. Until now they ran only on a contributor's + own machine, while this feature's live suite was repeatedly the only thing that caught a + whole class of bug — a `reltuples = -1` sentinel that suppressed every proposal, redaction + dismembering `$N` placeholders, a `toplevel` filter that produced confidently-wrong advice, + and a workload statement that failed on the wire for every default run. The job fails if the + suite skipped or executed nothing, since every one of those tests skips itself when no server + answers and `pytest` exits 0 on a skip. +- The integration compose file publishes host port 27432 instead of 55432, which collided with + an unrelated container in practice — and because `docker compose up` neither binds nor fails + in that case, the suite silently talked to whatever else was listening. The fixture now also + verifies which server answered (database name, and `pg_stat_statements` in + `shared_preload_libraries`) and fails naming a port collision as the likely cause, rather + than trusting that a successful connection reached the right database. +- `--ddl`'s guarantee that every line of the generated script is either an intended statement + or a `--` comment now holds for all ten codepoints `str.splitlines()` treats as a line + boundary, on both the Postgres and Redshift renderers. The guard tested only `\n` and `\r`, + while everything that splits the text uses `splitlines()`, so an introspected identifier + containing `\v`, `\f`, `\x1c`, `\x1d`, `\x1e`, `\x85`, `U+2028` or `U+2029` — all legal + inside a quoted Postgres identifier — produced a second physical line the guard never + examined, and the tail of the statement was emitted looking like a bare statement of its own. +- ADV004 (partial index) now consults the existing-index list, which it was alone among the + index-creating rules in never doing. A plain index leading with the guarded column now + suppresses the proposal — that index already serves the lookup, and the partial index's only + advantage is a size saving this tool cannot measure against a second index's write cost (and + which ADV003 would never flag as redundant, since its prefix check is restricted to plain + indexes). Where a check genuinely could not run it is now stated rather than skipped: an + unreadable existing-index list caps confidence at LOW and says so, and an existing *partial* + or expression index that leads with the same column is named, since sqlquality does not + compare index predicates and so cannot tell whether the proposal is already applied. New + evidence keys `partial_indexes_not_compared` and `expression_indexes`; deliberately not + ADV001's `partial_indexes_skipped`, which records a different fact. - dbt enrichment now discloses itself in the terminal on **every** engine. The stderr disclosure line counted only ADV302's config-block rewrite, which no Redshift proposal can reach (nothing Redshift emits is a `CREATE INDEX`), so a `--project-dir` run on Redshift warned in `rationale` and in the `--ddl` note that `dbt run` may undo an hours-long full-table rewrite while the terminal row stayed byte-identical to a dbt-free run. Any - proposal whose DDL cannot be expressed as dbt config is now counted and reported too. + proposal whose DDL cannot be expressed as dbt config is now counted and reported too, as is + an index *drop* (ADV002, ADV003) on a dbt-managed relation — the case reachable on Postgres, + where a run proposing only drops enriched every one of them and still said nothing, so an + operator applied a drop that the next `dbt run` recreated from the model's `indexes:` config. + Each of the three outcomes is reported as its own clause, because each calls for a different + action: paste a config block, expect a runnable statement not to survive the next rebuild, or + delete a config entry as well as running the drop. - `IS NOT NULL` predicates were classified as `IS NULL` when sqlglot 30.13 or newer was installed, because that release moved the negation from a wrapping `Not` node onto a `negate` flag on the `Is` node itself. Both encodings are now read. This was not cosmetic: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9c376e3..d2c035d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,4 +62,11 @@ docker compose -f tests/integration/docker-compose.yml down These are deselected by default, so `uv run pytest` stays green without Docker. Point them at your own server with `SQLQUALITY_TEST_DSN`. They need the `postgres` extra -(`uv sync --extra postgres`). +(`uv sync --extra postgres`), and a server with `pg_stat_statements` in +`shared_preload_libraries` — `CREATE EXTENSION` alone is not enough, which is why the compose +file passes it as a server flag. + +The compose file publishes host port **27432**. If something else already holds it, `docker +compose up` neither binds nor fails, and the suite would talk to whatever is listening — so +the fixture checks which server answered and fails with that diagnosis rather than producing +a puzzle. Free the port (`docker ps --filter publish=27432`) or set `SQLQUALITY_TEST_DSN`. diff --git a/README.md b/README.md index bd09e5d..4da7507 100644 --- a/README.md +++ b/README.md @@ -410,7 +410,7 @@ statement is not valid SQL to copy out and run. | ADV001 | Composite index candidate: hot equality columns, then one range/sort column, arity ≤ 3, and only columns some single query group filters on *together* | cost share, NDV, row estimate, joint co-occurring fingerprint count, absence of a covering index | | ADV002 | Drop an index with zero recorded scans (excludes unique/primary-key indexes) | scans since last stats reset, size | | ADV003 | Drop an index whose column list is a strict prefix of a wider index | both column lists | -| ADV004 | Partial index: a hot equality column guarded by a hot, co-occurring `IS [NOT] NULL` check | cost share, co-occurring fingerprint count | +| ADV004 | Partial index: a hot equality column guarded by a hot, co-occurring `IS [NOT] NULL` check | cost share, co-occurring fingerprint count, absence of a plain index leading with the guarded column | | ADV005 | Non-sargable predicate — a cast/function on a column, or a leading-wildcard `LIKE` | cost share | | ADV006 | Hot `SELECT *` on a wide table (≥15 columns) | cost share, column count | | ADV007 | Add index on a hot join key with no existing index leading with it | cost share, NDV, row estimate, absence of a covering index | diff --git a/src/sqlquality/workload/dbt.py b/src/sqlquality/workload/dbt.py index 53e83c2..b941820 100644 --- a/src/sqlquality/workload/dbt.py +++ b/src/sqlquality/workload/dbt.py @@ -536,6 +536,17 @@ def _prepend_note(existing: str | None, dbt_note: str) -> str: warning is a caveat *on* it, and `dbt_note` is emitted exactly once — a note already carrying this warning is returned unchanged rather than accumulating a second copy, so a second enrichment pass over an already-enriched proposal cannot double it. + + **Known and deliberately not fixed: only 2 of `_classify`'s 5 `note=` sites go through + this.** The `DROP INDEX` branch and the generic non-index branch do; the three remaining + ones — unrecognised materialization, and the two "cannot be expressed as config" bail-outs + — build their `note` from `_dbt_ddl_note(...)` directly and would discard an existing note + if one ever reached them. Today none can: every proposal that takes those three paths is a + `CREATE INDEX` from ADV001/004/007/008, and no Postgres rule sets `note` before enrichment + runs. The reachable case is a rule that both sets its own `note` *and* emits index-creating + DDL — Redshift's ADV105 sets a note, but Redshift emits no `CREATE INDEX`, so it lands in + the generic branch, which is already covered. Route the other three through this function + when such a rule appears; changing them now would add three untestable paths. """ if not existing: return dbt_note @@ -695,6 +706,14 @@ def _classify(proposal: Proposal, model: ModelNode) -> tuple[Proposal | None, _I "removed as well or the drop will not stick — and this proposal will come " "back on the next run." ) + # A flag for the same reason `dbt_index_config` and + # `dbt_ddl_not_expressed_as_config` are: the warning above lives in `rationale` + # and `note`, and the terminal prints neither, so this row is byte-identical to + # the same ADV002/ADV003 proposal from a dbt-free run. Without this, + # `describe_rewrites` was silent on the whole class of *Postgres* run that emits + # only index drops for dbt-managed relations — enrichment fired, the operator was + # told nothing, and the drop they applied came back on the next `dbt run`. + evidence["dbt_drop_may_be_recreated"] = True return ( dataclasses.replace( proposal, @@ -709,7 +728,14 @@ def _classify(proposal: Proposal, model: ModelNode) -> tuple[Proposal | None, _I # this stays covered when one does. It is no longer hypothetical: Redshift's # ADV101-103 (ALTER SORTKEY/DISTKEY/DISTSTYLE, table rewrites), ADV104 # (VACUUM/ANALYZE) and ADV105 (Advisor's own DDL, relayed verbatim) all land here, - # since none of them is a CREATE/DROP INDEX. See + # since none of them is a CREATE/DROP INDEX. + # + # Known gap, deliberately not closed: this branch is exercised only from the Redshift + # side. It is engine-agnostic by construction — it keys on the DDL prefix, not on the + # adapter — but every test that reaches it today arrives with a Redshift-shaped + # proposal, so a Postgres rule emitting non-index DDL (an ALTER TABLE, a CLUSTER, a + # REINDEX) would land here with no test of its own. Writing one now would mean + # inventing a proposal shape no rule produces; add it with that rule. See # `test_dbt_warning_is_attached_to_a_redshift_rewrite_proposal` in # `tests/test_workload_redshift_rules.py` — this generic path was previously # unexercised by any test. @@ -936,14 +962,23 @@ def describe_rewrites(proposals: list[Proposal]) -> str | None: same title. Without this line a user who reads only the terminal cannot tell enrichment happened. - **Both kinds are counted, not only ADV302's.** Counting the config-block rewrite alone - made this function return `None` for every Redshift run: nothing Redshift emits is a + **All three kinds are counted, not only ADV302's.** Counting the config-block rewrite + alone made this function return `None` for every Redshift run: nothing Redshift emits is a `CREATE INDEX`, so every Redshift proposal for a dbt-managed relation takes `_classify`'s generic path instead — it gets the warning that `dbt run` may undo an hours-long full-table rewrite, and then the terminal said nothing at all. That is verbatim the failure mode this function exists to prevent, on the engine where the wasted work is hours rather than seconds. + The third kind — an index *drop* on a dbt-managed relation, which `dbt run` may put + straight back from the model's `indexes:` config — was the same hole one branch further + along, and reachable on the adapter this module was written for: a Postgres run whose only + proposals for dbt-managed relations are ADV002/ADV003 drops enriches every one of them and + used to print nothing. It is counted separately rather than folded into the "cannot be + expressed as dbt config" clause because the required action is the opposite one — there + the operator keeps the statement and expects it not to last, here they must *also* delete + a config entry or the drop silently reverts. + Counted off evidence flags rather than by searching the DDL or rationale text for "ADV302", which would depend on the wording of a string meant for humans. """ @@ -952,7 +987,10 @@ def describe_rewrites(proposals: list[Proposal]) -> str | None: unexpressed = sum( 1 for p in proposals if p.evidence.get("dbt_ddl_not_expressed_as_config") is True ) - if not rewritten and not merged and not unexpressed: + recreatable_drops = sum( + 1 for p in proposals if p.evidence.get("dbt_drop_may_be_recreated") is True + ) + if not rewritten and not merged and not unexpressed and not recreatable_drops: return None clauses: list[str] = [] if rewritten or merged: @@ -972,6 +1010,12 @@ def describe_rewrites(proposals: list[Proposal]) -> str | None: "as dbt config: the statement is still runnable, but the next `dbt run` may undo " "it — see each proposal's note in --ddl, or its rationale in --markdown/--json" ) + if recreatable_drops: + clauses.append( + f"{recreatable_drops} index drop(s) target a dbt-managed relation: if the index is " + "declared in the model's `indexes:` config, remove that entry too or the next " + "`dbt run` recreates it and the drop does not stick" + ) return "; ".join(clauses) diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index 90f4f1a..91a6b20 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -914,9 +914,11 @@ def _first_co_occurring( def propose_partial_indexes( usage: Sequence[ColumnUsage], facts: Mapping[Relation, TableFacts], + existing: Mapping[Relation, Sequence[PgIndex]], *, min_cost_share: float, min_rows: int = MIN_ROWS_FOR_INDEX, + have_index_data: bool = True, ) -> list[Proposal]: """ADV004 — index the hot equality column, restricted by a hot null-check predicate. @@ -926,6 +928,37 @@ def propose_partial_indexes( Gated on table size exactly as ADV001 is: both rules create an index, and below the floor a sequential scan is the right plan whichever rule proposed it. An unknown row count caps confidence at LOW rather than being assumed large. + + **This rule was the only index-creating rule that never consulted the existing-index list + at all**, so it could propose an index an existing one already serves and its rationale + said nothing about the gap — a check that silently did not run, which is the one thing + this rule set is built to refuse. It now runs `_covered` like ADV001, ADV007 and ADV008, + with two differences that follow from the proposal itself being *partial*: + + * **A plain index leading with the guarded column suppresses this proposal.** A partial + index's only advantage over such an index is size — the access path is already there, + since `WHERE leading = $1 AND guard IS NULL` can be served by a plain index on + `(leading)` with the null check applied as a filter. Size is exactly what this tool + cannot measure: nothing here knows what fraction of the table satisfies the guard, so + "smaller" is an assertion, not evidence, and it would be traded against a second + index's write cost on every insert and update. Worse, nothing downstream would catch + the pair: ADV003's redundant-prefix check is restricted to plain indexes, so a partial + index shadowed by a plain one is never flagged on a later run. Suppressing is therefore + the honest call, not merely the conservative one. + * **A *partial* index that leads with the same column is disclosed, not treated as + coverage.** `_covered` skips partial and expression indexes deliberately (see its + docstring), and for this rule that exclusion cuts the other way than it does for + ADV001: an existing partial index on the same column may be *precisely* this proposal + already applied. Nothing here compares predicates — the existing index's `WHERE` clause + is not parsed, and this proposal's guard is reconstructed from redacted usage — so + whether it is the same index is genuinely unknown and is stated as unknown, naming the + index so an operator can settle it in one glance. + + `have_index_data` is False when the existing-index catalog query was denied, and is + handled exactly as the other three rules handle it: the cost evidence is real so the + proposal survives, but confidence is capped at LOW and the rationale says which check + could not run. `existing` being empty cannot distinguish "no such index" from "could not + look", which is why the flag is separate from the mapping. """ proposals: list[Proposal] = [] for relation, items in sorted(_by_relation(usage).items()): @@ -957,13 +990,50 @@ def propose_partial_indexes( cost_share = max(leading.cost_share, guard.cost_share) if cost_share < min_cost_share: continue + table_indexes = existing.get(relation, ()) + # A plain index leading with this column already provides the access path; only the + # size differs, and this rule cannot measure that. See the docstring. + if _covered((leading.column,), table_indexes) is not None: + continue + # Not coverage, and not the same gap ADV001 discloses: an existing *partial* index + # leading with this column may be this very proposal, already applied. Predicates are + # not compared, so that is unknown rather than either answer. + partial_indexes = tuple( + index.name + for index in table_indexes + if index.is_partial and _is_prefix((leading.column,), index.columns) + ) + # Same whole-identifier matching as ADV001/ADV007/ADV008, for the same reason: a + # substring test reports an index on `lower(guid)` as "mentions id". + expression_indexes = tuple( + index.name + for index in table_indexes + if index.has_expressions and mentions_identifier(leading.column, index.definition or "") + ) predicate = _NULL_ROLE_PREDICATE[guard.role] rationale = ( "The hot predicates always pair this lookup with the same null check, " "so a partial index covers them at a fraction of the size." ) + if not have_index_data: + rationale += ( + " The existing-index list could not be read, so whether an index already " + "serves this lookup is unknown — check before applying." + ) if rows is None: rationale += _UNKNOWN_ROWS_NOTE + if partial_indexes: + rationale += ( + f" A partial index ({', '.join(partial_indexes)}) already leads with this " + "column; sqlquality does not compare its WHERE predicate to this proposal's, " + "so it cannot tell whether this index already exists — check before applying." + ) + if expression_indexes: + rationale += ( + f" An expression index ({', '.join(expression_indexes)}) mentions " + f"{leading.column}; sqlquality cannot tell whether it already serves this " + "lookup, so confirm before applying." + ) proposals.append( Proposal( code="ADV004", @@ -984,8 +1054,18 @@ def propose_partial_indexes( #: How many query groups filter on both columns together. This is what #: makes the proposal supported rather than a guess. "co_occurring_fingerprints": len(shared), + #: Named `partial_indexes_not_compared`, not ADV001's + #: `partial_indexes_skipped`: there the partial index is known not to + #: cover an unfiltered lookup, here it may be this exact proposal already + #: applied and the difference is that nobody compared the predicates. A + #: shared key name would have made two different facts indistinguishable + #: in `--json`, which renders evidence as bare `k=v` pairs. + "partial_indexes_not_compared": partial_indexes, + "expression_indexes": expression_indexes, }, - confidence=Confidence.LOW if rows is None else Confidence.MEDIUM, + confidence=( + Confidence.LOW if rows is None or not have_index_data else Confidence.MEDIUM + ), ddl=( f"CREATE INDEX ON {_qualified(relation.schema, relation.table)} " f"({_quote_ident(leading.column)}) " @@ -1189,6 +1269,30 @@ def _comment_lines(text: str) -> list[str]: return [f"-- {line}" for line in text.splitlines()] +def _has_line_break(text: str) -> bool: + """True when `text` would occupy more than one physical line in the rendered script. + + Defined as "`str.splitlines` disagrees that this is exactly one line" rather than as a + membership test against a list of characters, and that is the whole point of the function. + Both `render_ddl` implementations used to guard with `"\\n" in ddl or "\\r" in ddl`, while + every place that actually splits the text — `_comment_lines`, `_is_fully_commented`, and + the tests asserting no bare line is ever emitted — uses `splitlines()`, which also splits + on `\\v`, `\\f`, `\\x1c`, `\\x1d`, `\\x1e`, `\\x85`, `\\u2028` and `\\u2029`. An identifier + carrying any of those eight produced a second physical line in the file that the guard + never examined, so `render_ddl` skipped the NOT-RENDERED fallback and emitted something a + reader sees as a bare, statement-shaped line — exactly the invariant this script format + states unconditionally. Postgres permits every one of them inside a quoted identifier. + + Deriving the answer from `splitlines` rather than restating its character set keeps the + guard and the splitting in lockstep by construction: a future CPython that recognises one + more line boundary cannot reopen the hole, and there is no second list to forget to update. + + Empty text is not a line break — it has no lines at all — and is answered False rather + than by the raw `splitlines() != [text]` comparison, which would say True for `""`. + """ + return bool(text) and text.splitlines() != [text] + + def _is_fully_commented(ddl: str) -> bool: """True when every physical line of `ddl` already begins with `--`. @@ -1930,7 +2034,13 @@ def propose( min_cost_share=min_cost_share, have_index_data=have_index_data, ), - *propose_partial_indexes(aggregation.usage, facts, min_cost_share=min_cost_share), + *propose_partial_indexes( + aggregation.usage, + facts, + existing, + min_cost_share=min_cost_share, + have_index_data=have_index_data, + ), *propose_sargability(aggregation.usage, workload, min_cost_share=min_cost_share), *propose_select_star( workload, facts, min_cost_share=min_cost_share, dialect=self.engine @@ -1960,9 +2070,7 @@ def render_ddl(self, proposals: list[Proposal]) -> str: for proposal in proposals: if not proposal.ddl: continue - if ("\n" in proposal.ddl or "\r" in proposal.ddl) and not _is_fully_commented( - proposal.ddl - ): + if _has_line_break(proposal.ddl) and not _is_fully_commented(proposal.ddl): # An identifier containing a line break cannot be emitted as a single-line # statement. Quoting already makes it *semantically* safe — psql parses the # whole thing as one quoted identifier, so nothing extra executes — but the diff --git a/src/sqlquality/workload/redshift.py b/src/sqlquality/workload/redshift.py index 2d3c2f7..b7cb2a9 100644 --- a/src/sqlquality/workload/redshift.py +++ b/src/sqlquality/workload/redshift.py @@ -81,6 +81,7 @@ from sqlquality.workload.postgres import ( _by_relation, _comment_lines, + _has_line_break, _is_fully_commented, _sentences, ) @@ -1531,10 +1532,11 @@ def render_ddl(self, proposals: list[Proposal]) -> str: tool generated. Reuses rather than reimplements: `cost_share_of` (bool-safe cost-share formatting) - and `_is_fully_commented`'s line-break guard, both imported from `models.py` and - `postgres.py` respectively — the same protection that guarantees a hostile - identifier (one carrying an embedded newline, `\\r`, or a `--`/`;` sequence) cannot - produce a bare, executable-looking line in this script either. See + and `_has_line_break`/`_is_fully_commented`'s line-break guard, imported from + `models.py` and `postgres.py` respectively — the same protection that guarantees a + hostile identifier (one carrying any codepoint `str.splitlines` breaks on, or a + `--`/`;` sequence) cannot produce a bare, executable-looking line in this script + either. See `test_render_ddl_never_emits_a_bare_uncommented_line` in `tests/test_workload_redshift_rules.py`. """ @@ -1564,9 +1566,7 @@ def render_ddl(self, proposals: list[Proposal]) -> str: for proposal in proposals: if not proposal.ddl: continue - if ("\n" in proposal.ddl or "\r" in proposal.ddl) and not _is_fully_commented( - proposal.ddl - ): + if _has_line_break(proposal.ddl) and not _is_fully_commented(proposal.ddl): # Identical guard to `PostgresWorkloadAdapter.render_ddl`: an identifier # carrying a literal line break is already semantically safe once quoted # (the whole thing parses as one identifier), but a raw newline would still diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 7452f61..bcec715 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -14,20 +14,112 @@ Bring the server up with: docker compose -f tests/integration/docker-compose.yml up -d uv run pytest -m integration + +`live_dsn` verifies it reached the server compose started rather than trusting the +connection — a published port that is already taken makes `docker compose up` a silent no-op. """ from __future__ import annotations import os from pathlib import Path +from urllib.parse import unquote, urlparse import pytest -DEFAULT_DSN = "postgresql://postgres:sqlquality@127.0.0.1:55432/sqlquality_test" +from sqlquality.workload.secrets import scrub + +#: Must match the host port `docker-compose.yml` publishes — see the comment there for why it +#: is 27432 and not 55432. +DEFAULT_PORT = 27432 +#: The database `docker-compose.yml` creates. Checked, not assumed: see `live_dsn`. +EXPECTED_DATABASE = "sqlquality_test" +DEFAULT_DSN = f"postgresql://postgres:sqlquality@127.0.0.1:{DEFAULT_PORT}/{EXPECTED_DATABASE}" _PACKAGE_DIR = Path(__file__).parent +def describe_dsn(dsn: str) -> str: + """`host:port/database` — where we connected, with the credentials left out. + + These messages are printed by CI now, and `SQLQUALITY_TEST_DSN` can point anywhere, so the + DSN itself must not be echoed: the project's rule that no credential appears in any output + applies to a fixture's failure text as much as to the tool's. + + A keyword-form DSN (`host=... password=...`) is not a URL, and `urlparse` puts the whole + string — password included — in `path`, so anything but a recognised URI scheme with a + hostname is described generically rather than picked apart. Host and port are safe by + construction; `path` is only ever the database name once a scheme parsed. + """ + parsed = urlparse(dsn) + if parsed.scheme not in {"postgres", "postgresql"} or not parsed.hostname: + return "the server SQLQUALITY_TEST_DSN points at" + database = (parsed.path or "").lstrip("/") or "(no database in the DSN)" + return f"{parsed.hostname}:{parsed.port or 5432}/{database}" + + +def dsn_secrets(dsn: str) -> tuple[str, ...]: + """The DSN's password, in both the encoded and decoded forms a driver may echo. + + Same reasoning as `sqlquality.workload.secrets.secrets_for`, which cannot be reused + directly: it takes a `ConnectionParams`, and reconstructing one here to reach one field + would couple this fixture to a model it has no other use for. The *scrubbing* is reused — + only the token extraction is local. + """ + encoded = urlparse(dsn).password + if not encoded: + return () + decoded = unquote(encoded) + return (encoded, decoded) if decoded != encoded else (encoded,) + + +def server_mismatches(database: str, preloaded: str) -> list[str]: + """Every reason the server we reached is not the one this suite needs, or `[]`. + + A module-level function rather than inline fixture code so it can be exercised without + Docker: the whole point is what happens when the server is *wrong*, and a check that only + runs when the server is right is a check nobody ever sees run. + + Both facts are cheap single-round-trip reads and both discriminate a stranger's Postgres + from this suite's. `shared_preload_libraries` is the one that matters most: without + `pg_stat_statements` preloaded, `CREATE EXTENSION` still succeeds and every later read of + its view fails, deep inside a test, with an error that says nothing about the real cause. + """ + problems = [] + if database != EXPECTED_DATABASE: + problems.append(f"connected to database {database!r}, expected {EXPECTED_DATABASE!r}") + if "pg_stat_statements" not in preloaded: + problems.append( + "the server has no pg_stat_statements in shared_preload_libraries " + f"(it reports {preloaded!r}), so the workload tests cannot read query history" + ) + return problems + + +def collision_hint(dsn: str, problems: list[str]) -> str: + """The failure message for a server that answered but is the wrong one. + + It names a port collision explicitly. That is not a guess dressed up as a diagnosis: it is + the *only* way this state is normally reached, and the reason it cost four people time was + that the symptom (a password-authentication failure, or an unexpected schema) points + anywhere but at the port. It also says *how to look*, since the collision is invisible from + compose's own output. + + The DSN is described, not printed — see `describe_dsn`. + """ + return ( + f"reached a Postgres at {describe_dsn(dsn)}, but it is not this suite's server:\n" + + "\n".join(f" - {p}" for p in problems) + + f"\nThe likely cause is a port collision: something else already holds {DEFAULT_PORT}, " + "and `docker compose up` neither binds nor fails in that case, so the suite connects to " + "whatever is listening.\n" + f"Check `docker ps --filter publish={DEFAULT_PORT}`, then either free the port or point " + "SQLQUALITY_TEST_DSN at the right server.\n" + "Bring the intended one up with: " + "docker compose -f tests/integration/docker-compose.yml up -d" + ) + + def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: """Mark every test collected under this package as `integration`. @@ -43,7 +135,30 @@ def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: @pytest.fixture(scope="session") def live_dsn() -> str: - """A reachable Postgres, or skip with an actionable message.""" + """A reachable Postgres that is verifiably *this* suite's server, or skip. + + **Connecting is not the same as connecting to the right server, and the difference is not + cosmetic.** A published host port that is already taken does not stop `docker compose up`: + compose reports success, the port keeps belonging to whatever bound it first, and every + test in this package silently talks to a stranger's database. That happened for the whole + of this feature's development — an unrelated `postgres:16` container held the old 55432 — + and it surfaced as a password-authentication failure that three reviewers and the author + all read as a code bug. + + So two cheap facts are checked before any test runs, and a mismatch is a hard **failure**, + not a skip: a skip is how the original problem stayed invisible, and by the time this + fixture runs the caller has explicitly asked for `-m integration`. + + * the database name, which pins that this is the server compose created rather than one + that merely answers on the port; + * `shared_preload_libraries`, because `pg_stat_statements` cannot be loaded by `CREATE + EXTENSION` alone. Without it the extension installs and then every read of its view + fails deep inside a test, which says nothing about the real cause. + + An unreachable port stays a *skip*: "no Docker" is the documented, supported state for a + contributor running the default suite. Only a server that answers and is the wrong one + fails. + """ psycopg = pytest.importorskip( "psycopg", reason="integration tests need the postgres extra: uv sync --extra postgres" ) @@ -52,12 +167,26 @@ def live_dsn() -> str: try: with psycopg.connect(dsn, connect_timeout=3) as conn: with conn.cursor() as cur: - cur.execute("SELECT 1") + cur.execute( + "SELECT current_database(), current_setting('shared_preload_libraries')" + ) + database, preloaded = cur.fetchone() except Exception as exc: # driver-specific; the message is what matters + # The driver's own text is scrubbed with the project's own helper before being shown: + # the most common real connect failure *is* an authentication failure, and this message + # now reaches CI logs. pytest.skip( - f"no Postgres at {dsn}: {exc}\n" - "start one with: docker compose -f tests/integration/docker-compose.yml up -d" + f"no Postgres at {describe_dsn(dsn)}: {scrub(str(exc), dsn_secrets(dsn))}\n" + "start one with: docker compose -f tests/integration/docker-compose.yml up -d\n" + f"if that was already running, something else may hold port {DEFAULT_PORT}: compose " + "neither binds an already-taken port nor fails, so this can equally be a stranger's " + f"server rejecting our credentials. Check `docker ps --filter publish={DEFAULT_PORT}` " + f"and `lsof -nP -iTCP:{DEFAULT_PORT} -sTCP:LISTEN`." ) + + problems = server_mismatches(database, preloaded) + if problems: + pytest.fail(collision_hint(dsn, problems)) return dsn diff --git a/tests/integration/docker-compose.yml b/tests/integration/docker-compose.yml index 3492181..23be1b5 100644 --- a/tests/integration/docker-compose.yml +++ b/tests/integration/docker-compose.yml @@ -13,7 +13,20 @@ services: - -c - pg_stat_statements.track=all ports: - - "55432:5432" + # 27432, not 55432, and not any other digit-shuffle of 5432. 55432 collided with an + # unrelated `postgres:16` container on a development machine for the whole of this + # feature's work, and when that happens `docker compose up` does not bind: it reports + # success, the port stays owned by the other container, and the suite talks to whatever + # is already listening. That surfaced as a password-authentication failure that read as a + # code bug and cost four people time. 27432 is unregistered, is outside the ephemeral + # port range on both Linux (32768+) and macOS (49152+) so no outbound socket can take it + # first, and is not one of the ports Postgres tooling gravitates to (5432, 5433, 54320, + # 54321, 55432, 15432). + # + # A port can still collide, so `live_dsn` verifies it reached *this* server rather than + # trusting the connection — see tests/integration/conftest.py. Do not change this number + # without changing DEFAULT_PORT there; set SQLQUALITY_TEST_DSN to point elsewhere instead. + - "27432:5432" healthcheck: test: ["CMD-SHELL", "pg_isready -U postgres -d sqlquality_test"] interval: 2s diff --git a/tests/integration/test_redshift_connect_live.py b/tests/integration/test_redshift_connect_live.py index bba6083..00107ac 100644 --- a/tests/integration/test_redshift_connect_live.py +++ b/tests/integration/test_redshift_connect_live.py @@ -22,10 +22,11 @@ def _with_wrong_password(dsn: str) -> str: """Swap whatever password a DSN carries for a wrong one, however it is shaped. A DSN-shaped string substitution (``dsn.replace(":sqlquality@", ":wr0ng-p4ss@")``) is - a silent no-op whenever the real password is not literally ``sqlquality`` — exactly - the normal case here: host port 55432 is frequently held by an unrelated container on - this machine, so this suite is routinely run against a custom - ``SQLQUALITY_TEST_DSN`` with different credentials. Parsing the DSN's authority + a silent no-op whenever the real password is not literally ``sqlquality`` — a case that + has to keep working: the published host port can be held by an unrelated container (which + is why it moved off 55432 and why ``live_dsn`` now verifies which server answered), so + this suite is routinely run against a custom ``SQLQUALITY_TEST_DSN`` with different + credentials. Parsing the DSN's authority component and re-encoding it with a substituted password works for whatever DSN is handed in, not only the one hardcoded default. """ diff --git a/tests/test_advise_cli.py b/tests/test_advise_cli.py index 5a1bc42..15f3776 100644 --- a/tests/test_advise_cli.py +++ b/tests/test_advise_cli.py @@ -1052,10 +1052,15 @@ def test_adv302_rewrites_an_index_proposal_into_dbt_config_through_the_cli(monke replacing that one line with `pass` — which disables the branch's headline feature completely — left all 665 tests passing. The only guard was `tests/integration/test_advise_live.py`, and `pyproject.toml` sets - `addopts = "-m 'not integration'"` while `ci.yml` provisions no Postgres, so CI never runs - it: ADV302 could have been deleted from the CLI with every check green. This is the third - instance of that defect class on this branch, so it is pinned here — no Docker, no extras, - no live database, because the `no-extras` CI job depends on that. + `addopts = "-m 'not integration'"` while `ci.yml` provisioned no Postgres at the time, so CI + never ran it: ADV302 could have been deleted from the CLI with every check green. This is the + third instance of that defect class on this branch, so it is pinned here — no Docker, no + extras, no live database, because the `no-extras` CI job depends on that. + + CI now has an `integration` job that does provision Postgres, which closes the other half of + that gap. It does not make this test redundant: the live suite needs Docker and the postgres + extra, so it is still the wrong place to pin CLI wiring that must hold for every contributor + running a bare `uv run pytest`. """ _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) result = runner.invoke( diff --git a/tests/test_ci_integration_job.py b/tests/test_ci_integration_job.py new file mode 100644 index 0000000..25ace03 --- /dev/null +++ b/tests/test_ci_integration_job.py @@ -0,0 +1,282 @@ +"""The CI job that runs the live suite, pinned structurally. + +This cannot prove the job works — only a run on a PR can, and until then it is unverified. What +it *can* do is pin every piece of wiring whose silent breakage would turn the live suite back +into something that never runs, which is the state this job was added to end: + +* the job existing at all; +* the step that starts the server publishing the port and creating the database the fixture + actually looks for (a mismatch makes every test skip, and a skip exits 0); +* the two `pg_stat_statements` server flags, without which the suite connects and then fails on + every workload read; +* the explicit readiness wait — `docker run` has no health check to gate the job's steps — and + the final step that refuses a run which skipped or executed nothing. + +Each of those is a one-line edit away from being silently wrong, and none of them is visible in +a green run. `tests/integration/conftest.py`'s own constants are the source of truth here, so +the port and database name cannot drift between the compose file, the fixture and CI. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest +import yaml + +_TESTS_DIR = Path(__file__).parent +if str(_TESTS_DIR) not in sys.path: # pragma: no cover - pytest normally does this itself + sys.path.insert(0, str(_TESTS_DIR)) + +from integration.conftest import DEFAULT_PORT, EXPECTED_DATABASE # noqa: E402 + +_CI = _TESTS_DIR.parent / ".github" / "workflows" / "ci.yml" +_JOB = "integration" + + +@pytest.fixture(scope="module") +def workflow() -> dict: + """Also the test that ci.yml is valid YAML at all — a workflow that does not parse is + silently not run by GitHub, with no failing check to notice.""" + return yaml.safe_load(_CI.read_text(encoding="utf-8")) + + +@pytest.fixture(scope="module") +def job(workflow: dict) -> dict: + assert _JOB in workflow["jobs"], ( + f"ci.yml has no {_JOB!r} job, so the 23 live tests run on nobody's machine but the " + f"author's. Jobs present: {sorted(workflow['jobs'])}" + ) + return workflow["jobs"][_JOB] + + +def _steps_text(job: dict) -> str: + return "\n".join(str(step.get("run", "")) for step in job["steps"]) + + +@pytest.fixture(scope="module") +def server_step(job: dict) -> str: + """The step that starts Postgres, as shell text. + + Deliberately a step and not a `services:` container, and the history is worth keeping + because the obvious approach fails on a real runner. A service container cannot be given + a command — GitHub's schema has no `command` key, and `options` is passed to + `docker create` *before* the image — so the `-c shared_preload_libraries=…` flags the + compose file uses cannot be expressed there. The first version worked around that with + `ALTER SYSTEM` plus a restart and failed in CI: `ALTER SYSTEM SET + pg_stat_statements.track` is rejected outright with "unrecognized configuration + parameter", because that setting does not exist until the library is loaded — which is + what the restart was supposed to accomplish. Getting the order right would need two + restarts. + + `docker run` takes the same flags as `tests/integration/docker-compose.yml`, so the + server CI tests against is described once rather than twice. + """ + steps = [ + str(step.get("run", "")) + for step in job["steps"] + if "docker run" in str(step.get("run", "")) + ] + assert len(steps) == 1, ( + "expected exactly one step starting the server; found " + f"{len(steps)}. Without it the live suite skips itself and the job exits 0." + ) + return steps[0] + + +def test_the_server_is_postgres_16(server_step: str): + """The live suite reads `pg_stat_statements` columns and `reltuples` semantics that are + version-dependent; the compose file pins 16 and CI must not silently test another major.""" + assert "postgres:16" in server_step + + +def test_the_server_publishes_the_port_the_fixture_connects_to(server_step: str): + """A port that drifts from the fixture's makes every test skip — and pytest exits 0 on a + skip, so the job would pass having run nothing. The final step catches that; this catches + it earlier and says which side is wrong.""" + assert f"-p {DEFAULT_PORT}:5432" in server_step + + +def test_the_server_creates_the_database_the_fixture_verifies(server_step: str): + """`live_dsn` fails when `current_database()` is not this name, so a change here would turn + the whole job red with a port-collision message that is not the real cause.""" + assert f"POSTGRES_DB={EXPECTED_DATABASE}" in server_step + assert "POSTGRES_PASSWORD=" in server_step, "the fixture's DSN authenticates with a password" + + +def test_ci_waits_for_the_server_before_running_anything(server_step: str): + """`docker run` has no health check to gate the job's steps, so the wait is explicit. + Without it the suite races startup, every test skips itself, and the job exits 0 — the + exact silent no-op this job was added to end. The final `pg_isready` outside the retry + loop is what turns a server that never came up into a failure rather than a skip.""" + assert "pg_isready" in server_step + assert "seq 1" in server_step, "a bounded retry loop, not a single optimistic check" + + +def test_ci_passes_both_pg_stat_statements_settings_as_server_flags(server_step: str): + """Neither can be reached by `CREATE EXTENSION`: `shared_preload_libraries` is loaded at + postmaster start, and `track = all` is what makes nested statements (the DECLARE ... CURSOR + case the live suite unwraps) appear at all. + + Asserted as **command flags**, which is the only form that works. Applying them with + `ALTER SYSTEM` and a restart was tried and failed in CI: `ALTER SYSTEM SET + pg_stat_statements.track` is rejected with "unrecognized configuration parameter", because + the setting does not exist until the library is loaded. These same two flags appear in + `tests/integration/docker-compose.yml`; a drift between the two would mean local and CI + runs test differently configured servers. + """ + assert "-c shared_preload_libraries=pg_stat_statements" in server_step + assert "-c pg_stat_statements.track=all" in server_step + + +def test_ci_verifies_the_preload_took_effect_before_running_any_test(job: dict): + """Otherwise a failed ALTER SYSTEM surfaces as an error deep inside a seeding fixture, + which says nothing about the real cause.""" + steps = _steps_text(job) + assert "current_setting('shared_preload_libraries')" in steps + assert "current_setting('pg_stat_statements.track')" in steps + + +def test_ci_points_the_fixture_at_the_service_it_started(job: dict): + """The fixture reads `SQLQUALITY_TEST_DSN`. Set explicitly rather than relying on the + fixture's default, so this file and the fixture cannot disagree silently.""" + dsn = next( + step["env"]["SQLQUALITY_TEST_DSN"] + for step in job["steps"] + if "SQLQUALITY_TEST_DSN" in step.get("env", {}) + ) + assert f":{DEFAULT_PORT}/" in dsn + assert dsn.endswith(f"/{EXPECTED_DATABASE}") + + +def test_ci_actually_selects_the_integration_marker(job: dict): + """`pytest` with no `-m` runs the *default* suite, which deselects every one of these + tests: the job would be green, fast, and a complete no-op.""" + steps = _steps_text(job) + assert "-m integration" in steps + assert "--strict-markers" in steps, "a typo'd marker would otherwise select nothing" + + +def test_ci_refuses_a_run_that_skipped_or_executed_nothing(job: dict): + """The reason this job needs a guard at all: every test in the package skips itself when no + Postgres answers, and pytest exits 0 on a skip. A service that never became ready, a wrong + port, or a marker typo would all produce a passing job that ran nothing — the same failure + the `no-extras` job refuses for the same reason. + """ + steps = _steps_text(job) + assert "--junitxml" in steps, ( + "the count has to come from pytest's own machine-readable report, not a regex over " + "output written for humans" + ) + assert "skipped" in steps + assert "executed == 0" in steps + guard = next(step for step in job["steps"] if "executed == 0" in str(step.get("run", ""))) + assert guard.get("if") == "always()", ( + "a failing suite must still report whether it skipped everything or genuinely failed" + ) + + +def _guard_script(job: dict) -> str: + """The guard's Python body, exactly as the shell pipes it to the interpreter. + + Deliberately no `textwrap.dedent`: YAML has already stripped the block scalar's common + indentation, so what CI feeds Python is this string verbatim. Dedenting here would hide a + real indentation error in the file. + """ + run = next(step["run"] for step in job["steps"] if "executed == 0" in str(step.get("run", ""))) + lines = run.splitlines() + assert "PY" in lines, "the heredoc terminator must be a line of its own" + assert lines[lines.index("PY")] == "PY", ( + "the terminator is indented, so `<<'PY'` never closes and the shell reads to EOF" + ) + return run.split("<<'PY'\n", 1)[1].rsplit("\nPY", 1)[0] + + +def test_the_guard_script_is_valid_python(job: dict): + """It is a heredoc inside YAML inside a shell script, three layers away from anything that + would catch a syntax error before CI runs it — and it only executes at the very end of the + job, after everything expensive has already run.""" + import ast + + tree = ast.parse(_guard_script(job)) + assert len(tree.body) > 5, "a near-empty parse would make this test vacuous" + + +def _junit_for(source: str, tmp_path: Path, *, select: str = "") -> Path: + """A **real** pytest JUnit report, not a hand-written one. + + The guard reads attributes off pytest's XML, so a fixture built from what those attributes + are believed to be would prove only self-consistency. Generating the report with pytest + itself is what makes the parse test meaningful. + """ + import subprocess + + (tmp_path / "test_sample.py").write_text(source, encoding="utf-8") + xml = tmp_path / "integration.xml" + subprocess.run( + [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider", f"--junitxml={xml}"] + + (["-m", select] if select else []), + cwd=tmp_path, + capture_output=True, + check=False, + ) + assert xml.exists(), "pytest wrote no report; the rest of this test would prove nothing" + return xml + + +def _run_guard(job: dict, xml: Path) -> tuple[int, str]: + import subprocess + + result = subprocess.run( + [sys.executable, "-c", _guard_script(job)], + cwd=xml.parent, + capture_output=True, + text=True, + ) + return result.returncode, result.stdout + result.stderr + + +def test_the_guard_accepts_a_run_where_every_test_executed(job: dict, tmp_path): + """The control. A guard that fails on everything would block every honest run, and the + obvious way to write one — treating a nonzero `skipped` attribute as the only signal — + reads `tests` and `skipped` off the wrong element and fails universally.""" + xml = _junit_for("def test_a(): pass\ndef test_b(): pass\n", tmp_path) + code, output = _run_guard(job, xml) + assert code == 0, output + assert "executed=2" in output, output + + +def test_the_guard_fails_a_run_that_skipped(job: dict, tmp_path): + """The failure mode this job exists to refuse. Every test in tests/integration/ skips itself + when no Postgres answers, and pytest exits 0 on a skip — so a service container that never + became ready would otherwise be indistinguishable from a passing run. + """ + xml = _junit_for( + "import pytest\ndef test_a(): pass\ndef test_b(): pytest.skip('no server')\n", tmp_path + ) + code, output = _run_guard(job, xml) + assert code != 0, output + assert "skipped" in output + assert "must not read as a pass" in output + + +def test_the_guard_explains_a_missing_report_instead_of_raising(job: dict, tmp_path): + """Reachable because the step is `if: always()`: the pytest step can die before writing a + report, and a traceback over `ET.parse` buries the real log.""" + code, output = _run_guard(job, tmp_path / "integration.xml") + assert code != 0 + assert "never written" in output + assert "Traceback" not in output + + +def test_the_guard_fails_a_run_that_collected_nothing(job: dict, tmp_path): + """A marker typo, a renamed package, or a deselect-everything change: pytest reports zero + tests, and without this the job's only other signal is an exit code the previous step + already consumed.""" + xml = _junit_for( + "import pytest\n@pytest.mark.other\ndef test_a(): pass\n", tmp_path, select="nothingmatches" + ) + code, output = _run_guard(job, xml) + assert code != 0, output + assert "zero integration tests ran" in output diff --git a/tests/test_integration_fixture.py b/tests/test_integration_fixture.py new file mode 100644 index 0000000..0e8cb68 --- /dev/null +++ b/tests/test_integration_fixture.py @@ -0,0 +1,172 @@ +"""The integration suite's own preflight checks, tested without Docker. + +These run in the *default* suite deliberately. The checks in `tests/integration/conftest.py` +exist for the case where the server is wrong, so testing them only from a run against a +correct server would never exercise them at all — and they would be free to rot until the next +time somebody lost a day to a port collision. + +Nothing here imports psycopg or connects to anything: `server_mismatches` and `collision_hint` +are pure functions over two strings, which is why they were lifted out of the fixture body. +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import yaml + +_TESTS_DIR = Path(__file__).parent +if str(_TESTS_DIR) not in sys.path: # pragma: no cover - pytest normally does this itself + sys.path.insert(0, str(_TESTS_DIR)) + +from integration.conftest import ( # noqa: E402 + DEFAULT_DSN, + DEFAULT_PORT, + EXPECTED_DATABASE, + collision_hint, + describe_dsn, + dsn_secrets, + server_mismatches, +) + +_COMPOSE = _TESTS_DIR / "integration" / "docker-compose.yml" +_PRELOADED = "pg_stat_statements" + + +def _published_ports() -> list[str]: + compose = yaml.safe_load(_COMPOSE.read_text(encoding="utf-8")) + return list(compose["services"]["postgres"]["ports"]) + + +def test_the_fixtures_port_matches_the_port_compose_actually_publishes(): + """Two files naming the same port, and nothing tying them together. + + If they drift, `docker compose up` starts a server the fixture never looks at, and the + fixture skips with "no Postgres" — a green run that ran none of the 23 live tests. That is + the same class of invisible failure the port change was made to end, so it is pinned rather + than left to review. + """ + assert _published_ports() == [f"{DEFAULT_PORT}:5432"] + assert f":{DEFAULT_PORT}/" in DEFAULT_DSN + + +def test_compose_still_publishes_a_port_no_common_postgres_tooling_claims(): + """The port change is the first half of the fix and is otherwise unpinned: reverting it to + 55432 (or any of the other ports Postgres tooling gravitates to) would leave every test + green while restoring the collision. Also asserts it is outside the ephemeral range on + Linux and macOS, so no outbound socket can take it before compose binds.""" + assert DEFAULT_PORT not in {5432, 5433, 15432, 54320, 54321, 55432} + assert 1024 < DEFAULT_PORT < 32768 + + +def test_compose_preloads_pg_stat_statements_with_full_tracking(): + """What the fixture's `shared_preload_libraries` check is checking *for*. Dropping either + `-c` flag from the compose command leaves a server that accepts connections and then fails + every workload read.""" + compose = yaml.safe_load(_COMPOSE.read_text(encoding="utf-8")) + command = compose["services"]["postgres"]["command"] + assert f"shared_preload_libraries={_PRELOADED}" in command + assert "pg_stat_statements.track=all" in command + assert compose["services"]["postgres"]["environment"]["POSTGRES_DB"] == EXPECTED_DATABASE + + +def test_the_expected_server_reports_no_mismatches(): + """The control. A check that fails on everything is not a check — it would turn every + correct run into a hard failure.""" + assert server_mismatches(EXPECTED_DATABASE, f"{_PRELOADED},auto_explain") == [] + assert server_mismatches(EXPECTED_DATABASE, _PRELOADED) == [] + + +def test_a_stranger_on_the_port_is_caught_by_its_database_name(): + """The measured failure: an unrelated `postgres:16` held the port, so the suite connected + to a different database entirely (or was refused by it). Connecting proves nothing about + *what* answered.""" + [problem] = server_mismatches("some_other_app", _PRELOADED) + assert "some_other_app" in problem + assert EXPECTED_DATABASE in problem + + +def test_a_server_without_pg_stat_statements_preloaded_is_caught(): + """A same-named database on a plain `postgres:16` is the hardest collision to spot: it + connects, it seeds, and then it fails inside a test with an error about the extension that + says nothing about the port. `CREATE EXTENSION` cannot fix this — the library has to be + loaded at server start.""" + [problem] = server_mismatches(EXPECTED_DATABASE, "auto_explain") + assert "shared_preload_libraries" in problem + assert "auto_explain" in problem, "name what the server actually reports" + + +def test_both_mismatches_are_reported_together_rather_than_one_at_a_time(): + """Reporting only the first would send someone to fix the database name and then hit the + preload failure on the next run.""" + assert len(server_mismatches("postgres", "")) == 2 + + +def test_the_failure_message_names_a_port_collision_as_the_likely_cause(): + """The whole point of the message. The symptom — a password failure, or an unexpected + schema — points anywhere but at the port, which is why this cost the author and three + reviewers time. It must also say how to look, not just what happened. + """ + message = collision_hint(DEFAULT_DSN, server_mismatches("other", "")) + assert "port collision" in message + assert str(DEFAULT_PORT) in message + assert f"docker ps --filter publish={DEFAULT_PORT}" in message + assert "SQLQUALITY_TEST_DSN" in message + assert "docker compose -f tests/integration/docker-compose.yml up -d" in message + assert "neither binds nor fails" in message, ( + "the non-obvious fact that makes this diagnosable: compose does not report the collision" + ) + + +def test_the_failure_message_carries_no_credential(): + """These messages reach CI logs, and `SQLQUALITY_TEST_DSN` can point anywhere, so the DSN + must be described rather than echoed — the project's no-credential-in-any-output rule + applies to a fixture's failure text too.""" + message = collision_hint(DEFAULT_DSN, server_mismatches("other", "")) + assert "postgres:sqlquality@" not in message + assert "sqlquality@" not in message + # Still says where it connected: a message that redacts the location too is unactionable. + assert f"127.0.0.1:{DEFAULT_PORT}/{EXPECTED_DATABASE}" in message + + +def test_describe_dsn_keeps_the_location_and_drops_the_credentials(): + assert describe_dsn(DEFAULT_DSN) == f"127.0.0.1:{DEFAULT_PORT}/{EXPECTED_DATABASE}" + assert describe_dsn("postgresql://user:pw@db.example:6543/analytics") == ( + "db.example:6543/analytics" + ) + # No port in the DSN means libpq's default, which is what the reader needs told. + assert describe_dsn("postgresql://u:pw@db.example/analytics") == "db.example:5432/analytics" + + +def test_describe_dsn_refuses_to_pick_apart_a_keyword_form_dsn(): + """`urlparse` puts a whole keyword-form DSN — password included — in `path`, so anything + that is not a recognised URI is described generically instead of dissected. Getting this + wrong prints the password verbatim, which is precisely the failure being guarded.""" + keyword = "host=db.example port=6543 dbname=analytics user=u password=hunter2" + described = describe_dsn(keyword) + assert "hunter2" not in described + assert "password" not in described + assert described == "the server SQLQUALITY_TEST_DSN points at" + + +def test_dsn_secrets_yields_the_password_in_both_the_forms_a_driver_may_echo(): + """Same trap `secrets_for` documents: `urlparse` returns the password still + percent-encoded, while libpq decodes it before authenticating, so an auth-failure message + carries the *decoded* form and a token of only the encoded one never matches.""" + assert dsn_secrets("postgresql://u:p%40ss@h/db") == ("p%40ss", "p@ss") + assert dsn_secrets("postgresql://u:plain@h/db") == ("plain",) + assert dsn_secrets("postgresql://h/db") == () + + +def test_a_driver_message_echoing_the_password_is_scrubbed_before_it_is_shown(): + """The measured failure was an authentication failure, whose text is the one place a + password can surface. Asserted through the same `scrub` the tool uses, over a message shaped + like libpq's own.""" + from sqlquality.workload.secrets import scrub + + dsn = "postgresql://postgres:s3cretpw@127.0.0.1:27432/sqlquality_test" + libpq = 'connection failed: password authentication failed for user "postgres" (s3cretpw)' + scrubbed = scrub(libpq, dsn_secrets(dsn)) + assert "s3cretpw" not in scrubbed + assert "password authentication failed" in scrubbed diff --git a/tests/test_workload_dbt.py b/tests/test_workload_dbt.py index e888250..f5c35c1 100644 --- a/tests/test_workload_dbt.py +++ b/tests/test_workload_dbt.py @@ -1348,6 +1348,100 @@ def test_describe_rewrites_reports_both_kinds_in_one_line(): assert "1 proposal(s) target a dbt-managed relation" in line +def _drop_proposal(relation, *, code="ADV002", index="idx_cold"): + """An index-drop proposal — the shape ADV002 (unused) and ADV003 (redundant) emit, and the + only shape that reaches `_classify`'s `DROP INDEX` branch.""" + return Proposal( + code=code, + title=f"Drop unused index {index} on {relation}", + rationale="no scans.", + evidence={ + "schema": relation.schema, + "table": relation.table, + "index": index, + "columns": ("status",), + }, + confidence=Confidence.MEDIUM, + ddl=f'DROP INDEX "{relation.schema}"."{index}";', + ) + + +def test_describe_rewrites_reports_an_index_drop_that_a_dbt_run_may_recreate(): + """The third enrichment outcome, and the one reachable on the adapter this module was + written for. A Postgres run whose only proposals for dbt-managed relations are ADV002 / + ADV003 drops enriches every one of them — rationale *and* note — and the terminal said + nothing at all, because only the config-block rewrite and the generic non-index warning + were counted. The operator then applies a drop that the next `dbt run` puts straight back + from the model's `indexes:` config, and this tool proposes the same drop again next run. + """ + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + out = enrich_proposals( + [ + _drop_proposal(relation, code="ADV002", index="idx_cold"), + _drop_proposal(relation, code="ADV003", index="idx_narrow"), + ], + context, + ) + assert [p.ddl for p in out] == [ + 'DROP INDEX "main"."idx_cold";', + 'DROP INDEX "main"."idx_narrow";', + ], "the drops themselves are unchanged; only the disclosure is new" + line = describe_rewrites(out) + assert line is not None + assert "2 index drop(s)" in line + assert "`indexes:` config" in line + assert "does not stick" in line + assert "\n" not in line, "one stderr line" + + +def test_describe_rewrites_is_still_silent_for_an_unmanaged_index_drop(): + """Control for the test above: the new count must depend on `DbtContext.model_for` + actually matching, not merely on a proposal whose DDL is a `DROP INDEX`.""" + context = DbtContext.from_project(_project()) + out = enrich_proposals([_drop_proposal(Relation("public", "unmanaged"))], context) + assert describe_rewrites(out) is None + + +def test_describe_rewrites_counts_the_drop_branch_separately_from_the_other_two(): + """The count has to discriminate *this* branch, not merely be non-zero when enrichment + fired. A flag set in `_classify`'s generic non-index path (or read off the same key as the + config rewrite) would satisfy a bare "the line mentions drops" assertion while reporting + the wrong number for a mixed run, and would report drops on a run that had none. + + All three outcomes call for different actions, which is why they are three clauses: paste + a config block, expect a runnable statement not to last, or delete a config entry as well. + """ + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + + only_drops = enrich_proposals([_drop_proposal(relation)], context) + line = describe_rewrites(only_drops) + assert line is not None + assert "1 index drop(s)" in line + assert "ADV302" not in line, "a drop is not expressed as an `indexes` config block" + assert "cannot be expressed as dbt config" not in line, "that is the generic branch" + + # The other two branches must not be counted as drops. + for other in ( + enrich_proposals([_index_proposal(relation)], context), + enrich_proposals([_non_index_proposal(relation)], context), + ): + other_line = describe_rewrites(other) + assert other_line is not None + assert "index drop(s)" not in other_line, other_line + + all_three = enrich_proposals( + [_index_proposal(relation), _non_index_proposal(relation), _drop_proposal(relation)], + context, + ) + mixed = describe_rewrites(all_three) + assert mixed is not None + assert "ADV302 expressed 1 index proposal(s)" in mixed + assert "1 proposal(s) target a dbt-managed relation" in mixed + assert "1 index drop(s)" in mixed + + def test_prepend_note_keeps_the_existing_note_first_and_emits_the_dbt_note_once(): """Order and non-duplication, neither of which was pinned: losing the existing note was caught by two tests, but swapping the concatenation order and emitting the dbt warning diff --git a/tests/test_workload_redshift_rules.py b/tests/test_workload_redshift_rules.py index b049782..4cd4f06 100644 --- a/tests/test_workload_redshift_rules.py +++ b/tests/test_workload_redshift_rules.py @@ -45,6 +45,13 @@ propose_sortkey, ) +# Sibling test module, not a package import: `tests/` has no `__init__.py`, so pytest puts it +# on `sys.path` and its modules are importable by bare name. Imported rather than copied +# because two hand-maintained lists of invisible control characters are two lists free to +# drift apart, and the reason this file asserts the same property again is that the *call site* +# differs (each adapter's `render_ddl` calls the shared guard separately), not the character set. +from test_workload_rules import EXTRA_LINE_BREAKS # noqa: E402 + R = Relation(schema="public", table="orders") R2 = Relation(schema="public", table="customers") @@ -1416,6 +1423,32 @@ def test_render_ddl_never_emits_a_bare_uncommented_line_for_a_hostile_identifier assert _uncommented(script) == [], script +@pytest.mark.parametrize( + ("char", "name"), EXTRA_LINE_BREAKS, ids=[name for _char, name in EXTRA_LINE_BREAKS] +) +def test_every_splitlines_line_break_in_an_identifier_reaches_the_not_rendered_fallback(char, name): + """The same hole, closed on this adapter too, and pinned here rather than only on the + Postgres side: the guard is shared code (`_has_line_break`, imported from `postgres.py`), + but the two `render_ddl` implementations call it in two separate places, so a fix applied + to one is not a fix applied to both. The codepoint list is imported from the Postgres + renderer's tests deliberately — two copies would be free to drift apart, and this file's + whole reason for asserting it again is that the *call site* is different, not the set. + """ + ddl = f'ALTER TABLE "public"."or{char}ders" ALTER SORTKEY ("created_at");' + assert len(ddl.splitlines()) == 2, f"{name} is not a splitlines boundary" + proposal = Proposal( + code="ADV101", + title="Consider SORTKEY on public.orders(created_at)", + rationale="r", + evidence={"schema": "public", "table": "orders", "cost_share": 0.4}, + confidence=Confidence.MEDIUM, + ddl=ddl, + ) + script = RedshiftWorkloadAdapter().render_ddl([proposal]) + assert "NOT RENDERED" in script, name + assert _uncommented(script) == [], script + + def test_render_ddl_emits_a_pre_commented_multiline_block_verbatim(): """`_is_fully_commented`, reused rather than reimplemented: a `ddl` that already reads as a `--`-commented, multi-line disclosure on every line is not the identifier-with-a- diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index ebee524..c5f42ac 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -1,5 +1,7 @@ from pathlib import Path +import pytest + from sqlquality.models import ( Aggregation, ColumnRole, @@ -15,6 +17,7 @@ from sqlquality.workload.postgres import ( PgIndex, PostgresWorkloadAdapter, + _has_line_break, _is_fully_commented, _quote_ident, propose_grouping_indexes, @@ -888,6 +891,7 @@ def test_partial_index_proposed_for_a_hot_not_null_check(): _usage(_ORDERS, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(), + {}, min_cost_share=0.01, ) assert codes(proposals) == ["ADV004"] @@ -901,6 +905,7 @@ def test_partial_index_polarity_follows_the_predicate(): _usage(_ORDERS, "shipped_at", ColumnRole.NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(), + {}, min_cost_share=0.01, ) assert "IS NULL" in proposals[0].ddl @@ -927,6 +932,7 @@ def test_no_partial_index_when_the_columns_never_co_occur(): ), ], _facts_map(), + {}, min_cost_share=0.01, ) assert proposals == [] @@ -946,6 +952,7 @@ def test_partial_index_reports_the_co_occurrence_that_justifies_it(): ), ], _facts_map(), + {}, min_cost_share=0.01, ) assert codes(proposals) == ["ADV004"] @@ -968,6 +975,7 @@ def test_partial_index_picks_the_costliest_pair_that_actually_co_occurs(): ), ], _facts_map(columns=("status", "region", "shipped_at")), + {}, min_cost_share=0.01, ) assert proposals[0].evidence["columns"] == ("region",) @@ -986,6 +994,7 @@ def test_partial_index_is_suppressed_on_a_small_table(): _usage(_ORDERS, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(rows=50), + {}, min_cost_share=0.01, ) assert proposals == [] @@ -999,6 +1008,7 @@ def test_partial_index_with_an_unknown_row_count_is_low_and_says_why(): _usage(_ORDERS, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(rows=None), + {}, min_cost_share=0.01, ) assert codes(proposals) == ["ADV004"] @@ -1007,10 +1017,128 @@ def test_partial_index_with_an_unknown_row_count_is_low_and_says_why(): assert "unknown" in proposals[0].rationale.lower() +def _partial_usage(relation=None): + """The minimal co-occurring pair ADV004 needs: a hot equality column and a hot null check + on the same query group.""" + relation = relation or _ORDERS + return [ + _usage(relation, "status", ColumnRole.EQUALITY, cost_ms=90.0), + _usage(relation, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), + ] + + +def test_partial_index_is_suppressed_when_a_plain_index_already_leads_with_the_column(): + """ADV004 was the only index-creating rule that never called `_covered` at all. + + A plain index on `(status, created_at)` already provides the access path this proposal + asks for — `WHERE status = $1 AND shipped_at IS NOT NULL` reads it and applies the null + check as a filter — so the partial index buys size alone, and nothing here can measure + that against a second index's write cost. Nothing downstream catches the pair either: + ADV003's redundant-prefix check is restricted to plain indexes, so a partial index + shadowed by a plain one is never flagged on any later run. + """ + existing = {_ORDERS: (PgIndex("idx_plain", ("status", "created_at"), False, False, 5, 1),)} + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), existing, min_cost_share=0.01 + ) + assert proposals == [] + + +def test_partial_index_still_fires_when_no_existing_index_leads_with_the_column(): + """Control for the suppression above: it must key on the *leading* column, exactly as + `_covered` does for every other index-creating rule. An index on `(created_at, status)` + cannot be probed by `status` alone, so it is not coverage and the proposal stands.""" + existing = { + _ORDERS: (PgIndex("idx_wrong_order", ("created_at", "status"), False, False, 5, 1),) + } + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), existing, min_cost_share=0.01 + ) + assert codes(proposals) == ["ADV004"] + + +def test_partial_index_names_an_existing_partial_index_rather_than_comparing_predicates(): + """`_covered` skips partial indexes deliberately, and for *this* rule that cuts the other + way than it does for ADV001: an existing partial index leading with the same column may be + precisely this proposal, already applied. Nothing here parses its WHERE clause, and this + proposal's guard is reconstructed from redacted usage, so the honest report is "unknown, + here is its name" — not silence, and not a suppression the evidence cannot support. + """ + existing = { + _ORDERS: ( + PgIndex("idx_open", ("status",), False, False, 5, 1, is_partial=True, predicate="..."), + ) + } + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), existing, min_cost_share=0.01 + ) + assert codes(proposals) == ["ADV004"], "a partial index is not treated as coverage" + assert "idx_open" in proposals[0].rationale + assert "does not compare its WHERE predicate" in proposals[0].rationale + assert proposals[0].evidence["partial_indexes_not_compared"] == ("idx_open",) + assert "partial_indexes_skipped" not in proposals[0].evidence, ( + "a different fact from ADV001's: there the index is known not to cover an unfiltered " + "lookup, here nobody compared the predicates" + ) + + +def test_partial_index_names_an_expression_index_mentioning_the_same_column(): + """Same gap ADV001, ADV007 and ADV008 already disclose: an expression index's `columns` + tuple understates it, so `_covered` cannot match against it and silence would leave the + operator to discover the overlap themselves.""" + existing = { + _ORDERS: ( + PgIndex( + "idx_lower_status", + (), + False, + False, + 5, + 1, + has_expressions=True, + definition="CREATE INDEX idx_lower_status ON orders (lower(status))", + ), + ) + } + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), existing, min_cost_share=0.01 + ) + assert codes(proposals) == ["ADV004"] + assert "idx_lower_status" in proposals[0].rationale + assert proposals[0].evidence["expression_indexes"] == ("idx_lower_status",) + + +def test_partial_index_discloses_that_the_existing_index_list_could_not_be_read(): + """The confidence discipline the whole rule set turns on: a check that could not run is + disclosed and caps confidence, rather than being silently skipped. ADV001, ADV007 and + ADV008 all did this; ADV004 did not even have the flag.""" + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), {}, min_cost_share=0.01, have_index_data=False + ) + assert codes(proposals) == ["ADV004"], "the cost evidence is real, so the advice survives" + assert proposals[0].confidence is Confidence.LOW + assert "existing-index list could not be read" in proposals[0].rationale + + +def test_partial_index_is_medium_and_silent_about_coverage_when_the_check_did_run(): + """Control for the two disclosures above: neither sentence may appear on the ordinary run + where the list was read and nothing covered the candidate — a rule that always says a + check was skipped tells the operator nothing.""" + proposals = propose_partial_indexes( + _partial_usage(), _facts_map(), {}, min_cost_share=0.01, have_index_data=True + ) + assert proposals[0].confidence is Confidence.MEDIUM + assert "could not be read" not in proposals[0].rationale + assert "does not compare" not in proposals[0].rationale + assert proposals[0].evidence["partial_indexes_not_compared"] == () + assert proposals[0].evidence["expression_indexes"] == () + + def test_no_partial_index_without_an_equality_column_to_index(): proposals = propose_partial_indexes( [_usage(_ORDERS, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4)], _facts_map(), + {}, min_cost_share=0.01, ) assert proposals == [] @@ -1990,6 +2118,7 @@ def test_partial_index_ddl_is_schema_qualified(): _usage(relation, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(relation), + {}, min_cost_share=0.01, ) assert proposals[0].ddl.startswith('CREATE INDEX ON "analytics"."orders" ("status")') @@ -2057,6 +2186,7 @@ def test_adv004_evidence_reports_the_bare_table_name_and_its_own_schema(): _usage(relation, "shipped_at", ColumnRole.NOT_NULL_CHECK, cost_share=0.4, cost_ms=40.0), ], _facts_map(relation), + {}, min_cost_share=0.01, ) assert proposals[0].evidence["schema"] == "staging" @@ -2763,6 +2893,59 @@ def test_a_carriage_return_in_an_identifier_is_not_rendered_as_a_statement(): assert _uncommented(script) == [], script +#: Every codepoint `str.splitlines()` treats as a line boundary *other than* `\n` and `\r` — +#: the only two the guard used to test for. Postgres permits all of them inside a quoted +#: identifier, so each one is a real way for an introspected name to occupy two physical lines +#: in the generated script. Parametrized one per codepoint, deliberately not asserted as a +#: block: a guard that handled six of the eight would still pass a single test built from a +#: string containing all of them, since one unhandled codepoint is enough to trip it. +EXTRA_LINE_BREAKS = [ + ("\v", "VT-000B"), + ("\f", "FF-000C"), + ("\x1c", "FS-001C"), + ("\x1d", "GS-001D"), + ("\x1e", "RS-001E"), + ("\x85", "NEL-0085"), + ("\u2028", "LS-2028"), + ("\u2029", "PS-2029"), +] +_BREAK_IDS = [name for _char, name in EXTRA_LINE_BREAKS] + + +@pytest.mark.parametrize(("char", "name"), EXTRA_LINE_BREAKS, ids=_BREAK_IDS) +def test_every_splitlines_line_break_in_an_identifier_reaches_the_not_rendered_fallback(char, name): + """The guard tested `"\\n" in ddl or "\\r" in ddl`, but every place that actually splits + the text uses `splitlines()`, which breaks on eight further codepoints. An identifier + carrying one of them therefore produced a second physical line in the file that the guard + never examined: the fallback was skipped and the tail of the statement was emitted as a + bare, statement-shaped line, in the one file whose stated purpose is that nothing + unintended is executable. + """ + ddl = f'CREATE INDEX ON "main"."or{char}ders" ("status");' + # The premise, asserted rather than assumed. If this codepoint were not in fact a + # `splitlines()` boundary, everything below would pass while proving nothing at all — the + # exact way a guard test can look green and discriminate nothing. + assert len(ddl.splitlines()) == 2, f"{name} is not a splitlines boundary" + script = PostgresWorkloadAdapter().render_ddl([_ddl_proposal(ddl=ddl)]) + assert "NOT RENDERED" in script, name + assert _uncommented(script) == [], script + + +def test_has_line_break_agrees_with_splitlines_on_every_boundary(): + """The guard is derived from `splitlines()` rather than from a restated character list, + which is what keeps it in lockstep with `_comment_lines`, `_is_fully_commented` and the + tests. Pinned directly as well as through the renderer, since this is the predicate the + whole "nothing unintended is executable" promise now rests on. + """ + for char, name in [("\n", "LF-000A"), ("\r", "CR-000D"), *EXTRA_LINE_BREAKS]: + assert _has_line_break(f"a{char}b") is True, name + assert _has_line_break(f"trailing{char}") is True, name + assert _has_line_break('CREATE INDEX ON "main"."orders" ("status");') is False + assert _has_line_break("a\tb c") is False, "a tab or a space is not a line boundary" + # No lines at all is not a line break, and `splitlines() != [text]` alone would say it is. + assert _has_line_break("") is False + + def test_is_fully_commented_requires_every_line_to_be_a_comment(): """The guard that lets a pre-commented multi-line `ddl` skip the NOT-RENDERED fallback.