diff --git a/src/Checkup.php b/src/Checkup.php index 130604f..73905f0 100644 --- a/src/Checkup.php +++ b/src/Checkup.php @@ -7,10 +7,12 @@ use Difflock\Contracts\MigrationAnalyzer; use Difflock\Contracts\SchemaDiffer; use Difflock\Contracts\SchemaInspector; +use Difflock\Database\DatabaseContextFactory; use Difflock\Diff\SchemaDiff; use Difflock\Migration\MigrationScope; use Difflock\Risk\RiskLevel; use Difflock\Schema\Baseline; +use Difflock\Schema\DatabaseSchema; use Throwable; /** @@ -28,6 +30,7 @@ public function __construct( private SchemaDiffer $differ, private MigrationAnalyzer $analyzer, private Baseline $baseline, + private ?DatabaseContextFactory $contexts = null, ) {} public function run(RiskLevel $threshold, ?string $connection = null): CheckupResult @@ -62,9 +65,31 @@ private function drift(?string $connection): array } try { - return [$this->differ->diff($this->baseline->read(), $this->inspector->inspect($connection)), null]; + return [$this->differ->diff($this->baseline->read(), $this->live($connection)), null]; } catch (Throwable $exception) { return [null, $exception->getMessage()]; } } + + /** + * The live schema, reusing the one the rules are about to be given. + * + * A check reads the schema for drift and the analyzer reads it again to give the + * rules their context. Reading it twice is not free — on a 99-table PostgreSQL + * database it measured 598 queries and 3.7 seconds, half of it repeated — so the + * two share one reading, held by the context factory for exactly the length of + * this run and no longer. + * + * An explicit `--connection` is the one case that cannot share: the factory is + * built around the configured connection, and inspecting a different one is a + * different question. That path reads for itself. + */ + private function live(?string $connection): DatabaseSchema + { + if ($connection !== null || ! $this->contexts instanceof DatabaseContextFactory) { + return $this->inspector->inspect($connection); + } + + return $this->contexts->make()->schema; + } } diff --git a/src/Console/Commands/DifflockCommand.php b/src/Console/Commands/DifflockCommand.php index 692f002..e569c5c 100644 --- a/src/Console/Commands/DifflockCommand.php +++ b/src/Console/Commands/DifflockCommand.php @@ -84,7 +84,7 @@ public function handle(Checkup $checkup, Repository $config, CheckupRenderer $re Banner::render($this->output); - $renderer->render($this->output, $result); + $renderer->overview($this->output, $result); foreach (Text::wrap( 'difflock:diff compares schemas · difflock:lint analyses migrations · ' diff --git a/src/Console/Commands/DoctorCommand.php b/src/Console/Commands/DoctorCommand.php new file mode 100644 index 0000000..3a37628 --- /dev/null +++ b/src/Console/Commands/DoctorCommand.php @@ -0,0 +1,256 @@ +setHelp(<<<'HELP' + Prints the ground every other command stands on: which connection is being + inspected, what engine and version answered, whether the role Difflock + connects as is able to write, how many tables and migrations it can see, + which rules are registered, and where the baseline and accepted-findings + files live. + + The write-privilege line is the one worth reading. Difflock has no code path + that writes to the inspected database — but a read-only role is the version of + that promise which does not depend on trusting the code, and this says + whether you have one. + + Exit codes: 0 everything answered, 2 the database could not be reached. + HELP); + } + + public function handle( + Repository $config, + ConnectionResolverInterface $connections, + DatabaseContextFactory $contexts, + MigrationLocator $locator, + RuleRegistry $rules, + Baseline $baseline, + AcceptedFindings $accepted, + ): int { + $name = $this->option('connection'); + $name = is_string($name) && $name !== '' ? $name : null; + + $context = $contexts->make(); + $writable = $this->writable($connections, $name ?? $this->configured($config)); + + $report = [ + 'enabled' => $config->get('difflock.enabled') !== false, + 'connection' => $context->schema->connection, + 'driver' => $context->driver(), + 'version' => $context->version, + 'environment' => $context->environment, + 'reachable' => $context->available, + 'writable' => $writable, + 'tables' => count($context->schema->tables), + 'pending_migrations' => count($locator->locate(MigrationScope::Pending)), + 'all_migrations' => count($locator->locate(MigrationScope::All)), + 'rules' => array_map( + static fn (object $rule): string => method_exists($rule, 'identifier') ? (string) $rule->identifier() : $rule::class, + $rules->resolve($this->laravel), + ), + 'baseline' => ['path' => $baseline->path(), 'recorded' => $baseline->exists()], + 'accepted' => ['path' => $accepted->path(), 'recorded' => $accepted->exists()], + ]; + + if ($this->wantsJson()) { + $this->writeJson(['difflock' => JsonReport::VERSION] + $report); + + return $context->available ? self::SUCCESS : self::INVALID; + } + + $this->render($report); + + return $context->available ? self::SUCCESS : self::INVALID; + } + + /** + * @param array $report + */ + private function render(array $report): void + { + Banner::render($this->output, 'Difflock · Doctor'); + + $this->section('Database', [ + 'Connection' => $this->text($report['connection']), + 'Driver' => $this->text($report['driver']), + 'Version' => $this->text($report['version']), + 'Environment' => $this->text($report['environment']), + 'Reachable' => $report['reachable'] === true ? 'yes' : 'no', + 'Tables visible' => $this->text($report['tables']), + ]); + + $this->privileges($report['writable']); + + $this->section('Migrations', [ + 'Pending' => $this->text($report['pending_migrations']), + 'Total' => $this->text($report['all_migrations']), + ]); + + $rules = []; + + foreach (is_array($report['rules']) ? $report['rules'] : [] as $rule) { + if (is_string($rule)) { + $rules[] = $rule; + } + } + + $this->section('Rules', ['Registered' => count($rules).' — '.implode(', ', $rules)]); + + $baseline = is_array($report['baseline']) ? $report['baseline'] : []; + $accepted = is_array($report['accepted']) ? $report['accepted'] : []; + + $this->section('Files', [ + 'Baseline' => ($baseline['recorded'] === true ? 'recorded' : 'not recorded').' '.$this->text($baseline['path'] ?? ''), + 'Accepted' => ($accepted['recorded'] === true ? 'recorded' : 'not recorded').' '.$this->text($accepted['path'] ?? ''), + ]); + } + + /** + * The line this command exists for. + */ + private function privileges(mixed $writable): void + { + $this->output->writeln(' Privileges'); + + [$glyph, $colour, $line, $note] = match ($writable) { + false => ['✓', 'green', 'The role Difflock connects as cannot write to this database.', + 'That is the strongest form of the guarantee: not that Difflock will not write, but that it could not.'], + true => ['⚠', 'yellow', 'The role Difflock connects as is able to write to this database.', + 'Difflock has no code path that writes to the inspected connection, but nothing outside the code enforces that. ' + .'Point `difflock.connection` at a read-only role and the promise stops depending on trust.'], + default => ['·', 'gray', 'Whether the role can write could not be determined.', + 'The probe is a read-only transaction that is always rolled back; a driver that does not support one answers nothing.'], + }; + + $this->output->writeln(' '.$glyph.' '.$line); + + foreach (Text::wrap($note, ' ') as $wrapped) { + $this->output->writeln(''.$wrapped.''); + } + + $this->output->writeln(''); + } + + /** + * Whether the connected role can write, or null when it cannot be established. + * + * Asked by opening a transaction, attempting the cheapest possible write, and + * rolling back — always, on both paths. Nothing is created: the statement is + * deliberately one that fails on a missing table for a *different* reason than it + * fails on a missing privilege, and the two are told apart by the SQLSTATE. + */ + private function writable(ConnectionResolverInterface $connections, ?string $name): ?bool + { + try { + $connection = $connections->connection($name); + + if (! $connection instanceof Connection) { + return null; + } + + $connection->beginTransaction(); + + try { + // 42P01/42S02 "no such table" means the statement was allowed and + // only the object was missing — so the role may write. A privilege + // error means it may not. + $connection->statement('create table difflock_write_probe (id int)'); + $connection->rollBack(); + + return true; + } catch (Throwable $exception) { + $connection->rollBack(); + + return $this->deniedByPrivilege($exception) ? false : null; + } + } catch (Throwable) { + return null; + } + } + + private function deniedByPrivilege(Throwable $exception): bool + { + $message = strtolower($exception->getMessage()); + + foreach (['permission denied', 'access denied', 'insufficient privilege', 'read-only', 'readonly'] as $needle) { + if (str_contains($message, $needle)) { + return true; + } + } + + return false; + } + + /** + * @param array $rows + */ + private function section(string $title, array $rows): void + { + $this->output->writeln(' '.$title.''); + + foreach ($rows as $label => $value) { + $this->output->writeln(' '.Text::pad($label, 16).''.$value); + } + + $this->output->writeln(''); + } + + private function text(mixed $value): string + { + return is_scalar($value) ? (string) $value : 'unknown'; + } + + private function configured(Repository $config): ?string + { + $connection = $config->get('difflock.connection'); + + return is_string($connection) && $connection !== '' ? $connection : null; + } +} diff --git a/src/Console/Renderers/CheckupRenderer.php b/src/Console/Renderers/CheckupRenderer.php index 03ff026..f79fd45 100644 --- a/src/Console/Renderers/CheckupRenderer.php +++ b/src/Console/Renderers/CheckupRenderer.php @@ -48,6 +48,53 @@ public function render(OutputInterface $output, CheckupResult $result, bool $ter $output->writeln(''); } + /** + * The overview: the same verdict, without reprinting the whole analysis. + * + * `difflock` used to render the full findings list, which on a real application + * meant the two summary lines a reader actually came for were buried under + * everything else. Here it shows the summary, any drift, and only the worst level + * of finding — then says where the rest are. + */ + public function overview(OutputInterface $output, CheckupResult $result): void + { + $this->schemaLine($output, $result); + $this->migrationLine($output, $result); + + $output->writeln(''); + + if ($result->drifted() && $result->drift instanceof SchemaDiff) { + $output->writeln(' Schema drift'); + $output->writeln(''); + + $this->diffs->render($output, $result->drift); + } + + $summary = $result->report->summary(); + + if ($summary->total > 0) { + $worst = $summary->highest; + + $this->reports->render($output, $result->report->only(atLeast: $worst)); + + $remaining = $summary->total - $summary->count($worst); + + if ($remaining > 0) { + $output->writeln( + ' '.$remaining.' finding'.($remaining === 1 ? '' : 's').' below ' + .$worst->label().' not shown — php artisan difflock:lint', + ); + $output->writeln(''); + } + } + + $output->writeln($result->failed() + ? ' Result: FAIL' + : ' Result: PASS'); + + $output->writeln(''); + } + private function schemaLine(OutputInterface $output, CheckupResult $result): void { $output->writeln(' Schema'); diff --git a/src/DifflockServiceProvider.php b/src/DifflockServiceProvider.php index 9f0a67c..4da2220 100644 --- a/src/DifflockServiceProvider.php +++ b/src/DifflockServiceProvider.php @@ -7,6 +7,7 @@ use Difflock\Console\Commands\CheckCommand; use Difflock\Console\Commands\DiffCommand; use Difflock\Console\Commands\DifflockCommand; +use Difflock\Console\Commands\DoctorCommand; use Difflock\Console\Commands\LintCommand; use Difflock\Console\Commands\MigrateCommand; use Difflock\Contracts\MigrationAnalyzer; @@ -109,15 +110,10 @@ public function register(): void $this->migrationPaths($app), )); - $this->app->bind(MigrationAnalyzer::class, fn (Application $app): MigrationAnalyzer => new RuleMigrationAnalyzer( - $app->make(MigrationLocator::class), - $app->make(MigrationParser::class), - $app->make(Filesystem::class), - $app->make(DatabaseContextFactory::class), - $app->make(RuleRegistry::class)->resolve($app), - IgnoreList::fromConfig($this->section($app, 'difflock.ignore')), - $app->make(AcceptedFindings::class), - )); + $this->app->bind( + MigrationAnalyzer::class, + fn (Application $app): MigrationAnalyzer => $this->analyzer($app, $app->make(DatabaseContextFactory::class)), + ); $this->app->bind(AcceptedFindings::class, fn (Application $app): AcceptedFindings => new AcceptedFindings( $app->make(Filesystem::class), @@ -138,12 +134,21 @@ public function register(): void $app->make(ProtectionPolicy::class), )); - $this->app->bind(Checkup::class, fn (Application $app): Checkup => new Checkup( - $app->make(SchemaInspector::class), - $app->make(SchemaDiffer::class), - $app->make(MigrationAnalyzer::class), - $app->make(Baseline::class), - )); + // Checkup and the analyzer it drives are given the *same* context factory, so + // the schema is read once for the whole run rather than once for drift and + // again for the rules. Built here rather than resolved twice, because the + // factory is deliberately transient — its memo must not outlive the run. + $this->app->bind(Checkup::class, function (Application $app): Checkup { + $contexts = $app->make(DatabaseContextFactory::class); + + return new Checkup( + $app->make(SchemaInspector::class), + $app->make(SchemaDiffer::class), + $this->analyzer($app, $contexts), + $app->make(Baseline::class), + $contexts, + ); + }); $this->app->bind(Difflock::class, fn (Application $app): Difflock => new Difflock( $app->make(SchemaInspector::class), @@ -165,6 +170,7 @@ public function boot(): void DifflockCommand::class, CheckCommand::class, DiffCommand::class, + DoctorCommand::class, LintCommand::class, MigrateCommand::class, ]); @@ -174,6 +180,25 @@ public function boot(): void ], 'difflock-config'); } + /** + * An analyzer wired to a particular context factory. + * + * Taking the factory as an argument rather than resolving it is what lets a + * caller share one schema reading across everything it drives. + */ + private function analyzer(Application $app, DatabaseContextFactory $contexts): MigrationAnalyzer + { + return new RuleMigrationAnalyzer( + $app->make(MigrationLocator::class), + $app->make(MigrationParser::class), + $app->make(Filesystem::class), + $contexts, + $app->make(RuleRegistry::class)->resolve($app), + IgnoreList::fromConfig($this->section($app, 'difflock.ignore')), + $app->make(AcceptedFindings::class), + ); + } + /** * The rule classes named in configuration. * diff --git a/tests/Feature/DoctorTest.php b/tests/Feature/DoctorTest.php new file mode 100644 index 0000000..9854711 --- /dev/null +++ b/tests/Feature/DoctorTest.php @@ -0,0 +1,90 @@ + $table->id()); + + config()->set('difflock.migrations.paths', [fixtures()]); +}); + +it('reports the ground every other command stands on', function (): void { + [$exit, $output] = runCommand('difflock:doctor'); + + expect($exit)->toBe(0) + ->and($output)->toContain('Difflock · Doctor') + ->toContain('Database') + ->toContain('sqlite') + ->toContain('Privileges') + ->toContain('Migrations') + ->toContain('Rules') + ->toContain('drop-column') + ->toContain('Files'); +}); + +it('says whether the role could write, whatever the answer', function (): void { + [, $output] = runCommand('difflock:doctor'); + + expect($output)->toContain('Privileges') + ->and($output)->toMatch('/(cannot write|is able to write|could not be determined)/'); +}); + +it('counts the tables and migrations it can see', function (): void { + [, $output] = runCommand('difflock:doctor', ['--format' => 'json']); + + $document = json_decode(trim($output), true, flags: JSON_THROW_ON_ERROR); + + expect($document['reachable'])->toBeTrue() + ->and($document['driver'])->toBe('sqlite') + ->and($document['tables'])->toBeGreaterThan(0) + ->and($document['all_migrations'])->toBe(3) + ->and($document['rules'])->toContain('drop-column', 'sensitive-column', 'unindexed-foreign-key') + ->and($document['enabled'])->toBeTrue(); +}); + +it('reports where the baseline and accepted files live and whether they exist', function (): void { + [, $output] = runCommand('difflock:doctor', ['--format' => 'json']); + + $document = json_decode(trim($output), true, flags: JSON_THROW_ON_ERROR); + + expect($document['baseline']['recorded'])->toBeFalse() + ->and($document['accepted']['recorded'])->toBeFalse() + ->and($document['baseline']['path'])->toContain('schema.json'); + + runCommand('difflock:diff', ['--save' => true]); + + [, $output] = runCommand('difflock:doctor', ['--format' => 'json']); + + expect(json_decode(trim($output), true, flags: JSON_THROW_ON_ERROR)['baseline']['recorded'])->toBeTrue(); +}); + +it('fails when the database cannot be reached, rather than reporting an empty one', function (): void { + config()->set('database.connections.broken', ['driver' => 'sqlite', 'database' => '/no/such/file.sqlite']); + config()->set('difflock.connection', 'broken'); + + [$exit, $output] = runCommand('difflock:doctor'); + + expect($exit)->toBe(2) + ->and($output)->toContain('Reachable') + ->toContain('no'); +}); + +/** + * The probe opens a transaction, tries the cheapest write there is, and rolls back + * on both paths. If it ever leaked, this table would survive it. + */ +it('leaves nothing behind when it probes for write access', function (): void { + runCommand('difflock:doctor'); + + expect(Schema::hasTable('difflock_write_probe'))->toBeFalse(); +}); + +it('emits JSON with no ANSI in it', function (): void { + [, $output] = runCommand('difflock:doctor', ['--format' => 'json'], decorated: true); + + expect($output)->not->toContain("\033") + ->and(json_decode(trim($output), true, flags: JSON_THROW_ON_ERROR))->toHaveKey('difflock'); +}); diff --git a/tests/Feature/SharedSchemaReadTest.php b/tests/Feature/SharedSchemaReadTest.php new file mode 100644 index 0000000..c58b8fd --- /dev/null +++ b/tests/Feature/SharedSchemaReadTest.php @@ -0,0 +1,86 @@ +id(); + $table->string('label')->nullable(); + $table->index('label'); + }); + } + + config()->set('difflock.migrations.paths', [fixtures('safe')]); +}); + +/** + * `difflock:check` used to read the whole schema twice — once for drift, once to + * give the rules their context. On a 99-table PostgreSQL database that measured 598 + * queries and 3.7 seconds, half of it repeated. + * + * The assertion is a ratio rather than a number, because the absolute count depends + * on the driver and the number of tables. Two full readings would be at least twice + * one; anything close to a single reading means they are sharing. + */ +it('reads the schema once for a whole check, not once per question', function (): void { + runCommand('difflock:diff', ['--save' => true]); + + $inspect = queriesFor(fn () => app(SchemaInspector::class)->inspect()); + + expect($inspect)->toBeGreaterThan(5); + + $check = queriesFor(fn () => app(Checkup::class)->run(RiskLevel::Critical)); + + expect($check)->toBeLessThan($inspect * 2); +}); + +it('still detects drift while sharing that reading', function (): void { + runCommand('difflock:diff', ['--save' => true]); + + Schema::table('orders', fn (Blueprint $table) => $table->string('added')->nullable()); + + $result = app(Checkup::class)->run(RiskLevel::Critical); + + expect($result->drifted())->toBeTrue() + ->and($result->drift?->count())->toBe(1) + ->and($result->failed())->toBeTrue(); +}); + +/** + * An explicit `--connection` asks about a different database than the one the rules + * are given, so it cannot share the reading — and must still be correct. + */ +it('reads separately when asked about another connection', function (): void { + config()->set('database.connections.other', config('database.connections.testing')); + + runCommand('difflock:diff', ['--save' => true]); + + $result = app(Checkup::class)->run(RiskLevel::Critical, 'other'); + + expect($result->baselineRecorded)->toBeTrue() + ->and($result->baselineError)->toBeNull(); +});