diff --git a/CHANGELOG.md b/CHANGELOG.md index 1429a8a..5dec6f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,35 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +## [1.1.0] - 2026-09-08 + +### Added + +- **The schema advisor, and `vacuum:lint`.** `vacuum:check` reads what a database has been *doing* — dead tuples, bloat, freeze age, `pg_stat_statements` — and every one of those needs a database with a history behind it. A pipeline has the opposite thing: a container ninety seconds old with the migrations freshly applied, where fifteen of the twenty rules find nothing and the command reports a perfect score on a database nobody has ever used. That is the same failure the command's own guard was written to prevent, arrived at from the other direction. `vacuum:lint` asks the questions that *are* answerable there — the ones the catalog can answer the moment `migrate` returns, without a single row existing. + +- **Six rules that need no statistics.** `unindexed-foreign-key`, the one that matters most: PostgreSQL indexes a primary key and a unique constraint and creates **nothing** for a foreign key, so `foreignId()->constrained()` writes half of what it looks like it wrote, and the cost lands on the *parent's* deletes as a sequential scan of the child under a lock. `foreign-key-type-mismatch`, where an `integer` key referencing a `bigint` is accepted, enforced correctly, and quietly un-indexable because the comparison crosses types — nothing in the catalog is marked wrong and the delete is simply slow forever. `int4-primary-key`, which is the wraparound story on a different clock: `increments('id')` counts to 2,147,483,647 and the next insert fails, with no warning and no slowdown first. `missing-primary-key`, which breaks logical replication before it inconveniences anybody. `unindexed-morphs`, and `json-not-jsonb`. `duplicate-index` is carried across from the dashboard's tier, because two migrations creating one index is visible on an empty database and is exactly what this is for. + +- **Schema findings are scored separately, by their own advisor.** `Health` computes the score from the findings and from nothing else — deliberately, so the grade can never disagree with the list beneath it — and the consequence of that property is that adding rules to the shared advisor would silently re-grade every installation that upgraded. Somebody's A becomes a C on a `composer update`, with no breaking change to point at. So `SchemaAdvisor` merges its own tier and scores it alone; the two numbers are never added and never averaged. The dashboard is untouched. + +- **A CI-shaped command.** `vacuum:lint` defaults to `--fail-on=warning` where `vacuum:check` defaults to `critical`, because the two mean different things by the word: a warning from `check` is a database drifting and should not redden a build overnight, while a warning from `lint` is a schema that was wrong the moment somebody typed it. It keeps `check`'s guard that a disabled Vacuum **fails rather than passes**, and carries its own `--format=json` document, which adds `evidence` and `table` — a schema finding is always about a table, and whatever reads the document will want to say which. + +- **New public API, covered from 1.1.** The `SchemaRule` contract, the `TableSchema` and `IndexDefinition` value objects, the `SCHEMA_RULES` tag a custom schema rule is registered under, the `vacuum:lint --format=json` document, and configuration under `vacuum.lint.*`. `SchemaRule` returns a *list* of findings where every other rule contract returns one or null: a table with four unindexed foreign keys has four problems on four columns, each fixed by a different statement, and collapsing them would read acceptably in a terminal and be useless anywhere else. + +- **`IndexColumns`, a catalog read describing what an index could serve** rather than whether anything has used it. `pg_stat_user_indexes` cannot say anything at all about a database created ninety seconds ago; `pg_index` is true as soon as `CREATE INDEX` returns. Expression indexes are excluded, because their `indkey` carries a placeholder that joins to no attribute and a column list with a silent gap in it is worse than no column list. + +### Fixed + +- **A partial index no longer counts as covering a foreign key.** `constraints.sql` tested `indisvalid` and not `indpred`, so an index with a `WHERE` clause satisfied the coverage check — while `TableSchema::hasIndexLeadingWith()`, written later, correctly refused it. The two disagreed, and the permissive one fed the headline rule: a foreign key covered only by a partial index was silently passed. A partial index holds only the rows its predicate admits and the referential-integrity check looks for arbitrary parent rows, so it cannot serve one. The `unindexed-foreign-keys` lesson reads the same column and is corrected with it, and the query that lesson hands the reader to run themselves is now pinned to the shipped one by a test, since it had already drifted once. + +- **Constraint column types are no longer split on a delimiter they can contain.** The type lists were aggregated with a comma and split on one, and `format_type` renders `numeric(10,2)` — so a foreign key onto a `decimal` column arrived as two fragments, failed its own equality test, and produced the unparseable remediation `ALTER TABLE … TYPE numeric(10;`. All three lists are aggregated on a newline now, which neither a rendered type nor an identifier can contain. + +### Changed + +- `Constraint` gains `$columnTypes`, `$referencedColumnTypes` and `typesMatch()`. Both parameters are trailing and optional, so existing construction is unaffected; `Constraint` is not among the value objects frozen at 1.0. +- `CheckCommand`'s severity handling and finding rendering are extracted to `SeverityBar` and `FindingReporter` and shared with `vacuum:lint`, so the rule that `Severity::Unknown` never fails a build has one implementation rather than two. `CheckCommand`'s behaviour is unchanged and its tests were not touched. + +## [1.0.0] - 2026-07-21 + ### Fixed — the advice - **The configuration audit read a timeout Vacuum had set on itself.** Every query in the package runs through `ReadOnlyExecutor`, which issues `SET LOCAL statement_timeout` before the statement; the audit then selected `pg_settings.setting` from inside that same transaction and reported the number back as a fact about the server. Two rules were wrong because of it, in opposite directions. `lock-timeout-ineffective` compared a real `lock_timeout` against the 5000 Vacuum had just injected, so a textbook-correct server — `statement_timeout` 30s, `lock_timeout` 10s, the lock timeout firing first exactly as intended — was told its lock timeout could never fire, and acting on that advice would have degraded a correct configuration. `timeouts-unset` required `statement_timeout` to read `0` and therefore could not fire at all, on any server, ever: it had never detected anything. Settings are now read from `reset_val`, which is what the role, the database and `postgresql.conf` say and is provably immune to `SET LOCAL`. The session's own view stays available as `Settings::runtimeValue()` for the caller that genuinely wants it, but it is no longer the default, because a configuration rule asking "what is this server configured to" must not be answerable by the observer. @@ -109,6 +138,8 @@ First release. - **A Filament v4 panel** (optional peer — nothing changes for a Blade-only install): a **Vacuum** navigation group with an **Overview** dashboard (health score and grade, database vitals, charts, the findings with copyable remediation, and live running vacuums) and read-only resources for **Tables**, **Indexes**, **Sessions** and **Statements**. Every surface shares the one `Vacuum::auth()` gate and opts out of tenant scoping, so it is at home in a multi-tenant panel. - **Extensibility.** Application rules can be tagged onto the advisor per subject (`TABLE_RULES`, `INDEX_RULES`, and the rest), and both the config and the dashboard views are publishable. -[Unreleased]: https://github.com/heyosseus/vacuum/compare/v0.3.0...HEAD +[Unreleased]: https://github.com/heyosseus/vacuum/compare/v1.1.0...HEAD +[1.1.0]: https://github.com/heyosseus/vacuum/compare/v1.0.1...v1.1.0 +[1.0.0]: https://github.com/heyosseus/vacuum/compare/v0.3.0...v1.0.0 [0.3.0]: https://github.com/heyosseus/vacuum/compare/v0.1.0...v0.3.0 [0.1.0]: https://github.com/heyosseus/vacuum/releases/tag/v0.1.0 diff --git a/README.md b/README.md index f565ef1..8636ce5 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ Vacuum reads what PostgreSQL already knows about itself — `pg_stat_user_tables It shows you that statement. It never runs it. -> **Status: 1.0.0.** The public API is frozen — see [What semver covers](#what-semver-covers). A breaking change to the rule contracts, `Finding`, the value objects, the configuration keys or the `--format=json` document now requires a major version. +> **Status: 1.1.0.** The public API is frozen — see [What semver covers](#what-semver-covers). A breaking change to the rule contracts, `Finding`, the value objects, the configuration keys or the `--format=json` documents now requires a major version. ## Quick start @@ -41,6 +41,7 @@ Already running a Filament panel? `php artisan vacuum:install --filament` puts t - [The standalone dashboard](#the-standalone-dashboard) · [Inside Filament](#inside-filament) - [Who may look](#who-may-look) · [Which database](#which-database) - [In your pipeline](#in-your-pipeline) — `vacuum:check`, and failing a build +- [Linting the schema](#linting-the-schema) — `vacuum:lint`, and what is wrong before a row exists - [History over time](#history-over-time) — direction, forecasts, and what changed - [The SQL console](#the-sql-console) — and what actually makes it safe - [Tuning the thresholds](#tuning-the-thresholds) · [Writing your own rule](#writing-your-own-rule) · [Restyling the dashboard](#restyling-the-dashboard) @@ -230,6 +231,47 @@ php artisan vacuum:check --format=json # score, grade, deductions, finding Two things worth knowing. It **never writes** — the remediation is printed for you to read and decide on, exactly as it is on the page. And if Vacuum is disabled it **fails rather than passing**: a check that goes green because it never looked is worse than no check at all. +### Linting the schema + +`vacuum:check` reads what the database has been *doing*, and a pipeline's database +has not done anything. A Postgres container ninety seconds old with the migrations +freshly applied has no dead tuples, no bloat, no freeze age and no statements — +so most of the rules find nothing, and a perfect score on a database nobody has +ever used is exactly the kind of green number this package exists to argue against. + +`vacuum:lint` asks the questions that *are* answerable there: + +```bash +php artisan vacuum:lint +``` + +| Rule | Finds | +| --- | --- | +| `unindexed-foreign-key` | Foreign keys PostgreSQL created no index for, which `->constrained()` never does | +| `foreign-key-type-mismatch` | A key referencing a different type, so the index exists and cannot be used | +| `int4-primary-key` | A primary key that stops accepting rows at 2,147,483,647 | +| `missing-primary-key` | Tables nothing can address a single row of | +| `unindexed-morphs` | A polymorphic pair with no composite index leading on the type | +| `json-not-jsonb` | `json` where `jsonb` was almost certainly meant | + +Every one of them is true the moment `php artisan migrate` finishes, so this belongs +in `require-dev` and in the job that already runs your tests. + +It **defaults to failing on a warning**, where `vacuum:check` defaults to critical. +The two commands mean different things by the word: a warning from `check` is a +database drifting, and a build should not go red because bloat grew overnight. A +warning from `lint` is a schema that was wrong the moment somebody typed it. + +```bash +php artisan vacuum:lint --fail-on=critical # critical, warning, info, or never +php artisan vacuum:lint --format=json # score, grade, deductions, findings +``` + +Schema findings are scored on their own and are **not** part of the dashboard's +health score. Adding rules to that score would silently re-grade every existing +installation on a `composer update`, and a grade that moves for a reason nobody +asked for is worse than one rule fewer. + ## History over time Vacuum is point-in-time by default: every page and every `vacuum:check` reads the database as it is this instant. Switch history on and it records a snapshot on a schedule, so it can tell you which way a number is *moving* — bloat that is growing, a freeze age climbing since the last time anything froze it, a cache-hit ratio measured over the last hour rather than over the life of the server. @@ -385,14 +427,15 @@ The stylesheet is inlined rather than fetched from a CDN, on the grounds that th ## What semver covers -From 1.0, these are public API and a breaking change to any of them requires a major version: +From 1.0 — and from 1.1 where a line says so — these are public API and a breaking change to any of them requires a major version: -- **The rule contracts** — `TableRule`, `IndexRule`, `SessionRule`, `StatementRule`, `BloatRule`, `CacheRule`, `DuplicateRule`, `ConfigurationRule`, `SettingRule` — and the `Inspection` contract behind them. +- **The rule contracts** — `TableRule`, `IndexRule`, `SessionRule`, `StatementRule`, `BloatRule`, `CacheRule`, `DuplicateRule`, `ConfigurationRule`, `SettingRule` (1.0) and `SchemaRule` (1.1) — and the `Inspection` contract behind them. - **`Finding`, `Severity` and `Grade`**, including `Finding`'s constructor signature. A custom rule constructs one, so its parameters are as public as the interface that returns it. -- **The value objects the contracts hand a rule**: `TableStatistic`, `IndexStatistic`, `Session`, `Statement`, `CacheStatistic`, `Settings` and `Capabilities`. +- **The value objects the contracts hand a rule**: `TableStatistic`, `IndexStatistic`, `Session`, `Statement`, `CacheStatistic`, `Settings` and `Capabilities` (1.0), and `TableSchema` and `IndexDefinition` (1.1). - **Configuration keys** under `vacuum.*`, and the `VACUUM_*` environment variables that feed them. Keys may be added; existing ones will not change meaning. -- **The `vacuum:check --format=json` document**, which is what a pipeline parses. +- **The `vacuum:check --format=json` and `vacuum:lint --format=json` documents**, which are what a pipeline parses. - **Route names** (`vacuum.dashboard` and the rest) and the `Vacuum::auth()` gate. +- **The `SCHEMA_RULES` tag** (1.1), alongside the others a custom rule is registered under. Explicitly **not** covered, and free to change in a minor release: diff --git a/resources/sql/constraints.sql b/resources/sql/constraints.sql index 4b3d17d..07130e6 100644 --- a/resources/sql/constraints.sql +++ b/resources/sql/constraints.sql @@ -17,24 +17,66 @@ -- Array equality in PostgreSQL compares contents and element counts and not -- subscript bounds, so slicing indkey from 0 to n-1 and comparing it to conkey -- is correct, and the off-by-one it looks like it has, it does not. +-- +-- indpred IS NULL is part of "covered" too: a partial index holds only the rows +-- its predicate admits, and the referential-integrity check this answers for -- +-- does some row in the parent exist -- has to be able to see an arbitrary row, +-- not just the ones a WHERE clause let in. Values\TableSchema::hasIndexLeadingWith() +-- excludes a partial index for the same reason; this column has to agree with it, +-- or the same table can be told both that it is covered and that it is not. +-- +-- The types on both sides of a foreign key are carried because a mismatch between +-- them is invisible everywhere else. PostgreSQL accepts a foreign key from an +-- integer to a bigint without complaint, creates the constraint, enforces it +-- correctly -- and the planner then cannot use the parent's index for the check, +-- because the comparison is across types. Nothing in the catalog is marked wrong. +-- The only symptom is that a delete on the parent is slow forever. +-- +-- confkey is null for a primary key or a unique constraint, so it is coalesced to +-- an empty array: a constraint that references nothing has no referenced types, +-- which is different from having failed to look them up. +-- +-- Every list below is joined with a newline rather than a comma. format_type +-- renders a parameterised type with a comma already inside it -- numeric(10,2) -- +-- so a comma-joined list of column types splits that single type into two +-- pieces, which is a real shape in this package's own migrations. Neither +-- format_type's output nor a PostgreSQL identifier can contain a newline, so it +-- is a safe delimiter where a comma is not. All three lists change together +-- because Constraints::toConstraint() zips them by position, and the delimiter +-- never leaves that class. SELECT namespaces.nspname AS schemaname, tables.relname AS tablename, constraints.conname AS constraintname, constraints.contype::text AS kind, ( - SELECT coalesce(string_agg(attributes.attname, ',' ORDER BY keys.ordinality), '') + SELECT coalesce(string_agg(attributes.attname, E'\n' ORDER BY keys.ordinality), '') FROM unnest(constraints.conkey) WITH ORDINALITY AS keys (attnum, ordinality) JOIN pg_attribute AS attributes ON attributes.attrelid = constraints.conrelid AND attributes.attnum = keys.attnum ) AS columns, + ( + SELECT coalesce(string_agg(format_type(attributes.atttypid, attributes.atttypmod), E'\n' ORDER BY keys.ordinality), '') + FROM unnest(constraints.conkey) WITH ORDINALITY AS keys (attnum, ordinality) + JOIN pg_attribute AS attributes + ON attributes.attrelid = constraints.conrelid + AND attributes.attnum = keys.attnum + ) AS columntypes, + ( + SELECT coalesce(string_agg(format_type(attributes.atttypid, attributes.atttypmod), E'\n' ORDER BY keys.ordinality), '') + FROM unnest(coalesce(constraints.confkey, '{}'::int2[])) WITH ORDINALITY AS keys (attnum, ordinality) + JOIN pg_attribute AS attributes + ON attributes.attrelid = constraints.confrelid + AND attributes.attnum = keys.attnum + ) AS referencedcolumntypes, coalesce(referenced.relname, '') AS referencedtable, EXISTS ( SELECT 1 FROM pg_index AS indexes WHERE indexes.indrelid = constraints.conrelid AND indexes.indisvalid + AND indexes.indpred IS NULL AND (indexes.indkey::int2[])[0:cardinality(constraints.conkey) - 1] = constraints.conkey ) AS indexed FROM pg_constraint AS constraints diff --git a/resources/sql/index_columns.sql b/resources/sql/index_columns.sql new file mode 100644 index 0000000..8a9169e --- /dev/null +++ b/resources/sql/index_columns.sql @@ -0,0 +1,40 @@ +-- Every index's key columns, in the order the index stores them. +-- +-- This is the shape question rather than the usage question. pg_stat_user_indexes +-- says whether anything has read an index, which a database created ninety seconds +-- ago in a pipeline cannot say anything about at all; pg_index says what the index +-- would be able to serve, which is true the moment CREATE INDEX returns. +-- +-- indkey is an int2vector and is 0-based, so the key columns are the slice from 0 +-- to indnkeyatts - 1. The columns past that slice are an INCLUDE payload: stored in +-- the leaf, not searchable, and therefore not part of what the index can serve. +-- +-- Indexes over expressions are excluded. Their indkey carries a 0 where the +-- expression is, which joins to no pg_attribute row, and a column list with a +-- silent gap in it is worse than no column list: every rule reading this asks +-- whether an index leads with named plain columns, and an expression index is +-- never the answer to that question. +SELECT + namespaces.nspname AS schemaname, + tables.relname AS tablename, + indexes.relname AS indexname, + ( + SELECT coalesce(string_agg(attributes.attname, ',' ORDER BY keys.ordinality), '') + FROM unnest((idx.indkey::int2[])[0:idx.indnkeyatts - 1]) WITH ORDINALITY AS keys (attnum, ordinality) + JOIN pg_attribute AS attributes + ON attributes.attrelid = idx.indrelid + AND attributes.attnum = keys.attnum + ) AS columns, + idx.indisunique AS isunique, + idx.indisvalid AS isvalid, + (idx.indpred IS NOT NULL) AS ispartial, + methods.amname AS method +FROM pg_index AS idx +JOIN pg_class AS indexes ON indexes.oid = idx.indexrelid +JOIN pg_class AS tables ON tables.oid = idx.indrelid +JOIN pg_namespace AS namespaces ON namespaces.oid = tables.relnamespace +JOIN pg_am AS methods ON methods.oid = indexes.relam +WHERE idx.indexprs IS NULL + AND tables.relkind IN ('r', 'p') + AND namespaces.nspname <> ALL (string_to_array(?, ',')) +ORDER BY namespaces.nspname, tables.relname, indexes.relname diff --git a/src/Advisor/Inspections/SchemaInspection.php b/src/Advisor/Inspections/SchemaInspection.php new file mode 100644 index 0000000..fd820f9 --- /dev/null +++ b/src/Advisor/Inspections/SchemaInspection.php @@ -0,0 +1,46 @@ + $rules + */ + public function __construct( + private Schemas $schemas, + private iterable $rules, + ) {} + + /** + * @return list + */ + public function findings(): array + { + $findings = []; + + foreach ($this->schemas->all() as $schema) { + foreach ($this->rules as $rule) { + foreach ($rule->inspect($schema) as $finding) { + $findings[] = $finding; + } + } + } + + return $findings; + } +} diff --git a/src/Advisor/Rules/ForeignKeyTypeMismatch.php b/src/Advisor/Rules/ForeignKeyTypeMismatch.php new file mode 100644 index 0000000..9dcdd03 --- /dev/null +++ b/src/Advisor/Rules/ForeignKeyTypeMismatch.php @@ -0,0 +1,203 @@ +integer('customer_id') with a parent + * whose key is bigIncrements, which is what the framework's own default has been + * since 5.8. + * + * This rule offers no remediation in two cases where the obvious one-line ALTER + * would be wrong rather than merely unhelpful. The first is a composite key: the + * finding used to rewrite column zero unconditionally, even when the mismatch + * was actually in a later column, which takes the table's ACCESS EXCLUSIVE lock + * and index rebuild for a column that already had the right type. The second is + * a parent that is the *narrower* side of the mismatch -- a legacy + * increments('id') parent referenced by a newer foreignId() child, most often. + * Narrowing the child there would entrench the very 32-bit ceiling + * int4-primary-key is warning about on the parent, and it would fail outright + * the first time a value exceeds what the narrower type can hold. In both cases + * the finding still fires, because the mismatch is real and still costs a scan + * on every delete; only the ALTER is withheld, because this rule cannot know the + * shape of the migration that would actually fix it. + */ +final readonly class ForeignKeyTypeMismatch implements SchemaRule +{ + /** + * Rank, narrowest first, of the integer types this rule knows how to compare + * for width. Anything outside this family -- or a mismatch where one side + * is not in it, such as a foreign key crossing into text or uuid -- has no + * narrow/wide relationship this rule can reason about, so it is left to the + * existing child-widening advice. + */ + private const array INTEGER_FAMILY_RANK = [ + 'smallint' => 1, + 'integer' => 2, + 'bigint' => 3, + ]; + + /** + * @return list + */ + public function inspect(TableSchema $table): array + { + $findings = []; + + foreach ($table->foreignKeys() as $key) { + if ($key->columns === []) { + continue; + } + + // An empty list on either side is the query declining to answer, not + // evidence of a mismatch. A finding built from missing evidence is how + // a tool teaches people to stop reading it. + if ($key->columnTypes === []) { + continue; + } + + if ($key->referencedColumnTypes === []) { + continue; + } + + if ($key->typesMatch()) { + continue; + } + + $leading = $key->columns[0]; + $here = implode(', ', $key->columnTypes); + $there = implode(', ', $key->referencedColumnTypes); + + if (count($key->columns) > 1) { + $remediation = null; + $impact = $this->compositeImpact($key); + } elseif ($this->parentIsNarrower($key->columnTypes[0], $key->referencedColumnTypes[0])) { + $remediation = null; + $impact = $this->narrowParentImpact($key); + } else { + $wanted = $key->referencedColumnTypes[0]; + $remediation = 'ALTER TABLE '.Identifier::qualified($table->schema, $table->table) + .' ALTER COLUMN '.Identifier::quote($leading)." TYPE {$wanted};"; + $impact = $this->widenChildImpact($key); + } + + $findings[] = new Finding( + rule: 'foreign-key-type-mismatch', + subject: $table->qualifiedName().'.'.$leading, + severity: Severity::Warning, + summary: "The foreign key {$key->name} is {$here} and references {$there}. PostgreSQL accepts " + .'that and enforces it correctly; what it cannot do is use an index for the check.', + impact: $impact, + remediation: $remediation, + evidence: "{$here} references {$there}", + table: $table->qualifiedName(), + ); + } + + return $findings; + } + + /** + * Whether the referenced (parent) column is the narrower side of an + * integer-family mismatch -- smallint < integer < bigint. + */ + private function parentIsNarrower(string $childType, string $parentType): bool + { + if (! isset(self::INTEGER_FAMILY_RANK[$childType])) { + return false; + } + + if (! isset(self::INTEGER_FAMILY_RANK[$parentType])) { + return false; + } + + return self::INTEGER_FAMILY_RANK[$parentType] < self::INTEGER_FAMILY_RANK[$childType]; + } + + private function widenChildImpact(Constraint $key): string + { + return 'Because the comparison crosses types, the planner cannot use the index on ' + ."{$key->referencedTable} to prove a row is unreferenced, so every delete and key update " + .'on the parent falls back to a scan. Nothing is marked wrong anywhere: not the ' + .'constraint, not the index, not the plan you would think to look at. Correcting the ' + .'type will rewrite the table and take a lock for the duration, so it is a migration to ' + .'plan rather than one to run at five on a Friday.'; + } + + private function narrowParentImpact(Constraint $key): string + { + $parentType = $key->referencedColumnTypes[0]; + $childType = $key->columnTypes[0]; + + return 'Because the comparison crosses types, the planner cannot use the index on ' + ."{$key->referencedTable} to prove a row is unreferenced, so every delete and key update " + .'on the parent falls back to a scan. Here the parent is the narrow side of the mismatch: ' + ."{$key->referencedTable}'s column is {$parentType} and this one is {$childType}. The fix " + .'has to start on the parent -- widening it is what actually raises the ceiling that ' + .'int4-primary-key already warns about there -- and it has to happen before anything about ' + .'this column changes: narrowing this column to match the parent would only entrench that ' + .'same ceiling, and widening the child on its own does not touch the parent at all, so it ' + .'would not help either. A constraint carries the referenced column\'s type but not its ' + .'name, so this rule cannot write that ALTER for you.'; + } + + /** + * Names the columns of a composite key that disagree, rather than rewriting + * whichever one happens to be first. A composite mismatch can have some + * columns already correct and only one -- not necessarily the leading one -- + * wrong, and an ALTER aimed unconditionally at column zero would take the + * table's lock and index rebuild to rewrite a column that already matched. + */ + private function compositeImpact(Constraint $key): string + { + $pairs = []; + + foreach ($key->columns as $index => $column) { + $childType = $key->columnTypes[$index] ?? null; + $parentType = $key->referencedColumnTypes[$index] ?? null; + + if ($childType === null) { + continue; + } + + if ($parentType === null) { + continue; + } + + if ($childType === $parentType) { + continue; + } + + $pairs[] = "{$column} ({$childType} vs {$parentType})"; + } + + $disagreeing = implode(', ', $pairs); + + return 'Because the comparison crosses types, the planner cannot use the index on ' + ."{$key->referencedTable} to prove a row is unreferenced, so every delete and key update " + .'on the parent falls back to a scan. This foreign key is composite, and only ' + ."{$disagreeing} disagree while the rest already match. Rewriting the leading column the " + .'way a single-column mismatch would risks rewriting a column that is already correct and ' + .'missing the one that is not, so this finding names the columns rather than prescribing ' + .'an ALTER; work out which of them actually needs to change and write that migration by hand.'; + } +} diff --git a/src/Advisor/Rules/Int4PrimaryKey.php b/src/Advisor/Rules/Int4PrimaryKey.php new file mode 100644 index 0000000..71f4893 --- /dev/null +++ b/src/Advisor/Rules/Int4PrimaryKey.php @@ -0,0 +1,110 @@ +increments('id') produced exactly this, and it was the + * framework's default until 5.8 -- so the schemas carrying it are, by + * construction, the oldest and busiest ones, which are also the ones nearest the + * ceiling and the hardest to migrate. Widening the column rewrites the whole + * table under an ACCESS EXCLUSIVE lock, and every foreign key pointing at it has + * to be widened in the same breath or it becomes a type mismatch instead. That is + * a planned migration with a maintenance window, and the reason to say so now is + * that it only gets more expensive. + * + * One table is deliberately exempt: public.migrations. It is written by + * Laravel's own DatabaseMigrationRepository, which still creates it with + * $table->increments('id') in every currently supported release, so this rule + * would otherwise fire on a stock `laravel new` application with no code the + * application owns to change. A table that gains exactly one row per migration + * run is never going to approach a ceiling that a busy application table could + * reach in months, so the finding would be pure noise -- and vacuum:lint's + * default --fail-on=warning would fail that build on a table nobody can fix. + */ +final readonly class Int4PrimaryKey implements SchemaRule +{ + /** What format_type renders for the integer types narrower than bigint. */ + private const array NARROW = ['integer', 'smallint']; + + /** + * Laravel's own migration-tracking table, written by + * DatabaseMigrationRepository::createRepository() with $table->increments('id') + * and never touched by the application. See the class docblock for why this + * rule stays silent about it. + */ + private const string FRAMEWORK_MIGRATIONS = 'migrations'; + + /** + * @return list + */ + public function inspect(TableSchema $table): array + { + if ($table->table === self::FRAMEWORK_MIGRATIONS) { + return []; + } + + $key = $table->primaryKey(); + + if (! $key instanceof Constraint) { + return []; + } + + $findings = []; + + foreach ($key->columns as $name) { + $column = $table->column($name); + if (! $column instanceof Column) { + continue; + } + if (! in_array($column->type, self::NARROW, true)) { + continue; + } + + $ceiling = $column->type === 'smallint' ? '32,767' : '2,147,483,647'; + + $findings[] = new Finding( + rule: 'int4-primary-key', + subject: $table->qualifiedName().'.'.$name, + severity: Severity::Warning, + summary: "The primary key column {$name} is {$column->type}, so it can count to {$ceiling} " + .'and no further. The insert after that one fails.', + impact: 'There is no warning and no gradual slowdown: the table is perfectly healthy right up ' + .'to the last value and refuses the next row. Widening it later rewrites the whole table ' + .'under a lock that blocks every reader and writer, and widening every foreign key that ' + .'points at it in the same migration -- leave one behind and it becomes a type mismatch, ' + .'which is slow forever and invisible. The cost of this migration only grows.', + remediation: 'ALTER TABLE '.Identifier::qualified($table->schema, $table->table) + .' ALTER COLUMN '.Identifier::quote($name).' TYPE bigint;', + evidence: "{$name} {$column->type}", + + // How much of the counter has actually been spent. The schema says + // the ceiling exists; only the sequence says how close it is. + query: "SELECT last_value, max_value, round(100.0 * last_value / max_value, 2) AS percent_used\n" + .'FROM pg_sequences'."\n" + .'WHERE schemaname = '.Identifier::literal($table->schema) + .' AND sequencename LIKE '.Identifier::literal($table->table.'%').';', + table: $table->qualifiedName(), + ); + } + + return $findings; + } +} diff --git a/src/Advisor/Rules/JsonNotJsonb.php b/src/Advisor/Rules/JsonNotJsonb.php new file mode 100644 index 0000000..4ba2a06 --- /dev/null +++ b/src/Advisor/Rules/JsonNotJsonb.php @@ -0,0 +1,64 @@ +json() maps to json on PostgreSQL, so this is usually not a + * decision anybody made -- which is exactly why it is worth saying. + * + * Info, not a warning. Keeping the document byte-for-byte is a real requirement + * for a signed payload or an audit record, and a build that went red over it + * would be a build people learn to ignore. + */ +final readonly class JsonNotJsonb implements SchemaRule +{ + /** + * @return list + */ + public function inspect(TableSchema $table): array + { + $findings = []; + + foreach ($table->columns as $column) { + if ($column->type !== 'json') { + continue; + } + + $findings[] = new Finding( + rule: 'json-not-jsonb', + subject: $table->qualifiedName().'.'.$column->name, + severity: Severity::Info, + summary: "{$column->name} is json rather than jsonb. On PostgreSQL those are different types " + .'with different costs, and Laravel\'s $table->json() picks this one.', + impact: 'A json column stores the document as text and has to reparse all of it on every ' + .'access, however small the part being read. It also cannot carry a GIN index, so a ' + .'containment or key-existence query over it reads every row. jsonb parses once on write ' + .'and indexes. The reason to keep json is if the exact bytes matter -- a signed payload, ' + .'an audit record -- because jsonb normalises whitespace, key order and duplicate keys.', + remediation: 'ALTER TABLE '.Identifier::qualified($table->schema, $table->table) + .' ALTER COLUMN '.Identifier::quote($column->name).' TYPE jsonb USING ' + .Identifier::quote($column->name).'::jsonb;', + evidence: "{$column->name} json", + table: $table->qualifiedName(), + ); + } + + return $findings; + } +} diff --git a/src/Advisor/Rules/MissingPrimaryKey.php b/src/Advisor/Rules/MissingPrimaryKey.php new file mode 100644 index 0000000..91032bf --- /dev/null +++ b/src/Advisor/Rules/MissingPrimaryKey.php @@ -0,0 +1,105 @@ +foreignId('role_id')->foreignId('user_id') and a + * unique index over the pair, which is a perfectly good key that nobody declared + * as one. The consequences are not aesthetic. Logical replication cannot + * replicate an update or a delete against a table with no replica identity, and + * the default replica identity is the primary key -- so such a table silently + * breaks a replication setup that works for everything else. Eloquent cannot + * update or delete a model it cannot address by key. And nothing stops a bug, a + * retried job or a double-submitted form from writing the same row twice, after + * which telling the copies apart is guesswork. + * + * Where a unique constraint already exists, the remedy is to promote it. Where + * one does not, this offers no statement at all: which columns identify a row is + * a question about the domain, and a guess dressed up as a migration is worse + * advice than none. + */ +final readonly class MissingPrimaryKey implements SchemaRule +{ + /** + * @return list + */ + public function inspect(TableSchema $table): array + { + if ($table->primaryKey() instanceof Constraint) { + return []; + } + + $promotable = $this->promotable($table); + + return [new Finding( + rule: 'missing-primary-key', + subject: $table->qualifiedName(), + severity: Severity::Warning, + summary: $promotable instanceof Constraint + ? "This table has no primary key. It does have the unique constraint {$promotable->name}, " + .'which is already a key in everything but name.' + : 'This table has no primary key and no unique constraint that could become one.', + impact: 'Logical replication cannot replicate an update or a delete against a table with no ' + .'replica identity, and the default replica identity is the primary key -- so this table ' + .'quietly breaks a replication setup that works for every other table. Eloquent cannot ' + .'update or delete a model it has no key to address. And nothing prevents a retried job or ' + .'a resubmitted form from writing the same row twice, after which telling the copies apart ' + .'is guesswork.', + remediation: $promotable instanceof Constraint + ? 'ALTER TABLE '.Identifier::qualified($table->schema, $table->table).' ADD PRIMARY KEY (' + .implode(', ', array_map(Identifier::quote(...), $promotable->columns)).');' + : null, + evidence: $promotable instanceof Constraint + ? 'UNIQUE ('.implode(', ', $promotable->columns).')' + : null, + table: $table->qualifiedName(), + )]; + } + + /** + * A unique constraint over columns that are all NOT NULL, which is what a + * primary key requires. A unique constraint on a nullable column is not a + * key: PostgreSQL treats nulls as distinct, so it permits many rows that a + * primary key would refuse, and promoting it would fail. + */ + private function promotable(TableSchema $table): ?Constraint + { + foreach ($table->constraints as $constraint) { + if ($constraint->kind !== 'u') { + continue; + } + + if ($constraint->columns === []) { + continue; + } + + foreach ($constraint->columns as $name) { + $column = $table->column($name); + + if (! $column instanceof Column) { + continue 2; + } + + if ($column->nullable) { + continue 2; + } + } + + return $constraint; + } + + return null; + } +} diff --git a/src/Advisor/Rules/UnindexedForeignKey.php b/src/Advisor/Rules/UnindexedForeignKey.php new file mode 100644 index 0000000..4deefd7 --- /dev/null +++ b/src/Advisor/Rules/UnindexedForeignKey.php @@ -0,0 +1,73 @@ +foreignId('customer_id') + * ->constrained() writes a constraint and no index, and the framework gives no + * hint that half of what you asked for was not done. + * + * The cost lands somewhere nobody is watching. It is not the child's reads that + * suffer -- it is the parent's writes: every DELETE and every key UPDATE on the + * parent has to prove no child still references the row, and with no index that + * proof is a sequential scan of the child table, taken while holding a lock. A + * table that is fine for two years becomes a table that times out. + */ +final readonly class UnindexedForeignKey implements SchemaRule +{ + /** + * @return list + */ + public function inspect(TableSchema $table): array + { + $findings = []; + + foreach ($table->foreignKeys() as $key) { + if ($key->indexed) { + continue; + } + if ($key->columns === []) { + continue; + } + $leading = $key->columns[0]; + $columns = implode(', ', $key->columns); + $quoted = implode(', ', array_map(Identifier::quote(...), $key->columns)); + + $findings[] = new Finding( + rule: 'unindexed-foreign-key', + subject: $table->qualifiedName().'.'.$leading, + severity: Severity::Warning, + summary: "The foreign key {$key->name} on ({$columns}) has no index behind it. PostgreSQL " + .'creates one for a primary key and for a unique constraint, and none for this.', + impact: "Every delete and every key update on {$key->referencedTable} has to prove that no row " + ."in {$table->table} still points at it. With no index that proof is a sequential scan of " + ."{$table->table}, taken while holding a lock, and it gets slower every time the table " + .'grows. Nothing about it shows up where you would look for it, because the cost is paid ' + .'by the parent and the missing index is on the child.', + remediation: 'CREATE INDEX CONCURRENTLY ON ' + .Identifier::qualified($table->schema, $table->table)." ({$quoted});", + evidence: "FOREIGN KEY ({$columns}) REFERENCES {$key->referencedTable}", + query: "SELECT indexname, indexdef\n" + ."FROM pg_indexes\n" + .'WHERE schemaname = '.Identifier::literal($table->schema) + .' AND tablename = '.Identifier::literal($table->table)."\n" + .'ORDER BY indexname;', + table: $table->qualifiedName(), + ); + } + + return $findings; + } +} diff --git a/src/Advisor/Rules/UnindexedMorphs.php b/src/Advisor/Rules/UnindexedMorphs.php new file mode 100644 index 0000000..2551a7d --- /dev/null +++ b/src/Advisor/Rules/UnindexedMorphs.php @@ -0,0 +1,88 @@ +morphs('commentable') creates the pair and the index; the columns + * written by hand, or a nullableMorphs() that a later migration replaced, often + * arrive without it. A morph is always queried by both columns at once -- the + * relation cannot resolve a row from the id alone, because the id is only unique + * within a type -- so a lookup with no index on the pair reads the whole table + * every time a morphed relation is loaded. + * + * The order is not incidental. The index has to lead with the type, because that + * is the column with the low cardinality and it is the one the relation fixes + * first. An index on (id, type) contains both columns and answers a question + * nobody is asking. + * + * The pair is recognised in the schema, by convention, exactly as the Learn + * section does it: a string *_type beside an integer *_id. The catalog can prove + * the two columns exist and cannot prove that morphs() is what wrote them. + */ +final readonly class UnindexedMorphs implements SchemaRule +{ + /** + * @return list + */ + public function inspect(TableSchema $table): array + { + $findings = []; + + foreach ($table->columns as $column) { + if (! str_ends_with($column->name, '_type')) { + continue; + } + + // A morph type column holds a class name, so it is a string. text and + // character varying(n) are both what Laravel might have written; an + // integer *_type column is somebody else's convention entirely. + if ($column->type !== 'text' && ! str_starts_with($column->type, 'character varying')) { + continue; + } + + $name = substr($column->name, 0, -strlen('_type')); + $id = $table->column($name.'_id'); + + if (! $id instanceof Column) { + continue; + } + + if (! in_array($id->type, ['bigint', 'integer'], true)) { + continue; + } + + if ($table->hasIndexLeadingWith($column->name, $id->name)) { + continue; + } + + $findings[] = new Finding( + rule: 'unindexed-morphs', + subject: $table->qualifiedName().'.'.$column->name, + severity: Severity::Warning, + summary: "The polymorphic pair ({$column->name}, {$id->name}) has no index leading with both " + .'columns in that order.', + impact: 'A morphed relation is always resolved by type and id together -- the id alone is only ' + ."unique within a type -- so every load of this relation reads all of {$table->table}. An " + .'index that leads with the id instead does not help: the relation fixes the type first, ' + .'and that is the column the index has to start with.', + remediation: 'CREATE INDEX CONCURRENTLY ON '.Identifier::qualified($table->schema, $table->table) + .' ('.Identifier::quote($column->name).', '.Identifier::quote($id->name).');', + evidence: "{$column->name} {$column->type}, {$id->name} {$id->type}", + table: $table->qualifiedName(), + ); + } + + return $findings; + } +} diff --git a/src/Advisor/SchemaAdvisor.php b/src/Advisor/SchemaAdvisor.php new file mode 100644 index 0000000..e6a9379 --- /dev/null +++ b/src/Advisor/SchemaAdvisor.php @@ -0,0 +1,37 @@ + + */ + public function findings(): array + { + return $this->advisor->findings(); + } +} diff --git a/src/Advisor/SchemaRule.php b/src/Advisor/SchemaRule.php new file mode 100644 index 0000000..2d14cb6 --- /dev/null +++ b/src/Advisor/SchemaRule.php @@ -0,0 +1,28 @@ + + */ + public function inspect(TableSchema $table): array; +} diff --git a/src/Console/Commands/CheckCommand.php b/src/Console/Commands/CheckCommand.php index 38a63b8..6bbc877 100644 --- a/src/Console/Commands/CheckCommand.php +++ b/src/Console/Commands/CheckCommand.php @@ -7,7 +7,8 @@ use Heyosseus\Vacuum\Advisor\Advisor; use Heyosseus\Vacuum\Advisor\Finding; use Heyosseus\Vacuum\Advisor\Health; -use Heyosseus\Vacuum\Advisor\Severity; +use Heyosseus\Vacuum\Console\Support\FindingReporter; +use Heyosseus\Vacuum\Console\Support\SeverityBar; use Illuminate\Console\Command; use Illuminate\Contracts\Config\Repository; @@ -31,8 +32,11 @@ final class CheckCommand extends Command protected $description = 'Inspect the database and fail if the advisor finds something serious'; - public function handle(Advisor $advisor, Repository $config): int - { + public function handle( + Advisor $advisor, + Repository $config, + FindingReporter $reporter, + ): int { // The master switch means Vacuum runs no queries, so it would find nothing, // and a gate that goes green because it never looked is worse than no gate. // This is the one case where an empty result is not a pass. @@ -44,11 +48,12 @@ public function handle(Advisor $advisor, Repository $config): int return self::INVALID; } - $bar = $this->option('fail-on'); + $option = $this->option('fail-on'); + $bar = SeverityBar::parse(is_string($option) ? $option : null); - if (! is_string($bar) || ! $this->recognised($bar)) { + if (! $bar instanceof SeverityBar) { $this->components->error( - "There is no severity called '".(is_string($bar) ? $bar : '')."'. " + "There is no severity called '".(is_string($option) ? $option : '')."'. " .'Use critical, warning, info or never.', ); @@ -57,62 +62,20 @@ public function handle(Advisor $advisor, Repository $config): int $findings = $advisor->findings(); $health = Health::from($findings); - $failed = $this->worthFailing($findings, $bar); + $failed = $bar->fails($findings); if ($this->option('format') === 'json') { $this->output->writeln($this->json($health, $findings, $failed)); } else { - $this->report($health, $findings); - } - - return $failed ? self::FAILURE : self::SUCCESS; - } - - /** - * @param list $findings - */ - private function report(Health $health, array $findings): void - { - $this->newLine(); - $this->line(" {$health->score} / 100 Grade {$health->grade->value}"); - - if ($findings === []) { - $this->newLine(); - $this->line(' Nothing to report. Every table, index and session is inside its thresholds.'); - $this->newLine(); - - return; - } - - $this->newLine(); - - foreach ($findings as $finding) { - $severity = str_pad($finding->severity->value, 8); - - $this->line( - " colour($finding->severity)};options=bold>{$severity}" - ." {$finding->subject} {$finding->rule}", + $reporter->report( + $this->output, + $health, + $findings, + 'Every table, index and session is inside its thresholds.', ); - - $this->line(" {$finding->summary}"); - - if ($finding->remediation !== null) { - // Printed, never run. Vacuum has no code path that writes to the - // database it inspects, and a command that offered to fix things for - // you would be the first. - foreach (explode("\n", $finding->remediation) as $line) { - $this->line(" {$line}"); - } - } - - $this->newLine(); } - foreach ($health->deductions as $rule => $cost) { - $this->line(' '.str_pad($rule, 24, '.').' -'.$cost.''); - } - - $this->newLine(); + return $failed ? self::FAILURE : self::SUCCESS; } /** @@ -136,48 +99,4 @@ private function json(Health $health, array $findings, bool $failed): string ], $findings), ], JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR); } - - /** - * Whether anything found is at or above the bar the caller set. - * - * The default bar is critical, so an info finding -- a fact rather than a fault, - * the server saying what it cannot see -- fails nothing unless somebody puts the - * bar there on purpose. - * - * @param list $findings - */ - private function worthFailing(array $findings, string $bar): bool - { - if ($bar === 'never') { - return false; - } - - $limit = Severity::from($bar); - - foreach ($findings as $finding) { - // An unknown is excluded from the gate by rank today, but only by - // accident of its number. Said out loud, it survives somebody - // reordering the ranks: a build should go red because the database - // has a problem, never because the role was short a grant. - if ($finding->severity !== Severity::Unknown && $finding->severity->rank() <= $limit->rank()) { - return true; - } - } - - return false; - } - - private function recognised(string $bar): bool - { - return $bar === 'never' || Severity::tryFrom($bar) instanceof Severity; - } - - private function colour(Severity $severity): string - { - return match ($severity) { - Severity::Critical => 'red', - Severity::Warning => 'yellow', - Severity::Info, Severity::Unknown => 'blue', - }; - } } diff --git a/src/Console/Commands/LintCommand.php b/src/Console/Commands/LintCommand.php new file mode 100644 index 0000000..18ba853 --- /dev/null +++ b/src/Console/Commands/LintCommand.php @@ -0,0 +1,143 @@ +get('vacuum.enabled') !== true) { + $this->components->error( + 'Vacuum is disabled, so nothing was inspected. Set VACUUM_ENABLED=true to lint this schema.', + ); + + return self::INVALID; + } + + if ($this->baselineRequested()) { + $this->components->error( + 'Baselines are not in this release yet. Run without --baseline, --generate-baseline or ' + .'--no-baseline, or pin an earlier expectation of them.', + ); + + return self::INVALID; + } + + $option = $this->option('fail-on'); + $bar = SeverityBar::parse(is_string($option) ? $option : null); + + if (! $bar instanceof SeverityBar) { + $this->components->error( + "There is no severity called '".(is_string($option) ? $option : '')."'. " + .'Use critical, warning, info or never.', + ); + + return self::INVALID; + } + + $findings = $advisor->findings(); + $health = Health::from($findings); + $failed = $bar->fails($findings); + + if ($this->option('format') === 'json') { + $this->output->writeln($this->json($health, $findings, $failed)); + } else { + $reporter->report( + $this->output, + $health, + $findings, + 'Every table has a key, every foreign key has an index, and every type lines up.', + ); + } + + return $failed ? self::FAILURE : self::SUCCESS; + } + + /** + * Whether the caller asked for a baseline. The options are declared now so + * that the command's signature does not change when the feature lands; + * accepting them silently in the meantime would be worse than refusing them. + */ + private function baselineRequested(): bool + { + if ($this->option('generate-baseline') === true) { + return true; + } + + if ($this->option('no-baseline') === true) { + return true; + } + + return is_string($this->option('baseline')) && $this->option('baseline') !== ''; + } + + /** + * The document a pipeline parses. + * + * Separate from vacuum:check's, and carries `table` where that one does not: + * a schema finding is always about a table, and the thing reading this is + * going to want to say which one. + * + * @param list $findings + */ + private function json(Health $health, array $findings, bool $failed): string + { + return json_encode([ + 'score' => $health->score, + 'grade' => $health->grade->value, + 'failed' => $failed, + 'deductions' => $health->deductions, + 'findings' => array_map(static fn (Finding $finding): array => [ + 'rule' => $finding->rule, + 'subject' => $finding->subject, + 'severity' => $finding->severity->value, + 'summary' => $finding->summary, + 'impact' => $finding->impact, + 'remediation' => $finding->remediation, + 'evidence' => $finding->evidence, + 'table' => $finding->table, + ], $findings), + ], JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR); + } +} diff --git a/src/Console/Support/FindingReporter.php b/src/Console/Support/FindingReporter.php new file mode 100644 index 0000000..a91fca0 --- /dev/null +++ b/src/Console/Support/FindingReporter.php @@ -0,0 +1,74 @@ + $findings + */ + public function report(OutputStyle $output, Health $health, array $findings, string $clean): void + { + $output->newLine(); + $output->writeln(" {$health->score} / 100 Grade {$health->grade->value}"); + + if ($findings === []) { + $output->newLine(); + $output->writeln(" Nothing to report. {$clean}"); + $output->newLine(); + + return; + } + + $output->newLine(); + + foreach ($findings as $finding) { + $severity = str_pad($finding->severity->value, 8); + + $output->writeln( + " colour($finding->severity)};options=bold>{$severity}" + ." {$finding->subject} {$finding->rule}", + ); + + $output->writeln(" {$finding->summary}"); + + if ($finding->remediation !== null) { + // Printed, never run. Vacuum has no code path that writes to the + // database it inspects, and a command that offered to fix things for + // you would be the first. + foreach (explode("\n", $finding->remediation) as $line) { + $output->writeln(" {$line}"); + } + } + + $output->newLine(); + } + + foreach ($health->deductions as $rule => $cost) { + $output->writeln(' '.str_pad($rule, 24, '.').' -'.$cost.''); + } + + $output->newLine(); + } + + private function colour(Severity $severity): string + { + return match ($severity) { + Severity::Critical => 'red', + Severity::Warning => 'yellow', + Severity::Info, Severity::Unknown => 'blue', + }; + } +} diff --git a/src/Console/Support/SeverityBar.php b/src/Console/Support/SeverityBar.php new file mode 100644 index 0000000..07dae9f --- /dev/null +++ b/src/Console/Support/SeverityBar.php @@ -0,0 +1,60 @@ + $findings + */ + public function fails(array $findings): bool + { + if (! $this->limit instanceof Severity) { + return false; + } + + foreach ($findings as $finding) { + if ($finding->severity !== Severity::Unknown && $finding->severity->rank() <= $this->limit->rank()) { + return true; + } + } + + return false; + } +} diff --git a/src/Learn/Lessons/UnindexedForeignKeys.php b/src/Learn/Lessons/UnindexedForeignKeys.php index 7ea4ab7..3a134ec 100644 --- a/src/Learn/Lessons/UnindexedForeignKeys.php +++ b/src/Learn/Lessons/UnindexedForeignKeys.php @@ -178,6 +178,7 @@ public function tryIt(): string ." select 1 from pg_index\n" ." where indrelid = conrelid\n" ." and indisvalid\n" + ." and indpred is null\n" ." and (indkey::int2[])[0:cardinality(conkey) - 1] = conkey\n" .');'; } diff --git a/src/Queries/Constraints.php b/src/Queries/Constraints.php index 5147129..b61abcb 100644 --- a/src/Queries/Constraints.php +++ b/src/Queries/Constraints.php @@ -49,15 +49,25 @@ public function all(): array private function toConstraint(array $row): Constraint { $columns = Cast::text($row['columns'] ?? null); + $columnTypes = Cast::text($row['columntypes'] ?? null); + $referencedColumnTypes = Cast::text($row['referencedcolumntypes'] ?? null); return new Constraint( schema: Cast::text($row['schemaname'] ?? null), table: Cast::text($row['tablename'] ?? null), name: Cast::text($row['constraintname'] ?? null), kind: Cast::text($row['kind'] ?? null), - columns: $columns === '' ? [] : explode(',', $columns), + // constraints.sql joins these three lists with a newline rather than a + // comma: format_type renders numeric(10,2) with a comma already inside + // it, and comma-splitting that would break one type into two. Neither + // format_type's output nor a PostgreSQL identifier can contain a + // newline, so it is a safe delimiter, and it is internal to this class + // -- nothing downstream sees it. + columns: $columns === '' ? [] : explode("\n", $columns), referencedTable: Cast::text($row['referencedtable'] ?? null), indexed: Cast::boolean($row['indexed'] ?? null), + columnTypes: $columnTypes === '' ? [] : explode("\n", $columnTypes), + referencedColumnTypes: $referencedColumnTypes === '' ? [] : explode("\n", $referencedColumnTypes), ); } } diff --git a/src/Queries/IndexColumns.php b/src/Queries/IndexColumns.php new file mode 100644 index 0000000..412a494 --- /dev/null +++ b/src/Queries/IndexColumns.php @@ -0,0 +1,62 @@ + + */ + public function all(): array + { + $ignored = implode(',', $this->ignored->all()); + + return array_map( + $this->toDefinition(...), + $this->executor->select($this->sql->get(self::STATEMENT), [$ignored]), + ); + } + + /** + * @param array $row + */ + private function toDefinition(array $row): IndexDefinition + { + $columns = Cast::text($row['columns'] ?? null); + + return new IndexDefinition( + schema: Cast::text($row['schemaname'] ?? null), + table: Cast::text($row['tablename'] ?? null), + name: Cast::text($row['indexname'] ?? null), + columns: $columns === '' ? [] : explode(',', $columns), + unique: Cast::boolean($row['isunique'] ?? null), + valid: Cast::boolean($row['isvalid'] ?? null), + partial: Cast::boolean($row['ispartial'] ?? null), + method: Cast::text($row['method'] ?? null), + ); + } +} diff --git a/src/Queries/Schemas.php b/src/Queries/Schemas.php new file mode 100644 index 0000000..f1338d8 --- /dev/null +++ b/src/Queries/Schemas.php @@ -0,0 +1,24 @@ + + */ + public function all(): array; +} diff --git a/src/Queries/TableSchemas.php b/src/Queries/TableSchemas.php new file mode 100644 index 0000000..b8099b6 --- /dev/null +++ b/src/Queries/TableSchemas.php @@ -0,0 +1,73 @@ + + */ + public function all(): array + { + /** @var array> $columns */ + $columns = []; + + foreach ($this->columns->all() as $column) { + $columns[$column->qualifiedName()][] = $column; + } + + /** @var array> $constraints */ + $constraints = []; + + foreach ($this->constraints->all() as $constraint) { + $constraints[$constraint->qualifiedName()][] = $constraint; + } + + /** @var array> $indexes */ + $indexes = []; + + foreach ($this->indexes->all() as $index) { + $indexes[$index->schema.'.'.$index->table][] = $index; + } + + $schemas = []; + + foreach ($columns as $qualified => $tableColumns) { + $first = $tableColumns[0]; + + $schemas[] = new TableSchema( + schema: $first->schema, + table: $first->table, + columns: $tableColumns, + constraints: $constraints[$qualified] ?? [], + indexes: $indexes[$qualified] ?? [], + ); + } + + return $schemas; + } +} diff --git a/src/VacuumServiceProvider.php b/src/VacuumServiceProvider.php index 77d42a9..1fd1490 100644 --- a/src/VacuumServiceProvider.php +++ b/src/VacuumServiceProvider.php @@ -17,6 +17,7 @@ use Heyosseus\Vacuum\Advisor\Inspections\ConfigurationInspection; use Heyosseus\Vacuum\Advisor\Inspections\DuplicateInspection; use Heyosseus\Vacuum\Advisor\Inspections\IndexInspection; +use Heyosseus\Vacuum\Advisor\Inspections\SchemaInspection; use Heyosseus\Vacuum\Advisor\Inspections\SessionInspection; use Heyosseus\Vacuum\Advisor\Inspections\SettingInspection; use Heyosseus\Vacuum\Advisor\Inspections\StatementInspection; @@ -28,25 +29,34 @@ use Heyosseus\Vacuum\Advisor\Rules\DeadTuples; use Heyosseus\Vacuum\Advisor\Rules\DuplicateIndex; use Heyosseus\Vacuum\Advisor\Rules\EndOfLifeMajor; +use Heyosseus\Vacuum\Advisor\Rules\ForeignKeyTypeMismatch; use Heyosseus\Vacuum\Advisor\Rules\IdleInTransaction; +use Heyosseus\Vacuum\Advisor\Rules\Int4PrimaryKey; use Heyosseus\Vacuum\Advisor\Rules\InvalidIndex; use Heyosseus\Vacuum\Advisor\Rules\IoTimingOff; +use Heyosseus\Vacuum\Advisor\Rules\JsonNotJsonb; use Heyosseus\Vacuum\Advisor\Rules\LockTimeoutIneffective; +use Heyosseus\Vacuum\Advisor\Rules\MissingPrimaryKey; use Heyosseus\Vacuum\Advisor\Rules\MultixactWraparound; use Heyosseus\Vacuum\Advisor\Rules\PendingRestart; use Heyosseus\Vacuum\Advisor\Rules\SlowStatement; use Heyosseus\Vacuum\Advisor\Rules\StaleStatistics; use Heyosseus\Vacuum\Advisor\Rules\TableBloat; use Heyosseus\Vacuum\Advisor\Rules\TimeoutsUnset; +use Heyosseus\Vacuum\Advisor\Rules\UnindexedForeignKey; +use Heyosseus\Vacuum\Advisor\Rules\UnindexedMorphs; use Heyosseus\Vacuum\Advisor\Rules\UnpatchedServer; use Heyosseus\Vacuum\Advisor\Rules\UnusedIndex; use Heyosseus\Vacuum\Advisor\Rules\Wraparound; +use Heyosseus\Vacuum\Advisor\SchemaAdvisor; +use Heyosseus\Vacuum\Advisor\SchemaRule; use Heyosseus\Vacuum\Advisor\SessionRule; use Heyosseus\Vacuum\Advisor\SettingRule; use Heyosseus\Vacuum\Advisor\StatementRule; use Heyosseus\Vacuum\Advisor\TableRule; use Heyosseus\Vacuum\Console\Commands\CheckCommand; use Heyosseus\Vacuum\Console\Commands\InstallCommand; +use Heyosseus\Vacuum\Console\Commands\LintCommand; use Heyosseus\Vacuum\Console\Commands\SnapshotCommand; use Heyosseus\Vacuum\Filament\Install\PhpLintChecker; use Heyosseus\Vacuum\Filament\Install\SyntaxChecker; @@ -76,6 +86,7 @@ use Heyosseus\Vacuum\Queries\ServerSettings; use Heyosseus\Vacuum\Queries\Sessions; use Heyosseus\Vacuum\Queries\Statements; +use Heyosseus\Vacuum\Queries\TableSchemas; use Heyosseus\Vacuum\Queries\TableStatistics; use Heyosseus\Vacuum\Support\SqlRepository; use Heyosseus\Vacuum\Values\Capabilities; @@ -118,6 +129,12 @@ final class VacuumServiceProvider extends ServiceProvider /** A whole subject of its own: a query paired with the rules that judge it. */ public const string INSPECTIONS = 'vacuum.inspections'; + /** Rules that judge the shape of a table rather than its statistics. */ + public const string SCHEMA_RULES = 'vacuum.schema-rules'; + + /** Inspections the schema advisor merges, kept apart from the main advisor's. */ + public const string SCHEMA_INSPECTIONS = 'vacuum.schema-inspections'; + /** Tag a Lesson with this to have it appear in the Learn curriculum. */ public const string LESSONS = 'vacuum.lessons'; @@ -133,6 +150,9 @@ final class VacuumServiceProvider extends ServiceProvider */ private array $inspections = []; + /** @var list> */ + private array $schemaInspections = []; + /** * Register the package's services into the container. */ @@ -273,6 +293,29 @@ public function register(): void ), ); + $this->registerSchemaInspection( + SchemaInspection::class, + self::SCHEMA_RULES, + SchemaRule::class, + [ + UnindexedForeignKey::class, + ForeignKeyTypeMismatch::class, + Int4PrimaryKey::class, + MissingPrimaryKey::class, + UnindexedMorphs::class, + JsonNotJsonb::class, + ], + fn (Application $app, array $rules): Inspection => new SchemaInspection( + $app->make(TableSchemas::class), + $rules, + ), + ); + + // duplicate-index needs no statistics: two identical indexes are identical + // the moment both migrations have run. The same inspection therefore serves + // both advisors. + $this->schemaInspections[] = DuplicateInspection::class; + $this->app->tag($this->inspections, self::INSPECTIONS); $this->app->bind(Advisor::class, function (Application $app): Advisor { @@ -287,6 +330,20 @@ public function register(): void return new Advisor($inspections); }); + $this->app->tag($this->schemaInspections, self::SCHEMA_INSPECTIONS); + + $this->app->bind(SchemaAdvisor::class, function (Application $app): SchemaAdvisor { + $inspections = []; + + foreach ($app->tagged(self::SCHEMA_INSPECTIONS) as $inspection) { + if ($inspection instanceof Inspection) { + $inspections[] = $inspection; + } + } + + return new SchemaAdvisor(new Advisor($inspections)); + }); + // Registration order is teaching order within a tier: byTier() keeps // lessons in the order they were tagged, so this list is also the // sequence a reader meets them in. @@ -321,8 +378,6 @@ public function register(): void } /** - * Register one inspection: its rules, its binding, and its place in the tag. - * * @template TRule of object * * @param class-string $inspection @@ -336,6 +391,51 @@ private function registerInspection( string $contract, array $rules, Closure $make, + ): void { + $this->bindInspection($inspection, $tag, $contract, $rules, $make); + + $this->inspections[] = $inspection; + } + + /** + * The same, for the tier the schema advisor merges. Kept in its own list so + * that a schema rule can never reach the dashboard's score by accident. + * + * @template TRule of object + * + * @param class-string $inspection + * @param class-string $contract + * @param list> $rules + * @param Closure(Application, list): Inspection $make + */ + private function registerSchemaInspection( + string $inspection, + string $tag, + string $contract, + array $rules, + Closure $make, + ): void { + $this->bindInspection($inspection, $tag, $contract, $rules, $make); + + $this->schemaInspections[] = $inspection; + } + + /** + * Register one inspection: its rules, and its binding. + * + * @template TRule of object + * + * @param class-string $inspection + * @param class-string $contract + * @param list> $rules + * @param Closure(Application, list): Inspection $make + */ + private function bindInspection( + string $inspection, + string $tag, + string $contract, + array $rules, + Closure $make, ): void { $this->app->tag($rules, $tag); @@ -343,8 +443,6 @@ private function registerInspection( $inspection, fn (Application $app): Inspection => $make($app, $this->rules($app, $tag, $contract)), ); - - $this->inspections[] = $inspection; } /** @@ -384,7 +482,7 @@ public function boot(): void $this->registerSchedule(); if ($this->app->runningInConsole()) { - $this->commands([CheckCommand::class, InstallCommand::class, SnapshotCommand::class]); + $this->commands([CheckCommand::class, LintCommand::class, InstallCommand::class, SnapshotCommand::class]); $this->publishes([ __DIR__.'/../config/vacuum.php' => $this->app->configPath('vacuum.php'), diff --git a/src/Values/Constraint.php b/src/Values/Constraint.php index d6db49f..815d017 100644 --- a/src/Values/Constraint.php +++ b/src/Values/Constraint.php @@ -22,6 +22,10 @@ * @param bool $indexed Whether these columns are the LEADING columns of some index. A * trailing column of a composite index does not count, because such * an index cannot serve a lookup on it. + * @param list $columnTypes The types of the constrained columns, as format_type + * renders them, in the same order as $columns. + * @param list $referencedColumnTypes The types of the columns a foreign key points + * at, in the same order. Empty for the other kinds. */ public function __construct( public string $schema, @@ -31,6 +35,10 @@ public function __construct( public array $columns, public string $referencedTable, public bool $indexed, + /** @var list */ + public array $columnTypes = [], + /** @var list */ + public array $referencedColumnTypes = [], ) {} public function qualifiedName(): string @@ -42,4 +50,18 @@ public function isForeignKey(): bool { return $this->kind === 'f'; } + + /** + * Whether both sides of a foreign key are the same type all the way down. + * + * PostgreSQL will happily accept a foreign key from an integer to a bigint. + * It creates the constraint, it enforces it correctly, and it then cannot use + * the parent's index to check it, because the comparison crosses types. There + * is no error, no warning and no catalog flag; the delete is just slow, and + * stays slow. + */ + public function typesMatch(): bool + { + return $this->columnTypes === $this->referencedColumnTypes; + } } diff --git a/src/Values/IndexDefinition.php b/src/Values/IndexDefinition.php new file mode 100644 index 0000000..b036dc6 --- /dev/null +++ b/src/Values/IndexDefinition.php @@ -0,0 +1,56 @@ + $columns The key columns in indkey order. An INCLUDE payload is + * excluded: it is stored in the leaf and cannot be searched, + * so it is not part of what the index can serve. + * @param bool $partial Whether the index has a WHERE clause. A partial index serves only + * the rows its predicate admits, so it does not answer a general + * lookup on its leading columns and no rule here may treat it as if + * it did. + */ + public function __construct( + public string $schema, + public string $table, + public string $name, + public array $columns, + public bool $unique, + public bool $valid, + public bool $partial, + public string $method, + ) {} + + public function qualifiedName(): string + { + return $this->schema.'.'.$this->name; + } + + /** + * Whether these are the LEADING columns of this index, in this order. + * + * Not set membership, which is the mistake this method exists to prevent. An + * index on (status, customer_id) contains customer_id and cannot serve a + * lookup on it; one on (customer_id, status) can. The two are identical as + * sets and only one of them is useful. + */ + public function leadsWith(string ...$columns): bool + { + return array_slice($this->columns, 0, count($columns)) === array_values($columns); + } +} diff --git a/src/Values/TableSchema.php b/src/Values/TableSchema.php new file mode 100644 index 0000000..d869bb4 --- /dev/null +++ b/src/Values/TableSchema.php @@ -0,0 +1,105 @@ + $columns + * @param list $constraints + * @param list $indexes + */ + public function __construct( + public string $schema, + public string $table, + public array $columns, + public array $constraints, + public array $indexes, + ) {} + + public function qualifiedName(): string + { + return $this->schema.'.'.$this->table; + } + + public function column(string $name): ?Column + { + foreach ($this->columns as $column) { + if ($column->name === $name) { + return $column; + } + } + + return null; + } + + public function primaryKey(): ?Constraint + { + foreach ($this->constraints as $constraint) { + if ($constraint->kind === 'p') { + return $constraint; + } + } + + return null; + } + + /** + * @return list + */ + public function foreignKeys(): array + { + $keys = []; + + foreach ($this->constraints as $constraint) { + if ($constraint->isForeignKey()) { + $keys[] = $constraint; + } + } + + return $keys; + } + + /** + * Whether some index on this table could serve a lookup on these columns. + * + * A partial index does not count, because it holds only the rows its + * predicate admits and the lookup being asked about is a general one. An + * invalid index does not count either: every write maintains it and no query + * is allowed to use it, so treating it as coverage would hide the problem + * behind the very thing that is also causing one. + */ + public function hasIndexLeadingWith(string ...$columns): bool + { + foreach ($this->indexes as $index) { + if ($index->partial) { + continue; + } + + if (! $index->valid) { + continue; + } + + if ($index->leadsWith(...$columns)) { + return true; + } + } + + return false; + } +} diff --git a/tests/Feature/Advisor/SchemaAdvisorTest.php b/tests/Feature/Advisor/SchemaAdvisorTest.php new file mode 100644 index 0000000..241bf42 --- /dev/null +++ b/tests/Feature/Advisor/SchemaAdvisorTest.php @@ -0,0 +1,48 @@ +toBeInstanceOf(SchemaAdvisor::class); +}); + +it('does not put schema findings into the main advisor', function (): void { + // The whole point of the separation: nothing a schema rule finds may change + // the score an existing installation has been reading. + foreach (app(Advisor::class)->findings() as $finding) { + expect($finding->rule)->not->toBe('unindexed-foreign-key'); + } +}); + +it('carries duplicate-index into the schema tier as well', function (): void { + // Two migrations creating the same index is visible on an empty database and + // is exactly what the linter is for, so the one inspection serves both + // advisors rather than being reimplemented for this one. + DB::statement('DROP TABLE IF EXISTS lint_dupes CASCADE'); + DB::statement('CREATE TABLE lint_dupes (id bigserial PRIMARY KEY, label text)'); + DB::statement('CREATE INDEX lint_dupes_label_a ON lint_dupes (label)'); + DB::statement('CREATE INDEX lint_dupes_label_b ON lint_dupes (label)'); + + $rules = array_map( + static fn (object $finding): string => $finding->rule, + app(SchemaAdvisor::class)->findings(), + ); + + expect($rules)->toContain('duplicate-index'); + + DB::statement('DROP TABLE IF EXISTS lint_dupes CASCADE'); +}); + +it('does not carry unused-index into the schema tier', function (): void { + // It reads scan counts, and a database the pipeline created ninety seconds + // ago has none — so every index in the schema would be reported as unused. + $rules = array_map( + static fn (object $finding): string => $finding->rule, + app(SchemaAdvisor::class)->findings(), + ); + + expect($rules)->not->toContain('unused-index'); +}); diff --git a/tests/Feature/Command/LintCommandTest.php b/tests/Feature/Command/LintCommandTest.php new file mode 100644 index 0000000..2e762a0 --- /dev/null +++ b/tests/Feature/Command/LintCommandTest.php @@ -0,0 +1,113 @@ + $findings */ + public function __construct(private array $findings) {} + + /** @return list */ + public function findings(): array + { + return $this->findings; + } + }; + + app()->instance(SchemaAdvisor::class, new SchemaAdvisor(new Advisor([$inspection]))); +} + +function schemaFinding(Severity $severity = Severity::Warning): Finding +{ + return new Finding( + rule: 'unindexed-foreign-key', + subject: 'public.orders.customer_id', + severity: $severity, + summary: 'No index behind the foreign key.', + impact: 'Deletes on the parent scan the child.', + remediation: 'CREATE INDEX CONCURRENTLY ON "public"."orders" ("customer_id");', + table: 'public.orders', + ); +} + +it('passes on a clean schema', function (): void { + stubAdvisor(); + + $this->artisan('vacuum:lint', ['--no-interaction' => true]) + ->expectsOutputToContain('100 / 100') + ->assertExitCode(0); +}); + +it('fails on a warning by default', function (): void { + // check defaults to critical because a drifting database should not redden a + // build. A schema defect was wrong the moment it was written, so lint does + // not wait for it to become critical. + stubAdvisor(schemaFinding()); + + $this->artisan('vacuum:lint', ['--no-interaction' => true])->assertExitCode(1); +}); + +it('passes the same finding when the bar is critical', function (): void { + stubAdvisor(schemaFinding()); + + $this->artisan('vacuum:lint', ['--fail-on' => 'critical', '--no-interaction' => true])->assertExitCode(0); +}); + +it('refuses a severity it does not know', function (): void { + stubAdvisor(); + + $this->artisan('vacuum:lint', ['--fail-on' => 'catastrophic', '--no-interaction' => true]) + ->assertExitCode(2); +}); + +it('fails rather than passes when Vacuum is disabled', function (): void { + // A gate that goes green because it never looked is worse than no gate, and + // that is more true in a pipeline than anywhere else. + config()->set('vacuum.enabled', false); + + $this->artisan('vacuum:lint', ['--no-interaction' => true])->assertExitCode(2); +}); + +it('emits a json document a pipeline can parse', function (): void { + stubAdvisor(schemaFinding()); + + // Artisan::call, not $this->artisan(): the PendingCommand's output does not + // reach Artisan::output(), and Artisan::output() flushes, so it is read once + // into a variable and asserted from there. + $exit = Artisan::call('vacuum:lint', ['--format' => 'json', '--no-interaction' => true]); + $document = json_decode(Artisan::output(), true); + + expect($exit)->toBe(1); + + expect($document)->toHaveKeys(['score', 'grade', 'failed', 'deductions', 'findings']) + ->and($document['failed'])->toBeTrue() + ->and($document['findings'][0]['rule'])->toBe('unindexed-foreign-key') + ->and($document['findings'][0]['subject'])->toBe('public.orders.customer_id') + ->and($document['findings'][0]['table'])->toBe('public.orders'); +}); + +it('says the baseline is not here yet rather than ignoring the flag', function (): void { + // The option exists so that the command's signature does not change when the + // baseline lands. Accepting it silently would be worse than refusing it. + stubAdvisor(); + + $this->artisan('vacuum:lint', ['--generate-baseline' => true, '--no-interaction' => true]) + ->assertExitCode(2); + + $this->artisan('vacuum:lint', ['--no-baseline' => true, '--no-interaction' => true]) + ->assertExitCode(2); + + $this->artisan('vacuum:lint', ['--baseline' => 'baseline.json', '--no-interaction' => true]) + ->assertExitCode(2); +}); diff --git a/tests/Feature/Learn/UnindexedForeignKeysTest.php b/tests/Feature/Learn/UnindexedForeignKeysTest.php index d7eb1ca..e5adeeb 100644 --- a/tests/Feature/Learn/UnindexedForeignKeysTest.php +++ b/tests/Feature/Learn/UnindexedForeignKeysTest.php @@ -92,6 +92,20 @@ ->and($lesson->tryIt())->toContain('pg_constraint'); }); +/** + * constraints.sql excludes a partial index from "indexed" because it cannot + * serve a foreign key's referential-integrity check -- that check needs to see + * an arbitrary parent row, not just the ones a partial index's predicate let + * in. tryIt() hands the reader a copy-paste query meant to reproduce the same + * verdict, so it has to exclude partial indexes too, or the lesson's own table + * and the reader's copy of the query would disagree. + */ +it('excludes a partial index from its copy-paste query, matching constraints.sql', function (): void { + $lesson = new UnindexedForeignKeys(app(Constraints::class), app(TableProfiles::class)); + + expect($lesson->tryIt())->toContain('indpred is null'); +}); + it('names its slug, title, tier, hook and entry-point position', function (): void { $lesson = new UnindexedForeignKeys(app(Constraints::class), app(TableProfiles::class)); diff --git a/tests/Feature/Queries/ConstraintsTest.php b/tests/Feature/Queries/ConstraintsTest.php index a395ef6..1a158ea 100644 --- a/tests/Feature/Queries/ConstraintsTest.php +++ b/tests/Feature/Queries/ConstraintsTest.php @@ -63,3 +63,78 @@ function constraintOn(string $column): Constraint expect(constraintOn('bare_id')->referencedTable)->toBe('learn_parents') ->and(constraintOn('bare_id')->isForeignKey())->toBeTrue(); }); + +it('carries the types on both sides of a foreign key', function (): void { + // A foreign key from an integer to a bigint has an index that the planner + // cannot use, and nothing in the catalog complains about it. + DB::statement('DROP TABLE IF EXISTS lint_children CASCADE'); + DB::statement('DROP TABLE IF EXISTS lint_parents CASCADE'); + DB::statement('CREATE TABLE lint_parents (id bigserial PRIMARY KEY)'); + DB::statement('CREATE TABLE lint_children (id bigserial PRIMARY KEY, parent_id integer REFERENCES lint_parents (id))'); + + $foreignKey = null; + + foreach (app(Constraints::class)->all() as $constraint) { + if ($constraint->table === 'lint_children' && $constraint->isForeignKey()) { + $foreignKey = $constraint; + } + } + + expect($foreignKey?->columnTypes)->toBe(['integer']) + ->and($foreignKey?->referencedColumnTypes)->toBe(['bigint']) + ->and($foreignKey?->typesMatch())->toBeFalse(); + + DB::statement('DROP TABLE IF EXISTS lint_children CASCADE'); + DB::statement('DROP TABLE IF EXISTS lint_parents CASCADE'); +}); + +it('keeps a numeric(10,2) type as one element instead of splitting on its own comma', function (): void { + // format_type renders numeric(10,2) with a comma already inside it. This + // package's own migration uses $table->decimal('value', 20), so a + // comma-joined list splitting one type into two is not an exotic failure. + DB::statement('DROP TABLE IF EXISTS numeric_children CASCADE'); + DB::statement('DROP TABLE IF EXISTS numeric_parents CASCADE'); + DB::statement('CREATE TABLE numeric_parents (id numeric(10,2) PRIMARY KEY)'); + DB::statement('CREATE TABLE numeric_children (id bigserial PRIMARY KEY, parent_id numeric(10,2) REFERENCES numeric_parents (id))'); + + $foreignKey = null; + + foreach (app(Constraints::class)->all() as $constraint) { + if ($constraint->table === 'numeric_children' && $constraint->isForeignKey()) { + $foreignKey = $constraint; + } + } + + expect($foreignKey?->columnTypes)->toBe(['numeric(10,2)']) + ->and($foreignKey?->referencedColumnTypes)->toBe(['numeric(10,2)']) + ->and($foreignKey?->typesMatch())->toBeTrue(); + + DB::statement('DROP TABLE IF EXISTS numeric_children CASCADE'); + DB::statement('DROP TABLE IF EXISTS numeric_parents CASCADE'); +}); + +it('does not count a foreign key covered only by a partial index as indexed', function (): void { + // A partial index holds only the rows its predicate admits. The + // referential-integrity check this column answers for needs to see an + // arbitrary row in the parent, not just the ones a WHERE clause let in, so + // a partial index must not count as coverage here -- the same rule + // Values\TableSchema::hasIndexLeadingWith() already applies. + DB::statement('DROP TABLE IF EXISTS partial_children CASCADE'); + DB::statement('DROP TABLE IF EXISTS partial_parents CASCADE'); + DB::statement('CREATE TABLE partial_parents (id bigserial PRIMARY KEY)'); + DB::statement('CREATE TABLE partial_children (id bigserial PRIMARY KEY, parent_id bigint REFERENCES partial_parents (id), active boolean)'); + DB::statement('CREATE INDEX ON partial_children (parent_id) WHERE active'); + + $foreignKey = null; + + foreach (app(Constraints::class)->all() as $constraint) { + if ($constraint->table === 'partial_children' && $constraint->isForeignKey()) { + $foreignKey = $constraint; + } + } + + expect($foreignKey?->indexed)->toBeFalse(); + + DB::statement('DROP TABLE IF EXISTS partial_children CASCADE'); + DB::statement('DROP TABLE IF EXISTS partial_parents CASCADE'); +}); diff --git a/tests/Feature/Queries/IndexColumnsTest.php b/tests/Feature/Queries/IndexColumnsTest.php new file mode 100644 index 0000000..b92edff --- /dev/null +++ b/tests/Feature/Queries/IndexColumnsTest.php @@ -0,0 +1,59 @@ +all() as $index) { + if ($index->name === $name) { + return $index; + } + } + + return null; +} + +it('reports key columns in index order', function (): void { + expect(found('lint_indexes_pair')?->columns)->toBe(['kind', 'label']); +}); + +it('marks a partial index as partial', function (): void { + // A partial index serves only the rows its predicate admits, so a rule must + // not accept it as covering a general lookup. + expect(found('lint_indexes_partial')?->partial)->toBeTrue() + ->and(found('lint_indexes_pair')?->partial)->toBeFalse(); +}); + +it('excludes an index over an expression', function (): void { + // Its indkey carries a 0 where the expression is, and a column list with a + // silent gap in it would mislead every rule that reads this. + expect(found('lint_indexes_expression'))->toBeNull(); +}); + +it('excludes an INCLUDE payload from the key columns', function (): void { + // The payload is stored in the leaf and cannot be searched, so it is not part + // of what the index can serve. + expect(found('lint_indexes_include')?->columns)->toBe(['kind']); +}); + +it('reports the primary key index as unique and valid', function (): void { + expect(found('lint_indexes_pkey')?->unique)->toBeTrue() + ->and(found('lint_indexes_pkey')?->valid)->toBeTrue() + ->and(found('lint_indexes_pkey')?->method)->toBe('btree'); +}); diff --git a/tests/Feature/Queries/TableSchemasTest.php b/tests/Feature/Queries/TableSchemasTest.php new file mode 100644 index 0000000..bb19a8e --- /dev/null +++ b/tests/Feature/Queries/TableSchemasTest.php @@ -0,0 +1,48 @@ +all() as $schema) { + if ($schema->table === $table) { + return $schema; + } + } + + return null; +} + +it('brings a table its columns, constraints and indexes together', function (): void { + $orders = assembled('lint_orders'); + + expect($orders?->column('customer_id')?->type)->toBe('bigint') + ->and($orders?->primaryKey()?->columns)->toBe(['id']) + ->and($orders?->foreignKeys())->toHaveCount(1) + ->and($orders?->hasIndexLeadingWith('customer_id'))->toBeTrue(); +}); + +it('gives a table with no constraints of its own an empty list rather than nothing', function (): void { + // A table without a foreign key must not be missing from the set, or a rule + // that counts tables would silently see fewer than exist. + $customers = assembled('lint_customers'); + + expect($customers)->not->toBeNull() + ->and($customers?->foreignKeys())->toBe([]); +}); diff --git a/tests/Unit/Advisor/Rules/ForeignKeyTypeMismatchTest.php b/tests/Unit/Advisor/Rules/ForeignKeyTypeMismatchTest.php new file mode 100644 index 0000000..2bcea23 --- /dev/null +++ b/tests/Unit/Advisor/Rules/ForeignKeyTypeMismatchTest.php @@ -0,0 +1,208 @@ +inspect(typedKey(['integer'], ['bigint'])); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->rule)->toBe('foreign-key-type-mismatch') + ->and($findings[0]->severity)->toBe(Severity::Warning) + ->and($findings[0]->subject)->toBe('public.orders.customer_id') + ->and($findings[0]->evidence)->toBe('integer references bigint'); +}); + +it('says nothing when both sides agree', function (): void { + expect(app(ForeignKeyTypeMismatch::class)->inspect(typedKey(['bigint'], ['bigint'])))->toBe([]); +}); + +it('says nothing when the types were not looked up', function (): void { + // An empty list on either side means the query did not answer, not that the + // types differ. Reporting a mismatch from missing evidence is the way a tool + // teaches people to ignore it. + expect(app(ForeignKeyTypeMismatch::class)->inspect(typedKey([], [])))->toBe([]) + ->and(app(ForeignKeyTypeMismatch::class)->inspect(typedKey(['bigint'], [])))->toBe([]); +}); + +it('explains that the index exists and cannot be used', function (): void { + $findings = app(ForeignKeyTypeMismatch::class)->inspect(typedKey(['integer'], ['bigint'])); + + expect($findings[0]->impact)->toContain('cannot') + ->and($findings[0]->impact)->toContain('customers'); +}); + +it('offers the type change, and warns that it rewrites the table', function (): void { + $findings = app(ForeignKeyTypeMismatch::class)->inspect(typedKey(['integer'], ['bigint'])); + + expect($findings[0]->remediation) + ->toContain('ALTER TABLE "public"."orders"') + ->and($findings[0]->remediation)->toContain('TYPE bigint') + ->and($findings[0]->impact)->toContain('rewrite'); +}); + +it('refuses to advise narrowing when the parent is the narrow side', function (): void { + // A legacy increments('id') parent referenced by a newer foreignId() child + // is the most common real shape of this defect: the parent is integer, the + // child is bigint. Narrowing the child to match would entrench the very + // ceiling int4-primary-key already warns about on the parent, and it would + // fail outright once a value in the child exceeds what integer can hold. + $findings = app(ForeignKeyTypeMismatch::class)->inspect(typedKey(['bigint'], ['integer'])); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->remediation)->toBeNull() + ->and($findings[0]->impact)->toContain('customers') + ->and($findings[0]->impact)->toContain('narrow') + ->and($findings[0]->impact)->toContain('widen'); +}); + +it('still offers the widening ALTER when the parent is the wider side', function (): void { + // The opposite direction is the existing, correct behaviour: the child is + // the narrow side, so widening it to match the parent is a real fix. + $findings = app(ForeignKeyTypeMismatch::class)->inspect(typedKey(['integer'], ['bigint'])); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->remediation) + ->toBe('ALTER TABLE "public"."orders" ALTER COLUMN "customer_id" TYPE bigint;'); +}); + +it('offers the widening ALTER when the mismatch is outside the integer family entirely', function (): void { + // uuid vs bigint has no narrow/wide relationship this rule knows how to + // reason about, so the narrow-parent guard must decline to apply and the + // existing widen-the-child advice has to stand. + $findings = app(ForeignKeyTypeMismatch::class)->inspect(typedKey(['uuid'], ['bigint'])); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->remediation)->not->toBeNull(); +}); + +it('offers the widening ALTER when only the child side is in the integer family', function (): void { + // The child (integer) is rankable but the parent (uuid) is not, so there is + // still no narrow/wide relationship to reason about and the guard must not + // fire from the child side alone. + $findings = app(ForeignKeyTypeMismatch::class)->inspect(typedKey(['integer'], ['uuid'])); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->remediation)->not->toBeNull(); +}); + +it('skips a composite column pair whose own type was not looked up', function (): void { + // Not reachable from the catalog, where columnTypes is always as long as + // columns for a composite key -- but the loop indexes into both lists + // positionally and must not do so blindly. + $table = new TableSchema('public', 'orders', [], [ + new Constraint( + schema: 'public', + table: 'orders', + name: 'orders_composite_short_here', + kind: 'f', + columns: ['tenant_id', 'customer_id'], + referencedTable: 'customers', + indexed: true, + columnTypes: ['bigint'], + referencedColumnTypes: ['bigint', 'bigint'], + ), + ], []); + + $findings = app(ForeignKeyTypeMismatch::class)->inspect($table); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->remediation)->toBeNull() + ->and($findings[0]->impact)->not->toContain('customer_id ('); +}); + +it('skips a composite column pair whose parent type was not looked up', function (): void { + // Not reachable from the catalog, where referencedColumnTypes is always as + // long as columns for a composite key -- but the loop indexes into both + // lists positionally and must not do so blindly. + $table = new TableSchema('public', 'orders', [], [ + new Constraint( + schema: 'public', + table: 'orders', + name: 'orders_composite_short_there', + kind: 'f', + columns: ['tenant_id', 'customer_id'], + referencedTable: 'customers', + indexed: true, + columnTypes: ['bigint', 'integer'], + referencedColumnTypes: ['bigint'], + ), + ], []); + + $findings = app(ForeignKeyTypeMismatch::class)->inspect($table); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->remediation)->toBeNull() + ->and($findings[0]->impact)->not->toContain('customer_id ('); +}); + +it('says nothing about a foreign key with no columns', function (): void { + // Not reachable from the catalog, but the rule indexes into the column list + // to name its subject and must not do so blindly. + $table = new TableSchema('public', 'orders', [], [ + new Constraint( + schema: 'public', + table: 'orders', + name: 'orders_empty', + kind: 'f', + columns: [], + referencedTable: 'customers', + indexed: true, + columnTypes: ['integer'], + referencedColumnTypes: ['bigint'], + ), + ], []); + + expect(app(ForeignKeyTypeMismatch::class)->inspect($table))->toBe([]); +}); + +it('refuses to advise an ALTER on a composite key, naming the columns that disagree', function (): void { + // Column 0 (tenant_id) already matches; only column 1 (customer_id) does + // not. The bug this guards against rewrote column 0 unconditionally -- + // taking the table's lock and rebuilding an index to fix a column that was + // never wrong, while leaving the real mismatch untouched. + $table = new TableSchema('public', 'orders', [], [ + new Constraint( + schema: 'public', + table: 'orders', + name: 'orders_composite_foreign', + kind: 'f', + columns: ['tenant_id', 'customer_id'], + referencedTable: 'customers', + indexed: true, + columnTypes: ['bigint', 'integer'], + referencedColumnTypes: ['bigint', 'bigint'], + ), + ], []); + + $findings = app(ForeignKeyTypeMismatch::class)->inspect($table); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->rule)->toBe('foreign-key-type-mismatch') + ->and($findings[0]->subject)->toBe('public.orders.tenant_id') + ->and($findings[0]->remediation)->toBeNull() + ->and($findings[0]->impact)->toContain('composite') + ->and($findings[0]->impact)->toContain('customer_id (integer vs bigint)') + ->and($findings[0]->impact)->not->toContain('tenant_id ('); +}); diff --git a/tests/Unit/Advisor/Rules/Int4PrimaryKeyTest.php b/tests/Unit/Advisor/Rules/Int4PrimaryKeyTest.php new file mode 100644 index 0000000..5c79bb8 --- /dev/null +++ b/tests/Unit/Advisor/Rules/Int4PrimaryKeyTest.php @@ -0,0 +1,95 @@ +inspect(keyed('integer')); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->rule)->toBe('int4-primary-key') + ->and($findings[0]->severity)->toBe(Severity::Warning) + ->and($findings[0]->subject)->toBe('public.orders.id') + ->and($findings[0]->summary)->toContain('2,147,483,647'); +}); + +it('reports a primary key on smallint', function (): void { + expect(app(Int4PrimaryKey::class)->inspect(keyed('smallint')))->toHaveCount(1); +}); + +it('says nothing about a bigint key', function (): void { + expect(app(Int4PrimaryKey::class)->inspect(keyed('bigint')))->toBe([]); +}); + +it('says nothing about a uuid or text key', function (): void { + expect(app(Int4PrimaryKey::class)->inspect(keyed('uuid')))->toBe([]) + ->and(app(Int4PrimaryKey::class)->inspect(keyed('text')))->toBe([]); +}); + +it('says nothing about a table with no primary key', function (): void { + // That is missing-primary-key's finding to make, not this one's. Two rules + // firing on one defect is how a report becomes noise. + expect(app(Int4PrimaryKey::class)->inspect(new TableSchema('public', 'orders', [], [], [])))->toBe([]); +}); + +it('reports each narrow column of a composite key', function (): void { + expect(app(Int4PrimaryKey::class)->inspect(keyed('integer', ['tenant_id', 'order_id'])))->toHaveCount(2); +}); + +it('says nothing when the key column is not in the column list', function (): void { + $orphan = new TableSchema('public', 'orders', [], [ + new Constraint( + schema: 'public', table: 'orders', name: 'orders_pkey', + kind: 'p', columns: ['id'], referencedTable: '', indexed: true, + ), + ], []); + + expect(app(Int4PrimaryKey::class)->inspect($orphan))->toBe([]); +}); + +it('says nothing about Laravel\'s own migrations table', function (): void { + // DatabaseMigrationRepository still creates this table with + // $table->increments('id') in every supported Laravel release, so without + // this exemption every stock `laravel new` app would fail vacuum:lint's + // default --fail-on=warning on a framework table it cannot change and + // that gains one row per migration run. + $migrations = new TableSchema('public', 'migrations', [ + new Column(schema: 'public', table: 'migrations', name: 'id', type: 'integer', nullable: false), + ], [ + new Constraint( + schema: 'public', table: 'migrations', name: 'migrations_pkey', + kind: 'p', columns: ['id'], referencedTable: '', indexed: true, + ), + ], []); + + expect(app(Int4PrimaryKey::class)->inspect($migrations))->toBe([]); +}); + +it('offers the widening, and says it rewrites the table', function (): void { + $findings = app(Int4PrimaryKey::class)->inspect(keyed('integer')); + + expect($findings[0]->remediation) + ->toBe('ALTER TABLE "public"."orders" ALTER COLUMN "id" TYPE bigint;') + ->and($findings[0]->impact)->toContain('rewrite'); +}); diff --git a/tests/Unit/Advisor/Rules/JsonNotJsonbTest.php b/tests/Unit/Advisor/Rules/JsonNotJsonbTest.php new file mode 100644 index 0000000..8224ed7 --- /dev/null +++ b/tests/Unit/Advisor/Rules/JsonNotJsonbTest.php @@ -0,0 +1,55 @@ + $type) { + $columns[] = new Column( + schema: 'public', table: 'events', name: 'payload'.$index, type: $type, nullable: true, + ); + } + + return new TableSchema('public', 'events', $columns, [], []); +} + +it('reports a json column', function (): void { + $findings = app(JsonNotJsonb::class)->inspect(events('json')); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->rule)->toBe('json-not-jsonb') + ->and($findings[0]->subject)->toBe('public.events.payload0'); +}); + +it('is information rather than a fault', function (): void { + // A json column is a defensible choice often enough that failing a build on + // it would be presumptuous. Info costs the score nothing. + expect(app(JsonNotJsonb::class)->inspect(events('json'))[0]->severity)->toBe(Severity::Info); +}); + +it('says nothing about jsonb', function (): void { + expect(app(JsonNotJsonb::class)->inspect(events('jsonb')))->toBe([]); +}); + +it('says nothing about a text column that merely holds json', function (): void { + expect(app(JsonNotJsonb::class)->inspect(events('text')))->toBe([]); +}); + +it('explains what json cannot do that jsonb can', function (): void { + $findings = app(JsonNotJsonb::class)->inspect(events('json')); + + expect($findings[0]->impact)->toContain('GIN') + ->and($findings[0]->impact)->toContain('reparse'); +}); + +it('offers the type change', function (): void { + expect(app(JsonNotJsonb::class)->inspect(events('json'))[0]->remediation) + ->toBe('ALTER TABLE "public"."events" ALTER COLUMN "payload0" TYPE jsonb USING "payload0"::jsonb;'); +}); diff --git a/tests/Unit/Advisor/Rules/MissingPrimaryKeyTest.php b/tests/Unit/Advisor/Rules/MissingPrimaryKeyTest.php new file mode 100644 index 0000000..045f630 --- /dev/null +++ b/tests/Unit/Advisor/Rules/MissingPrimaryKeyTest.php @@ -0,0 +1,116 @@ +inspect(unkeyed()); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->rule)->toBe('missing-primary-key') + ->and($findings[0]->severity)->toBe(Severity::Warning) + ->and($findings[0]->subject)->toBe('public.role_user') + ->and($findings[0]->table)->toBe('public.role_user'); +}); + +it('says nothing about a table that has one', function (): void { + $primary = new Constraint( + schema: 'public', table: 'role_user', name: 'role_user_pkey', + kind: 'p', columns: ['id'], referencedTable: '', indexed: true, + ); + + expect(app(MissingPrimaryKey::class)->inspect(unkeyed($primary)))->toBe([]); +}); + +it('suggests promoting an existing unique constraint rather than inventing a column', function (): void { + // A pivot table with a unique pair already has a key; it just has not been + // told that it is one. Suggesting a new surrogate id would be worse advice. + $findings = app(MissingPrimaryKey::class)->inspect(unkeyed(unique(['role_id', 'user_id']))); + + expect($findings[0]->remediation) + ->toBe('ALTER TABLE "public"."role_user" ADD PRIMARY KEY ("role_id", "user_id");') + ->and($findings[0]->summary)->toContain('role_user_unique'); +}); + +it('offers no statement when there is no unique constraint to promote', function (): void { + // Which column should be the key is a question about the domain, and a guess + // dressed as a migration is worse than saying nothing. + expect(app(MissingPrimaryKey::class)->inspect(unkeyed())[0]->remediation)->toBeNull(); +}); + +it('explains replication and Eloquent, not just tidiness', function (): void { + $findings = app(MissingPrimaryKey::class)->inspect(unkeyed()); + + expect($findings[0]->impact)->toContain('replica') + ->and($findings[0]->impact)->toContain('Eloquent'); +}); + +it('does not offer to promote a unique constraint over a nullable column', function (): void { + // PostgreSQL treats nulls as distinct, so such a constraint permits rows a + // primary key would refuse and the promotion would simply fail. + $table = new TableSchema( + 'public', + 'role_user', + [pivotColumn('role_id', nullable: true)], + [unique(['role_id'])], + [], + ); + + expect(app(MissingPrimaryKey::class)->inspect($table)[0]->remediation)->toBeNull(); +}); + +it('ignores a unique constraint with no columns', function (): void { + // Not reachable from the catalog, but the rule indexes into the column list + // to check nullability and must not do so blindly. + expect(app(MissingPrimaryKey::class)->inspect(unkeyed(unique([])))[0]->remediation)->toBeNull(); +}); + +it('ignores a unique constraint over a column the table does not have', function (): void { + // Defensive, for the same reason: promoting a constraint means reading each + // of its columns' nullability, and a name the catalog did not also hand + // back as a column is not one the rule can vouch for. + expect(app(MissingPrimaryKey::class)->inspect(unkeyed(unique(['ghost_column'])))[0]->remediation)->toBeNull(); +}); + +it('does not consider a foreign key promotable', function (): void { + // A table with no primary key can still have other constraints. Only a + // unique constraint is a candidate; every other kind is skipped outright, + // without ever looking at its columns. + $foreign = new Constraint( + schema: 'public', table: 'role_user', name: 'role_user_role_id_foreign', + kind: 'f', columns: ['role_id'], referencedTable: 'roles', indexed: true, + ); + + expect(app(MissingPrimaryKey::class)->inspect(unkeyed($foreign))[0]->remediation)->toBeNull(); +}); diff --git a/tests/Unit/Advisor/Rules/UnindexedForeignKeyTest.php b/tests/Unit/Advisor/Rules/UnindexedForeignKeyTest.php new file mode 100644 index 0000000..b52401c --- /dev/null +++ b/tests/Unit/Advisor/Rules/UnindexedForeignKeyTest.php @@ -0,0 +1,100 @@ +inspect(orders(foreignKey('customer_id', indexed: false))); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->rule)->toBe('unindexed-foreign-key') + ->and($findings[0]->severity)->toBe(Severity::Warning) + ->and($findings[0]->subject)->toBe('public.orders.customer_id') + ->and($findings[0]->table)->toBe('public.orders'); +}); + +it('says nothing about a foreign key that is indexed', function (): void { + expect(app(UnindexedForeignKey::class)->inspect(orders(foreignKey('customer_id', indexed: true))))->toBe([]); +}); + +it('reports each unindexed key on a table separately', function (): void { + // Four unindexed keys are four problems on four columns, each fixed by a + // different statement. One finding carrying four of them would be unusable + // as an annotation. + $findings = app(UnindexedForeignKey::class)->inspect(orders( + foreignKey('customer_id', indexed: false), + foreignKey('warehouse_id', indexed: false, references: 'warehouses'), + )); + + expect($findings)->toHaveCount(2) + ->and($findings[0]->subject)->toBe('public.orders.customer_id') + ->and($findings[1]->subject)->toBe('public.orders.warehouse_id'); +}); + +it('ignores primary keys and unique constraints', function (): void { + $primary = new Constraint( + schema: 'public', table: 'orders', name: 'orders_pkey', + kind: 'p', columns: ['id'], referencedTable: '', indexed: true, + ); + + expect(app(UnindexedForeignKey::class)->inspect(orders($primary)))->toBe([]); +}); + +it('offers the CREATE INDEX that would fix it', function (): void { + $findings = app(UnindexedForeignKey::class)->inspect(orders(foreignKey('customer_id', indexed: false))); + + expect($findings[0]->remediation) + ->toBe('CREATE INDEX CONCURRENTLY ON "public"."orders" ("customer_id");'); +}); + +it('names the parent whose deletes are paying for it', function (): void { + $findings = app(UnindexedForeignKey::class)->inspect(orders(foreignKey('customer_id', indexed: false))); + + expect($findings[0]->impact)->toContain('customers') + ->and($findings[0]->impact)->toContain('sequential'); +}); + +it('reports a composite key under its leading column', function (): void { + $composite = new Constraint( + schema: 'public', table: 'orders', name: 'orders_composite_foreign', + kind: 'f', columns: ['tenant_id', 'customer_id'], referencedTable: 'customers', indexed: false, + ); + + $findings = app(UnindexedForeignKey::class)->inspect(orders($composite)); + + expect($findings[0]->subject)->toBe('public.orders.tenant_id') + ->and($findings[0]->evidence)->toContain('tenant_id, customer_id'); +}); + +it('says nothing about a foreign key with no columns', function (): void { + // Not reachable from the catalog, but the rule indexes into the column list + // and must not do so blindly. + $empty = new Constraint( + schema: 'public', table: 'orders', name: 'orders_empty', + kind: 'f', columns: [], referencedTable: 'customers', indexed: false, + ); + + expect(app(UnindexedForeignKey::class)->inspect(orders($empty)))->toBe([]); +}); diff --git a/tests/Unit/Advisor/Rules/UnindexedMorphsTest.php b/tests/Unit/Advisor/Rules/UnindexedMorphsTest.php new file mode 100644 index 0000000..2186c0c --- /dev/null +++ b/tests/Unit/Advisor/Rules/UnindexedMorphsTest.php @@ -0,0 +1,98 @@ +inspect(comments($pair)); + + expect($findings)->toHaveCount(1) + ->and($findings[0]->rule)->toBe('unindexed-morphs') + ->and($findings[0]->severity)->toBe(Severity::Warning) + ->and($findings[0]->subject)->toBe('public.comments.commentable_type'); +}); + +it('says nothing when the composite index exists', function () use ($pair): void { + $indexed = comments($pair, [pairIndex(['commentable_type', 'commentable_id'])]); + + expect(app(UnindexedMorphs::class)->inspect($indexed))->toBe([]); +}); + +it('still reports when the index leads with the id instead of the type', function () use ($pair): void { + // Laravel queries a morph by type and id together, and morphs() creates the + // index in that order. An index leading with the id serves a different + // question from the one the relation asks. + $wrong = comments($pair, [pairIndex(['commentable_id', 'commentable_type'])]); + + expect(app(UnindexedMorphs::class)->inspect($wrong))->toHaveCount(1); +}); + +it('ignores a type column with no matching id column', function (): void { + expect(app(UnindexedMorphs::class)->inspect(comments([morphColumn('commentable_type', 'text')])))->toBe([]); +}); + +it('ignores an id column whose partner is not a string', function (): void { + $notMorphs = [ + morphColumn('commentable_type', 'integer'), + morphColumn('commentable_id', 'bigint'), + ]; + + expect(app(UnindexedMorphs::class)->inspect(comments($notMorphs)))->toBe([]); +}); + +it('ignores an id column that is not an integer', function (): void { + // The type column has a matching *_id, but nothing here says it is the + // integer half of a morphs() pair, so the rule declines to guess. + $notBigint = [ + morphColumn('commentable_type', 'character varying(255)'), + morphColumn('commentable_id', 'uuid'), + ]; + + expect(app(UnindexedMorphs::class)->inspect(comments($notBigint)))->toBe([]); +}); + +it('offers the composite index in the order the relation queries it', function () use ($pair): void { + $findings = app(UnindexedMorphs::class)->inspect(comments($pair)); + + expect($findings[0]->remediation) + ->toBe('CREATE INDEX CONCURRENTLY ON "public"."comments" ("commentable_type", "commentable_id");'); +}); + +it('reports two morphs pairs on one table separately', function (): void { + $two = [ + morphColumn('commentable_type', 'character varying(255)'), + morphColumn('commentable_id', 'bigint'), + morphColumn('authorable_type', 'character varying(255)'), + morphColumn('authorable_id', 'bigint'), + ]; + + expect(app(UnindexedMorphs::class)->inspect(comments($two)))->toHaveCount(2); +}); diff --git a/tests/Unit/Advisor/SchemaInspectionTest.php b/tests/Unit/Advisor/SchemaInspectionTest.php new file mode 100644 index 0000000..695ad49 --- /dev/null +++ b/tests/Unit/Advisor/SchemaInspectionTest.php @@ -0,0 +1,65 @@ + */ + public function all(): array + { + return [ + new TableSchema('public', 'orders', [], [], []), + new TableSchema('public', 'customers', [], [], []), + ]; + } + }; +} + +function ruleFinding(int $count): SchemaRule +{ + return new readonly class($count) implements SchemaRule + { + public function __construct(private int $count) {} + + public function inspect(TableSchema $table): array + { + $findings = []; + + for ($i = 0; $i < $this->count; $i++) { + $findings[] = new Finding( + rule: 'stub', + subject: $table->qualifiedName().'.'.$i, + severity: Severity::Warning, + summary: 'stub', + impact: 'stub', + ); + } + + return $findings; + } + }; +} + +it('puts every rule to every table and flattens what they return', function (): void { + // Two tables, one rule returning two findings each: four findings, not two. + // A schema rule may find several problems on one table, which is why this + // contract returns a list where the others return one finding or null. + $findings = (new SchemaInspection(twoTables(), [ruleFinding(2)]))->findings(); + + expect($findings)->toHaveCount(4) + ->and($findings[0]->subject)->toBe('public.orders.0') + ->and($findings[3]->subject)->toBe('public.customers.1'); +}); + +it('says nothing when no rule finds anything', function (): void { + expect((new SchemaInspection(twoTables(), [ruleFinding(0)]))->findings())->toBe([]); +}); diff --git a/tests/Unit/Console/SeverityBarTest.php b/tests/Unit/Console/SeverityBarTest.php new file mode 100644 index 0000000..09cabeb --- /dev/null +++ b/tests/Unit/Console/SeverityBarTest.php @@ -0,0 +1,44 @@ +toBeInstanceOf(SeverityBar::class); + } +}); + +it('refuses a name it does not know', function (): void { + expect(SeverityBar::parse('catastrophic'))->toBeNull() + ->and(SeverityBar::parse(null))->toBeNull(); +}); + +it('fails on findings at or above the bar', function (): void { + expect(SeverityBar::parse('warning')?->fails([barFinding(Severity::Critical)]))->toBeTrue() + ->and(SeverityBar::parse('warning')?->fails([barFinding(Severity::Warning)]))->toBeTrue() + ->and(SeverityBar::parse('warning')?->fails([barFinding(Severity::Info)]))->toBeFalse(); +}); + +it('never fails at the never bar', function (): void { + expect(SeverityBar::parse('never')?->fails([barFinding(Severity::Critical)]))->toBeFalse(); +}); + +it('never fails a build on an unknown', function (): void { + // A build should go red because the database has a problem, never because + // the role was short a grant. Said out loud so that reordering the ranks + // cannot quietly change it. + expect(SeverityBar::parse('info')?->fails([barFinding(Severity::Unknown)]))->toBeFalse(); +}); diff --git a/tests/Unit/Values/IndexDefinitionTest.php b/tests/Unit/Values/IndexDefinitionTest.php new file mode 100644 index 0000000..8fc4918 --- /dev/null +++ b/tests/Unit/Values/IndexDefinitionTest.php @@ -0,0 +1,45 @@ +qualifiedName())->toBe('public.taggables_taggable_type_taggable_id_index'); +}); + +it('leads with the columns it starts with, in order', function (): void { + expect(definition()->leadsWith('taggable_type', 'taggable_id'))->toBeTrue() + ->and(definition()->leadsWith('taggable_type'))->toBeTrue(); +}); + +it('does not lead with columns it merely contains', function (): void { + // An index on (a, b) cannot serve a lookup on b alone. Set membership is the + // wrong question and answering it would be the bug this method exists to avoid. + expect(definition()->leadsWith('taggable_id'))->toBeFalse() + ->and(definition()->leadsWith('taggable_id', 'taggable_type'))->toBeFalse(); +}); + +it('cannot lead with more columns than it has', function (): void { + expect(definition(['taggable_type'])->leadsWith('taggable_type', 'taggable_id'))->toBeFalse(); +}); + +it('leads with nothing when asked for nothing', function (): void { + expect(definition()->leadsWith())->toBeTrue(); +}); diff --git a/tests/Unit/Values/TableSchemaTest.php b/tests/Unit/Values/TableSchemaTest.php new file mode 100644 index 0000000..1ea8fb8 --- /dev/null +++ b/tests/Unit/Values/TableSchemaTest.php @@ -0,0 +1,94 @@ +qualifiedName())->toBe('public.orders'); +}); + +it('finds a column by name and returns null for one it does not have', function (): void { + $table = schema([column('id'), column('customer_id')]); + + expect($table->column('customer_id')?->name)->toBe('customer_id') + ->and($table->column('nothing_here'))->toBeNull(); +}); + +it('separates the primary key from the other constraints', function (): void { + $table = schema(constraints: [ + constraint('f', ['customer_id'], name: 'orders_customer_id_foreign'), + constraint('p', ['id']), + ]); + + expect($table->primaryKey()?->columns)->toBe(['id']) + ->and($table->foreignKeys())->toHaveCount(1); +}); + +it('has no primary key when nothing declares one', function (): void { + expect(schema(constraints: [constraint('u', ['email'], name: 'orders_email_unique')])->primaryKey())->toBeNull(); +}); + +it('finds an index leading with the columns asked for', function (): void { + $table = schema(indexes: [indexOn(['taggable_type', 'taggable_id'])]); + + expect($table->hasIndexLeadingWith('taggable_type', 'taggable_id'))->toBeTrue(); +}); + +it('does not accept a partial index as covering a lookup', function (): void { + // A partial index answers only for the rows its predicate admits, so it does + // not answer the general question a rule is asking. + $table = schema(indexes: [indexOn(['taggable_type', 'taggable_id'], partial: true)]); + + expect($table->hasIndexLeadingWith('taggable_type', 'taggable_id'))->toBeFalse(); +}); + +it('does not accept an invalid index as covering a lookup', function (): void { + // An invalid index is maintained by every write and may not be used by any + // query. Counting it as coverage would hide the problem twice over. + $table = schema(indexes: [indexOn(['taggable_type', 'taggable_id'], valid: false)]); + + expect($table->hasIndexLeadingWith('taggable_type', 'taggable_id'))->toBeFalse(); +});