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.

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

[5.8] Sql Server issue with dropAllTables when foreign key constraints exist #28750

Merged
merged 4 commits into from
Jun 8, 2019

Conversation

walliby
Copy link

@walliby walliby commented Jun 6, 2019

Issue

There is an issue with migrations while using a sqlsrv connection. The issue occurs in the SqlServerBuilder->dropAllTables() method when there are foreign key constraints in the database and the database will throw a foreign key constraint exception while running commands like php artisan migrate:fresh. The SqlServerGrammer class returns the following command for this method: EXEC sp_msforeachtable "ALTER TABLE ? NOCHECK CONSTRAINT all"; The problem is that this sql server command only disables the constraints on INSERT and DELETE and does not work for TRUNCATE or DROP TABLE.

Solution

The only solution I have found to dropping tables with foreign key constraints in SQL Server is to first drop the foreign keys on every table.

Reproduction

An example of this issue can be found in the following project when configured with a sqlsrv connection. https://github.com/walliby/sql-server-drop-table-issue. Running phpunit, on the ExampleTest which uses RefreshDatabase will throw a db foreign key constraint error every other time you run the test. The reason the test fails every other time is because it will throw the error on the users table but still drop the role_user table which has the foreign keys. The second time, it will succeed, as the role_user table was deleted by the previous run.

@walliby walliby changed the title Sql Server issue with dropAllTables when foreign key constraints exist [5.8] Sql Server issue with dropAllTables when foreign key constraints exist Jun 6, 2019
@laurencei
Copy link
Contributor

Lol - I literally ran into this issue yesterday myself. You can also just to Telescope to see the problem (since Telescope uses foreign keys).

ping @staudenmeir - thoughts?

@laurencei
Copy link
Contributor

laurencei commented Jun 6, 2019

@taylorotwell - if you can wait 24-48 hours - I can test this against a known real life example to confirm it solves it before merging.

@staudenmeir
Copy link
Contributor

We definitely need time to review this.

@walliby
Copy link
Author

walliby commented Jun 6, 2019

FYI, I have only tested my proposed solution on SQL Server 2016 SP1. I used my example project with foreign key constraints: https://github.com/walliby/sql-server-drop-table-issue. The t-sql I used is fairly basic and should be correct back to at least SQL Server 2008.

@taylorotwell
Copy link
Member

@laurencei if you could test this that would be great.

public function compileDropAllForeignKeys()
{
return "DECLARE @sql NVARCHAR(MAX) = N'';
SELECT @sql += 'ALTER TABLE ' + QUOTENAME(OBJECT_SCHEMA_NAME(parent_object_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to specify the database name?

Copy link
Author

Choose a reason for hiding this comment

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

@staudenmeir I dont believe so because it's being called the same was as compileDropAllTables using $this->connection.

$this->connection->statement($this->grammar->compileDropAllForeignKeys());
$this->connection->statement($this->grammar->compileDropAllTables());

Copy link
Contributor

Choose a reason for hiding this comment

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

When I ran this code, it only removed the foreign keys on the one database (I checked the other one on the same server, and it's foreign keys are still intact).

@laurencei
Copy link
Contributor

laurencei commented Jun 8, 2019

@taylorotwell - worked for the example I have.

Was SQL Server 2016. Not sure if anyone wants to test on other versions first, but seems to be ok so far...

@walliby
Copy link
Author

walliby commented Jun 8, 2019

@laurencei I just tested the pure sql in a SQL 2014 Azure instance and that worked fine dropping 12 foreign keys in the sample AdventureWorks database...

SELECT *
FROM sys.foreign_keys;
-- Query succeeded: Affected rows: 12. 

DECLARE @sql NVARCHAR(MAX) = N'';
SELECT @sql += 'ALTER TABLE ' + QUOTENAME(OBJECT_SCHEMA_NAME(parent_object_id))
    + '.' + QUOTENAME(OBJECT_NAME(parent_object_id)) + 
    ' DROP CONSTRAINT ' + QUOTENAME(name) + ';'
FROM sys.foreign_keys;

EXEC sp_executesql @sql;

SELECT *
FROM sys.foreign_keys;
-- Query succeeded: Affected rows: 0. 

@taylorotwell taylorotwell merged commit fa6faa6 into laravel:5.8 Jun 8, 2019
taylorotwell added a commit that referenced this pull request Jun 10, 2019
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.

4 participants