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

Jump to bottom

SQLite: Do not dump system tables #383

Merged
merged 2 commits into from
Feb 21, 2023
Merged

SQLite: Do not dump system tables #383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c820cad
SQLite: Do not dump system tables
docapotamus Feb 17, 2023
5aa2c7b
create autoincrement table to fix test
amacneil Feb 21, 2023
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
2 changes: 1 addition & 1 deletion pkg/driver/sqlite/sqlite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func (drv *Driver) schemaMigrationsDump(db *sql.DB) ([]byte, error) {
// DumpSchema returns the current database schema
func (drv *Driver) DumpSchema(db *sql.DB) ([]byte, error) {
path := ConnectionString(drv.databaseURL)
schema, err := dbutil.RunCommand("sqlite3", path, ".schema")
schema, err := dbutil.RunCommand("sqlite3", path, ".schema --nosys")
Copy link
Owner

Choose a reason for hiding this comment

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

Pretty sure these should be separate arguments right?

Suggested change
schema, err := dbutil.RunCommand("sqlite3", path, ".schema --nosys")
schema, err := dbutil.RunCommand("sqlite3", path, ".schema", "--nosys")

Are you sure this change did anything for you? Since you mentioned the test never failed, in which situations did you see those tables get created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can recreate this using the steps in the issue just not in the unit test.

If I add the .schema --nosys as one string the sqlite_sequence does not get dumped to the schema. If I add it as a sepeate string I receive an error.

No change applied

j.doherty@test my_dbmate % DATABASE_URL=sqlite:db/db.sqlite ./dist/dbmate dump
Writing: ./db/schema.sql
j.doherty@test my_dbmate % cat ./db/schema.sql 
CREATE TABLE IF NOT EXISTS "schema_migrations" (version varchar(255) primary key);
CREATE TABLE t (
      id INTEGER PRIMARY KEY AUTOINCREMENT
);
CREATE TABLE sqlite_sequence(name,seq);
-- Dbmate schema migrations
INSERT INTO "schema_migrations" (version) VALUES
  ('20230220094634');

With my patch applied

j.doherty@test my_dbmate % DATABASE_URL=sqlite:db/db.sqlite ./dist/dbmate dump
Writing: ./db/schema.sql
j.doherty@test my_dbmate % cat ./db/schema.sql                                
CREATE TABLE IF NOT EXISTS "schema_migrations" (version varchar(255) primary key);
CREATE TABLE t (
      id INTEGER PRIMARY KEY AUTOINCREMENT
);
-- Dbmate schema migrations
INSERT INTO "schema_migrations" (version) VALUES
  ('20230220094634');

With the flag in a seperate arg

j.doherty@test my_dbmate % DATABASE_URL=sqlite:db/db.sqlite ./dist/dbmate dump
Error: sqlite3: Error: unknown option: -nosys
Use -help for a list of options.

Copy link
Owner

Choose a reason for hiding this comment

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

I see. That table only shows up in the dump if you have an AUTOINCREMENT field. I added one to the test and it correctly fails now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot. Don't know why I didn't think to check the test migration.

if err != nil {
return nil, err
}
Expand Down
11 changes: 9 additions & 2 deletions pkg/driver/sqlite/sqlite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,22 +180,29 @@ func TestSQLiteDumpSchema(t *testing.T) {
err = drv.InsertMigration(db, "abc2")
require.NoError(t, err)

// create a table that will trigger `sqlite_sequence` system table
_, err = db.Exec("CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)")
require.NoError(t, err)

// DumpSchema should return schema
schema, err := drv.DumpSchema(db)
require.NoError(t, err)
require.Contains(t, string(schema), "CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT)")
require.Contains(t, string(schema), "CREATE TABLE IF NOT EXISTS \"test_migrations\"")
require.Contains(t, string(schema), ");\n-- Dbmate schema migrations\n"+
"INSERT INTO \"test_migrations\" (version) VALUES\n"+
" ('abc1'),\n"+
" ('abc2');\n")

// sqlite_* tables should not be present in the dump (.schema --nosys)
require.NotContains(t, string(schema), "sqlite_")

// DumpSchema should return error if command fails
drv.databaseURL = dbutil.MustParseURL(".")
schema, err = drv.DumpSchema(db)
require.Nil(t, schema)
require.Error(t, err)
require.EqualError(t, err, "Error: unable to open database \"/.\": "+
"unable to open database file")
require.EqualError(t, err, "Error: unable to open database \"/.\": unable to open database file")
}

func TestSQLiteDatabaseExists(t *testing.T) {
Expand Down