diff options
22 files changed, 326 insertions, 64 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 bff217789d..590fe2f587 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -2,6 +2,10 @@ *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/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 5747e4d1ee..4f8490fa2b 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -579,14 +579,15 @@ module ActiveRecord exception end - def log(sql, name = "SQL", binds = [], statement_name = nil) + def log(sql, name = "SQL", binds = [], type_casted_binds = [], statement_name = nil) @instrumenter.instrument( "sql.active_record", - :sql => sql, - :name => name, - :connection_id => object_id, - :statement_name => statement_name, - :binds => binds) { yield } + sql: sql, + name: name, + binds: binds, + type_casted_binds: type_casted_binds, + statement_name: statement_name, + connection_id: object_id) { yield } rescue => e raise translate_exception_class(e, sql) end diff --git a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb index 87f0ff7d85..cc19d95669 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb @@ -79,7 +79,7 @@ module ActiveRecord type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) } - log(sql, name, binds) do + log(sql, name, binds, type_casted_binds) do if cache_stmt cache = @statements[sql] ||= { stmt: @connection.prepare(sql) 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/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index ddfc560747..487165d511 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -598,14 +598,14 @@ module ActiveRecord def exec_no_cache(sql, name, binds) type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) } - log(sql, name, binds) { @connection.async_exec(sql, type_casted_binds) } + log(sql, name, binds, type_casted_binds) { @connection.async_exec(sql, type_casted_binds) } end def exec_cache(sql, name, binds) stmt_key = prepare_statement(sql) type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) } - log(sql, name, binds, stmt_key) do + log(sql, name, binds, type_casted_binds, stmt_key) do @connection.exec_prepared(stmt_key, type_casted_binds) end rescue ActiveRecord::StatementInvalid => e diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 3384012c49..7e23f44ddf 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -190,7 +190,7 @@ module ActiveRecord def exec_query(sql, name = nil, binds = [], prepare: false) type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) } - log(sql, name, binds) do + log(sql, name, binds, type_casted_binds) do # Don't cache statements if they are not prepared unless prepare stmt = @connection.prepare(sql) 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/log_subscriber.rb b/activerecord/lib/active_record/log_subscriber.rb index 37a092d7a2..c4998ba686 100644 --- a/activerecord/lib/active_record/log_subscriber.rb +++ b/activerecord/lib/active_record/log_subscriber.rb @@ -20,18 +20,14 @@ module ActiveRecord @odd = false end - def render_bind(attribute) - value = if attribute.type.binary? && attribute.value - if attribute.value.is_a?(Hash) - "<#{attribute.value_for_database.to_s.bytesize} bytes of binary data>" - else - "<#{attribute.value.bytesize} bytes of binary data>" - end + def render_bind(attr, type_casted_value) + value = if attr.type.binary? && attr.value + "<#{attr.value_for_database.to_s.bytesize} bytes of binary data>" else - type_cast(attribute.value_for_database) + type_casted_value end - [attribute.name, value] + [attr.name, value] end def sql(event) @@ -48,7 +44,9 @@ module ActiveRecord binds = nil unless (payload[:binds] || []).empty? - binds = " " + payload[:binds].map { |attr| render_bind(attr) }.inspect + binds = " " + payload[:binds].zip(payload[:type_casted_binds]).map { |attr, value| + render_bind(attr, value) + }.inspect end name = colorize_payload_name(name, payload[:name]) @@ -91,10 +89,6 @@ module ActiveRecord def logger ActiveRecord::Base.logger end - - def type_cast(value) - ActiveRecord::Base.connection.type_cast(value) - end end end 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/bind_parameter_test.rb b/activerecord/test/cases/bind_parameter_test.rb index fa924fe4cb..3f01885489 100644 --- a/activerecord/test/cases/bind_parameter_test.rb +++ b/activerecord/test/cases/bind_parameter_test.rb @@ -57,10 +57,13 @@ module ActiveRecord end def test_logs_bind_vars_after_type_cast + binds = [Relation::QueryAttribute.new("id", "10", Type::Integer.new)] + type_casted_binds = binds.map { |attr| type_cast(attr.value_for_database) } payload = { :name => 'SQL', :sql => 'select * from topics where id = ?', - :binds => [Relation::QueryAttribute.new("id", "10", Type::Integer.new)] + :binds => binds, + :type_casted_binds => type_casted_binds } event = ActiveSupport::Notifications::Event.new( 'foo', @@ -84,6 +87,12 @@ module ActiveRecord logger.sql event assert_match([[@pk.name, 10]].inspect, logger.debugs.first) end + + private + + def type_cast(value) + ActiveRecord::Base.connection.type_cast(value) + end end end end 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/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..1e3810f6f7 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 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 diff --git a/guides/source/action_cable_overview.md b/guides/source/action_cable_overview.md index 0d00b7f07b..02db86888c 100644 --- a/guides/source/action_cable_overview.md +++ b/guides/source/action_cable_overview.md @@ -333,7 +333,7 @@ class ChatChannel < ApplicationCable::Channel end def receive(data) - ChatChannel.broadcast_to("chat_#{params[:room]}", data) + ActionCable.server.broadcast("chat_#{params[:room]}", data) end end ``` |