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

Truncating tables in mysql, sqlite, sqlserver abstractions do not cascade, postgres does. This is unexpected and a leaky abstraction #35157

Closed
kdocki opened this issue Nov 9, 2020 · 17 comments

Comments

@kdocki
Copy link
Contributor

kdocki commented Nov 9, 2020

  • Laravel Version: 5.8 (or higher)
  • PHP Version: 7.4
  • Database Driver & Version: postgresql 12

Description:

When you truncate tables using the laravel illuminate db builder it truncates the table as expected. However, postgresql is different because it changes the DEFAULT behavior of truncate from RESTRICT to CASCADE. This means that you can loose all your data in other "related" tables (something that doesn't happen with the other sql drivers)

Steps To Reproduce:

Fire up postgresql 12 and mysql.
Create 2 tables in both databases. table_a has a foreign key constraint to table_b.
Change your .env to point to mysql db
Run this code DB::table('table_b')->truncate();
you get an error with mysql, because you cannot truncate tables with foreign key constraints by default - this is expected!!!

Change your .env to point to postgersql db
Run this code DB::table('table_b')->truncate();
it deletes your data in both table_b and table_a, which is totally UNEXPECTED behavior

@driesvints
Copy link
Member

Thanks. I've sent in a PR to the docs to warn about this: #35157

Most likely we won't change this behavior as it'd be a breaking change to anyone expecting the current behavior.

@tabennett
Copy link
Contributor

tabennett commented Nov 10, 2020

@driesvints I don't think anyone is expecting the current behavior. This almost wrecked our company's production servers and is going end up really screwing something up for someone else. Also, this behavior itself is a breaking change that was introduced in #26389 two years ago. I understand that this is a BC break, but is there a reason we can't get a fix for this on the 9x branch?

@driesvints
Copy link
Member

@tabennett unfortunately we can't revert the current behavior as there are two things we need to keep in mind here: the breaking change for people expecting the current behavior and the fact that if we revert it it'll be impossible to truncate tables in PostgreSQL when foreign keys are enabled. So it would essentially prevent certain apps from using it at all.

See @staudenmeir's reply here: #29506 (comment)

We could maybe look into to pass some sort of flag somehow so you can at least toggle the behavior.

@tabennett
Copy link
Contributor

@driesvints Just wanted to say thank you for your help and time. I understand that this is a hard thing to abstract because the behaviors of these two DBMS are entirely different. Having it as a flag to truncate() would be a much better solution (if you're using Msql or something besides postgres, the flag could be a no op) and prevent a nightmare scenario from unfolding for someone.

@driesvints
Copy link
Member

Thanks. I'm currently thinking of a specific truncate method for the postgres grammar maybe. That wouldn't be breaking I believe. We're open to PRs if anyone wants to give that a go.

@kdocki
Copy link
Contributor Author

kdocki commented Nov 10, 2020

If you want to truncate a table with foreign keys on it, you should call raw DB::statement('truncate table_b cascade') because you actually know what you're doing to your database. Having cascade override the default is definitely not expected behavior, it is restrict as you can see in the postgresql docs: 😄

https://www.postgresql.org/docs/9.1/sql-truncate.html

RESTRICT
Refuse to truncate if any of the tables have foreign-key references from tables that are not listed in the command. This is the default.

It would appear that this terrible pull request was accepted into laravel like 2 years ago and now we are afraid to change it. I don't think we should be. The worst thing that happens is that their code breaks because they can't do a cascade delete, in which a simple update to the code would allow them to truncate cascade. If we leave this bug in place, it will potentially wreck people's databases because cascading truncates are not expected at all.

@driesvints
Copy link
Member

Like I said several times already, we aren't going to revert the current behavior at this time. Especially not in a stable release.

@kdocki
Copy link
Contributor Author

kdocki commented Nov 10, 2020

Okay. I understand. What about in a future release? This change could be mentioned in a upgrade changes document and anyone who is relying on this weird quirky behavior would have an upgrade path?

@driesvints
Copy link
Member

driesvints commented Nov 10, 2020

We're only looking at a way to provide the other behavior as well at this time, sorry. This has now also been properly documented.

@localusercamp
Copy link
Contributor

3 years passed, Laravel users still truncates their entire databases...

@guilheb
Copy link

guilheb commented Jul 19, 2023

We were also a victim of this behavior this morning, fortunately we were on a test database. Very dangerous!

It is documented in the Query Builder section of the documentation, but not in the Eloquent section.

@Antobai
Copy link

Antobai commented Feb 22, 2024

Hi, I'm sorry I have to reply to and old and closed isssue, but I have to ask you to please reconsider your opinion on this matter.

A simple method, available to everyone using laravel, on a basic model instance, can wipe out your entire database without warning if you're using postgres and have a constrained database pattern with many relations.
I appreciate that the documentation mentions it now but I don't think it's enough for such a dangerous behavior.
Truncate method when using postgres should trigger a warning and not just do a "truncate cascade" silently. I understand this is a breaking change but it is a necessary breaking change.

@localusercamp
Copy link
Contributor

I think @Antobai is absolutely right. Now is a good time to fix this, especially because there are many database changes in Laravel 11 already.
People still wiping out their databases =)

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@mokevnin
Copy link

mokevnin commented Jul 1, 2024

Guess how I found out about this issue? Table::truncate() inside the migration deleted our production database yesterday :D

@OlegLustenko
Copy link

Well, it makes sense to open an issue and add a callout to documentation that warns the using of "truncate" can drop database

@marcobax
Copy link

This is still an ongoing issue.

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

No branches or pull requests

10 participants