Skip to content
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

Filter 'public' schema from listSchemaNames #5600

Closed
wants to merge 1 commit into from
Closed

Filter 'public' schema from listSchemaNames #5600

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
1 change: 1 addition & 0 deletions src/Schema/PostgreSQLSchemaManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ public function listSchemaNames(): array
FROM information_schema.schemata
WHERE schema_name NOT LIKE 'pg\_%'
AND schema_name != 'information_schema'
AND schema_name != 'public'
SQL
);
}
Expand Down
5 changes: 4 additions & 1 deletion tests/Functional/Schema/PostgreSQLSchemaManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,14 @@ public function testGetSearchPath(): void

public function testGetSchemaNames(): void
{
$createSchemaSQL = 'CREATE SCHEMA schema_retrieval_test';
$this->connection->executeStatement($createSchemaSQL);

assert($this->schemaManager instanceof PostgreSQLSchemaManager);

$names = $this->schemaManager->getSchemaNames();

self::assertContains('public', $names, 'The public schema should be found.');
Copy link
Member

Choose a reason for hiding this comment

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

This behavior was fine up until 3.4.0, so probably the root cause is somewhere else. Could you implement a test that would pass prior to 3.4.0 and fails now? This change doesn't look right.

Copy link
Contributor Author

@allan-simon allan-simon Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I really do think the root cause is here and it was just simply not "important" before

doctrine/migrations#441
doctrine/migrations#1196 (comment)
#1110
#1188 (comment)
doctrine/orm#4777

(and yes my migrations in my projects contain all CREATE SCHEMA public in down migration for some years now, at least 2018 with even commit specifically deleting them from down migration which I think everyone was doing)

it becomes important with 3.4.0 because now the schema list is used by dbal for its "drop" component , so we now (3.4+) "correctly" drop schemas created by the application but as a side effect we incorrectly drop the public schema too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and actually these problems to "create schema public is added in down migration" is specifically because of that faulty behaviour

because if 'public' is reported in schema list but as nothing report it in the application as needing it (as you don't need to explicitly precise something is in public schema, not precising schema = public schema ) , then "up" would have deleted it

BUT it was not reported by the stuff generating DROP , only the part generating "CREATE SCHEMA"

I can see this behaviour for other schema as well

in my migration Version20201111213025.php I can see

/**
 * Auto-generated Migration: Please modify to your needs!
 */
final class Version20201111213025 extends AbstractMigration
{
    public function getDescription() : string
    {
        return '';
    }

    public function up(Schema $schema) : void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('CREATE SCHEMA IF NOT EXISTS schema_name');
     }

    public function down(Schema $schema) : void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->addSql('CREATE SCHEMA public');
     }

which fit the explanation above (i.e it generates "create" but not "drop" )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you implement a test that would pass prior to 3.4.0 and fails now? This change doesn't look right.

Checking that Schema\Schema::toDropSql should not generate a "DROP SCHEMA public" ? would that be good for you

the core of the issue is "Do we agree that public is not to be touched by the application layer ?"

Copy link
Member

Choose a reason for hiding this comment

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

From #5596 (comment):

actually I think there's different face of the main behaviour change the fact that DropSchemaSqlCollector was not collecting schema/namespace at all.

@allan-simon if that's the change that caused the regression, should the new builder be adjusted accordingly and not drop any schemas?

Checking that Schema\Schema::toDropSql should not generate a "DROP SCHEMA public" ? would that be good for you

the core of the issue is "Do we agree that public is not to be touched by the application layer ?"

No. We need to address the root cause, not a particular symptom.

I really do think the root cause is here and it was just simply not "important" before

The public schema is just another schema alongside others. Why do you think the right behavior is to return all schemas except for specifically public? Note that this method may be used by other applications for different purposes, so changing its behavior to address a symptom of another issue sounds like a bad idea to me.

Copy link
Member

Choose a reason for hiding this comment

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

I recommend that those who are affected by the issue focus on providing the steps to reproduce it instead of making code changes. You can build a test project like it was done in #5572 or #5584 (comment).

I will not consider any related code changes until the issue is reproduced in an isolated environment.

Copy link
Contributor Author

@allan-simon allan-simon Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No. We need to address the root cause, not a particular symptom.

that's exactly what I mean, so we're aligned, so let's address it :)

the root cause is I disagree with your statement The public schema is just another schema alongside others

-> no it is not. the public schema is the default schema provided by postgresql in any single installation of postgresql, the same as you already don't return "information_schema" and any namespace starting with pg_ , why is public different from them ?

Copy link
Contributor Author

@allan-simon allan-simon Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why do you think the right behavior is to return all schemas except for specifically public?

i already answered that :(

  1. we already filter out schema that are managed by postgres
  2. public is a special one (cf my PR description)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allan-simon if that's the change that caused the regression, should the new builder be adjusted accordingly and not drop any schemas?

no because then it will continue to want to create them (and we will continue to have bug report saying that doctrine migration etc. are trying to:

  1. create schema public in down migration without a drop schema public in the up migration
  2. create schema my_application_specific_schema in up migration without a drop in down

-> in both case you have up and down migration that needs to edited manually (I made a quick survey around my php friends who also use postgresql and all have said "ah yes i have been manually editing migration for the last 5 years to remove these sneaky create schema" , of course I don't have formal proof of that outside of the numerous github issue I've found and documented in my comment above )

Copy link
Contributor Author

@allan-simon allan-simon Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I will not consider any related code changes until the issue is reproduced in an isolated environment.

I don't have any issue with that, I will spend the time as soon as we agree (or reach a strong disagreement) on the state

PUBLIC is a special schema of postgres , and as all special internal stuff of postgres, should be left untouched by application code and tool

self::assertContains('schema_retrieval_test', $names, 'The schema_retrieval_test schema should be found.');
}

public function testSupportDomainTypeFallback(): void
Expand Down