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

refactor: change sqlite dropAllTables implementation #1001

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

Julien-R44
Copy link
Member

❓ Type of change

  • 🐞 Bug fix (a non-breaking change that fixes an issue)

πŸ“š Description

Fix #999

I have updated the implementation of dropAllTables for better-sqlite3 and sqlite :

  • The implementation was causing problems for better-sqlite3 as explained in the linked issue. i also preferred to modify the implementation for sqlite by modifying the BaseSqlite class so that we don't have two different methods to maintain

  • The problem is linked to the unsafeMode explained here: https://github.com/WiseLibs/better-sqlite3/blob/master/docs/unsafe.md and therefore to our use of PRAGMA writable_schema.
    image

As a result, I've decided to drop all tables with multiples queries rather than modifying sqlite_master directly. this way, we don't need to activate the unsafe mode. the method is probably slower, but imo it doesn't matter at all

indexes and triggers are automatically deleted if linked to a deleted table

@RomainLanz
Copy link
Member

LGTM

@thetutlage
Copy link
Member

I did not read much about it. Does SQLite has such thing called FOREIGN_KEY_CHECKS, which might stop from dropping tables?

This is our implementation for MYSQL https://github.com/adonisjs/lucid/blob/f84a710c192228b3147e3b93369da46a60cded5c/src/dialects/mysql.ts

@thetutlage
Copy link
Member

This is what Laravel does. They call compileEnableWriteableSchema, then drops the tables and then turns it off via compileDisableWriteableSchema.

https://github.com/laravel/framework/blob/10.x/src/Illuminate/Database/Schema/Grammars/SQLiteGrammar.php#L543-L556

https://github.com/laravel/framework/blob/10.x/src/Illuminate/Database/Schema/SQLiteBuilder.php#L87

@Julien-R44
Copy link
Member Author

Julien-R44 commented Feb 16, 2024

  • nice catch as always. sqlite has this as the equivalent of FOREIGN_KEY_CHECKS: https://www.sqlite.org/pragma.html#pragma_foreign_keys but it's not enabled by default. So I've updated the PR to take this into account: disable the pragma if enabled, and reset it to its initial value after dropping the tables. also added a test for this
  • Laravel does use the writable_schema pragma, which allows to modify the sqlite_master table, but that's exactly the problem we're having. We had the same implementation, but better-sqlite3 doesn't allow us to use this pragma without activating the unsafe mode. see my very first comment

should be all good now

@RomainLanz RomainLanz merged commit 5c18c76 into develop Feb 23, 2024
32 checks passed
@RomainLanz RomainLanz deleted the fix/drop-all-tables-better-sqlite branch February 23, 2024 12:01
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.

migration:fresh & db:wipe fail with "ERROR: table sqlite_master may not be modified"
3 participants