Skip to content

[9.x] Fix "artisan db:wipe" command does not drop tables in all schemas #41541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[9.x] Fix "artisan db:wipe" command does not drop tables in all schemas #41541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions src/Illuminate/Database/Schema/Grammars/PostgresGrammar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ public function compileDropIfExists(Blueprint $blueprint, Fluent $command)
*/
public function compileDropAllTables($tables)
{
return 'drop table "'.implode('","', $tables).'" cascade';
return 'drop table '.implode(',', $this->escapeObjectReferences($tables)).' cascade';
}

/**
Expand All @@ -283,7 +283,7 @@ public function compileDropAllTables($tables)
*/
public function compileDropAllViews($views)
{
return 'drop view "'.implode('","', $views).'" cascade';
return 'drop view '.implode(',', $this->escapeObjectReferences($views)).' cascade';
}

/**
Expand All @@ -294,7 +294,7 @@ public function compileDropAllViews($views)
*/
public function compileDropAllTypes($types)
{
return 'drop type "'.implode('","', $types).'" cascade';
return 'drop type '.implode(',', $this->escapeObjectReferences($types)).' cascade';
}

/**
Expand All @@ -305,7 +305,7 @@ public function compileDropAllTypes($types)
*/
public function compileGetAllTables($searchPath)
{
return "select tablename from pg_catalog.pg_tables where schemaname in ('".implode("','", (array) $searchPath)."')";
return "select tablename, schemaname from pg_catalog.pg_tables where schemaname in ('".implode("','", (array) $searchPath)."')";
}

/**
Expand All @@ -316,7 +316,7 @@ public function compileGetAllTables($searchPath)
*/
public function compileGetAllViews($searchPath)
{
return "select viewname from pg_catalog.pg_views where schemaname in ('".implode("','", (array) $searchPath)."')";
return "select viewname, schemaname from pg_catalog.pg_views where schemaname in ('".implode("','", (array) $searchPath)."')";
}

/**
Expand Down Expand Up @@ -1070,4 +1070,31 @@ protected function modifyStoredAs(Blueprint $blueprint, Fluent $column)
return " generated always as ({$column->storedAs}) stored";
}
}

/**
* Escape database object references consitently, schema-qualified or not.
*
* @param array $objects
* @return array
*/
public function escapeObjectReferences(array $objects): array
{
$escapedObjects = [];

foreach ($objects as $object) {
$parts = explode('.', $object);

$newParts = [];

array_walk($parts, function (&$part) use (&$newParts) {
$part = str_replace(['"', "'"], '', $part);

$newParts[] = $part;
});

$escapedObjects[] = '"'.implode('"."', $parts).'"';
}

return $escapedObjects;
}
}
8 changes: 5 additions & 3 deletions src/Illuminate/Database/Schema/PostgresBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,14 @@ public function dropAllTables()

$excludedTables = $this->connection->getConfig('dont_drop') ?? ['spatial_ref_sys'];

$excludedTables = $this->grammar->escapeObjectReferences($excludedTables);

foreach ($this->getAllTables() as $row) {
$row = (array) $row;

$table = reset($row);
$table = '"'.$row['schemaname'].'"."'.$row['tablename'].'"';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a 9.x breaking change for the $excludedTables check on line 72. Existing apps with values in config('database.pgsql.dont_drop') won't be schema-qualified with wrapping quotes. Even the hardcoded $excludedTables = ['spatial_ref_sys'] default value used by 99.9% of Laravel/Postgres apps will fail to be skipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! Thank you! This is exactly why I said the following in the related Issue:

As is the case with all things search_path-related, the fix is likely to be nuanced and will have to be vetted carefully so as not to break anything else, as not all of the affected code has test coverage (particularly the older code).

I'll work on a solution to this. Thanks again!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever it's convenient, please take a look at the new commit that I pushed. My hope is that it resolves the concern that you raise here.

Now, tables are excluded in a backwards-compatible way, so that all of the following table notations would be excluded:

'dont_drop' => [
    'users',
    '"password_resets"',
    'public.failed_jobs',
    '"public"."personal_access_tokens"',
    'spatial_ref_sys'
],

This configuration produces the following DROP query in a stock Laravel installation in which php artisan migrate has been run (and migrations is the only table we didn't exclude):

"drop table "public"."migrations" cascade"

So, in essence, the user can schema-qualify those dont_drop references if desired, and if not, the unqualified references will be treated just as they have been prior to this PR. That is to say, when a reference is unqualified, the table will not be dropped from any schema in which it exists.

As a related aside, the the current logic around the spatial_ref_sys table that exists when the postgis extension is enabled seems a little odd to me.

In essence, the logic is, "If the user has not specified tables to be excluded, then exclude spatial_ref_sys, else, exclude the specified tables (in which case spatial_ref_sys will be dropped)."

Is that necessarily the desired behavior? It seems all too easy to drop spatial_ref_sys without even realizing it's happening, merely by specifying some other, completely unrelated table(s) to be excluded. I suppose once the user realizes what's happening, they'll add spatial_ref_sys to the dont_drop array, too.

The same is true of the dropAllViews() method. It'll drop the two views that are created for postgis (geography_columns and geometry_columns):

laravel=# CREATE EXTENSION postgis;            
CREATE EXTENSION                               
laravel=# \d                                   
               List of relations               
 Schema |       Name        | Type  |  Owner   
--------+-------------------+-------+----------
 public | geography_columns | view  | postgres 
 public | geometry_columns  | view  | postgres 
 public | spatial_ref_sys   | table | postgres 
(3 rows)

I don't know if any of this is of concern to anybody, I just figured I'd mention it while on the subject.


if (! in_array($table, $excludedTables)) {
if (! in_array('"'.$row['tablename'].'"', $excludedTables) && ! in_array($table, $excludedTables)) {
$tables[] = $table;
}
}
Expand All @@ -95,7 +97,7 @@ public function dropAllViews()
foreach ($this->getAllViews() as $row) {
$row = (array) $row;

$views[] = reset($row);
$views[] = '"'.$row['schemaname'].'"."'.$row['viewname'].'"';
}

if (empty($views)) {
Expand Down
27 changes: 15 additions & 12 deletions tests/Database/DatabasePostgresBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,10 +238,11 @@ public function testDropAllTablesWhenSearchPathIsString()
$connection->shouldReceive('getConfig')->with('dont_drop')->andReturn(['foo']);
$grammar = m::mock(PostgresGrammar::class);
$connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar);
$grammar->shouldReceive('compileGetAllTables')->with(['public'])->andReturn("select tablename from pg_catalog.pg_tables where schemaname in ('public')");
$connection->shouldReceive('select')->with("select tablename from pg_catalog.pg_tables where schemaname in ('public')")->andReturn(['users']);
$grammar->shouldReceive('compileDropAllTables')->with(['users'])->andReturn('drop table "'.implode('","', ['users']).'" cascade');
$connection->shouldReceive('statement')->with('drop table "'.implode('","', ['users']).'" cascade');
$grammar->shouldReceive('compileGetAllTables')->with(['public'])->andReturn("select tablename, schemaname from pg_catalog.pg_tables where schemaname in ('public')");
$connection->shouldReceive('select')->with("select tablename, schemaname from pg_catalog.pg_tables where schemaname in ('public')")->andReturn([['schemaname' => 'public', 'tablename' => 'users']]);
$grammar->shouldReceive('escapeObjectReferences')->with(['foo'])->andReturn(['"foo"']);
$grammar->shouldReceive('compileDropAllTables')->with(['"public"."users"'])->andReturn('drop table "public"."users" cascade');
$connection->shouldReceive('statement')->with('drop table "public"."users" cascade');
$builder = $this->getBuilder($connection);

$builder->dropAllTables();
Expand All @@ -255,10 +256,11 @@ public function testDropAllTablesWhenSearchPathIsStringOfMany()
$connection->shouldReceive('getConfig')->with('dont_drop')->andReturn(['foo']);
$grammar = m::mock(PostgresGrammar::class);
$connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar);
$grammar->shouldReceive('compileGetAllTables')->with(['foouser', 'public', 'foo_bar-Baz.Áüõß'])->andReturn("select tablename from pg_catalog.pg_tables where schemaname in ('foouser','public','foo_bar-Baz.Áüõß')");
$connection->shouldReceive('select')->with("select tablename from pg_catalog.pg_tables where schemaname in ('foouser','public','foo_bar-Baz.Áüõß')")->andReturn(['users', 'users']);
$grammar->shouldReceive('compileDropAllTables')->with(['users', 'users'])->andReturn('drop table "'.implode('","', ['users', 'users']).'" cascade');
$connection->shouldReceive('statement')->with('drop table "'.implode('","', ['users', 'users']).'" cascade');
$grammar->shouldReceive('compileGetAllTables')->with(['foouser', 'public', 'foo_bar-Baz.Áüõß'])->andReturn("select tablename, schemaname from pg_catalog.pg_tables where schemaname in ('foouser','public','foo_bar-Baz.Áüõß')");
$connection->shouldReceive('select')->with("select tablename, schemaname from pg_catalog.pg_tables where schemaname in ('foouser','public','foo_bar-Baz.Áüõß')")->andReturn([['schemaname' => 'users', 'tablename' => 'users']]);
$grammar->shouldReceive('escapeObjectReferences')->with(['foo'])->andReturn(['"foo"']);
$grammar->shouldReceive('compileDropAllTables')->with(['"users"."users"'])->andReturn('drop table "users"."users" cascade');
$connection->shouldReceive('statement')->with('drop table "users"."users" cascade');
$builder = $this->getBuilder($connection);

$builder->dropAllTables();
Expand All @@ -277,10 +279,11 @@ public function testDropAllTablesWhenSearchPathIsArrayOfMany()
$connection->shouldReceive('getConfig')->with('dont_drop')->andReturn(['foo']);
$grammar = m::mock(PostgresGrammar::class);
$connection->shouldReceive('getSchemaGrammar')->once()->andReturn($grammar);
$grammar->shouldReceive('compileGetAllTables')->with(['foouser', 'dev', 'test', 'spaced schema'])->andReturn("select tablename from pg_catalog.pg_tables where schemaname in ('foouser','dev','test','spaced schema')");
$connection->shouldReceive('select')->with("select tablename from pg_catalog.pg_tables where schemaname in ('foouser','dev','test','spaced schema')")->andReturn(['users', 'users']);
$grammar->shouldReceive('compileDropAllTables')->with(['users', 'users'])->andReturn('drop table "'.implode('","', ['users', 'users']).'" cascade');
$connection->shouldReceive('statement')->with('drop table "'.implode('","', ['users', 'users']).'" cascade');
$grammar->shouldReceive('compileGetAllTables')->with(['foouser', 'dev', 'test', 'spaced schema'])->andReturn("select tablename, schemaname from pg_catalog.pg_tables where schemaname in ('foouser','dev','test','spaced schema')");
$connection->shouldReceive('select')->with("select tablename, schemaname from pg_catalog.pg_tables where schemaname in ('foouser','dev','test','spaced schema')")->andReturn([['schemaname' => 'users', 'tablename' => 'users']]);
$grammar->shouldReceive('escapeObjectReferences')->with(['foo'])->andReturn(['"foo"']);
$grammar->shouldReceive('compileDropAllTables')->with(['"users"."users"'])->andReturn('drop table "users"."users" cascade');
$connection->shouldReceive('statement')->with('drop table "users"."users" cascade');
$builder = $this->getBuilder($connection);

$builder->dropAllTables();
Expand Down