Fixed support for DATABASE_URL for rake db tasks #7521

Merged
merged 1 commit into from Sep 12, 2012

Conversation

Projects
None yet
7 participants
Contributor

graceliu commented Sep 4, 2012

As Aaron Patterson mentioned, 3.2+ should have support for a DATABASE_URL environment variable. (For background discussion please refer to https://groups.google.com/forum/#!topic/rubyonrails-core/ge1HCeBqz_s)

  • added tests to confirm establish_connection uses DATABASE_URL and
    Rails.env correctly even when no arguments are passed in.
  • updated rake db tasks to support DATABASE_URL, and added tests to
    confirm correct behavior for these rake tasks. (Removed
    establish_connection call from some tasks since in those cases
    the :environment task already made sure the function would be called)
  • updated Resolver so that when it resolves the database url, it
    removes hash values with empty strings from the config spec (e.g.
    to support connection to postgresql when no username is specified).

This fix can also address issues #6736 and #6833 (as slightly different implementation solution).

...record/lib/active_record/connection_adapters/connection_specification.rb
@@ -72,7 +72,7 @@ def connection_url_to_hash(url) # :nodoc:
:port => config.port,
:database => config.path.sub(%r{^/},""),
:host => config.host }
- spec.reject!{ |_,value| !value }
+ spec.reject!{ |_,value| (!value || (value == ""))}
@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

Can we use value.blank? here?

activerecord/lib/active_record/tasks/database_tasks.rb
@@ -1,8 +1,11 @@
module ActiveRecord
module Tasks # :nodoc:
module DatabaseTasks # :nodoc:
+
@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

Could you not add this blank line please?

activerecord/lib/active_record/tasks/database_tasks.rb
@@ -14,6 +17,14 @@ def register_task(pattern, task)
register_task(/postgresql/, ActiveRecord::Tasks::PostgreSQLDatabaseTasks)
register_task(/sqlite/, ActiveRecord::Tasks::SQLiteDatabaseTasks)
+ def set_current_config(env = Rails.env)
+ if ENV['DATABASE_URL']
@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

I think this is better:

@current_config ||= if ENV['DATABASE_URL']
                      database_url_config
                    else
                      ActiveRecord::Base.configurations[env]
                    end
activerecord/test/cases/connection_handling_test.rb
+ class ConnectionHandlingTest < ActiveRecord::TestCase
+ def test_establish_connection_with_no_arguments_passed_in
+
+ require "rails"
@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

We should not require rails here

@graceliu

graceliu Sep 5, 2012

Contributor

I added it because without it, Rails.env would be undefined, and the test is demonstrating that the config of the established connection is the same as that of Rails.env.
Sorry, I'm new to contributing to Rails and developing in RoR in general; can you explain why require rails should not be here?
Then, I can think about alternative solutions.

@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

If you need to test something using Rails.env then you should test the behavior in railties. The tests in the frameworks should not know about the Rails

@graceliu

graceliu Sep 5, 2012

Contributor

Ok. Will do. I'll make all the changes requested and get back to you when I am done.

activerecord/test/cases/connection_handling_test.rb
+ end
+ end
+end
+
@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

Remove the empty lines

+ set_database_url
+ db_test_load_structure
+ end
+
@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

Remove this empty line

railties/test/application/rake/dbs_test.rb
+
+ def db_structure_dump_and_load
+ Dir.chdir(app_path) do
+ `rails generate model book title:string`
@rafaelfranca

rafaelfranca Sep 5, 2012

Owner

The indentation is wrong here

Member

schneems commented Sep 5, 2012

Linking issues #6736 and #6833.

dball commented Sep 5, 2012

Should the environmental variables used or allowed by rails adhere to a common naming convention? I might have expected to see RAILS_DATABASE_URL.

Member

steveklabnik commented Sep 5, 2012

Heroku (and others) already use DATABASE_URL.

Contributor

graceliu commented Sep 6, 2012

Sorry. Please wait for me to say that all changes have passed test re-run before reviewing new commit.

Member

schneems commented Sep 6, 2012

Sure thing. Just leave a comment when you are ready or if you have questions or need help.

Richard Schneeman
http://heroku.com
@schneems

Sent from the road

On Wednesday, September 5, 2012 at 5:10 PM, graceliu wrote:

Sorry. Please wait for me to say that all changes have passed test re-run before reviewing new commit.


Reply to this email directly or view it on GitHub (#7521 (comment)).

Contributor

graceliu commented Sep 6, 2012

Thank you!
I have verified the changes with test re-run and the code is ready for review.
(Note that in addition to the changes requested, I changed verification of connected database to use assert_match instead of assert_equal, since the connected database name may include the full path, whereas the config database name contains only the relative path.)

...record/lib/active_record/connection_adapters/connection_specification.rb
@@ -72,7 +72,7 @@ def connection_url_to_hash(url) # :nodoc:
:port => config.port,
:database => config.path.sub(%r{^/},""),
:host => config.host }
- spec.reject!{ |_,value| !value }
+ spec.reject!{ |_,value| (!value || value.blank?)}
@lexmag

lexmag Sep 6, 2012

Contributor

Might we use only blank?

Contributor

graceliu commented Sep 6, 2012

Yes. (Now I see that's what Rafael meant.)
Will comment again when the tests are re-run and fix is ready for review.

Contributor

graceliu commented Sep 9, 2012

I have verified changes with test re-run and the code is ready for review.

activerecord/lib/active_record/tasks/database_tasks.rb
@@ -3,6 +3,8 @@ module Tasks # :nodoc:
module DatabaseTasks # :nodoc:
extend self
+ attr_accessor :current_config
@rafaelfranca

rafaelfranca Sep 9, 2012

Owner

Since we are not using the current_config= method this should be only attr_reader

@rafaelfranca

rafaelfranca Sep 9, 2012

Owner

Sorry, we are using. I think is better to define only the attr_writer and rename the set_current_config to def current_config(env = Rails.env)

Member

schneems commented Sep 9, 2012

👍 I was able to run your branch connecting to a remote postgres server locally using the env var DATABASE_URL. I was able to migrate the db, add records, and query the db all using the env var. If there is anything else, specifically you want tested out let me know, otherwise everything checks out for me.

Owner

rafaelfranca commented Sep 10, 2012

@schneems thanks to test it. I'll merge it today

Owner

rafaelfranca commented Sep 10, 2012

@graceliu could you work in the current_config think that I commented and add a CHANGELOG entry?

Also, we will need a pull request to 3-2-stable too

Contributor

graceliu commented Sep 10, 2012

@schneems Thanks for your help!

@rafaelfranca:

My latest commit doesn't add the CHANGELOG entry yet; will fix that soon.

Thanks very much for your advice on current_config. Note that the default definition for the current_config writer method was being used in rake db:test:load_structure. This is why I originally had a separate method for set_current_config. Based on your feedback I've folded set_current_config into the current_config writer method.

  • when current_config is called from rake db:test:load_structure, the config is set to 'test', rather than based on environment variables (and then reset back to nil to support any ensuing tasks)
  • when current_config is called elsewhere, the config is set based on environment variables

I have validated this fix by re-running the rake db tasks tests in dbs_test.rb.

Let me know if there's any more changes required? If not, I'll re-run the test suite and let you know when I am done.

Owner

rafaelfranca commented Sep 10, 2012

Seems good 👍

Contributor

graceliu commented Sep 11, 2012

CHANGELOG updated and test suite were re-run w/ no problems

re support for 3-2-stable: I will work on this as soon as I can

Member

steveklabnik commented Sep 11, 2012

It's out of date again. Damn CHANGELOG.

Contributor

graceliu commented Sep 11, 2012

I'm a little confused. Let me know if I need to do something?

Member

steveklabnik commented Sep 12, 2012

Sorry, what I mean is that you'll need to rebase this again.

fixed support for DATABASE_URL for rake db tasks
- added tests to confirm establish_connection uses DATABASE_URL and
  Rails.env correctly even when no arguments are passed in.
- updated rake db tasks to support DATABASE_URL, and added tests to
  confirm correct behavior for these rake tasks.  (Removed
  establish_connection call from some tasks since in those cases
  the :environment task already made sure the function would be called)
- updated Resolver so that when it resolves the database url, it
  removes hash values with empty strings from the config spec (e.g.
  to support connection to postgresql when no username is specified).
Contributor

graceliu commented Sep 12, 2012

Got it. Done.

@graceliu graceliu closed this Sep 12, 2012

@graceliu graceliu reopened this Sep 12, 2012

rafaelfranca added a commit that referenced this pull request Sep 12, 2012

Merge pull request #7521 from graceliu/fix_database_url_support
Fixed support for DATABASE_URL for rake db tasks

@rafaelfranca rafaelfranca merged commit 9a81866 into rails:master Sep 12, 2012

Owner

rafaelfranca commented Sep 12, 2012

Thank you

graceliu added a commit to graceliu/rails that referenced this pull request Oct 22, 2012

fixed support for DATABASE_URL for rake db tasks
backport for #7521
- added tests to confirm establish_connection uses DATABASE_URL and
  Rails.env correctly even when no arguments are passed in.
- updated rake db tasks to support DATABASE_URL, and added tests to
  confirm correct behavior for these rake tasks.  (Removed
  establish_connection call from some tasks since in those cases
  the :environment task already made sure the function would be called)
- updated Resolver so that when it resolves the database url, it
  removes hash values with empty strings from the config spec (e.g.
  to support connection to postgresql when no username is specified).
- updated ResolverTest to use current_adapter? to check the type of
  the current adapter.

rafaelfranca added a commit that referenced this pull request Oct 29, 2012

fixed support for DATABASE_URL for rake db tasks
Backport for #7521

- added tests to confirm establish_connection uses DATABASE_URL and
  Rails.env correctly even when no arguments are passed in.
- updated rake db tasks to support DATABASE_URL, and added tests to
  confirm correct behavior for these rake tasks.  (Removed
  establish_connection call from some tasks since in those cases
  the :environment task already made sure the function would be called)
- updated Resolver so that when it resolves the database url, it
  removes hash values with empty strings from the config spec (e.g.
  to support connection to postgresql when no username is specified).
- updated ResolverTest to use current_adapter? to check the type of
  the current adapter.
+ db_test_load_structure
+ end
+
+ test 'db:test:load_structure with database_url' do
@josevalim

josevalim Mar 2, 2013

Contributor

This test is not actually testing we are using the database_url because Rails.application.config.database_configuration is still being read and set into ActiveRecord::Base.configurations on line 159.

Contributor

josevalim commented Mar 2, 2013

I disagree with the general direction of this patch. This patch has sprinkled if ENV["DATABASE_URL"] checks throughout Active Record code base what ideally we would like to handle those transparently via an uniform API. We already have the logic that converts an URL to a Hash specification, why all the work arounds?!

Contributor

josevalim commented Mar 2, 2013

Just a warning: this pull request is mostly incomplete and can be misleading. If an user uses the rake task to retrieve the charset, Active Record is still going to use config/database.yml (via ActiveRecord::Base.configurations) to lookup the DB information, completely ignoring DATABASE_URL. This happens because this patch chose to attack some particular areas and explicitly check for ENV["DATABASE_URL"], while the ideal solution is to ensure the DATABASE_URL configuration ends up at ActiveRecord::Base.configurations. Ideally, there should be only one place where DATABASE_URL is mentioned, everything else should work transparently.

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