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

[cbj4074]

#41483 describes a flaw that manifests in the Artisan db:wipe command, whereby the tables are not deleted from all schemas.

The problem is not so much with the db:wipe command itself, but rather, how the underlying library acquires the names of the tables to be deleted.

This PR seeks to fix the underlying problem in a backwards-compatible way, and adds a escapeObjectReferences() method that offloads the table escaping (quoting) that is applied to schema-qualified and non-schema-qualified table object references to a dedicated method (instead of repeating the same implode() code all over the place).

[driesvints]

@cbj4074 remember that draft PR's aren't reviewed.

[cbj4074]

I'm not entirely certain whether the same types of changes that I made to compileGetAllTables() and compileGetAllViews() need to be made to compileGetAllTypes() here, as I'm not 100% sure as to the intent in this method:

framework/src/Illuminate/Database/Schema/Grammars/PostgresGrammar.php

Lines 327 to 330 in 82f3ccf

	public function compileGetAllTypes()
	{
	return 'select distinct pg_type.typname from pg_type inner join pg_enum on pg_enum.enumtypid = pg_type.oid';
	}

Types in PostgreSQL can definitely be qualified at the schema level, but I'm not sure if this method needs to be modified to behave in its intended way. @richards-square , would you mind taking a peek at this, if you're available (since you wrote it)? Much appreciated!

[derekmd]

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.

[cbj4074]

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!

[cbj4074]

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.

[cbj4074]

Regarding my earlier comment, I do in fact believe that this query is currently broken/problematic, too, because it returns rows for user-created types that exist in all schemas to which the effective user has access, even schemas that are not within the effective search_path. Furthermore, the type names are not qualified correctly in the DROP ... query. This can be tested easily by creating a type in another schema, e.g.:

CREATE TYPE "foobar"."foo" AS ENUM ('bar', 'bat', 'baz');

and then running php artisan db:wipe --drop-types, which fails with:

SQLSTATE[42704]: Undefined object: 7 ERROR:  type "foo" does not exist (SQL: drop type "foo" cascade)

Upon cursory analysis, the fix for this might simply be to modify the SELECT ... query to something more like this:

laravel=# select distinct pg_type.typname, nspname from pg_type inner join pg_enum on pg_enum.enumtypid = pg_type.oid inner join pg_namespace on pg_type.typnamespace = pg_namespace.oid;
  typname   | nspname
------------+---------
 foo        | foobar
(1 row)

Now, we can qualify the types fully in the DROP ... statement.

This change alone doesn't address the fact that the SELECT ... query returns types that are not in the effective search_path. If we wanted to enforce that behavior, we would need to do what we've done in similar circumstances, i.e., add

where nspname in ('".implode("','", (array) $searchPath)."')

to

framework/src/Illuminate/Database/Schema/Grammars/PostgresGrammar.php

Lines 327 to 330 in 89e43ae

	public function compileGetAllTypes()
	{
	return 'select distinct pg_type.typname from pg_type inner join pg_enum on pg_enum.enumtypid = pg_type.oid';
	}

I'm open to any thoughts on any of this, of course.

[derekmd]

@driesvints This can be closed now that #41701 has been merged.

@cbj4074: The compileGetAllTypes() / compileDropAllTypes() fixes may need to be explored in another PR, targeting 10.x if it introduces breaking changes like public function compileGetAllTypes($searchPath) with a new argument. The other merged PR I submitted (based on your commit) at least fixes dropping tables & views across schemas.

[driesvints]

Thanks all!

[cbj4074]

@derekmd Thanks for taking the torch and running with it here. I haven't had much time to dedicate to this over the past week, so I appreciate your help.

As you noted above, we should still try to "close the loop" on a handful of remaining issues, which, in my estimation, are as follows:

1. A QueryException occurs when the user has created a type in a schema that is not within the search_path and executes php artisan db:wipe --drop-types.
2. If a type with the same name exists in more than one schema, it will only be dropped from the first schema in which it is found when traversing the effective search_path, which seems to be inconsistent with the intent.
3. dropAllTypes(), and therefore db:wipe, drops all types to which the user has access, even if they are not within the effective search_path. It may be worth discussing whether this behavior is desirable or not.
4. How postgis-related types are handled when dropping all types. I don't have a "solution" to propose as yet, but it seems undesirable to drop the geography_columns and geometry_columns views in a default configuration (while at the same time preserving the spatial_ref_sys table). And now that I think about it, we may want to investigate whether postgis adds any new types, and to what extent those are preserved (or not) when dropAllTypes() is executed.

I'll try to work-up some tests that prove these various issues, and, ideally, submit another PR that seeks to address them, provided you (and anybody else who has input) don't see any holes in my logic.

Thanks again!