From a5664fe27b1797983537c0764003e618bd3d2801 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Mon, 7 Apr 2014 18:52:21 -0700 Subject: Follow up to bbe7fe41 to fix enum leakage across classes. The original attempt didn't really fix the problem and wasn't testing the problematic area. This commit corrected those issues in the original commit. Also removed the private `enum_mapping_for` method. As `defined_enums` is now a method, this method doesn't provide much value anymore. --- activerecord/lib/active_record/enum.rb | 9 ++++++--- activerecord/lib/active_record/validations/uniqueness.rb | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 167f8b7a49..6e0d19ad31 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -1,3 +1,5 @@ +require 'active_support/core_ext/object/deep_dup' + module ActiveRecord # Declare an enum attribute where the values map to integers in the database, # but can be queried by name. Example: @@ -70,8 +72,9 @@ module ActiveRecord base.defined_enums = {} end - def enum_mapping_for(attr_name) # :nodoc: - defined_enums[attr_name.to_s] + def inherited(base) + base.defined_enums = defined_enums.deep_dup + super end def enum(definitions) @@ -136,7 +139,7 @@ module ActiveRecord mod = Module.new do private def save_changed_attribute(attr_name, value) - if (mapping = self.class.enum_mapping_for(attr_name)) + if (mapping = self.class.defined_enums[attr_name]) if attribute_changed?(attr_name) old = changed_attributes[attr_name] diff --git a/activerecord/lib/active_record/validations/uniqueness.rb b/activerecord/lib/active_record/validations/uniqueness.rb index 71c71cb4b1..ee080451a9 100644 --- a/activerecord/lib/active_record/validations/uniqueness.rb +++ b/activerecord/lib/active_record/validations/uniqueness.rb @@ -93,7 +93,7 @@ module ActiveRecord end def map_enum_attribute(klass, attribute, value) - mapping = klass.enum_mapping_for(attribute.to_s) + mapping = klass.defined_enums[attribute.to_s] value = mapping[value] if value && mapping value end -- cgit v1.2.3 From 77692ff9c32c1b4999bd4ff51af6b84186729478 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Mon, 7 Apr 2014 19:08:08 -0700 Subject: Ensure we are looking with string keys --- activerecord/lib/active_record/enum.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb index 6e0d19ad31..18f1ca26de 100644 --- a/activerecord/lib/active_record/enum.rb +++ b/activerecord/lib/active_record/enum.rb @@ -139,7 +139,7 @@ module ActiveRecord mod = Module.new do private def save_changed_attribute(attr_name, value) - if (mapping = self.class.defined_enums[attr_name]) + if (mapping = self.class.defined_enums[attr_name.to_s]) if attribute_changed?(attr_name) old = changed_attributes[attr_name] -- cgit v1.2.3 From 2b817cde622e625f08a638d909f3e9b12f0b3066 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Tue, 8 Apr 2014 03:54:23 +0930 Subject: Only apply DATABASE_URL for Rails.env As we like ENV vars, also support DATABASE_URL_#{env}, for more obscure use cases. --- .../lib/active_record/connection_handling.rb | 42 ++++++++-------------- 1 file changed, 14 insertions(+), 28 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index bbb866cedf..5a83797ee5 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -58,9 +58,8 @@ module ActiveRecord end class MergeAndResolveDefaultUrlConfig # :nodoc: - def initialize(raw_configurations, url = ENV['DATABASE_URL']) + def initialize(raw_configurations) @raw_config = raw_configurations.dup - @url = url end # Returns fully resolved connection hashes. @@ -71,35 +70,22 @@ module ActiveRecord private def config - if @url - raw_merged_into_default - else - @raw_config + cfg = Hash.new do |hash, key| + entry = @raw_config[key] + env_url = nil + if key.to_s == DEFAULT_ENV.call.to_s + env_url = ENV["DATABASE_URL"] + end + env_url ||= ENV["DATABASE_URL_#{key.upcase}"] + entry ||= {} if env_url + entry.merge!("url" => env_url) { |h, v1, v2| v1 || v2 } if entry.is_a?(Hash) + hash[key] = entry if entry end - end - def raw_merged_into_default - default = default_url_hash + @raw_config.keys.each {|k| cfg[k] } + cfg[DEFAULT_ENV.call.to_s] - @raw_config.each do |env, values| - default[env] = values || {} - default[env].merge!("url" => @url) { |h, v1, v2| v1 || v2 } if default[env].is_a?(Hash) - end - default - end - - # When the raw configuration is not present and ENV['DATABASE_URL'] - # is available we return a hash with the connection information in - # the connection URL. This hash responds to any string key with - # resolved connection information. - def default_url_hash - Hash.new do |hash, key| - hash[key] = if key.is_a? String - ActiveRecord::ConnectionAdapters::ConnectionSpecification::ConnectionUrlResolver.new(@url).to_hash - else - nil - end - end + cfg end end -- cgit v1.2.3 From e533826f483d6a1f45d044a2017ff4e9d19a154e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 7 Apr 2014 19:05:50 -0300 Subject: Make sure DEFAULT_URL only override current environment database configuration --- activerecord/lib/active_record/connection_handling.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 5a83797ee5..567d121091 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -78,7 +78,7 @@ module ActiveRecord end env_url ||= ENV["DATABASE_URL_#{key.upcase}"] entry ||= {} if env_url - entry.merge!("url" => env_url) { |h, v1, v2| v1 || v2 } if entry.is_a?(Hash) + entry.merge!("url" => env_url) { |h, v1, v2| v1 || v2 } if entry.is_a?(Hash) && env_url hash[key] = entry if entry end -- cgit v1.2.3 From 2b6d1da6b4f2a5dc1d51223ddb33b86ceab98479 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 7 Apr 2014 19:17:06 -0300 Subject: Only call DEFAULT_ENV proc one time --- activerecord/lib/active_record/connection_handling.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 567d121091..a1fb83fde5 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -70,10 +70,13 @@ module ActiveRecord private def config + env = DEFAULT_ENV.call.to_s + cfg = Hash.new do |hash, key| entry = @raw_config[key] env_url = nil - if key.to_s == DEFAULT_ENV.call.to_s + + if key.to_s == env env_url = ENV["DATABASE_URL"] end env_url ||= ENV["DATABASE_URL_#{key.upcase}"] @@ -83,7 +86,7 @@ module ActiveRecord end @raw_config.keys.each {|k| cfg[k] } - cfg[DEFAULT_ENV.call.to_s] + cfg[env] cfg end -- cgit v1.2.3 From d035ba143a46e64e6110b6a690e0655e8574fd49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 7 Apr 2014 19:17:43 -0300 Subject: Check env_url only once --- activerecord/lib/active_record/connection_handling.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index a1fb83fde5..08849b3722 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -79,9 +79,14 @@ module ActiveRecord if key.to_s == env env_url = ENV["DATABASE_URL"] end + env_url ||= ENV["DATABASE_URL_#{key.upcase}"] - entry ||= {} if env_url - entry.merge!("url" => env_url) { |h, v1, v2| v1 || v2 } if entry.is_a?(Hash) && env_url + + if env_url + entry ||= {} + entry.merge!("url" => env_url) { |h, v1, v2| v1 || v2 } if entry.is_a?(Hash) + end + hash[key] = entry if entry end -- cgit v1.2.3 From 0ec047533a1d1ed14e001cfe7fdee818c0c18124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 7 Apr 2014 19:19:28 -0300 Subject: entry is always a Hash --- activerecord/lib/active_record/connection_handling.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 08849b3722..1c8fbcb625 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -84,7 +84,7 @@ module ActiveRecord if env_url entry ||= {} - entry.merge!("url" => env_url) { |h, v1, v2| v1 || v2 } if entry.is_a?(Hash) + entry.merge!("url" => env_url) { |h, v1, v2| v1 || v2 } end hash[key] = entry if entry -- cgit v1.2.3 From de9f2f63b8548e2a8950c1727f0a1a6893713505 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Tue, 8 Apr 2014 14:49:03 +0930 Subject: Rearrange the config merger some more This seems to simplify the operative part. Most importantly, by pre-loading all the configs supplied in ENV, we ensure the list is complete: if the developer specifies an unknown config, the exception includes a list of valid ones. --- .../lib/active_record/connection_handling.rb | 35 ++++++++++------------ 1 file changed, 16 insertions(+), 19 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 1c8fbcb625..f83829bb26 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -60,6 +60,7 @@ module ActiveRecord class MergeAndResolveDefaultUrlConfig # :nodoc: def initialize(raw_configurations) @raw_config = raw_configurations.dup + @env = DEFAULT_ENV.call.to_s end # Returns fully resolved connection hashes. @@ -70,30 +71,26 @@ module ActiveRecord private def config - env = DEFAULT_ENV.call.to_s - - cfg = Hash.new do |hash, key| - entry = @raw_config[key] - env_url = nil - - if key.to_s == env - env_url = ENV["DATABASE_URL"] + @raw_config.dup.tap do |cfg| + urls_in_environment.each do |key, url| + cfg[key] ||= {} + cfg[key]["url"] ||= url end + end + end - env_url ||= ENV["DATABASE_URL_#{key.upcase}"] - - if env_url - entry ||= {} - entry.merge!("url" => env_url) { |h, v1, v2| v1 || v2 } + def urls_in_environment + {}.tap do |mapping| + ENV.each do |k, v| + if k =~ /\ADATABASE_URL_(.*)/ + mapping[$1.downcase] = v + end end - hash[key] = entry if entry + # Check for this last, because it is prioritised over the + # longer "DATABASE_URL_#{@env}" spelling + mapping[@env] = ENV['DATABASE_URL'] if ENV['DATABASE_URL'] end - - @raw_config.keys.each {|k| cfg[k] } - cfg[env] - - cfg end end -- cgit v1.2.3 From 23692184bbcc2e411ed6bc6d1eaea09aa0f0474f Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Tue, 8 Apr 2014 15:00:38 +0930 Subject: Give a deprecation message even when the lookup fails If the supplied string doesn't contain a colon, it clearly cannot be a database URL. They must have intended to do a key lookup, so even though it failed, give the explanatory deprecation warning, and raise the exception that lists the known configs. Conveniently, this also simplifies our logical behaviour: if the string matches a known configuration, or doesn't contain a colon (and is therefore clearly not a URL), then we output a deprecation warning, and behave exactly as we would if it were a symbol. --- .../lib/active_record/connection_adapters/connection_specification.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_adapters/connection_specification.rb b/activerecord/lib/active_record/connection_adapters/connection_specification.rb index e0715f7ce9..0cc92fa397 100644 --- a/activerecord/lib/active_record/connection_adapters/connection_specification.rb +++ b/activerecord/lib/active_record/connection_adapters/connection_specification.rb @@ -220,10 +220,10 @@ module ActiveRecord # an environment key or a URL spec, so we have deprecated # this ambiguous behaviour and in the future this function # can be removed in favor of resolve_url_connection. - if configurations.key?(spec) + if configurations.key?(spec) || spec !~ /:/ ActiveSupport::Deprecation.warn "Passing a string to ActiveRecord::Base.establish_connection " \ "for a configuration lookup is deprecated, please pass a symbol (#{spec.to_sym.inspect}) instead" - resolve_connection(configurations[spec]) + resolve_symbol_connection(spec) else resolve_url_connection(spec) end -- cgit v1.2.3 From 615e0dcdf1391a8b71ad6556c2e7b9cedf6ffa12 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Wed, 9 Apr 2014 00:17:13 +0930 Subject: Less ambition, more deprecation The "DATABASE_URL_*" idea was moving in the wrong direction. Instead, let's deprecate the situation where we end up using ENV['DATABASE_URL'] at all: the Right Way is to explicitly include it in database.yml with ERB. --- .../lib/active_record/connection_handling.rb | 32 +++++++++++----------- 1 file changed, 16 insertions(+), 16 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index f83829bb26..3667a42864 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -72,24 +72,24 @@ module ActiveRecord private def config @raw_config.dup.tap do |cfg| - urls_in_environment.each do |key, url| - cfg[key] ||= {} - cfg[key]["url"] ||= url - end - end - end - - def urls_in_environment - {}.tap do |mapping| - ENV.each do |k, v| - if k =~ /\ADATABASE_URL_(.*)/ - mapping[$1.downcase] = v + if url = ENV['DATABASE_URL'] + if cfg[@env] + if cfg[@env]["url"] + # Nothing to do + else + ActiveSupport::Deprecation.warn "Overriding database configuration with DATABASE_URL without using an ERB tag in database.yml is deprecated. Please update the entry for #{@env.inspect}:\n\n" \ + " #{@env}:\n url: <%= ENV['DATABASE_URL'] %>\n\n"\ + "This will be required in Rails 4.2" + cfg[@env]["url"] = url + end + else + cfg[@env] = {} + ActiveSupport::Deprecation.warn "Supplying DATABASE_URL without a matching entry in database.yml is deprecated. Please add an entry for #{@env.inspect}:\n\n" \ + " #{@env}:\n url: <%= ENV['DATABASE_URL'] %>\n\n"\ + "This will be required in Rails 4.2" + cfg[@env]["url"] ||= url end end - - # Check for this last, because it is prioritised over the - # longer "DATABASE_URL_#{@env}" spelling - mapping[@env] = ENV['DATABASE_URL'] if ENV['DATABASE_URL'] end end end -- cgit v1.2.3 From 88cd65b174d9a75d9cb0e5e06a57615ccc80fff1 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Wed, 9 Apr 2014 00:55:49 +0930 Subject: Don't deprecate after all --- activerecord/lib/active_record/connection_handling.rb | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 3667a42864..31e7390bf7 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -73,22 +73,8 @@ module ActiveRecord def config @raw_config.dup.tap do |cfg| if url = ENV['DATABASE_URL'] - if cfg[@env] - if cfg[@env]["url"] - # Nothing to do - else - ActiveSupport::Deprecation.warn "Overriding database configuration with DATABASE_URL without using an ERB tag in database.yml is deprecated. Please update the entry for #{@env.inspect}:\n\n" \ - " #{@env}:\n url: <%= ENV['DATABASE_URL'] %>\n\n"\ - "This will be required in Rails 4.2" - cfg[@env]["url"] = url - end - else - cfg[@env] = {} - ActiveSupport::Deprecation.warn "Supplying DATABASE_URL without a matching entry in database.yml is deprecated. Please add an entry for #{@env.inspect}:\n\n" \ - " #{@env}:\n url: <%= ENV['DATABASE_URL'] %>\n\n"\ - "This will be required in Rails 4.2" - cfg[@env]["url"] ||= url - end + cfg[@env] ||= {} + cfg[@env]["url"] ||= url end end end -- cgit v1.2.3 From d72a0cbc807a14d3eec02a53317d11b9d9fa5815 Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Wed, 9 Apr 2014 01:41:11 +0930 Subject: Drop in @jeremy's new database.yml template text In passing, allow multi-word adapters to be referenced in a URL: underscored_name must become hyphened-name. --- .../lib/active_record/connection_adapters/connection_specification.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/connection_adapters/connection_specification.rb b/activerecord/lib/active_record/connection_adapters/connection_specification.rb index 0cc92fa397..a8ab52be74 100644 --- a/activerecord/lib/active_record/connection_adapters/connection_specification.rb +++ b/activerecord/lib/active_record/connection_adapters/connection_specification.rb @@ -33,7 +33,7 @@ module ActiveRecord def initialize(url) raise "Database URL cannot be empty" if url.blank? @uri = URI.parse(url) - @adapter = @uri.scheme + @adapter = @uri.scheme.gsub('-', '_') @adapter = "postgresql" if @adapter == "postgres" if @uri.opaque -- cgit v1.2.3