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

Respect 'SchemaDumper.ignore_tables' in databases structure dump #29077

Merged
merged 6 commits into from May 16, 2017

Conversation

Projects
None yet
3 participants
@guilleiguaran
Member

guilleiguaran commented May 14, 2017

This includes changes from #29060, fix a broken test and adds support for MySQL and SQLite3 as well.

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran May 14, 2017

Member

Will squash some commits and add CHANGELOG to this after of getting review 🙌

Member

guilleiguaran commented May 14, 2017

Will squash some commits and add CHANGELOG to this after of getting review 🙌

Show outdated Hide outdated activerecord/lib/active_record/tasks/sqlite_database_tasks.rb
ignore_tables = ActiveRecord::SchemaDumper.ignore_tables
if ignore_tables.any?
tables = `sqlite3 #{dbfile} .tables`.split - ignore_tables
condition = tables.map { |table| "tbl_name = '#{table}'" }.join(" OR ")

This comment has been minimized.

@matthewd

matthewd May 14, 2017

Member

It feels like it might be more consistent, if nothing else, to use a NOT IN (..) here, instead of doing the subtraction ourselves.

@matthewd

matthewd May 14, 2017

Member

It feels like it might be more consistent, if nothing else, to use a NOT IN (..) here, instead of doing the subtraction ourselves.

This comment has been minimized.

@matthewd

matthewd May 14, 2017

Member

We should also use the proper string-quoting method.

@matthewd

matthewd May 14, 2017

Member

We should also use the proper string-quoting method.

Show outdated Hide outdated activerecord/lib/active_record/tasks/sqlite_database_tasks.rb
tables = `sqlite3 #{dbfile} .tables`.split - ignore_tables
condition = tables.map { |table| "tbl_name = '#{table}'" }.join(" OR ")
statement = "SELECT sql FROM sqlite_master WHERE #{condition} ORDER BY tbl_name, type DESC, name"
`sqlite3 #{flags} #{dbfile} "#{statement}" > #{filename}`

This comment has been minimized.

@matthewd

matthewd May 14, 2017

Member

We should be anyway, but this level of control over the contents of the string makes it even more important for this to be using the array form, instead of the string-construction form, of command invocation.

@matthewd

matthewd May 14, 2017

Member

We should be anyway, but this level of control over the contents of the string makes it even more important for this to be using the array form, instead of the string-construction form, of command invocation.

Show outdated Hide outdated activerecord/lib/active_record/tasks/sqlite_database_tasks.rb
@@ -64,6 +66,18 @@ def configuration
def root
@root
end
def run_cmd(cmd, args, out)
#fail run_cmd_error(cmd, args) unless Kernel.system(cmd, *args, out: out)

This comment has been minimized.

@guilleiguaran

guilleiguaran May 15, 2017

Member

Not sure if I'm doing something wrong but this fails:

Kernel.system(cmd, *args, out: out)

while this works correctly:

Kernel.system("#{cmd} #{args.join(' ')}", out: out)

@guilleiguaran

guilleiguaran May 15, 2017

Member

Not sure if I'm doing something wrong but this fails:

Kernel.system(cmd, *args, out: out)

while this works correctly:

Kernel.system("#{cmd} #{args.join(' ')}", out: out)

This comment has been minimized.

@matthewd

matthewd May 15, 2017

Member

My guess would be the quoting on statement, which isn't required for the splat form.

@matthewd

matthewd May 15, 2017

Member

My guess would be the quoting on statement, which isn't required for the splat form.

This comment has been minimized.

@guilleiguaran

guilleiguaran May 15, 2017

Member

yup, that worked 👍

@guilleiguaran

guilleiguaran May 15, 2017

Member

yup, that worked 👍

@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran May 15, 2017

Member

@matthewd I've done all the suggested changes in SQLite rake task 🙌

Member

guilleiguaran commented May 15, 2017

@matthewd I've done all the suggested changes in SQLite rake task 🙌

Rusty Geldmacher and others added some commits May 12, 2017

Respect `ignore_tables` in Postgres structure dump
When using `sql` as the schema format, or even just doing `rake
db:structure:dump`, it would be good to respect the list of ignored
tables that has been configured.
Improvements for SQLite rake task.
* Use NOT IN in SQL query
* Quote table names propertly
* Use array form of command invocation

@guilleiguaran guilleiguaran merged commit d7a0d28 into master May 16, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
codeclimate no new or fixed issues
Details

@guilleiguaran guilleiguaran deleted the ignore-tables-in-sql-dump branch May 16, 2017

kamipo added a commit to kamipo/rails that referenced this pull request Jun 1, 2017

Should use `quote` for a string literal
Follow up of #29077.

Before:

```sql
SELECT sql FROM sqlite_master WHERE tbl_name NOT IN ("foo") ORDER BY tbl_name, type DESC, name
```

After:

```sql
SELECT sql FROM sqlite_master WHERE tbl_name NOT IN ('foo') ORDER BY tbl_name, type DESC, name
```

> If a keyword in double quotes (ex: "key" or "glob") is used in a
context where it cannot be resolved to an identifier but where a string
literal is allowed, then the token is understood to be a string literal
instead of an identifier.

http://www.sqlite.org/lang_keywords.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment