Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Refactor db:structure:dump task. #6782

Merged
merged 1 commit into from

2 participants

@kennyj
Collaborator

I'm also inspired by #6761. I've moved the logic for db:structure:dump into the task classes.

FYI: I'm trying to solve #5547, and #6648 was my first PR for this problem.

@rafaelfranca

@kennyj seems fine, also remember to link the issue that you was trying to solve.

@rafaelfranca rafaelfranca commented on the diff
activerecord/lib/active_record/tasks/database_tasks.rb
@@ -60,6 +60,12 @@ def purge(configuration)
class_for_adapter(configuration['adapter']).new(configuration).purge
end
+ def structure_dump(*arguments)
@rafaelfranca Owner

as we only accepts two arguments I think that is better to use they explicitly here.

@kennyj Collaborator
kennyj added a note

My first version was more explicitly.
But in only sqlite testcase, we need to pass Rails.root ...

What do you think about the following signature ?

def structure_dump(configuration, filename, root = Rails.root)
@rafaelfranca Owner

I don't think that this will work. We really need to pass Rails.root to the initializer?

@kennyj Collaborator
kennyj added a note

If we don't pass root_path, we'll face the following an exception.

test_structure_dump(ActiveRecord::SqliteStructureDumpTest):
NameError: uninitialized constant ActiveRecord::Tasks::SQLiteDatabaseTasks::Rails
    /home/kennyj/rails/activerecord/lib/active_record/tasks/sqlite_database_tasks.rb:7:in `initialize'
...

and related code

  1 module ActiveRecord
  2   module Tasks # :nodoc:
  3     class SQLiteDatabaseTasks # :nodoc:
  4
  5       delegate :connection, :establish_connection, to: ActiveRecord::Base
  6
  7       def initialize(configuration, root = Rails.root)
  8         @configuration, @root = configuration, root
  9       end
 10

So when executing testcase in sqlite, I think we must pass a path to structure_dump method.
Please see also https://github.com/rails/rails/blob/master/activerecord/test/cases/sqlite_rake_test.rb (we always pass a path).

@kennyj Collaborator
kennyj added a note

Certainly, my snippet isn't work ;-)

@rafaelfranca Owner

I think that should be

def initialize(configuration, root = ::Rails.root)
  @configuration, @root = configuration, root
end
@kennyj Collaborator
kennyj added a note

NameError: uninitialized constant Rails ...

I guess we need this default argument to separate Rails from AR.

@rafaelfranca Owner

Ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca

Oops, this one is not trying to change the mysql dump task, is only a refactoring.

@kennyj
Collaborator

@rafaelfranca
What do you means ? Please tell me more detalis. I think I didn't change the behavior.

@rafaelfranca

@kennyj yes, I thought that you added the code for the mysqldump pull request.

@kennyj
Collaborator

@rafaelfranca You are assured of that problem. It is my next challenge :-)

@rafaelfranca

@kennyj all tests are passing?

@kennyj
Collaborator

rake test_sqlite3 test_mysql2 test_postgresql is all green.
And cd railties; ruby -Ilib:test test/application/rake_test.rb is also all green.

So I guess all tests are passing.

@rafaelfranca rafaelfranca merged commit 9edeb32 into from
@kennyj
Collaborator

thanks @rafaelfranca .
I'll try to structure:load tomorrow. good night :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 19, 2012
  1. @kennyj
This page is out of date. Refresh to see the latest.
View
16 activerecord/lib/active_record/railties/databases.rake
@@ -275,21 +275,11 @@ db_namespace = namespace :db do
abcs = ActiveRecord::Base.configurations
filename = ENV['DB_STRUCTURE'] || File.join(Rails.root, "db", "structure.sql")
case abcs[Rails.env]['adapter']
- when /mysql/, 'oci', 'oracle'
+ when /mysql/, /postgresql/, /sqlite/
+ ActiveRecord::Tasks::DatabaseTasks.structure_dump(abcs[Rails.env], filename)
+ when 'oci', 'oracle'
ActiveRecord::Base.establish_connection(abcs[Rails.env])
File.open(filename, "w:utf-8") { |f| f << ActiveRecord::Base.connection.structure_dump }
- when /postgresql/
- set_psql_env(abcs[Rails.env])
- search_path = abcs[Rails.env]['schema_search_path']
- unless search_path.blank?
- search_path = search_path.split(",").map{|search_path_part| "--schema=#{Shellwords.escape(search_path_part.strip)}" }.join(" ")
- end
- `pg_dump -i -s -x -O -f #{Shellwords.escape(filename)} #{search_path} #{Shellwords.escape(abcs[Rails.env]['database'])}`
- raise 'Error dumping database' if $?.exitstatus == 1
- File.open(filename, "a") { |f| f << "SET search_path TO #{ActiveRecord::Base.connection.schema_search_path};\n\n" }
- when /sqlite/
- dbfile = abcs[Rails.env]['database']
- `sqlite3 #{dbfile} .schema > #{filename}`
when 'sqlserver'
`smoscript -s #{abcs[Rails.env]['host']} -d #{abcs[Rails.env]['database']} -u #{abcs[Rails.env]['username']} -p #{abcs[Rails.env]['password']} -f #{filename} -A -U`
when "firebird"
View
6 activerecord/lib/active_record/tasks/database_tasks.rb
@@ -60,6 +60,12 @@ def purge(configuration)
class_for_adapter(configuration['adapter']).new(configuration).purge
end
+ def structure_dump(*arguments)
@rafaelfranca Owner

as we only accepts two arguments I think that is better to use they explicitly here.

@kennyj Collaborator
kennyj added a note

My first version was more explicitly.
But in only sqlite testcase, we need to pass Rails.root ...

What do you think about the following signature ?

def structure_dump(configuration, filename, root = Rails.root)
@rafaelfranca Owner

I don't think that this will work. We really need to pass Rails.root to the initializer?

@kennyj Collaborator
kennyj added a note

If we don't pass root_path, we'll face the following an exception.

test_structure_dump(ActiveRecord::SqliteStructureDumpTest):
NameError: uninitialized constant ActiveRecord::Tasks::SQLiteDatabaseTasks::Rails
    /home/kennyj/rails/activerecord/lib/active_record/tasks/sqlite_database_tasks.rb:7:in `initialize'
...

and related code

  1 module ActiveRecord
  2   module Tasks # :nodoc:
  3     class SQLiteDatabaseTasks # :nodoc:
  4
  5       delegate :connection, :establish_connection, to: ActiveRecord::Base
  6
  7       def initialize(configuration, root = Rails.root)
  8         @configuration, @root = configuration, root
  9       end
 10

So when executing testcase in sqlite, I think we must pass a path to structure_dump method.
Please see also https://github.com/rails/rails/blob/master/activerecord/test/cases/sqlite_rake_test.rb (we always pass a path).

@kennyj Collaborator
kennyj added a note

Certainly, my snippet isn't work ;-)

@rafaelfranca Owner

I think that should be

def initialize(configuration, root = ::Rails.root)
  @configuration, @root = configuration, root
end
@kennyj Collaborator
kennyj added a note

NameError: uninitialized constant Rails ...

I guess we need this default argument to separate Rails from AR.

@rafaelfranca Owner

Ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ configuration = arguments.first
+ filename = arguments.delete_at 1
+ class_for_adapter(configuration['adapter']).new(*arguments).structure_dump(filename)
+ end
+
private
def class_for_adapter(adapter)
View
5 activerecord/lib/active_record/tasks/mysql_database_tasks.rb
@@ -44,6 +44,11 @@ def charset
connection.charset
end
+ def structure_dump(filename)
+ establish_connection configuration
+ File.open(filename, "w:utf-8") { |f| f << ActiveRecord::Base.connection.structure_dump }
+ end
+
private
def configuration
View
22 activerecord/lib/active_record/tasks/postgresql_database_tasks.rb
@@ -1,3 +1,5 @@
+require 'shellwords'
+
module ActiveRecord
module Tasks # :nodoc:
class PostgreSQLDatabaseTasks # :nodoc:
@@ -33,6 +35,19 @@ def purge
create true
end
+ def structure_dump(filename)
+ set_psql_env
+ search_path = configuration['schema_search_path']
+ unless search_path.blank?
+ search_path = search_path.split(",").map{|search_path_part| "--schema=#{Shellwords.escape(search_path_part.strip)}" }.join(" ")
+ end
+
+ command = "pg_dump -i -s -x -O -f #{Shellwords.escape(filename)} #{search_path} #{Shellwords.escape(configuration['database'])}"
+ raise 'Error dumping database' unless Kernel.system(command)
+
+ File.open(filename, "a") { |f| f << "SET search_path TO #{ActiveRecord::Base.connection.schema_search_path};\n\n" }
+ end
+
private
def configuration
@@ -49,6 +64,13 @@ def establish_master_connection
'schema_search_path' => 'public'
)
end
+
+ def set_psql_env
+ ENV['PGHOST'] = configuration['host'] if configuration['host']
+ ENV['PGPORT'] = configuration['port'].to_s if configuration['port']
+ ENV['PGPASSWORD'] = configuration['password'].to_s if configuration['password']
+ ENV['PGUSER'] = configuration['username'].to_s if configuration['username']
+ end
end
end
end
View
5 activerecord/lib/active_record/tasks/sqlite_database_tasks.rb
@@ -31,6 +31,11 @@ def charset
connection.encoding
end
+ def structure_dump(filename)
+ dbfile = configuration['database']
+ `sqlite3 #{dbfile} .schema > #{filename}`
+ end
+
private
def configuration
View
36 activerecord/test/cases/database_tasks_test.rb
@@ -303,7 +303,7 @@ def setup
returns @postgresql_tasks
ActiveRecord::Tasks::SQLiteDatabaseTasks.stubs(:new).returns @sqlite_tasks
end
-
+
def test_mysql_charset
@mysql_tasks.expects(:charset)
@@ -328,4 +328,38 @@ def test_sqlite_charset
ActiveRecord::Tasks::DatabaseTasks.charset 'adapter' => 'sqlite3'
end
end
+
+ class DatabaseTasksStructureDumpTest < ActiveRecord::TestCase
+ def setup
+ @mysql_tasks, @postgresql_tasks, @sqlite_tasks = stub, stub, stub
+ ActiveRecord::Tasks::MySQLDatabaseTasks.stubs(:new).returns @mysql_tasks
+ ActiveRecord::Tasks::PostgreSQLDatabaseTasks.stubs(:new).
+ returns @postgresql_tasks
+ ActiveRecord::Tasks::SQLiteDatabaseTasks.stubs(:new).returns @sqlite_tasks
+ end
+
+ def test_mysql_structure_dump
+ @mysql_tasks.expects(:structure_dump).with("awesome-file.sql")
+
+ ActiveRecord::Tasks::DatabaseTasks.structure_dump({'adapter' => 'mysql'}, "awesome-file.sql")
+ end
+
+ def test_mysql2_structure_dump
+ @mysql_tasks.expects(:structure_dump).with("awesome-file.sql")
+
+ ActiveRecord::Tasks::DatabaseTasks.structure_dump({'adapter' => 'mysql2'}, "awesome-file.sql")
+ end
+
+ def test_postgresql_structure_dump
+ @postgresql_tasks.expects(:structure_dump).with("awesome-file.sql")
+
+ ActiveRecord::Tasks::DatabaseTasks.structure_dump({'adapter' => 'postgresql'}, "awesome-file.sql")
+ end
+
+ def test_sqlite_structure_dump
+ @sqlite_tasks.expects(:structure_dump).with("awesome-file.sql")
+
+ ActiveRecord::Tasks::DatabaseTasks.structure_dump({'adapter' => 'sqlite3'}, "awesome-file.sql")
+ end
+ end
end
View
24 activerecord/test/cases/mysql_rake_test.rb
@@ -194,4 +194,28 @@ def test_db_retrieves_charset
ActiveRecord::Tasks::DatabaseTasks.charset @configuration
end
end
+
+ class MySQLStructureDumpTest < ActiveRecord::TestCase
+ def setup
+ @connection = stub(:structure_dump => true)
+ @configuration = {
+ 'adapter' => 'mysql',
+ 'database' => 'test-db'
+ }
+
+ ActiveRecord::Base.stubs(:connection).returns(@connection)
+ ActiveRecord::Base.stubs(:establish_connection).returns(true)
+ end
+
+ def test_structure_dump
+ filename = "awesome-file.sql"
+ ActiveRecord::Base.expects(:establish_connection).with(@configuration)
+ @connection.expects(:structure_dump)
+
+ ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, filename)
+ assert File.exists?(filename)
+ ensure
+ FileUtils.rm(filename)
+ end
+ end
end
View
25 activerecord/test/cases/postgresql_rake_test.rb
@@ -150,4 +150,29 @@ def test_db_retrieves_charset
ActiveRecord::Tasks::DatabaseTasks.charset @configuration
end
end
+
+ class PostgreSQLStructureDumpTest < ActiveRecord::TestCase
+ def setup
+ @connection = stub(:structure_dump => true)
+ @configuration = {
+ 'adapter' => 'postgresql',
+ 'database' => 'my-app-db'
+ }
+
+ ActiveRecord::Base.stubs(:connection).returns(@connection)
+ ActiveRecord::Base.stubs(:establish_connection).returns(true)
+ Kernel.stubs(:system)
+ end
+
+ def test_structure_dump
+ filename = "awesome-file.sql"
+ Kernel.expects(:system).with("pg_dump -i -s -x -O -f #{filename} my-app-db").returns(true)
+ @connection.expects(:schema_search_path).returns("foo")
+
+ ActiveRecord::Tasks::DatabaseTasks.structure_dump(@configuration, filename)
+ assert File.exists?(filename)
+ ensure
+ FileUtils.rm(filename)
+ end
+ end
end
View
22 activerecord/test/cases/sqlite_rake_test.rb
@@ -123,4 +123,26 @@ def test_db_retrieves_charset
ActiveRecord::Tasks::DatabaseTasks.charset @configuration, '/rails/root'
end
end
+
+ class SqliteStructureDumpTest < ActiveRecord::TestCase
+ def setup
+ @database = "db_create.sqlite3"
+ @configuration = {
+ 'adapter' => 'sqlite3',
+ 'database' => @database
+ }
+ end
+
+ def test_structure_dump
+ dbfile = @database
+ filename = "awesome-file.sql"
+
+ ActiveRecord::Tasks::DatabaseTasks.structure_dump @configuration, filename, '/rails/root'
+ assert File.exists?(dbfile)
+ assert File.exists?(filename)
+ ensure
+ FileUtils.rm(filename)
+ FileUtils.rm(dbfile)
+ end
+ end
end
Something went wrong with that request. Please try again.