diff --git a/.gitignore b/.gitignore index ccd8821..8a20270 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ Thumbs.db .vscode .env .superpowers/ +__pycache__/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dec6f6..a2a6b98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,20 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +## [1.2.0] - 2026-09-09 + +### Added + +- **A baseline, which is what makes `vacuum:lint` usable on a schema that predates it.** The first run against a five-year-old application prints several hundred findings, and the only available response was to stop running it. `--generate-baseline` writes `vacuum-baseline.json`, you commit it, and from then on the outstanding findings are excused and anything new fails the build. A finding is matched on its rule and its subject and on nothing else — not the prose, not the severity — so rewording a rule never invalidates a file somebody committed months ago. That is defensible only because schema-rule subjects are data-independent: `public.orders.customer_id` means the same thing on every run, which is not true of `slow-statement` and is why `vacuum:check` has no baseline. Entries that stop matching are reported as `Info` rather than silently carried, because a baseline nobody prunes becomes a place the next defect hides, and the score is computed over what is left with the suppressed count printed beside it. + +- **`--format=github`, so findings arrive on the pull request rather than in a log.** Each becomes a workflow-command annotation on the diff, and a markdown table is appended to `$GITHUB_STEP_SUMMARY` when the runner offers one. + +- **Findings are traced back to the migration that introduced them.** `database/migrations` is read with PHP's own tokenizer — the technique the Filament installer already uses, and with the same refusal to guess: a variable table name or a file that does not parse yields no anchor, and the finding is reported without one rather than pointed at a line it did not come from. An application that has run `schema:dump --prune` has little left to trace to — that flag is the one that deletes the migrations after squashing them, while plain `schema:dump` keeps them and is unaffected — which the README says plainly rather than leaving to be discovered. + +### Changed + +- **`int4-primary-key` is now `narrow-primary-key`.** The slug named a type rather than the defect, and the rule has always fired on `smallint` as well — it prints 32,767 or 2,147,483,647 as appropriate. The rename is deliberate and deliberately early: the slug is about to become a key in the baseline file users commit, and renaming it once anybody has a baseline would invalidate all of them. It appears as the `rule` value in the `vacuum:lint --format=json` document, so a pipeline filtering on the old string needs updating. + ## [1.1.0] - 2026-09-08 ### Added @@ -138,7 +152,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/v1.1.0...HEAD +[Unreleased]: https://github.com/heyosseus/vacuum/compare/v1.2.0...HEAD +[1.2.0]: https://github.com/heyosseus/vacuum/compare/v1.1.0...v1.2.0 [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 diff --git a/README.md b/README.md index 8636ce5..7ef0cc1 100644 --- a/README.md +++ b/README.md @@ -12,13 +12,21 @@ ![Vacuum — a PostgreSQL monitoring and tuning dashboard for Laravel](art/hero.png) -**A PostgreSQL monitoring and tuning dashboard for Laravel.** +**A PostgreSQL monitoring dashboard for Laravel — and a schema linter for the pipeline that ships it.** Vacuum reads what PostgreSQL already knows about itself — `pg_stat_user_tables`, `pg_stat_user_indexes`, `pg_stat_activity`, `pg_stat_database`, `pg_stat_statements`, `pg_class` — and turns it into a page that says what is wrong, what it is costing you, and the statement that would put it right. It shows you that statement. It never runs it. -> **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. +**It does not need a production database to be useful.** `vacuum:lint` reads the catalog rather than the statistics, so it has something to say against the empty Postgres container your test job already starts: a foreign key with no index behind it, a primary key that stops accepting rows at two billion, a table nothing can address a single row of. One line in the workflow you already have — + +```yaml +- run: php artisan vacuum:lint --format=github +``` + +— and the finding arrives as an annotation on the pull request that introduced it, on the line that introduced it. See [Linting the schema](#linting-the-schema). + +> **Status: 1.2.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 @@ -31,7 +39,9 @@ Then open `/vacuum`. That is all of it: the installer publishes the config and a ![How Vacuum works: six catalogs PostgreSQL maintains, thirteen rules, and a finding carrying the statement that fixes it](art/how-it-works.png) -Already running a Filament panel? `php artisan vacuum:install --filament` puts the same data inside it — see [Inside Filament](#inside-filament). Want it in CI instead of in a browser? `php artisan vacuum:check` — see [In your pipeline](#in-your-pipeline). +Already running a Filament panel? `php artisan vacuum:install --filament` puts the same data inside it — see [Inside Filament](#inside-filament). + +**In a pipeline there are two commands, and they answer different questions.** `vacuum:check` runs the full advisor against a database that has been *running* — see [In your pipeline](#in-your-pipeline). `vacuum:lint` runs against one that has only been *migrated*, which is what a test job actually has — see [Linting the schema](#linting-the-schema). The first belongs on a schedule against staging; the second belongs in `require-dev`, on every push. ## Contents @@ -41,7 +51,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 +- [Linting the schema](#linting-the-schema) — `vacuum:lint`, a baseline, and annotations on the pull request - [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) @@ -231,7 +241,7 @@ 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 +## 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 @@ -245,11 +255,13 @@ ever used is exactly the kind of green number this package exists to argue again php artisan vacuum:lint ``` +![vacuum:lint in a pipeline: the workflow step on the left, and the finding as an annotation on the pull request diff on the right](art/lint-in-ci.png) + | 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 | +| `narrow-primary-key` | A primary key too narrow to keep counting -- `integer` or `smallint` -- that stops accepting rows the moment it runs out of values | | `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 | @@ -272,6 +284,61 @@ 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. +### Adopting it on a schema that predates it + +Run `vacuum:lint` on a five-year-old application and it will find a great many +things. That is accurate and completely useless: nobody is going to fix four +hundred findings this afternoon, and a build that is red for reasons nobody +intends to act on is a build people learn to ignore. + +So write down what is already there, and let the linter tell you only what is new: + +```bash +php artisan vacuum:lint --generate-baseline +``` + +That writes `vacuum-baseline.json`. **Commit it.** From then on the outstanding +findings are excused and anything new fails the build, which is the only question +worth asking of a legacy schema. + +A baseline matches on the rule and the subject and on nothing else, so rewording a +rule — or making it more serious in a later release — never invalidates the file +you committed. When an entry stops matching anything, because somebody fixed it, +`vacuum:lint` says so as an `Info` finding rather than quietly carrying it: a +baseline nobody prunes becomes a place the next defect hides. + +```bash +php artisan vacuum:lint --no-baseline # report everything, baseline or not +php artisan vacuum:lint --baseline=path # somewhere other than the default +``` + +The score is computed over what is left after suppression, and the count of what was +suppressed is printed with the text output, carried as `suppressed` in the JSON document, +and emitted as a `::notice` for `--format=github`. A number that quietly ignored four +hundred findings would be the kind of green this package exists to argue against — +and the pull request is the one place that number matters most. + +### On the pull request + +`--format=github` emits GitHub Actions workflow commands, so each finding lands as +an annotation on the diff rather than in a log nobody opens. + +```yaml +- run: php artisan vacuum:lint --format=github +``` + +Findings are traced back to the migration that introduced them by parsing +`database/migrations` with PHP's own tokenizer — the same technique the Filament +installer uses, and with the same refusal to guess. A migration whose table name is +a variable, or that does not parse, yields no anchor; the finding is still +reported, without a file and a line. + +**If you have run `php artisan schema:dump --prune`, expect few anchors.** That flag deletes +`database/migrations` after squashing the schema into `database/schema/*.sql`, so the files +that declared your columns are gone and there is nothing left to trace to. Plain +`schema:dump` keeps the migrations and is unaffected. The findings are the same either way; +only the annotations lose their line numbers. + ## 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. @@ -436,6 +503,8 @@ From 1.0 — and from 1.1 where a line says so — these are public API and a br - **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. +- **The baseline file format** (1.2). It is committed to your repository, which makes it an interface whether or not it is called one. Keys may be added; the `findings` map will not change meaning. +- **`vacuum:lint --format=github`** (1.2) as an accepted value, and its severity mapping. The exact wording of an annotation is not covered. Explicitly **not** covered, and free to change in a minor release: @@ -443,6 +512,7 @@ Explicitly **not** covered, and free to change in a minor release: - The Blade views. Publishing them is supported; the markup inside them is not frozen. - Everything under `Internals` and `Learn`. Both are teaching surfaces, and pinning their shape would freeze the explanation as well as the code. - Anything marked `@internal`. +- `MigrationMap` and `SourceLocation`, which are console implementation detail rather than something a rule or a pipeline consumes. Vacuum supports the PostgreSQL major versions the PostgreSQL project still supports, and CI runs the suite against each of them. A major going end-of-life is a minor release here, not a major one. diff --git a/art/lint-in-ci.png b/art/lint-in-ci.png new file mode 100644 index 0000000..b6bcb03 Binary files /dev/null and b/art/lint-in-ci.png differ diff --git a/art/make_readme_art.py b/art/make_readme_art.py index 68a1992..2ea50ae 100644 --- a/art/make_readme_art.py +++ b/art/make_readme_art.py @@ -580,9 +580,152 @@ def cli_check() -> None: a.save("cli-check.png") +# --------------------------------------------------------------------------- +# 6. vacuum:lint in a pipeline +# --------------------------------------------------------------------------- + +WORKFLOW = [ + ("services:", DIM), + (" postgres: { image: postgres:17 }", GRAY), + ("", None), + ("steps:", DIM), + (" - run: php artisan migrate --force", GRAY), + (" - run: php artisan vacuum:lint --format=github", WHITE), +] + +# The diff as a reviewer sees it: the line that introduced the finding is the +# line the finding lands on. +DIFF = [ + (" ", "12", "Schema::create('orders', function (Blueprint $table) {", GRAY, False), + ("+", "13", " $table->id();", GRAY, True), + ("+", "14", " $table->foreignId('customer_id');", WHITE, True), + (" ", "15", "});", GRAY, False), +] + + +def lint_pr() -> None: + W, H = 1600, 620 + a = Art(W, H) + + a.text( + W / 2, + 48, + "The database is ninety seconds old and has no rows in it. " + "The finding still lands on the line that caused it.", + size=17, + fill=GRAY, + anchor="mm", + ) + + top, ph = 108, 400 + ax, aw = 70, 520 + bx, bw = 640, 890 + + a.panel(ax, top, aw, ph) + a.panel(bx, top, bw, ph) + a.arrow(600, top + ph / 2, 632, colour=DIM) + + # --- A: the workflow + y = top + 26 + a.text(ax + 24, y, "IN YOUR WORKFLOW", size=12, bold=True, fill=GRAY) + y += 36 + a.rect(ax + 24, y, aw - 48, 172, r=8, fill=(14, 15, 18), outline=LINE, width=1) + ly = y + 20 + for line, colour in WORKFLOW: + if line: + a.text(ax + 40, ly, line, size=13, fill=colour) + ly += 25 + y += 196 + + a.chip(ax + 24, y, "require-dev", TEAL, size=12) + y += 46 + for ln in ( + "No production database, no credentials, no", + "extension and no superuser. Every rule here", + "is answerable the moment migrate finishes.", + ): + a.text(ax + 24, y, ln, size=13.5, fill=DIM) + y += 22 + + # --- B: the pull request + y = top + 26 + a.text(bx + 24, y, "ON THE PULL REQUEST", size=12, bold=True, fill=GRAY) + y += 30 + a.text( + bx + 24, + y, + "database/migrations/2024_01_11_000000_create_orders_table.php", + size=13, + fill=DIM, + ) + y += 30 + + for mark, number, code, colour, added in DIFF: + if added: + a.rect(bx + 24, y - 4, bw - 48, 26, r=4, fill=(*TEAL, 16)) + a.text(bx + 34, y, number, size=13, fill=(70, 74, 82)) + a.text(bx + 72, y, mark, size=14, bold=True, fill=TEAL if added else DIM) + a.text(bx + 92, y, code, size=14, fill=colour) + y += 26 + + # the annotation, hung under the line that produced it + y += 16 + ah = 152 + a.rect(bx + 92, y, bw - 140, ah, r=8, fill=(14, 15, 18), outline=(*AMBER, 90), width=1) + a.rect(bx + 92, y, 4, ah, r=2, fill=AMBER) + + iy = y + 20 + cw = a.chip(bx + 116, iy, "WARNING", AMBER, size=11) + a.text(bx + 116 + cw + 14, iy + 12, "unindexed-foreign-key", size=12.5, fill=DIM, anchor="lm") + iy += 40 + a.text( + bx + 116, + iy, + "orders.customer_id has a foreign key and no index behind it.", + size=14, + fill=WHITE, + ) + iy += 26 + a.text( + bx + 116, + iy, + "PostgreSQL indexes a primary key and creates nothing for this.", + size=13, + fill=GRAY, + ) + iy += 30 + a.rect(bx + 116, iy, bw - 188, 32, r=6, fill=(21, 23, 28), outline=LINE, width=1) + a.text( + bx + 128, + iy + 16, + 'CREATE INDEX CONCURRENTLY ON "public"."orders" ("customer_id");', + size=12, + fill=TEAL, + anchor="lm", + ) + + # --- the verdict + by, bh = 526, 62 + a.rect(70, by, 1460, bh, r=10, fill=(*CORAL, 20), outline=(*CORAL, 70), width=1) + a.rect(70, by, 4, bh, r=2, fill=CORAL) + a.text( + 100, + by + bh / 2, + "Exit 1. A warning from vacuum:check is a database drifting; a warning from " + "vacuum:lint is a schema that was wrong the moment somebody typed it.", + size=17, + bold=True, + fill=CORAL, + anchor="lm", + ) + + a.save("lint-in-ci.png") + + if __name__ == "__main__": hero() how_it_works() scoring() safety() cli_check() + lint_pr() diff --git a/config/vacuum.php b/config/vacuum.php index 08561bb..4f47375 100644 --- a/config/vacuum.php +++ b/config/vacuum.php @@ -309,4 +309,29 @@ 'enabled' => env('VACUUM_LEARN_ENABLED', true), ], + /* + |-------------------------------------------------------------------------- + | Lint + |-------------------------------------------------------------------------- + | + | vacuum:lint reads the shape of the schema rather than its statistics, so it + | has something to say in a pipeline where every statistics-based rule finds + | nothing. These two keys are what it needs from the filesystem. + | + | 'baseline' is resolved against base_path() and is used automatically when + | the file exists. It is what makes the linter adoptable on a schema that + | predates it: without one, the first run on a legacy application prints + | several hundred findings and the only available response is to stop running + | it. + | + | 'migrations_path' is where findings are traced back to the line that + | introduced them. Null means database_path('migrations'). + | + */ + + 'lint' => [ + 'baseline' => env('VACUUM_LINT_BASELINE', 'vacuum-baseline.json'), + 'migrations_path' => env('VACUUM_LINT_MIGRATIONS_PATH'), + ], + ]; diff --git a/src/Advisor/Rules/ForeignKeyTypeMismatch.php b/src/Advisor/Rules/ForeignKeyTypeMismatch.php index 9dcdd03..1271b0b 100644 --- a/src/Advisor/Rules/ForeignKeyTypeMismatch.php +++ b/src/Advisor/Rules/ForeignKeyTypeMismatch.php @@ -35,7 +35,7 @@ * 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 + * narrow-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 @@ -153,7 +153,7 @@ private function narrowParentImpact(Constraint $key): string .'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 ' + .'narrow-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 ' diff --git a/src/Advisor/Rules/Int4PrimaryKey.php b/src/Advisor/Rules/NarrowPrimaryKey.php similarity index 96% rename from src/Advisor/Rules/Int4PrimaryKey.php rename to src/Advisor/Rules/NarrowPrimaryKey.php index 71f4893..ac330d7 100644 --- a/src/Advisor/Rules/Int4PrimaryKey.php +++ b/src/Advisor/Rules/NarrowPrimaryKey.php @@ -13,7 +13,7 @@ use Heyosseus\Vacuum\Values\TableSchema; /** - * Finds primary keys counted in 32 bits. + * Finds primary keys counted in too few bits. * * This is the wraparound story told in a different register. A four-byte integer * counts to 2,147,483,647 and then the next insert fails, and like wraparound it @@ -39,7 +39,7 @@ * 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 +final readonly class NarrowPrimaryKey implements SchemaRule { /** What format_type renders for the integer types narrower than bigint. */ private const array NARROW = ['integer', 'smallint']; @@ -81,7 +81,7 @@ public function inspect(TableSchema $table): array $ceiling = $column->type === 'smallint' ? '32,767' : '2,147,483,647'; $findings[] = new Finding( - rule: 'int4-primary-key', + rule: 'narrow-primary-key', subject: $table->qualifiedName().'.'.$name, severity: Severity::Warning, summary: "The primary key column {$name} is {$column->type}, so it can count to {$ceiling} " diff --git a/src/Console/Commands/LintCommand.php b/src/Console/Commands/LintCommand.php index 18ba853..58f70bd 100644 --- a/src/Console/Commands/LintCommand.php +++ b/src/Console/Commands/LintCommand.php @@ -4,10 +4,13 @@ namespace Heyosseus\Vacuum\Console\Commands; +use Composer\InstalledVersions; use Heyosseus\Vacuum\Advisor\Finding; use Heyosseus\Vacuum\Advisor\Health; use Heyosseus\Vacuum\Advisor\SchemaAdvisor; +use Heyosseus\Vacuum\Console\Support\Baseline; use Heyosseus\Vacuum\Console\Support\FindingReporter; +use Heyosseus\Vacuum\Console\Support\GithubReporter; use Heyosseus\Vacuum\Console\Support\SeverityBar; use Illuminate\Console\Command; use Illuminate\Contracts\Config\Repository; @@ -34,9 +37,9 @@ final class LintCommand extends Command { protected $signature = 'vacuum:lint {--fail-on=warning : The lowest severity that should fail the command: critical, warning, info or never} - {--format=text : text for a person, json for anything else} + {--format=text : text for a person, json for a pipeline, github for pull request annotations} {--baseline= : Path to a baseline file} - {--generate-baseline : Write the baseline and exit} + {--generate-baseline : Write every current finding to the baseline and exit} {--no-baseline : Ignore any baseline that exists}'; protected $description = 'Inspect the schema for defects that are visible without any data'; @@ -45,6 +48,7 @@ public function handle( SchemaAdvisor $advisor, Repository $config, FindingReporter $reporter, + GithubReporter $github, ): int { // A gate that goes green because it never looked is worse than no gate. if ($config->get('vacuum.enabled') !== true) { @@ -55,15 +59,6 @@ public function handle( 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); @@ -77,39 +72,162 @@ public function handle( } $findings = $advisor->findings(); - $health = Health::from($findings); - $failed = $bar->fails($findings); - if ($this->option('format') === 'json') { - $this->output->writeln($this->json($health, $findings, $failed)); + if ($this->option('generate-baseline') === true) { + $path = $this->baselinePath($config); + $document = Baseline::record($findings)->encode($this->version()); + + // A read-only checkout, a --baseline= pointing at a directory that does + // not exist, or a full disk all fail file_put_contents() the same way: + // silently, unless the return value is actually checked. + if (@file_put_contents($path, $document) !== strlen($document)) { + $this->components->error("Could not write the baseline to {$path}."); + + return self::FAILURE; + } + + $this->components->info( + count($findings).' findings written to '.$path.'. Commit it, and vacuum:lint will ' + .'report only what is new from now on.', + ); + + return self::SUCCESS; + } + + $baseline = $this->baseline($config); + $kept = []; + $suppressed = 0; + + foreach ($findings as $finding) { + if ($baseline->suppresses($finding)) { + $suppressed++; + + continue; + } + + $kept[] = $finding; + } + + // Stale entries are shown -- the reader needs to know the baseline wants + // pruning -- but never fed to the bar. They are Info because the defect + // being gone is good news, and good news must never be able to redden a + // build at --fail-on=info; fixing something is not a regression. + $display = [...$kept, ...$baseline->stale($findings)]; + + $health = Health::from($display); + $failed = $bar->fails($kept); + + $format = $this->option('format'); + + if ($format === 'json') { + $this->output->writeln($this->json($health, $display, $failed, $suppressed)); + } elseif ($format === 'github') { + $github->report($this->output, $display, $suppressed); + $this->appendStepSummary($github->summary($display)); } else { $reporter->report( $this->output, $health, - $findings, + $display, 'Every table has a key, every foreign key has an index, and every type lines up.', ); + + if ($suppressed > 0) { + $this->line(" {$suppressed} suppressed by the baseline."); + $this->newLine(); + } } 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. + * The baseline in force, which is none when the caller said so or when there + * is no file to read. */ - private function baselineRequested(): bool + private function baseline(Repository $config): Baseline { - if ($this->option('generate-baseline') === true) { - return true; + if ($this->option('no-baseline') === true) { + return Baseline::none(); } - if ($this->option('no-baseline') === true) { - return true; + $path = $this->baselinePath($config); + + if (! is_file($path)) { + return Baseline::none(); + } + + $contents = file_get_contents($path); + + return $contents === false ? Baseline::none() : Baseline::decode($contents); + } + + private function baselinePath(Repository $config): string + { + $option = $this->option('baseline'); + + if (is_string($option) && $option !== '') { + return base_path($option); + } + + $configured = $config->get('vacuum.lint.baseline'); + + return base_path(is_string($configured) ? $configured : 'vacuum-baseline.json'); + } + + /** + * GitHub gives a job a file to append a summary to, and gives it only inside + * Actions -- so the variable's presence is the feature flag, and no + * configuration key is needed for something the runner either offers or does + * not. + * + * A failure here does not fail the command. The step summary is a convenience + * the runner offers on top of the annotations that already carry every + * finding; unlike the baseline, losing it loses nothing the caller depended + * on. It only must not be mistaken for having worked, so a failed append is + * reported rather than swallowed. + */ + private function appendStepSummary(string $markdown): void + { + $path = getenv('GITHUB_STEP_SUMMARY'); + + if (! is_string($path) || $path === '') { + return; + } + + if (@file_put_contents($path, $markdown, FILE_APPEND) !== strlen($markdown)) { + $this->components->warn("Could not append the step summary to {$path}."); + } + } + + /** + * Stamped into the file so a reader knows which rule set produced it. + * + * Vacuum's own version, not the application's. In every real installation the + * application is the root package, and Vacuum is a dependency underneath it -- + * reading the root would stamp the baseline with the app's own tag, or with + * "dev-main" for a checkout, which says nothing about which rule set wrote the + * file. Composer is asked for "heyosseus/vacuum" by name first, so the + * baseline carries Vacuum's own version wherever Composer can report one. + * + * The root package is the fallback, for whenever Composer cannot answer that + * lookup -- which includes this package's own test suite, where Vacuum *is* + * the root and the root's version is the only one there is to report. + */ + private function version(): string + { + if (InstalledVersions::isInstalled('heyosseus/vacuum')) { + $version = InstalledVersions::getPrettyVersion('heyosseus/vacuum'); + + if ($version !== null) { + return $version; + } } - return is_string($this->option('baseline')) && $this->option('baseline') !== ''; + /** @var array{pretty_version: string} $root */ + $root = InstalledVersions::getRootPackage(); + + return $root['pretty_version']; } /** @@ -121,12 +239,13 @@ private function baselineRequested(): bool * * @param list $findings */ - private function json(Health $health, array $findings, bool $failed): string + private function json(Health $health, array $findings, bool $failed, int $suppressed): string { return json_encode([ 'score' => $health->score, 'grade' => $health->grade->value, 'failed' => $failed, + 'suppressed' => $suppressed, 'deductions' => $health->deductions, 'findings' => array_map(static fn (Finding $finding): array => [ 'rule' => $finding->rule, diff --git a/src/Console/Support/Baseline.php b/src/Console/Support/Baseline.php new file mode 100644 index 0000000..476d9b5 --- /dev/null +++ b/src/Console/Support/Baseline.php @@ -0,0 +1,165 @@ +> $entries Rule to the subjects excused under it. + */ + private function __construct(private array $entries) {} + + public static function none(): self + { + return new self([]); + } + + /** + * @param list $findings + */ + public static function record(array $findings): self + { + $entries = []; + + foreach ($findings as $finding) { + $entries[$finding->rule][] = $finding->subject; + } + + foreach ($entries as $rule => $subjects) { + $unique = array_values(array_unique($subjects)); + sort($unique); + $entries[$rule] = $unique; + } + + ksort($entries); + + return new self($entries); + } + + /** + * A baseline that could not be read is no baseline at all. + * + * Reporting everything is the safe direction to fail in: a corrupt file + * should not take a pipeline down, and it must not silently excuse findings + * it never actually listed. + */ + public static function decode(string $json): self + { + try { + /** @var mixed $document */ + $document = json_decode($json, true, 512, JSON_THROW_ON_ERROR); + } catch (JsonException) { + return self::none(); + } + + if (! is_array($document)) { + return self::none(); + } + + $findings = $document['findings'] ?? null; + + if (! is_array($findings)) { + return self::none(); + } + + $entries = []; + + foreach ($findings as $rule => $subjects) { + if (! is_string($rule)) { + continue; + } + if (! is_array($subjects)) { + continue; + } + + foreach ($subjects as $subject) { + if (is_string($subject)) { + $entries[$rule][] = $subject; + } + } + } + + return new self($entries); + } + + public function suppresses(Finding $finding): bool + { + return in_array($finding->subject, $this->entries[$finding->rule] ?? [], true); + } + + /** + * Entries that match nothing any more, as findings of their own. + * + * Reported rather than pruned silently, and Info rather than a fault: the + * defect being gone is good news, but a baseline nobody tidies becomes a + * place the next one hides. + * + * @param list $findings + * @return list + */ + public function stale(array $findings): array + { + $present = []; + + foreach ($findings as $finding) { + $present[$finding->rule.'|'.$finding->subject] = true; + } + + $stale = []; + + foreach ($this->entries as $rule => $subjects) { + foreach ($subjects as $subject) { + if (isset($present[$rule.'|'.$subject])) { + continue; + } + + $stale[] = new Finding( + rule: 'baseline-stale', + subject: $subject, + severity: Severity::Info, + summary: "The baseline still excuses {$rule} here, and the rule no longer fires.", + impact: 'Nothing is wrong with this subject any more. Regenerate the baseline so the ' + .'file describes what is actually outstanding, rather than accumulating entries ' + .'that excuse nothing and hide the next thing that does.', + ); + } + } + + return $stale; + } + + public function encode(string $version): string + { + return json_encode([ + 'generated_at' => date(DATE_ATOM), + 'vacuum' => $version, + 'findings' => $this->entries, + ], JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_THROW_ON_ERROR)."\n"; + } +} diff --git a/src/Console/Support/GithubReporter.php b/src/Console/Support/GithubReporter.php new file mode 100644 index 0000000..986bfd0 --- /dev/null +++ b/src/Console/Support/GithubReporter.php @@ -0,0 +1,124 @@ + $findings + */ + public function report(OutputStyle $output, array $findings, int $suppressed = 0): void + { + foreach ($findings as $finding) { + $properties = ['title' => $finding->rule]; + $location = $this->map->locate($finding); + + if ($location instanceof SourceLocation) { + $properties = ['file' => $this->relative($location->file), 'line' => (string) $location->line] + $properties; + } + + $pairs = []; + + foreach ($properties as $key => $value) { + $pairs[] = $key.'='.$this->property($value); + } + + $output->writeln( + '::'.$this->level($finding->severity).' '.implode(',', $pairs) + .'::'.$this->message($finding->subject.' — '.$finding->summary), + ); + } + + // The one place a pull request most needs this number: four hundred + // findings suppressed by the baseline is exactly the fact a reviewer + // reading only the diff's annotations would otherwise never see. + if ($suppressed > 0) { + $output->writeln('::notice::'.$this->message("{$suppressed} suppressed by the baseline.")); + } + } + + /** + * @param list $findings + */ + public function summary(array $findings): string + { + $rows = ['| Severity | Rule | Subject |', '| --- | --- | --- |']; + + foreach ($findings as $finding) { + $rows[] = '| '.$finding->severity->value.' | '.$finding->rule.' | `'.$finding->subject.'` |'; + } + + return implode("\n", $rows)."\n"; + } + + private function level(Severity $severity): string + { + return match ($severity) { + Severity::Critical => 'error', + Severity::Warning => 'warning', + Severity::Info, Severity::Unknown => 'notice', + }; + } + + /** + * The percent sign is escaped first, or it would escape the escapes. + */ + private function message(string $value): string + { + return str_replace(['%', "\r", "\n"], ['%25', '%0D', '%0A'], $value); + } + + private function property(string $value): string + { + return str_replace([':', ','], ['%3A', '%2C'], $this->message($value)); + } + + /** + * The path GitHub can actually match against the diff. + * + * The map's paths are absolute, correctly, for every other consumer -- but + * GitHub resolves an annotation's `file` against the repository root, not the + * runner's filesystem, so `/home/runner/work/app/app/database/migrations/…` + * matches nothing and the annotation attaches to no line at all. A path + * outside base_path() -- a migrations directory configured elsewhere, or a + * test fixture -- is left exactly as the map produced it rather than mangled + * into something that looks relative but is not. + */ + private function relative(string $file): string + { + $normalized = str_replace('\\', '/', $file); + $base = rtrim(str_replace('\\', '/', base_path()), '/').'/'; + + return str_starts_with($normalized, $base) ? substr($normalized, strlen($base)) : $file; + } +} diff --git a/src/Schema/MigrationMap.php b/src/Schema/MigrationMap.php new file mode 100644 index 0000000..c18339b --- /dev/null +++ b/src/Schema/MigrationMap.php @@ -0,0 +1,127 @@ +|null */ + private ?array $entries = null; + + public function __construct( + private readonly string $directory, + private readonly MigrationScanner $scanner, + ) {} + + /** + * A migration never writes a schema -- `Schema::create('orders', ...)`, not + * `Schema::create('public.orders', ...)` -- unless the application is + * multi-schema and deliberately qualifies it, so the scanner's entries are + * ordinarily unqualified even when the finding is not. Reducing straight to + * the unqualified key, as this used to, means `tenant.orders` and + * `public.orders` collide on the one entry `orders` records: a finding in + * the tenant schema anchors to the default schema's migration, which is + * wrong precisely on the multi-schema and multi-tenant Postgres this + * package's audience runs. The qualified key is tried first, so a migration + * that does write the schema wins the collision it would otherwise lose to. + */ + public function locate(Finding $finding): ?SourceLocation + { + if ($finding->table === null) { + return null; + } + + $entries = $this->entries(); + $qualified = $finding->table; + $table = $this->unqualified($qualified); + $column = $this->column($finding->subject, $qualified); + + if ($column !== null) { + $onQualifiedTable = $entries[$qualified.'.'.$column] ?? null; + + if ($onQualifiedTable instanceof SourceLocation) { + return $onQualifiedTable; + } + + $onUnqualifiedTable = $entries[$table.'.'.$column] ?? null; + + if ($onUnqualifiedTable instanceof SourceLocation) { + return $onUnqualifiedTable; + } + } + + return $entries[$qualified] ?? $entries[$table] ?? null; + } + + /** + * The part of a subject naming a column of the given table, or null when the + * subject is not one -- a table-level finding, or an index finding whose + * subject is the index rather than the table it belongs to. + */ + private function column(string $subject, string $table): ?string + { + $prefix = $table.'.'; + + return str_starts_with($subject, $prefix) ? substr($subject, strlen($prefix)) : null; + } + + /** + * Scanned once per run, and only when something asks. + * + * @return array + */ + private function entries(): array + { + if ($this->entries !== null) { + return $this->entries; + } + + $entries = []; + $files = glob(rtrim($this->directory, '/\\').DIRECTORY_SEPARATOR.'*.php'); + + foreach ($files === false ? [] : $files as $file) { + // A glob match that is not a readable file -- a directory named + // something.php is the reproducible case -- fails differently by + // platform: Windows returns false where Linux returns an empty + // string. Coalescing the two is not tidiness, it is what keeps this + // branch reachable on both, rather than covered on a laptop and dead + // in CI. + $source = @file_get_contents($file) ?: ''; + + if ($source === '') { + continue; + } + + foreach ($this->scanner->scan($source) as $key => $line) { + $entries[$key] ??= new SourceLocation($file, $line); + } + } + + return $this->entries = $entries; + } + + private function unqualified(string $table): string + { + $dot = strrpos($table, '.'); + + return $dot === false ? $table : substr($table, $dot + 1); + } +} diff --git a/src/Schema/MigrationScanner.php b/src/Schema/MigrationScanner.php new file mode 100644 index 0000000..9d76409 --- /dev/null +++ b/src/Schema/MigrationScanner.php @@ -0,0 +1,237 @@ + Keys are "table" and "table.column"; values are 1-based lines. + */ + public function scan(string $source): array + { + try { + // TOKEN_PARSE is what makes a genuinely broken migration yield + // nothing rather than whatever the tokenizer's lenient best-effort + // reading of invalid source happens to produce. + /** @var list $tokens */ + $tokens = @token_get_all($source, TOKEN_PARSE); + } catch (CompileError) { + // ParseError extends CompileError, so this also catches source that + // merely fails to parse; a valid parse can still fail to compile, such + // as `abstract final class C {}`, and that has to be caught too. + return []; + } + + $entries = []; + $table = null; + $depth = 0; + $scope = 0; + + foreach ($tokens as $index => $token) { + if ($token === '{') { + $depth++; + + continue; + } + + if ($token === '}') { + $depth--; + + if ($table !== null && $depth < $scope) { + $table = null; + } + + continue; + } + + if (! is_array($token)) { + continue; + } + + $opened = $this->schemaCall($tokens, $index); + + if ($opened !== null) { + $table = $opened[0]; + $scope = $depth + 1; + $entries[$table] ??= $opened[1]; + + continue; + } + + if ($table === null) { + continue; + } + + if ($token[0] !== T_OBJECT_OPERATOR) { + continue; + } + + foreach ($this->columns($tokens, $index) as $column) { + $entries[$table.'.'.$column] ??= $token[2]; + } + } + + return $entries; + } + + /** + * The table a `Schema::create('x', ...)` or `Schema::table('x', ...)` opens, + * with the line it sits on, or null if this is not one. + * + * The receiver has to be checked, not just the method name: `DB::table('x')` + * opens no schema scope at all -- it runs a query -- but shares the name + * `table` with the call that actually declares columns. Without this check a + * data-backfill migration's `DB::table('orders')` would open an `orders` + * scope of its own, and every `$var->method('literal')` after it in the same + * method body would register as a column of a table this migration never + * touched. + * + * A bare `Schema` is not the only way this receiver tokenizes, though: a + * fully-qualified call such as `\Illuminate\Support\Facades\Schema::create` + * tokenizes its receiver as a single T_NAME_FULLY_QUALIFIED token holding the + * whole path, and an aliased import can shorten or lengthen that path + * further, so both a fully- and a partially-qualified name are accepted as + * long as they end with `\Schema`. A name that merely contains the word, + * such as `MySchema`, is not accepted -- that is a different class that only + * happens to share a suffix. + * + * @param list $tokens + * @return array{0: string, 1: int}|null + */ + private function schemaCall(array $tokens, int $index): ?array + { + $token = $tokens[$index]; + + if (! is_array($token) || $token[0] !== T_DOUBLE_COLON) { + return null; + } + + $receiver = $tokens[$index - 1] ?? null; + + if (! is_array($receiver) || ! $this->isSchemaReceiver($receiver)) { + return null; + } + + $method = $tokens[$index + 1] ?? null; + + if (! is_array($method) || ! in_array($method[1], ['create', 'table'], true)) { + return null; + } + + $name = $tokens[$index + 3] ?? null; + + if (! is_array($name) || $name[0] !== T_CONSTANT_ENCAPSED_STRING) { + return null; + } + + return [trim($name[1], "'\""), $method[2]]; + } + + /** + * Whether a token naming the left side of a `::` is some spelling of the + * `Schema` class: a bare `T_STRING` for an unqualified reference, or a + * `T_NAME_FULLY_QUALIFIED` / `T_NAME_QUALIFIED` whose path ends with + * `\Schema` for a fully- or partially-qualified one. + * + * @param array{0: int, 1: string, 2: int} $receiver + */ + private function isSchemaReceiver(array $receiver): bool + { + if ($receiver[0] === T_STRING) { + return $receiver[1] === 'Schema'; + } + + if ($receiver[0] === T_NAME_FULLY_QUALIFIED || $receiver[0] === T_NAME_QUALIFIED) { + return str_ends_with($receiver[1], '\\Schema'); + } + + return false; + } + + /** + * The column names a `->method('x')` call creates, which is usually one and + * for a morphs pair is two. + * + * A chained call -- `$table->foreignId('customer_id')->constrained('users')` + * -- has an object operator too, and its receiver is `foreignId()`'s return + * value rather than the Blueprint. Treating 'users' as a column of orders + * would invent one the table does not have, so the receiver is checked + * before anything else here is even asked. The Blueprint's own parameter is + * conventionally named $table, but this accepts any variable rather than + * that one name specifically: matching a particular name would still be a + * guess, and this class declines rather than guesses. + * + * @param list $tokens + * @return list + */ + private function columns(array $tokens, int $index): array + { + if (! $this->hasVariableReceiver($tokens, $index)) { + return []; + } + + $method = $tokens[$index + 1] ?? null; + + if (! is_array($method) || $method[0] !== T_STRING) { + return []; + } + + $argument = $tokens[$index + 3] ?? null; + + if (! is_array($argument) || $argument[0] !== T_CONSTANT_ENCAPSED_STRING) { + return []; + } + + $name = trim($argument[1], "'\""); + + if (in_array($method[1], self::MORPHS, true)) { + return [$name.'_type', $name.'_id']; + } + + return [$name]; + } + + /** + * Whether the token immediately before an object operator -- skipping + * whitespace, since `$table ->foreignId('x')` is legal PHP -- is a + * variable. True for `$table->foreignId(...)`; false for the `)` a chained + * call's arrow actually follows. + * + * @param list $tokens + */ + private function hasVariableReceiver(array $tokens, int $index): bool + { + $previous = $index - 1; + + while ($previous >= 0 && is_array($tokens[$previous]) && $tokens[$previous][0] === T_WHITESPACE) { + $previous--; + } + + return $previous >= 0 && is_array($tokens[$previous]) && $tokens[$previous][0] === T_VARIABLE; + } +} diff --git a/src/Schema/SourceLocation.php b/src/Schema/SourceLocation.php new file mode 100644 index 0000000..d097977 --- /dev/null +++ b/src/Schema/SourceLocation.php @@ -0,0 +1,22 @@ +app->bind(SyntaxChecker::class, PhpLintChecker::class); + $this->app->bind(MigrationMap::class, function (Application $app): MigrationMap { + /** @var Repository $config */ + $config = $app->make(Repository::class); + $configured = $config->get('vacuum.lint.migrations_path'); + + return new MigrationMap( + is_string($configured) ? $configured : database_path('migrations'), + new MigrationScanner, + ); + }); + // Every panel wants to know what the server supports, and the answer // cannot change underneath a single request. $this->app->singleton( @@ -300,7 +313,7 @@ public function register(): void [ UnindexedForeignKey::class, ForeignKeyTypeMismatch::class, - Int4PrimaryKey::class, + NarrowPrimaryKey::class, MissingPrimaryKey::class, UnindexedMorphs::class, JsonNotJsonb::class, diff --git a/tests/Feature/Command/LintCommandTest.php b/tests/Feature/Command/LintCommandTest.php index 2e762a0..0d53a6d 100644 --- a/tests/Feature/Command/LintCommandTest.php +++ b/tests/Feature/Command/LintCommandTest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use Composer\InstalledVersions; use Heyosseus\Vacuum\Advisor\Advisor; use Heyosseus\Vacuum\Advisor\Finding; use Heyosseus\Vacuum\Advisor\Inspection; @@ -41,6 +42,53 @@ function schemaFinding(Severity $severity = Severity::Warning): Finding ); } +/** + * Runs the callback with Composer's InstalledVersions replaced by a single, + * fully controlled dataset, then restores exactly what was there before. + * + * InstalledVersions::reload() alone is not enough for this: it clears the + * per-vendor-directory cache but not the flag that decides whether the real + * registered class loaders are consulted at all, so this package's own + * vendor/composer/installed.php -- which lists heyosseus/vacuum whether or not + * it is the root, because in this checkout it genuinely is -- would still + * answer every lookup before the reloaded data got a turn. Forcing + * canGetVendors false is what actually isolates the test from the real + * installation this suite runs inside of. + * + * @param array{root: array{name: string, pretty_version: string, version: string, reference: string|null, type: string, install_path: string, aliases: array, dev: bool}, versions: array} $data + */ +function withInstalledVersions(array $data, Closure $callback): void +{ + $canGetVendors = new ReflectionProperty(InstalledVersions::class, 'canGetVendors'); + $installed = new ReflectionProperty(InstalledVersions::class, 'installed'); + $installedByVendor = new ReflectionProperty(InstalledVersions::class, 'installedByVendor'); + $installedIsLocalDir = new ReflectionProperty(InstalledVersions::class, 'installedIsLocalDir'); + + $original = [ + $canGetVendors->getValue(), + $installed->getValue(), + $installedByVendor->getValue(), + $installedIsLocalDir->getValue(), + ]; + + $canGetVendors->setValue(null, false); + InstalledVersions::reload($data); + + try { + $callback(); + } finally { + $canGetVendors->setValue(null, $original[0]); + $installed->setValue(null, $original[1]); + $installedByVendor->setValue(null, $original[2]); + $installedIsLocalDir->setValue(null, $original[3]); + } +} + +beforeEach(function (): void { + $this->originalStepSummary = getenv('GITHUB_STEP_SUMMARY'); + $this->stepSummaryFile = null; +}); + it('passes on a clean schema', function (): void { stubAdvisor(); @@ -97,17 +145,291 @@ function schemaFinding(Severity $severity = Severity::Warning): Finding ->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. +it('suppresses a finding listed in the baseline', function (): void { + stubAdvisor(schemaFinding()); + + $path = base_path('vacuum-baseline.json'); + file_put_contents($path, json_encode([ + 'findings' => ['unindexed-foreign-key' => ['public.orders.customer_id']], + ])); + + $exit = Artisan::call('vacuum:lint', ['--no-interaction' => true]); + + expect($exit)->toBe(0); +}); + +it('says how many it suppressed rather than quietly scoring around them', function (): void { + // A green number computed over findings it never mentioned is exactly the + // lie this package exists to argue against. + stubAdvisor(schemaFinding()); + + $path = base_path('vacuum-baseline.json'); + file_put_contents($path, json_encode([ + 'findings' => ['unindexed-foreign-key' => ['public.orders.customer_id']], + ])); + + Artisan::call('vacuum:lint', ['--no-interaction' => true]); + $output = Artisan::output(); + + expect($output)->toContain('1 suppressed by the baseline'); +}); + +it('ignores the baseline when told to', function (): void { + stubAdvisor(schemaFinding()); + + $path = base_path('vacuum-baseline.json'); + file_put_contents($path, json_encode([ + 'findings' => ['unindexed-foreign-key' => ['public.orders.customer_id']], + ])); + + $exit = Artisan::call('vacuum:lint', ['--no-baseline' => true, '--no-interaction' => true]); + + expect($exit)->toBe(1); +}); + +it('writes a baseline and exits zero even with findings outstanding', function (): void { + stubAdvisor(schemaFinding()); + + $path = base_path('vacuum-baseline.json'); + + $exit = Artisan::call('vacuum:lint', ['--generate-baseline' => true, '--no-interaction' => true]); + + $written = json_decode((string) file_get_contents($path), true); + + expect($exit)->toBe(0) + ->and($written['findings']['unindexed-foreign-key'])->toBe(['public.orders.customer_id']); +}); + +it('stamps the baseline with vacuum\'s own installed version, not the application\'s', function (): void { + stubAdvisor(schemaFinding()); + + $path = base_path('vacuum-baseline.json'); + + withInstalledVersions([ + 'root' => [ + 'name' => 'acme/app', + 'pretty_version' => 'v1.0.0', + 'version' => '1.0.0.0', + 'reference' => null, + 'type' => 'library', + 'install_path' => __DIR__, + 'aliases' => [], + 'dev' => true, + ], + 'versions' => [ + 'acme/app' => ['pretty_version' => 'v1.0.0', 'dev_requirement' => false], + 'heyosseus/vacuum' => ['pretty_version' => '1.2.3', 'dev_requirement' => false], + ], + ], function (): void { + Artisan::call('vacuum:lint', ['--generate-baseline' => true, '--no-interaction' => true]); + }); + + $written = json_decode((string) file_get_contents($path), true); + + expect($written['vacuum'])->toBe('1.2.3'); +}); + +it('falls back to the root package version when composer cannot report vacuum by name', function (): void { + // This is what makes this package's own test suite work: there vacuum is + // genuinely the root, and the root's version is the only one there is. + stubAdvisor(schemaFinding()); + + $path = base_path('vacuum-baseline.json'); + + withInstalledVersions([ + 'root' => [ + 'name' => 'heyosseus/vacuum', + 'pretty_version' => 'dev-feature', + 'version' => 'dev-feature', + 'reference' => null, + 'type' => 'library', + 'install_path' => __DIR__, + 'aliases' => [], + 'dev' => true, + ], + 'versions' => [ + // No heyosseus/vacuum entry: Composer has nothing to report by name. + ], + ], function (): void { + Artisan::call('vacuum:lint', ['--generate-baseline' => true, '--no-interaction' => true]); + }); + + $written = json_decode((string) file_get_contents($path), true); + + expect($written['vacuum'])->toBe('dev-feature'); +}); + +it('fails rather than claiming success when the baseline cannot be written', function (): void { + // The easiest trigger for an unwritable path: a --baseline= pointing into a + // directory that was never created. + stubAdvisor(schemaFinding()); + + $exit = Artisan::call('vacuum:lint', [ + '--generate-baseline' => true, + '--baseline' => 'no-such-directory/vacuum-baseline.json', + '--no-interaction' => true, + ]); + + expect($exit)->toBe(1) + ->and(Artisan::output())->toContain('Could not write the baseline to'); +}); + +it('reads a baseline from an explicit path', function (): void { + stubAdvisor(schemaFinding()); + + $path = base_path('custom-baseline.json'); + file_put_contents($path, json_encode([ + 'findings' => ['unindexed-foreign-key' => ['public.orders.customer_id']], + ])); + + $exit = Artisan::call('vacuum:lint', ['--baseline' => 'custom-baseline.json', '--no-interaction' => true]); + + expect($exit)->toBe(0); +}); + +it('reports a baseline entry that no longer matches, without failing the build', function (): void { stubAdvisor(); - $this->artisan('vacuum:lint', ['--generate-baseline' => true, '--no-interaction' => true]) - ->assertExitCode(2); + $path = base_path('vacuum-baseline.json'); + file_put_contents($path, json_encode([ + 'findings' => ['unindexed-foreign-key' => ['public.orders.long_since_fixed']], + ])); - $this->artisan('vacuum:lint', ['--no-baseline' => true, '--no-interaction' => true]) - ->assertExitCode(2); + $exit = Artisan::call('vacuum:lint', ['--no-interaction' => true]); + $output = Artisan::output(); - $this->artisan('vacuum:lint', ['--baseline' => 'baseline.json', '--no-interaction' => true]) - ->assertExitCode(2); + expect($exit)->toBe(0)->and($output)->toContain('baseline-stale'); +}); + +it('does not fail the build on a stale entry even at --fail-on=info', function (): void { + // Stale entries are Info because the defect being gone is good news, and + // good news must never be able to redden a build. Before this fix they were + // fed to the same bar as everything else, so fixing something could fail + // the build the moment somebody set --fail-on=info. + stubAdvisor(); + + $path = base_path('vacuum-baseline.json'); + file_put_contents($path, json_encode([ + 'findings' => ['unindexed-foreign-key' => ['public.orders.long_since_fixed']], + ])); + + $exit = Artisan::call('vacuum:lint', ['--fail-on' => 'info', '--no-interaction' => true]); + + expect($exit)->toBe(0); +}); + +it('counts suppressed findings in the json document', function (): void { + stubAdvisor(schemaFinding()); + + $path = base_path('vacuum-baseline.json'); + file_put_contents($path, json_encode([ + 'findings' => ['unindexed-foreign-key' => ['public.orders.customer_id']], + ])); + + Artisan::call('vacuum:lint', ['--format' => 'json', '--no-interaction' => true]); + $document = json_decode(Artisan::output(), true); + + expect($document['suppressed'])->toBe(1)->and($document['findings'])->toBe([]); +}); + +it('emits github annotations when asked for them', function (): void { + stubAdvisor(schemaFinding()); + + Artisan::call('vacuum:lint', ['--format' => 'github', '--no-interaction' => true]); + + // Read once: Artisan::output() flushes the buffer. + $output = Artisan::output(); + + expect($output)->toContain('::warning ')->and($output)->not->toContain('/ 100'); +}); + +it('reports how many findings the baseline suppressed as a notice', function (): void { + // The pull request is the one place four hundred silently-suppressed + // findings matter most, so this format must not stay quiet about the count + // the way the original implementation did. + stubAdvisor(schemaFinding()); + + $path = base_path('vacuum-baseline.json'); + file_put_contents($path, json_encode([ + 'findings' => ['unindexed-foreign-key' => ['public.orders.customer_id']], + ])); + + Artisan::call('vacuum:lint', ['--format' => 'github', '--no-interaction' => true]); + + expect(Artisan::output())->toContain('::notice::1 suppressed by the baseline.'); +}); + +it('appends a summary table when the runner offers one', function (): void { + stubAdvisor(schemaFinding()); + + $summary = tempnam(sys_get_temp_dir(), 'vacuum-summary'); + $this->stepSummaryFile = $summary; + putenv('GITHUB_STEP_SUMMARY='.$summary); + + Artisan::call('vacuum:lint', ['--format' => 'github', '--no-interaction' => true]); + + $written = (string) file_get_contents($summary); + + expect($written)->toContain('| Severity | Rule | Subject |'); +}); + +it('warns without failing the command when the step summary cannot be written', function (): void { + // Unlike the baseline, losing the step summary loses nothing the caller + // depended on -- every finding is already in the annotations above it -- so + // this must not be mistaken for having worked, but it also must not fail + // the build over a convenience the runner merely offers. + stubAdvisor(); + + $directory = sys_get_temp_dir().'/vacuum-step-summary-'.bin2hex((string) getmypid()); + putenv('GITHUB_STEP_SUMMARY='.$directory.'/no-such-directory/summary.md'); + + $exit = Artisan::call('vacuum:lint', ['--format' => 'github', '--no-interaction' => true]); + + expect($exit)->toBe(0) + ->and(Artisan::output())->toContain('Could not append the step summary to'); +}); + +it('writes no step summary when the runner does not offer one', function (): void { + // This has to unset the variable rather than rely on it being absent. On a + // developer's machine it is unset anyway, so every other github-format test + // covers this branch by accident; inside Actions the runner always sets it, + // and the branch would be covered locally and dead in CI. + stubAdvisor(schemaFinding()); + + putenv('GITHUB_STEP_SUMMARY'); + + $exit = Artisan::call('vacuum:lint', ['--format' => 'github', '--no-interaction' => true]); + + expect($exit)->toBe(1) + ->and(Artisan::output())->toContain('::warning '); +}); + +afterEach(function (): void { + // A leaked baseline file would silently suppress findings in every later + // test in the suite, so cleanup here does not depend on a preceding test + // actually reaching its own unlink() call. + foreach (['vacuum-baseline.json', 'custom-baseline.json'] as $file) { + $path = base_path($file); + + if (is_file($path)) { + unlink($path); + } + } + + // Only the file this file's own tests created, and only if one was made -- + // GITHUB_STEP_SUMMARY is set by the real runner to a file the job owns + // inside this project's own GitHub Actions run, and deleting whatever it + // currently points at would delete that runner's summary out from under it. + if (is_string($this->stepSummaryFile) && is_file($this->stepSummaryFile)) { + unlink($this->stepSummaryFile); + } + + // Restored to whatever it was before this file's tests ran, rather than + // cleared unconditionally, for the same reason: outside this suite the + // variable is real and belongs to the runner, not to this test file. + if ($this->originalStepSummary === false) { + putenv('GITHUB_STEP_SUMMARY'); + } else { + putenv('GITHUB_STEP_SUMMARY='.$this->originalStepSummary); + } }); diff --git a/tests/Unit/Advisor/Rules/ForeignKeyTypeMismatchTest.php b/tests/Unit/Advisor/Rules/ForeignKeyTypeMismatchTest.php index 2bcea23..7b10e49 100644 --- a/tests/Unit/Advisor/Rules/ForeignKeyTypeMismatchTest.php +++ b/tests/Unit/Advisor/Rules/ForeignKeyTypeMismatchTest.php @@ -66,7 +66,7 @@ function typedKey(array $here, array $there, string $column = 'customer_id'): Ta // 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 + // ceiling narrow-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'])); diff --git a/tests/Unit/Advisor/Rules/Int4PrimaryKeyTest.php b/tests/Unit/Advisor/Rules/NarrowPrimaryKeyTest.php similarity index 75% rename from tests/Unit/Advisor/Rules/Int4PrimaryKeyTest.php rename to tests/Unit/Advisor/Rules/NarrowPrimaryKeyTest.php index 5c79bb8..e5fe88b 100644 --- a/tests/Unit/Advisor/Rules/Int4PrimaryKeyTest.php +++ b/tests/Unit/Advisor/Rules/NarrowPrimaryKeyTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -use Heyosseus\Vacuum\Advisor\Rules\Int4PrimaryKey; +use Heyosseus\Vacuum\Advisor\Rules\NarrowPrimaryKey; use Heyosseus\Vacuum\Advisor\Severity; use Heyosseus\Vacuum\Values\Column; use Heyosseus\Vacuum\Values\Constraint; @@ -25,36 +25,36 @@ function keyed(string $type, array $keyColumns = ['id']): TableSchema } it('reports a primary key on integer', function (): void { - $findings = app(Int4PrimaryKey::class)->inspect(keyed('integer')); + $findings = app(NarrowPrimaryKey::class)->inspect(keyed('integer')); expect($findings)->toHaveCount(1) - ->and($findings[0]->rule)->toBe('int4-primary-key') + ->and($findings[0]->rule)->toBe('narrow-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); + expect(app(NarrowPrimaryKey::class)->inspect(keyed('smallint')))->toHaveCount(1); }); it('says nothing about a bigint key', function (): void { - expect(app(Int4PrimaryKey::class)->inspect(keyed('bigint')))->toBe([]); + expect(app(NarrowPrimaryKey::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([]); + expect(app(NarrowPrimaryKey::class)->inspect(keyed('uuid')))->toBe([]) + ->and(app(NarrowPrimaryKey::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([]); + expect(app(NarrowPrimaryKey::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); + expect(app(NarrowPrimaryKey::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 { @@ -65,7 +65,7 @@ function keyed(string $type, array $keyColumns = ['id']): TableSchema ), ], []); - expect(app(Int4PrimaryKey::class)->inspect($orphan))->toBe([]); + expect(app(NarrowPrimaryKey::class)->inspect($orphan))->toBe([]); }); it('says nothing about Laravel\'s own migrations table', function (): void { @@ -83,11 +83,11 @@ function keyed(string $type, array $keyColumns = ['id']): TableSchema ), ], []); - expect(app(Int4PrimaryKey::class)->inspect($migrations))->toBe([]); + expect(app(NarrowPrimaryKey::class)->inspect($migrations))->toBe([]); }); it('offers the widening, and says it rewrites the table', function (): void { - $findings = app(Int4PrimaryKey::class)->inspect(keyed('integer')); + $findings = app(NarrowPrimaryKey::class)->inspect(keyed('integer')); expect($findings[0]->remediation) ->toBe('ALTER TABLE "public"."orders" ALTER COLUMN "id" TYPE bigint;') diff --git a/tests/Unit/Console/BaselineTest.php b/tests/Unit/Console/BaselineTest.php new file mode 100644 index 0000000..5a60841 --- /dev/null +++ b/tests/Unit/Console/BaselineTest.php @@ -0,0 +1,124 @@ +suppresses(baselined()))->toBeFalse(); +}); + +it('suppresses a finding whose rule and subject both appear', function (): void { + $baseline = Baseline::record([baselined()]); + + expect($baseline->suppresses(baselined()))->toBeTrue(); +}); + +it('does not suppress the same subject under a different rule', function (): void { + // Matching on the pair is the whole point: one table can be wrong in two + // unrelated ways and silencing one must not silence the other. + $baseline = Baseline::record([baselined()]); + + expect($baseline->suppresses(baselined(rule: 'json-not-jsonb')))->toBeFalse(); +}); + +it('does not suppress a different subject under the same rule', function (): void { + $baseline = Baseline::record([baselined()]); + + expect($baseline->suppresses(baselined(subject: 'public.orders.warehouse_id')))->toBeFalse(); +}); + +it('ignores everything about a finding except its rule and subject', function (): void { + // Rewording a rule's prose, or raising its severity in a later release, must + // never invalidate a baseline somebody committed months ago. + $baseline = Baseline::record([baselined()]); + + $reworded = new Finding( + rule: 'unindexed-foreign-key', + subject: 'public.orders.customer_id', + severity: Severity::Critical, + summary: 'completely different words', + impact: 'completely different words', + remediation: 'CREATE INDEX something;', + ); + + expect($baseline->suppresses($reworded))->toBeTrue(); +}); + +it('encodes subjects sorted, so the file has no spurious diffs', function (): void { + $baseline = Baseline::record([ + baselined(subject: 'public.orders.warehouse_id'), + baselined(subject: 'public.orders.customer_id'), + ]); + + $document = json_decode($baseline->encode('1.2.0'), true); + + expect($document['findings']['unindexed-foreign-key']) + ->toBe(['public.orders.customer_id', 'public.orders.warehouse_id']) + ->and($document['vacuum'])->toBe('1.2.0') + ->and($document)->toHaveKey('generated_at'); +}); + +it('round-trips through encode and decode', function (): void { + $baseline = Baseline::record([baselined()]); + + expect(Baseline::decode($baseline->encode('1.2.0'))->suppresses(baselined()))->toBeTrue(); +}); + +it('treats a malformed file as no baseline rather than crashing a build', function (): void { + // A corrupt baseline should not take the pipeline down with it; reporting + // everything is the safe direction to fail in. + expect(Baseline::decode('not json at all')->suppresses(baselined()))->toBeFalse() + ->and(Baseline::decode('{"findings": "wrong shape"}')->suppresses(baselined()))->toBeFalse() + ->and(Baseline::decode('[]')->suppresses(baselined()))->toBeFalse() + ->and(Baseline::decode('"just a string"')->suppresses(baselined()))->toBeFalse(); +}); + +it('skips a malformed entry instead of discarding the whole baseline', function (): void { + // json_decode(..., true) turns a numeric-looking JSON key into an int array + // key, so a rule of "0" is indistinguishable from list noise; and a rule + // whose subjects are not a list is just as unusable. Either is dropped on + // its own, rather than a single bad entry invalidating every entry beside it. + $baseline = Baseline::decode( + '{"findings": {' + .'"0": ["public.orders.customer_id"], ' + .'"json-not-jsonb": "public.orders.payload", ' + .'"unindexed-foreign-key": ["public.orders.warehouse_id"]' + .'}}' + ); + + expect($baseline->suppresses(baselined(subject: 'public.orders.warehouse_id')))->toBeTrue() + ->and($baseline->suppresses(baselined()))->toBeFalse() + ->and($baseline->suppresses(baselined(rule: 'json-not-jsonb', subject: 'public.orders.payload')))->toBeFalse(); +}); + +it('reports an entry that no longer matches anything', function (): void { + // A baseline nobody prunes becomes a place defects hide. + $baseline = Baseline::record([baselined(), baselined(subject: 'public.orders.gone')]); + + $stale = $baseline->stale([baselined()]); + + expect($stale)->toHaveCount(1) + ->and($stale[0]->rule)->toBe('baseline-stale') + ->and($stale[0]->severity)->toBe(Severity::Info) + ->and($stale[0]->subject)->toBe('public.orders.gone') + ->and($stale[0]->summary)->toContain('unindexed-foreign-key'); +}); + +it('reports nothing stale when every entry still matches', function (): void { + expect(Baseline::record([baselined()])->stale([baselined()]))->toBe([]); +}); diff --git a/tests/Unit/Console/GithubReporterTest.php b/tests/Unit/Console/GithubReporterTest.php new file mode 100644 index 0000000..1ae07cc --- /dev/null +++ b/tests/Unit/Console/GithubReporterTest.php @@ -0,0 +1,179 @@ +report($output, $findings, $suppressed); + + return $buffer->fetch(); +} + +function annotatable( + Severity $severity = Severity::Warning, + string $summary = 'No index behind the foreign key.', + ?string $remediation = null, + string $subject = 'public.orders.customer_id', +): Finding { + return new Finding( + rule: 'unindexed-foreign-key', + subject: $subject, + severity: $severity, + summary: $summary, + impact: 'stub', + remediation: $remediation, + table: 'public.orders', + ); +} + +it('anchors an annotation to the migration that introduced it', function (): void { + expect(annotated([annotatable()])) + ->toContain('::warning file=') + ->and(annotated([annotatable()]))->toContain('create_orders_table') + ->and(annotated([annotatable()]))->toContain('line=15'); +}); + +it('maps severity onto the three levels GitHub understands', function (): void { + expect(annotated([annotatable(Severity::Critical)]))->toStartWith('::error ') + ->and(annotated([annotatable(Severity::Warning)]))->toStartWith('::warning ') + ->and(annotated([annotatable(Severity::Info)]))->toStartWith('::notice ') + ->and(annotated([annotatable(Severity::Unknown)]))->toStartWith('::notice '); +}); + +it('emits an annotation with no anchor when the map cannot place it', function (): void { + // GitHub attaches an unanchored annotation to the workflow rather than + // dropping it, so a finding the map cannot place is still seen. + $orphan = new Finding( + rule: 'missing-primary-key', + subject: 'public.nowhere', + severity: Severity::Warning, + summary: 'stub', + impact: 'stub', + table: 'public.nowhere', + ); + + expect(annotated([$orphan]))->toContain('::warning title=') + ->and(annotated([$orphan]))->not->toContain('file='); +}); + +it('escapes the characters that would otherwise break the command', function (): void { + // A remediation is multi-line SQL and a summary can contain a comma, so an + // unescaped implementation mangles exactly the findings people most want to + // read. + $awkward = annotatable( + summary: "100% wrong, on two lines\nlike this", + remediation: "CREATE INDEX a;\nCREATE INDEX b;", + ); + + $line = annotated([$awkward]); + + expect($line)->toContain('100%25 wrong') + ->and($line)->toContain('%0A') + ->and(substr_count(trim($line), "\n"))->toBe(0); +}); + +it('escapes a colon inside a property value', function (): void { + // A colon reaches a property value only through the migration file's absolute + // path, and only a Windows path begins "C:\\". Left unescaped it terminates the + // property and swallows the rest of the annotation. + $line = annotated([annotatable()]); + + expect($line)->not->toMatch('/file=[A-Za-z]:/') + ->and($line)->toContain('%3A'); +})->skipOnLinux()->skipOnMac(); + +it('renders a markdown table for the step summary', function (): void { + $summary = (new GithubReporter( + new MigrationMap(__DIR__.'/../../fixtures/migrations', new MigrationScanner) + ))->summary([annotatable()]); + + expect($summary)->toContain('| Severity | Rule | Subject |') + ->and($summary)->toContain('unindexed-foreign-key') + ->and($summary)->toContain('public.orders.customer_id'); +}); + +it('anchors the annotation with a path relative to the repository root', function (): void { + // GitHub matches an annotation's `file` against the diff by resolving it + // against the repository root, not the runner's filesystem. The map's own + // paths are absolute -- correct for every other consumer -- so this has to be + // fixed on the way out, and it has to hold on Windows, where an absolute path + // carries a drive letter and backslashes, exactly as it does on POSIX. + $directory = base_path('database/migrations'); + $created = ! is_dir($directory); + + if ($created) { + mkdir($directory, 0o777, true); + } + + $file = $directory.'/2024_01_05_000000_create_relative_path_table.php'; + file_put_contents($file, <<<'PHP' + id(); + }); + } + }; + PHP); + + try { + $map = new MigrationMap($directory, new MigrationScanner); + $buffer = new BufferedOutput; + $output = new OutputStyle(new ArrayInput([]), $buffer); + + $finding = new Finding( + rule: 'missing-primary-key', + subject: 'public.relative_path', + severity: Severity::Warning, + summary: 'stub', + impact: 'stub', + table: 'public.relative_path', + ); + + (new GithubReporter($map))->report($output, [$finding]); + $line = $buffer->fetch(); + + expect($line)->toContain('file=database/migrations/') + ->and($line)->not->toMatch('#file=/#') + ->and($line)->not->toMatch('#file=[A-Za-z]:#'); + } finally { + unlink($file); + + if ($created) { + rmdir($directory); + } + } +}); + +it('reports how many findings the baseline suppressed', function (): void { + // The pull request is the one place four hundred silently-suppressed findings + // matter most, so this is the one format that must not stay quiet about the + // count the way the original implementation did. + expect(annotated([annotatable()], suppressed: 3)) + ->toContain('::notice::3 suppressed by the baseline.'); +}); + +it('says nothing about suppression when nothing was suppressed', function (): void { + expect(annotated([annotatable()]))->not->toContain('suppressed by the baseline'); +}); diff --git a/tests/Unit/Schema/MigrationMapTest.php b/tests/Unit/Schema/MigrationMapTest.php new file mode 100644 index 0000000..6114261 --- /dev/null +++ b/tests/Unit/Schema/MigrationMapTest.php @@ -0,0 +1,140 @@ +locate(located('public.orders.customer_id')); + + expect($location?->line)->toBe(15) + ->and($location?->file)->toContain('create_orders_table'); +}); + +it('falls back to the table when the column is not in any migration', function (): void { + // A column added by a raw DB::statement is still on a table the map knows. + $location = mapped()->locate(located('public.orders.added_by_hand')); + + expect($location?->line)->toBe(13); +}); + +it('places a table-level finding on the create call', function (): void { + expect(mapped()->locate(located('public.orders'))?->line)->toBe(13); +}); + +it('returns nothing for a table no migration declares', function (): void { + expect(mapped()->locate(located('public.nowhere.column', table: 'public.nowhere')))->toBeNull(); +}); + +it('returns nothing for a finding that is about no table', function (): void { + expect(mapped()->locate(located('SchemaInspection', table: null)))->toBeNull(); +}); + +it('places nothing from a migration whose table name is a variable', function (): void { + expect(mapped()->locate(located('public.invisible.hidden_id', table: 'public.invisible')))->toBeNull(); +}); + +it('survives a directory that does not exist', function (): void { + $map = new MigrationMap(__DIR__.'/no-such-directory', new MigrationScanner); + + expect($map->locate(located('public.orders.customer_id')))->toBeNull(); +}); + +it('scans the directory once and reuses the result on a second lookup', function (): void { + $map = mapped(); + + $map->locate(located('public.orders')); + $second = $map->locate(located('public.orders.customer_id')); + + expect($second?->line)->toBe(15); +}); + +it('falls back to the unqualified entry when the scanner never recorded a schema', function (): void { + // Migrations do not ordinarily write a schema -- Schema::create('orders', ...), + // not Schema::create('tenant.orders', ...) -- so this is the everyday case for + // any schema other than the default one the fixture happens to use: the entry + // the scanner recorded is bare, and the qualified lookup has to miss before + // the unqualified one gets a chance to hit. + $location = mapped()->locate(located('tenant.orders.customer_id', table: 'tenant.orders')); + + expect($location?->line)->toBe(15) + ->and($location?->file)->toContain('create_orders_table'); +}); + +it('prefers a schema-qualified entry over an unqualified one from a different schema', function (): void { + // A multi-schema application can write the schema directly -- + // Schema::create('tenant.orders', ...) -- and when it does, that entry has to + // win the collision it would otherwise lose to public.orders's own migration: + // reducing straight to the unqualified key is exactly the bug this fixes. + $directory = sys_get_temp_dir().'/vacuum-migration-map-schema-'.bin2hex((string) getmypid()); + mkdir($directory, recursive: true); + $file = $directory.'/2024_01_03_000000_create_tenant_orders_table.php'; + + file_put_contents($file, <<<'PHP' + id(); + $table->foreignId('customer_id'); + }); + } + }; + PHP); + + try { + $map = new MigrationMap($directory, new MigrationScanner); + $location = $map->locate(located('tenant.orders.customer_id', table: 'tenant.orders')); + + expect($location?->file)->toContain('create_tenant_orders_table') + ->and($location?->line)->toBe(13); + } finally { + unlink($file); + rmdir($directory); + } +}); + +it('skips a glob match it cannot read as a file', function (): void { + // A directory whose name happens to end in .php still matches the glob, and + // file_get_contents on it fails the way an unreadable file would. Built under + // the system temp directory rather than as a fixture, because an empty + // directory is invisible to git and would not survive a checkout. + $directory = sys_get_temp_dir().'/vacuum-migration-map-'.bin2hex((string) getmypid()); + mkdir($directory.'/unreadable.php', recursive: true); + + try { + $map = new MigrationMap($directory, new MigrationScanner); + + expect($map->locate(located('public.orders.customer_id')))->toBeNull(); + } finally { + rmdir($directory.'/unreadable.php'); + rmdir($directory); + } +}); diff --git a/tests/Unit/Schema/MigrationScannerTest.php b/tests/Unit/Schema/MigrationScannerTest.php new file mode 100644 index 0000000..89b2cfd --- /dev/null +++ b/tests/Unit/Schema/MigrationScannerTest.php @@ -0,0 +1,289 @@ +scan("id(); + $table->foreignId('customer_id'); + }); + } +PHP); + + expect($entries)->toHaveKeys(['orders', 'orders.customer_id']); +}); + +it('records the line each declaration sits on', function (): void { + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::create('orders', function (Blueprint $table) { + $table->foreignId('customer_id'); + }); + } +PHP); + + // The fixture prepends four lines, so the column lands on line 8. + expect($entries['orders.customer_id'])->toBe(8); +}); + +it('expands a morphs pair into the two columns it actually creates', function (): void { + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::create('comments', function (Blueprint $table) { + $table->morphs('commentable'); + $table->nullableMorphs('authorable'); + }); + } +PHP); + + expect($entries)->toHaveKeys([ + 'comments.commentable_type', + 'comments.commentable_id', + 'comments.authorable_type', + 'comments.authorable_id', + ]); +}); + +it('refuses a table name it cannot read', function (): void { + // A variable table name is not something the tokenizer can resolve, and an + // anchor pointing at the wrong file is worse than no anchor at all. + expect(scan(<<<'PHP' + public function up(): void + { + Schema::create($name, function (Blueprint $table) { + $table->foreignId('customer_id'); + }); + } +PHP))->toBe([]); +}); + +it('leaves the table scope at the closing brace', function (): void { + // A column declared after the closure has ended belongs to no table. + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::create('orders', function (Blueprint $table) { + $table->foreignId('customer_id'); + }); + + $other->string('stray'); + } +PHP); + + expect($entries)->not->toHaveKey('orders.stray'); +}); + +it('reads Schema::table as well as Schema::create', function (): void { + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::table('orders', function (Blueprint $table) { + $table->string('note'); + }); + } +PHP); + + expect($entries)->toHaveKey('orders.note'); +}); + +it('keeps the first declaration when a column appears twice', function (): void { + // The first is where the column was written, which is where the defect was + // introduced; a later ->index() call is not. + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::create('orders', function (Blueprint $table) { + $table->foreignId('customer_id'); + }); + + Schema::table('orders', function (Blueprint $table) { + $table->foreignId('customer_id'); + }); + } +PHP); + + expect($entries['orders.customer_id'])->toBe(8); +}); + +it('ignores a method call whose first argument is not a literal', function (): void { + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::create('orders', function (Blueprint $table) { + $table->foreignId($column); + }); + } +PHP); + + expect($entries)->toBe(['orders' => 7]); +}); + +it('does not mistake a chained call for another column', function (): void { + // The receiver of ->constrained() is foreignId()'s return value, not the + // Blueprint, so 'users' is a referenced table and not a column of this one. + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::create('orders', function (Blueprint $table) { + $table->foreignId('customer_id')->constrained('users'); + }); + } +PHP); + + expect($entries)->toHaveKey('orders.customer_id') + ->and($entries)->not->toHaveKey('orders.users'); +}); + +it('reads a column declared with whitespace before the arrow', function (): void { + // Whitespace is a token, so the receiver check has to walk past it. + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::create('orders', function (Blueprint $table) { + $table + ->foreignId('customer_id'); + }); + } +PHP); + + expect($entries)->toHaveKey('orders.customer_id'); +}); + +it('ignores a Schema call that is neither create nor table', function (): void { + $entries = scan(<<<'PHP' + public function up(): void + { + if (Schema::hasTable('orders')) { + // + } + } +PHP); + + expect($entries)->toBe([]); +}); + +it('ignores a call whose method name is not itself a literal', function (): void { + // $table->$method('x') calls through a variable method name, which the + // tokenizer sees as a T_VARIABLE rather than the T_STRING a real column + // declaration would be. + $entries = scan(<<<'PHP' + public function up(): void + { + Schema::create('orders', function (Blueprint $table) { + $table->$method('customer_id'); + }); + } +PHP); + + expect($entries)->toBe(['orders' => 7]); +}); + +it('opens no table scope for DB::table, which runs a query rather than declaring a schema', function (): void { + // DB::table('orders') and Schema::table('orders') share a method name, but + // only one of them is a schema declaration. Without checking the receiver, a + // data-backfill migration's DB::table('orders') would open an 'orders' scope + // of its own, and every ->method('literal') that followed it in the same + // method body -- however unrelated -- would register as a column orders + // never actually gained. + $entries = scan(<<<'PHP' + public function up(): void + { + DB::table('orders')->where('id', 1)->update(['note' => 'x']); + } +PHP); + + expect($entries)->toBe([]); +}); + +it('still reads Schema::table when a DB::table call precedes it', function (): void { + $entries = scan(<<<'PHP' + public function up(): void + { + DB::table('orders')->where('id', 1)->update(['note' => 'x']); + + Schema::table('orders', function (Blueprint $table) { + $table->string('note'); + }); + } +PHP); + + expect($entries)->toHaveKey('orders.note'); +}); + +it('ignores a static call through a variable, whose receiver is not a literal class name', function (): void { + // $model::create(...) is a static call through a variable -- legal PHP -- and + // its receiver tokenizes as T_VARIABLE rather than the T_STRING a literal + // `Schema::` reference would be. + $entries = scan(<<<'PHP' + public function up(): void + { + $model::create('orders', function (Blueprint $table) { + $table->foreignId('customer_id'); + }); + } +PHP); + + expect($entries)->toBe([]); +}); + +it('reads a fully-qualified Schema::create call', function (): void { + // A fully-qualified reference tokenizes its receiver as a single + // T_NAME_FULLY_QUALIFIED token holding the whole path, not the T_STRING a + // bare `Schema` produces, and real migrations do call it fully qualified. + $entries = scan(<<<'PHP' + public function up(): void + { + \Illuminate\Support\Facades\Schema::create('orders', function (Blueprint $table) { + $table->foreignId('customer_id'); + }); + } +PHP); + + expect($entries)->toHaveKeys(['orders', 'orders.customer_id']); +}); + +it('ignores a receiver that merely contains the word Schema', function (): void { + // MySchema::create shares a suffix with Schema::create but is a different + // class entirely; matching on "ends with" rather than "equals" must not be + // fooled by that. + $entries = scan(<<<'PHP' + public function up(): void + { + MySchema::create('orders', function (Blueprint $table) { + $table->foreignId('customer_id'); + }); + } +PHP); + + expect($entries)->toBe([]); +}); + +it('returns nothing for a migration that does not parse as PHP at all', function (): void { + // TOKEN_PARSE is what makes this true: without it the tokenizer reads broken + // source leniently and hands back tokens anyway, which is exactly the silent + // best guess this class exists to refuse. Built directly rather than through + // scan()'s helper above, which always wraps the body in a syntactically valid + // class -- the whole point here is a file that never closes. + expect((new MigrationScanner)->scan('toBe([]); +}); + +it('returns nothing for a migration that parses but does not compile', function (): void { + // This parses fine -- it is only illegal once PHP tries to compile it, which + // TOKEN_PARSE raises as a bare CompileError rather than the ParseError a + // syntax error would raise. ParseError extends CompileError, so catching the + // parent has to be what scan() does, or a file like this would escape as an + // uncaught fatal instead of yielding nothing. + expect((new MigrationScanner)->scan('toBe([]); +}); diff --git a/tests/Unit/ServiceProviderTest.php b/tests/Unit/ServiceProviderTest.php index 6bf0f8b..aff0306 100644 --- a/tests/Unit/ServiceProviderTest.php +++ b/tests/Unit/ServiceProviderTest.php @@ -2,9 +2,12 @@ declare(strict_types=1); +use Heyosseus\Vacuum\Advisor\Finding; use Heyosseus\Vacuum\Advisor\Inspections\ConfigurationInspection; use Heyosseus\Vacuum\Advisor\Inspections\SettingInspection; use Heyosseus\Vacuum\Advisor\Inspections\TableInspection; +use Heyosseus\Vacuum\Advisor\Severity; +use Heyosseus\Vacuum\Schema\MigrationMap; use Heyosseus\Vacuum\VacuumServiceProvider; it('merges the package configuration into the application', function (): void { @@ -33,3 +36,33 @@ ->and($inspections)->toContain(SettingInspection::class) ->and($inspections)->toContain(ConfigurationInspection::class); }); + +it('points the migration map at database_path(migrations) when none is configured', function (): void { + $finding = new Finding( + rule: 'unindexed-foreign-key', + subject: 'public.orders.customer_id', + severity: Severity::Warning, + summary: 'stub', + impact: 'stub', + table: 'public.orders', + ); + + // The default application has no such migration, so this proves the map + // was built at all -- not what it finds. + expect(app(MigrationMap::class)->locate($finding))->toBeNull(); +}); + +it('points the migration map at the configured migrations path', function (): void { + config(['vacuum.lint.migrations_path' => __DIR__.'/../fixtures/migrations']); + + $finding = new Finding( + rule: 'unindexed-foreign-key', + subject: 'public.orders.customer_id', + severity: Severity::Warning, + summary: 'stub', + impact: 'stub', + table: 'public.orders', + ); + + expect(app(MigrationMap::class)->locate($finding)?->line)->toBe(15); +}); diff --git a/tests/fixtures/migrations/2024_01_01_000000_create_orders_table.php b/tests/fixtures/migrations/2024_01_01_000000_create_orders_table.php new file mode 100644 index 0000000..ecb4fcd --- /dev/null +++ b/tests/fixtures/migrations/2024_01_01_000000_create_orders_table.php @@ -0,0 +1,19 @@ +id(); + $table->foreignId('customer_id'); + $table->json('payload'); + }); + } +}; diff --git a/tests/fixtures/migrations/2024_01_02_000000_create_dynamic_table.php b/tests/fixtures/migrations/2024_01_02_000000_create_dynamic_table.php new file mode 100644 index 0000000..e6a51af --- /dev/null +++ b/tests/fixtures/migrations/2024_01_02_000000_create_dynamic_table.php @@ -0,0 +1,19 @@ +foreignId('hidden_id'); + }); + } +};