diff options
17 files changed, 318 insertions, 39 deletions
diff --git a/.rubocop.yml b/.rubocop.yml index de355eeb13..62fb263c9e 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -4,37 +4,40 @@ AllCops: # Two spaces, no tabs (for indentation). Style/IndentationWidth: - enabled: true + Enabled: true # No trailing whitespace. Style/TrailingWhitespace: - enabled: true + Enabled: true # Blank lines should not have any spaces. Style/TrailingBlankLines: - enabled: true + Enabled: true # Use Ruby >= 1.9 syntax for hashes. Prefer { a: :b } over { :a => :b }. Style/HashSyntax: - enabled: true + Enabled: true # Prefer &&/|| over and/or. Style/AndOr: - enabled: true + Enabled: true # Use my_method(my_arg) not my_method( my_arg ) or my_method my_arg. Lint/RequireParentheses: - enabled: true + Enabled: true Style/BracesAroundHashParameters: - enabled: true + Enabled: true Style/IndentationConsistency: - enabled: true + Enabled: true EnforcedStyle: rails Style/EmptyLinesAroundClassBody: - enabled: true + Enabled: true Style/EmptyLinesAroundModuleBody: - enabled: true + Enabled: true + +Lint/EndAlignment: + AlignWith: variable diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 1d0e4b32a3..590fe2f587 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,11 @@ +* Support calling the method `merge` in `scope`'s lambda. + + *Yasuhiro Sugino* + +* Fixes multi-parameter attributes conversion with invalid params. + + *Hiroyuki Ishii* + * Add newline between each migration in `structure.sql`. Keeps schema migration inserts as a single commit, but allows for easier diff --git a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb index e160460286..cbb4f0cff8 100644 --- a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb +++ b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb @@ -39,7 +39,7 @@ module ActiveRecord end def set_time_zone_without_conversion(value) - ::Time.zone.local_to_utc(value).in_time_zone + ::Time.zone.local_to_utc(value).in_time_zone if value end def map_avoiding_infinite_recursion(value) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 1d9579f1a8..dc5b305843 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -855,7 +855,7 @@ module ActiveRecord connection_id: object_id } if spec - payload[:class_name] = spec.name + payload[:spec_name] = spec.name payload[:config] = spec.config end @@ -896,7 +896,7 @@ module ActiveRecord # for (not necessarily the current class). def retrieve_connection(spec_name) #:nodoc: pool = retrieve_connection_pool(spec_name) - raise ConnectionNotEstablished, "No connection pool with id '#{spec_name}' found." unless pool + raise ConnectionNotEstablished, "No connection pool with '#{spec_name}' found." unless pool conn = pool.connection raise ConnectionNotEstablished, "No connection for '#{spec_name}' in connection pool" unless conn conn diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb index 45507e206a..cc606e4828 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb @@ -327,12 +327,12 @@ module ActiveRecord end # Returns the sequence name for a table's primary key or some other specified key. - def default_sequence_name(table_name, pk = nil) #:nodoc: - result = serial_sequence(table_name, pk || 'id') + def default_sequence_name(table_name, pk = 'id') #:nodoc: + result = serial_sequence(table_name, pk) return nil unless result Utils.extract_schema_qualified_name(result).to_s rescue ActiveRecord::StatementInvalid - PostgreSQL::Name.new(nil, "#{table_name}_#{pk || 'id'}_seq").to_s + PostgreSQL::Name.new(nil, "#{table_name}_#{pk}_seq").to_s end def serial_sequence(table, column) diff --git a/activerecord/lib/active_record/fixtures.rb b/activerecord/lib/active_record/fixtures.rb index 6d142285cd..0ec33f7d87 100644 --- a/activerecord/lib/active_record/fixtures.rb +++ b/activerecord/lib/active_record/fixtures.rb @@ -987,16 +987,11 @@ module ActiveRecord # When connections are established in the future, begin a transaction too @connection_subscriber = ActiveSupport::Notifications.subscribe('!connection.active_record') do |_, _, _, _, payload| - model_class = nil - begin - model_class = payload[:class_name].constantize if payload[:class_name] - rescue NameError - model_class = nil - end + spec_name = payload[:spec_name] if payload.key?(:spec_name) - if model_class + if spec_name begin - connection = ActiveRecord::Base.connection_handler.retrieve_connection(model_class) + connection = ActiveRecord::Base.connection_handler.retrieve_connection(spec_name) rescue ConnectionNotEstablished connection = nil end diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index 53ddd95bb0..f8dd35df0f 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -9,7 +9,7 @@ module ActiveRecord delegate :find_each, :find_in_batches, :in_batches, to: :all delegate :select, :group, :order, :except, :reorder, :limit, :offset, :joins, :left_joins, :left_outer_joins, :or, :where, :rewhere, :preload, :eager_load, :includes, :from, :lock, :readonly, - :having, :create_with, :uniq, :distinct, :references, :none, :unscope, to: :all + :having, :create_with, :uniq, :distinct, :references, :none, :unscope, :merge, to: :all delegate :count, :average, :minimum, :maximum, :sum, :calculate, to: :all delegate :pluck, :ids, to: :all diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 7ec0dfce7a..61a5c2c3b7 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -244,8 +244,9 @@ class HasManyAssociationsTest < ActiveRecord::TestCase def test_do_not_call_callbacks_for_delete_all car = Car.create(:name => 'honda') car.funky_bulbs.create! + assert_equal 1, car.funky_bulbs.count assert_nothing_raised { car.reload.funky_bulbs.delete_all } - assert_equal 0, Bulb.count, "bulbs should have been deleted using :delete_all strategy" + assert_equal 0, car.funky_bulbs.count, "bulbs should have been deleted using :delete_all strategy" end def test_delete_all_on_association_is_the_same_as_not_loaded diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb index 2bdabaee62..bbcb42d58e 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -347,8 +347,8 @@ module ActiveRecord payloads << payload end ActiveRecord::Base.establish_connection :arunit - assert_equal [:class_name, :config, :connection_id], payloads[0].keys.sort - assert_equal 'primary', payloads[0][:class_name] + assert_equal [:config, :connection_id, :spec_name], payloads[0].keys.sort + assert_equal 'primary', payloads[0][:spec_name] ensure ActiveSupport::Notifications.unsubscribe(subscription) if subscription end diff --git a/activerecord/test/cases/fixtures_test.rb b/activerecord/test/cases/fixtures_test.rb index a06aa4b247..df53bbf950 100644 --- a/activerecord/test/cases/fixtures_test.rb +++ b/activerecord/test/cases/fixtures_test.rb @@ -650,10 +650,10 @@ class TransactionalFixturesOnConnectionNotification < ActiveRecord::TestCase private def fire_connection_notification(connection) - ActiveRecord::Base.connection_handler.stubs(:retrieve_connection).with(Book).returns(connection) + ActiveRecord::Base.connection_handler.stubs(:retrieve_connection).with('book').returns(connection) message_bus = ActiveSupport::Notifications.instrumenter payload = { - class_name: 'Book', + spec_name: 'book', config: nil, connection_id: connection.object_id } diff --git a/activerecord/test/cases/multiparameter_attributes_test.rb b/activerecord/test/cases/multiparameter_attributes_test.rb index d05cb22740..59c340ceb7 100644 --- a/activerecord/test/cases/multiparameter_attributes_test.rb +++ b/activerecord/test/cases/multiparameter_attributes_test.rb @@ -202,6 +202,20 @@ class MultiParameterAttributeTest < ActiveRecord::TestCase Topic.reset_column_information end + def test_multiparameter_attributes_on_time_with_time_zone_aware_attributes_and_invalid_time_params + with_timezone_config aware_attributes: true do + Topic.reset_column_information + attributes = { + "written_on(1i)" => "2004", "written_on(2i)" => "", "written_on(3i)" => "" + } + topic = Topic.find(1) + topic.attributes = attributes + assert_nil topic.written_on + end + ensure + Topic.reset_column_information + end + def test_multiparameter_attributes_on_time_with_time_zone_aware_attributes_false with_timezone_config default: :local, aware_attributes: false, zone: -28800 do attributes = { diff --git a/activerecord/test/cases/query_cache_test.rb b/activerecord/test/cases/query_cache_test.rb index 03ec063671..085636553e 100644 --- a/activerecord/test/cases/query_cache_test.rb +++ b/activerecord/test/cases/query_cache_test.rb @@ -6,6 +6,8 @@ require 'models/post' require 'rack' class QueryCacheTest < ActiveRecord::TestCase + self.use_transactional_tests = false + fixtures :tasks, :topics, :categories, :posts, :categories_posts teardown do diff --git a/activerecord/test/cases/scoping/named_scoping_test.rb b/activerecord/test/cases/scoping/named_scoping_test.rb index 0e277ed235..f0dac07acc 100644 --- a/activerecord/test/cases/scoping/named_scoping_test.rb +++ b/activerecord/test/cases/scoping/named_scoping_test.rb @@ -46,6 +46,13 @@ class NamedScopingTest < ActiveRecord::TestCase assert_equal Topic.average(:replies_count), Topic.base.average(:replies_count) end + def test_calling_merge_at_first_in_scope + Topic.class_eval do + scope :calling_merge_at_first_in_scope, Proc.new { merge(Topic.replied) } + end + assert_equal Topic.calling_merge_at_first_in_scope.to_a, Topic.replied.to_a + end + def test_method_missing_priority_when_delegating klazz = Class.new(ActiveRecord::Base) do self.table_name = "topics" diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 6103857a41..3749dda9fc 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,36 @@ +* Introduce `assert_changes` and `assert_no_changes`. + + `assert_changes` is a more general `assert_difference` that works with any + value. + + assert_changes 'Error.current', from: nil, to: 'ERR' do + expected_bad_operation + end + + Can be called with strings, to be evaluated in the binding (context) of + the block given to the assertion, or a lambda. + + assert_changes -> { Error.current }, from: nil, to: 'ERR' do + expected_bad_operation + end + + The `from` and `to` arguments are compared with the case operator (`===`). + + assert_changes 'Error.current', from: nil, to: Error do + expected_bad_operation + end + + This is pretty useful, if you need to loosely compare a value. For example, + you need to test a token has been generated and it has that many random + characters. + + user = User.start_registration + assert_changes 'user.token', to: /\w{32}/ do + user.finish_registration + end + + *Genadi Samokovarov* + * Add `:fallback_string` option to `Array#to_sentence`. If an empty array calls the function and a fallback string option is set then it returns the fallback string other than an empty string. @@ -15,14 +48,14 @@ * `travel/travel_to` travel time helpers, now raise on nested calls, as this can lead to confusing time stubbing. - + Instead of: - + travel_to 2.days.from_now do # 2 days from today travel_to 3.days.from_now do # 5 days from today - end + end end preferred way to achieve above is: @@ -30,13 +63,12 @@ travel 2.days do # 2 days from today end - - travel 5.days do - # 5 days from today - end - + + travel 5.days do + # 5 days from today + end + *Vipul A M* - * Support parsing JSON time in ISO8601 local time strings in `ActiveSupport::JSON.decode` when `parse_json_times` is enabled. diff --git a/activesupport/lib/active_support/testing/assertions.rb b/activesupport/lib/active_support/testing/assertions.rb index ad83638572..7770aa8006 100644 --- a/activesupport/lib/active_support/testing/assertions.rb +++ b/activesupport/lib/active_support/testing/assertions.rb @@ -1,6 +1,8 @@ module ActiveSupport module Testing module Assertions + UNTRACKED = Object.new # :nodoc: + # Asserts that an expression is not truthy. Passes if <tt>object</tt> is # +nil+ or +false+. "Truthy" means "considered true in a conditional" # like <tt>if foo</tt>. @@ -92,6 +94,93 @@ module ActiveSupport def assert_no_difference(expression, message = nil, &block) assert_difference expression, 0, message, &block end + + # Assertion that the result of evaluating an expression is changed before + # and after invoking the passed in block. + # + # assert_changes 'Status.all_good?' do + # post :create, params: { status: { ok: false } } + # end + # + # You can pass the block as a string to be evaluated in the context of + # the block. A lambda can be passed for the block as well. + # + # assert_changes -> { Status.all_good? } do + # post :create, params: { status: { ok: false } } + # end + # + # The assertion is useful to test side effects. The passed block can be + # anything that can be converted to string with #to_s. + # + # assert_changes :@object do + # @object = 42 + # end + # + # The keyword arguments :from and :to can be given to specify the + # expected initial value and the expected value after the block was + # executed. + # + # assert_changes :@object, from: nil, to: :foo do + # @object = :foo + # end + # + # An error message can be specified. + # + # assert_changes -> { Status.all_good? }, 'Expected the status to be bad' do + # post :create, params: { status: { incident: true } } + # end + def assert_changes(expression, message = nil, from: UNTRACKED, to: UNTRACKED, &block) + exp = expression.respond_to?(:call) ? expression : -> { eval(expression.to_s, block.binding) } + + before = exp.call + retval = yield + + unless from == UNTRACKED + error = "#{expression.inspect} isn't #{from}" + error = "#{message}.\n#{error}" if message + assert from === before, error + end + + after = exp.call + + if to == UNTRACKED + error = "#{expression.inspect} didn't changed" + error = "#{message}.\n#{error}" if message + assert_not_equal before, after, error + else + message = "#{expression.inspect} didn't change to #{to}" + error = "#{message}.\n#{error}" if message + assert to === after, error + end + + retval + end + + # Assertion that the result of evaluating an expression is changed before + # and after invoking the passed in block. + # + # assert_no_changes 'Status.all_good?' do + # post :create, params: { status: { ok: true } } + # end + # + # An error message can be specified. + # + # assert_no_changes -> { Status.all_good? }, 'Expected the status to be good' do + # post :create, params: { status: { ok: false } } + # end + def assert_no_changes(expression, message = nil, &block) + exp = expression.respond_to?(:call) ? expression : -> { eval(expression.to_s, block.binding) } + + before = exp.call + retval = yield + after = exp.call + + error = "#{expression.inspect} did change to #{after}" + error = "#{message}.\n#{error}" if message + assert_equal before, after, error + + retval + end end end end diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb index 8002c14539..d4dec81b28 100644 --- a/activesupport/test/caching_test.rb +++ b/activesupport/test/caching_test.rb @@ -25,6 +25,18 @@ module ActiveSupport assert_nil LocalCacheRegistry.cache_for(key) end + def test_local_cache_cleared_and_response_should_be_present_on_invalid_parameters_error + key = "super awesome key" + assert_nil LocalCacheRegistry.cache_for key + middleware = Middleware.new('<3', key).new(->(env) { + assert LocalCacheRegistry.cache_for(key), 'should have a cache' + raise Rack::Utils::InvalidParameterError + }) + response = middleware.call({}) + assert response, 'response should exist' + assert_nil LocalCacheRegistry.cache_for(key) + end + def test_local_cache_cleared_on_exception key = "super awesome key" assert_nil LocalCacheRegistry.cache_for key @@ -128,6 +140,16 @@ class CacheKeyTest < ActiveSupport::TestCase end class CacheStoreSettingTest < ActiveSupport::TestCase + def test_memory_store_gets_created_if_no_arguments_passed_to_lookup_store_method + store = ActiveSupport::Cache.lookup_store + assert_kind_of(ActiveSupport::Cache::MemoryStore, store) + end + + def test_memory_store + store = ActiveSupport::Cache.lookup_store :memory_store + assert_kind_of(ActiveSupport::Cache::MemoryStore, store) + end + def test_file_fragment_cache_store store = ActiveSupport::Cache.lookup_store :file_store, "/path/to/cache/directory" assert_kind_of(ActiveSupport::Cache::FileStore, store) diff --git a/activesupport/test/test_case_test.rb b/activesupport/test/test_case_test.rb index 18228a2ac5..772c3cfca7 100644 --- a/activesupport/test/test_case_test.rb +++ b/activesupport/test/test_case_test.rb @@ -111,6 +111,112 @@ class AssertDifferenceTest < ActiveSupport::TestCase end end end + + def test_assert_changes_pass + assert_changes '@object.num' do + @object.increment + end + end + + def test_assert_changes_pass_with_lambda + assert_changes -> { @object.num } do + @object.increment + end + end + + def test_assert_changes_with_from_option + assert_changes '@object.num', from: 0 do + @object.increment + end + end + + def test_assert_changes_with_from_option_with_wrong_value + assert_raises Minitest::Assertion do + assert_changes '@object.num', from: -1 do + @object.increment + end + end + end + + def test_assert_changes_with_to_option + assert_changes '@object.num', to: 1 do + @object.increment + end + end + + def test_assert_changes_with_wrong_to_option + assert_raises Minitest::Assertion do + assert_changes '@object.num', to: 2 do + @object.increment + end + end + end + + def test_assert_changes_with_from_option_and_to_option + assert_changes '@object.num', from: 0, to: 1 do + @object.increment + end + end + + def test_assert_changes_with_from_and_to_options_and_wrong_to_value + assert_raises Minitest::Assertion do + assert_changes '@object.num', from: 0, to: 2 do + @object.increment + end + end + end + + def test_assert_changes_works_with_any_object + retval = silence_warnings do + assert_changes :@new_object, from: nil, to: 42 do + @new_object = 42 + end + end + + assert_equal 42, retval + end + + def test_assert_changes_works_with_nil + oldval = @object + + retval = assert_changes :@object, from: oldval, to: nil do + @object = nil + end + + assert_nil retval + end + + def test_assert_changes_with_to_and_case_operator + token = nil + + assert_changes 'token', to: /\w{32}/ do + token = SecureRandom.hex + end + end + + def test_assert_changes_with_to_and_from_and_case_operator + token = SecureRandom.hex + + assert_changes 'token', from: /\w{32}/, to: /\w{32}/ do + token = SecureRandom.hex + end + end + + def test_assert_no_changes_pass + assert_no_changes '@object.num' do + # ... + end + end + + def test_assert_no_changes_with_message + error = assert_raises Minitest::Assertion do + assert_no_changes '@object.num', '@object.num should not change' do + @object.increment + end + end + + assert_equal "@object.num should not change.\n\"@object.num\" did change to 1.\nExpected: 0\n Actual: 1", error.message + end end class AlsoDoingNothingTest < ActiveSupport::TestCase |