Skip to content

Fix Schema Division Handling#955

Open
mikebronner wants to merge 5 commits intotenancy:5.xfrom
mikebronner:patch-1
Open

Fix Schema Division Handling#955
mikebronner wants to merge 5 commits intotenancy:5.xfrom
mikebronner:patch-1

Conversation

@mikebronner
Copy link
Contributor

@mikebronner mikebronner commented Jul 28, 2020

This PR aims to fix some issues with with schema division:

  • only create the schema if it doesn't already exist.
  • don't drop privileges for schemas.
  • drop schema using cascade in order to clear out its contents.

This is useful if you are recreating the same client without having deleted the previous schema.
Dropping privileges when using a single database user across all tenants will attempt to drop all items in the main database and other client schemas as well.
@mikebronner mikebronner changed the title Only create database schema if it doesn't already exist. Fix Schema Division Handling Jul 28, 2020
@mikebronner mikebronner marked this pull request as draft July 28, 2020 17:39
@mikebronner mikebronner marked this pull request as ready for review July 28, 2020 17:40
protected function dropDatabase(IlluminateConnection $connection, array $config)
{
return $connection->statement("DROP SCHEMA IF EXISTS \"{$config['schema']}\"");
return $connection->statement("DROP SCHEMA IF EXISTS \"{$config['schema']}\" CASCADE");
Copy link
Member

Choose a reason for hiding this comment

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

Using CASCADE is quite a big danger it, as far as my PSQL knowledge goes, this would delete everything in the schema. I think it would be better to use RESTRICT if we should change this at all. I think RESTRICT is even the default.
If we change this, we could risk people ending up deleting entire schema's while that's something that they would not want.

Copy link
Contributor Author

@mikebronner mikebronner Jul 29, 2020

Choose a reason for hiding this comment

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

My thoughts on this method is that we shouldn't be executing this code if our intent is not to drop the schema. And if our intent is to do so, why wouldn't we follow through with it? The problem I ran into when not adding the CASCADE is that the schema would never be dropped automatically, because the tables it contained were never dropped, as I disabled blanked removal of privileges (see change above), which is very risky.

Comment on lines +132 to +134
if (config("tenancy.db.tenant-division-mode") === Connection::DIVISION_MODE_SEPARATE_SCHEMA) {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you give a little insight into why we should not do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my use-case I have only one database user, that is the main database user used on the System connection as well as all other Tenant connections. Removing privileges for a user would remove all the items they owned, regardless of schema they were in. That is dangerous, and in my case broke functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants