diff options
Diffstat (limited to 'activerecord')
22 files changed, 404 insertions, 101 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index c91e6662c9..1d0e4b32a3 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,27 @@ +* Add newline between each migration in `structure.sql`. + + Keeps schema migration inserts as a single commit, but allows for easier + git diffing. + + Fixes #25504. + + *Grey Baker*, *Norberto Lopes* + +* The flag `error_on_ignored_order_or_limit` has been deprecated in favor of + the current `error_on_ignored_order`. + + *Xavier Noria* + +* Batch processing methods support `limit`: + + Post.limit(10_000).find_each do |post| + # ... + end + + It also works in `find_in_batches` and `in_batches`. + + *Xavier Noria* + * Using `group` with an attribute that has a custom type will properly cast the hash keys after calling a calculation method like `count`. Fixes #25595. diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 3729e22e64..c91b5d3fe3 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -362,7 +362,7 @@ module ActiveRecord # end # end # - # If your model class is <tt>Project</tt>, the module is + # If your model class is <tt>Project</tt>, then the module is # named <tt>Project::GeneratedAssociationMethods</tt>. The +GeneratedAssociationMethods+ module is # included in the model class immediately after the (anonymous) generated attributes methods # module, meaning an association will override the methods for an attribute with the same name. @@ -845,8 +845,8 @@ module ActiveRecord # Post.includes(:author).each do |post| # # This references the name of the #belongs_to association that also used the <tt>:author</tt> - # symbol. After loading the posts, find will collect the +author_id+ from each one and load - # all the referenced authors with one query. Doing so will cut down the number of queries + # symbol. After loading the posts, +find+ will collect the +author_id+ from each one and load + # all of the referenced authors with one query. Doing so will cut down the number of queries # from 201 to 102. # # We can improve upon the situation further by referencing both associations in the finder with: @@ -873,7 +873,7 @@ module ActiveRecord # # Since only one table is loaded at a time, conditions or orders cannot reference tables # other than the main one. If this is the case, Active Record falls back to the previously - # used LEFT OUTER JOIN based strategy. For example: + # used <tt>LEFT OUTER JOIN</tt> based strategy. For example: # # Post.includes([:author, :comments]).where(['comments.approved = ?', true]) # @@ -881,18 +881,18 @@ module ActiveRecord # <tt>LEFT OUTER JOIN comments ON comments.post_id = posts.id</tt> and # <tt>LEFT OUTER JOIN authors ON authors.id = posts.author_id</tt>. Note that using conditions # like this can have unintended consequences. - # In the above example posts with no approved comments are not returned at all, because + # In the above example, posts with no approved comments are not returned at all because # the conditions apply to the SQL statement as a whole and not just to the association. # # You must disambiguate column references for this fallback to happen, for example # <tt>order: "author.name DESC"</tt> will work but <tt>order: "name DESC"</tt> will not. # - # If you want to load all posts (including posts with no approved comments) then write - # your own LEFT OUTER JOIN query using ON + # If you want to load all posts (including posts with no approved comments), then write + # your own <tt>LEFT OUTER JOIN</tt> query using <tt>ON</tt>: # # Post.joins("LEFT OUTER JOIN comments ON comments.post_id = posts.id AND comments.approved = '1'") # - # In this case it is usually more natural to include an association which has conditions defined on it: + # In this case, it is usually more natural to include an association which has conditions defined on it: # # class Post < ActiveRecord::Base # has_many :approved_comments, -> { where(approved: true) }, class_name: 'Comment' @@ -924,7 +924,7 @@ module ActiveRecord # # This will execute one query to load the addresses and load the addressables with one # query per addressable type. - # For example if all the addressables are either of class Person or Company then a total + # For example, if all the addressables are either of class Person or Company, then a total # of 3 queries will be executed. The list of addressable types to load is determined on # the back of the addresses loaded. This is not supported if Active Record has to fallback # to the previous implementation of eager loading and will raise ActiveRecord::EagerLoadPolymorphicError. @@ -1015,7 +1015,7 @@ module ActiveRecord # # == Bi-directional associations # - # When you specify an association there is usually an association on the associated model + # When you specify an association, there is usually an association on the associated model # that specifies the same relationship in reverse. For example, with the following models: # # class Dungeon < ActiveRecord::Base @@ -1032,7 +1032,7 @@ module ActiveRecord # end # # The +traps+ association on +Dungeon+ and the +dungeon+ association on +Trap+ are - # the inverse of each other and the inverse of the +dungeon+ association on +EvilWizard+ + # the inverse of each other, and the inverse of the +dungeon+ association on +EvilWizard+ # is the +evil_wizard+ association on +Dungeon+ (and vice-versa). By default, # Active Record can guess the inverse of the association based on the name # of the class. The result is the following: @@ -1062,7 +1062,7 @@ module ActiveRecord # # * does not work with <tt>:through</tt> associations. # * does not work with <tt>:polymorphic</tt> associations. - # * for #belongs_to associations #has_many inverse associations are ignored. + # * inverse associations for #belongs_to associations #has_many are ignored. # # For more information, see the documentation for the +:inverse_of+ option. # @@ -1070,7 +1070,7 @@ module ActiveRecord # # === Dependent associations # - # #has_many, #has_one and #belongs_to associations support the <tt>:dependent</tt> option. + # #has_many, #has_one, and #belongs_to associations support the <tt>:dependent</tt> option. # This allows you to specify that associated records should be deleted when the owner is # deleted. # diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index 5d1e7ffb73..17ccf5a86c 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -793,7 +793,7 @@ module ActiveRecord # Returns +true+ if the collection is empty. If the collection has been # loaded it is equivalent # to <tt>collection.size.zero?</tt>. If the collection has not been loaded, - # it is equivalent to <tt>collection.exists?</tt>. If the collection has + # it is equivalent to <tt>!collection.exists?</tt>. If the collection has # not already been loaded and you are going to fetch the records anyway it # is better to check <tt>collection.length.zero?</tt>. # 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 9b74c3a10f..1d9579f1a8 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -849,6 +849,21 @@ module ActiveRecord remove_connection(spec.name) owner_to_pool[spec.name] = ConnectionAdapters::ConnectionPool.new(spec) + + message_bus = ActiveSupport::Notifications.instrumenter + payload = { + connection_id: object_id + } + if spec + payload[:class_name] = spec.name + payload[:config] = spec.config + end + + message_bus.instrument('!connection.active_record', payload) do + owner_to_pool[spec.name] = ConnectionAdapters::ConnectionPool.new(spec) + end + + owner_to_pool[spec.name] end # Returns true if there are any active connections among the connection diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 396cb0b07a..9e9ace49db 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -998,8 +998,8 @@ module ActiveRecord sm_table = ActiveRecord::Migrator.schema_migrations_table_name if supports_multi_insert? - sql = "INSERT INTO #{sm_table} (version) VALUES " - sql << versions.map {|v| "('#{v}')" }.join(', ') + sql = "INSERT INTO #{sm_table} (version) VALUES\n" + sql << versions.map {|v| "('#{v}')" }.join(",\n") sql << ";\n\n" sql else 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_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 eb2268157b..7e23f44ddf 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -159,7 +159,7 @@ module ActiveRecord end # Returns 62. SQLite supports index names up to 64 - # characters. The rest is used by rails internally to perform + # characters. The rest is used by Rails internally to perform # temporary rename operations def allowed_index_name_length index_name_length - 2 @@ -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/core.rb b/activerecord/lib/active_record/core.rb index de337b24d6..3e3a7679ac 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -72,11 +72,31 @@ module ActiveRecord ## # :singleton-method: - # Specifies if an error should be raised on query limit or order being + # Specifies if an error should be raised if the query has an order being # ignored when doing batch queries. Useful in applications where the - # limit or scope being ignored is error-worthy, rather than a warning. - mattr_accessor :error_on_ignored_order_or_limit, instance_writer: false - self.error_on_ignored_order_or_limit = false + # scope being ignored is error-worthy, rather than a warning. + mattr_accessor :error_on_ignored_order, instance_writer: false + self.error_on_ignored_order = false + + def self.error_on_ignored_order_or_limit + ActiveSupport::Deprecation.warn(<<-MSG.squish) + The flag error_on_ignored_order_or_limit is deprecated. Limits are + now supported. Please use error_on_ignored_order instead. + MSG + self.error_on_ignored_order + end + + def error_on_ignored_order_or_limit + self.class.error_on_ignored_order_or_limit + end + + def self.error_on_ignored_order_or_limit=(value) + ActiveSupport::Deprecation.warn(<<-MSG.squish) + The flag error_on_ignored_order_or_limit is deprecated. Limits are + now supported. Please use error_on_ignored_order= instead. + MSG + self.error_on_ignored_order = value + end ## # :singleton-method: diff --git a/activerecord/lib/active_record/fixtures.rb b/activerecord/lib/active_record/fixtures.rb index 51bf12d0bf..6d142285cd 100644 --- a/activerecord/lib/active_record/fixtures.rb +++ b/activerecord/lib/active_record/fixtures.rb @@ -968,6 +968,7 @@ module ActiveRecord @fixture_cache = {} @fixture_connections = [] @@already_loaded_fixtures ||= {} + @connection_subscriber = nil # Load fixtures once and begin transaction. if run_in_transaction? @@ -977,10 +978,36 @@ module ActiveRecord @loaded_fixtures = load_fixtures(config) @@already_loaded_fixtures[self.class] = @loaded_fixtures end + + # Begin transactions for connections already established @fixture_connections = enlist_fixture_connections @fixture_connections.each do |connection| connection.begin_transaction joinable: false end + + # 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 + + if model_class + begin + connection = ActiveRecord::Base.connection_handler.retrieve_connection(model_class) + rescue ConnectionNotEstablished + connection = nil + end + + if connection && !@fixture_connections.include?(connection) + connection.begin_transaction joinable: false + @fixture_connections << connection + end + end + end + # Load fixtures for every test. else ActiveRecord::FixtureSet.reset_cache @@ -995,6 +1022,7 @@ module ActiveRecord def teardown_fixtures # Rollback changes if a transaction is active. if run_in_transaction? + ActiveSupport::Notifications.unsubscribe(@connection_subscriber) if @connection_subscriber @fixture_connections.each do |connection| connection.rollback_transaction if connection.transaction_open? end diff --git a/activerecord/lib/active_record/log_subscriber.rb b/activerecord/lib/active_record/log_subscriber.rb index 8e32af1c49..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 - 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]) diff --git a/activerecord/lib/active_record/railtie.rb b/activerecord/lib/active_record/railtie.rb index 98ea425d16..2c0ca62924 100644 --- a/activerecord/lib/active_record/railtie.rb +++ b/activerecord/lib/active_record/railtie.rb @@ -3,7 +3,7 @@ require "rails" require "active_model/railtie" # For now, action_controller must always be present with -# rails, so let's make sure that it gets required before +# Rails, so let's make sure that it gets required before # here. This is needed for correctly setting up the middleware. # In the future, this might become an optional require. require "action_controller/railtie" diff --git a/activerecord/lib/active_record/railties/controller_runtime.rb b/activerecord/lib/active_record/railties/controller_runtime.rb index 8727e46cb3..c13238cbe2 100644 --- a/activerecord/lib/active_record/railties/controller_runtime.rb +++ b/activerecord/lib/active_record/railties/controller_runtime.rb @@ -19,7 +19,7 @@ module ActiveRecord end def cleanup_view_runtime - if logger.info? && ActiveRecord::Base.connected? + if logger && logger.info? && ActiveRecord::Base.connected? db_rt_before_render = ActiveRecord::LogSubscriber.reset_runtime self.db_runtime = (db_runtime || 0) + db_rt_before_render runtime = super diff --git a/activerecord/lib/active_record/relation/batches.rb b/activerecord/lib/active_record/relation/batches.rb index 3639625722..20ed4526b0 100644 --- a/activerecord/lib/active_record/relation/batches.rb +++ b/activerecord/lib/active_record/relation/batches.rb @@ -1,8 +1,8 @@ -require "active_record/relation/batches/batch_enumerator" +require 'active_record/relation/batches/batch_enumerator' module ActiveRecord module Batches - ORDER_OR_LIMIT_IGNORED_MESSAGE = "Scoped order and limit are ignored, it's forced to be batch order and batch size." + ORDER_IGNORE_MESSAGE = "Scoped order is ignored, it's forced to be batch order." # Looping through a collection of records from the database # (using the Scoping::Named::ClassMethods.all method, for example) @@ -34,15 +34,19 @@ module ActiveRecord # * <tt>:start</tt> - Specifies the primary key value to start from, inclusive of the value. # * <tt>:finish</tt> - Specifies the primary key value to end at, inclusive of the value. # * <tt>:error_on_ignore</tt> - Overrides the application config to specify if an error should be raised when - # the order and limit have to be ignored due to batching. + # an order is present in the relation. # - # This is especially useful if you want multiple workers dealing with - # the same processing queue. You can make worker 1 handle all the records - # between id 0 and 10,000 and worker 2 handle from 10,000 and beyond - # (by setting the +:start+ and +:finish+ option on each worker). + # Limits are honored, and if present there is no requirement for the batch + # size, it can be less than, equal, or greater than the limit. # - # # Let's process for a batch of 2000 records, skipping the first 2000 rows - # Person.find_each(start: 2000, batch_size: 2000) do |person| + # The options +start+ and +finish+ are especially useful if you want + # multiple workers dealing with the same processing queue. You can make + # worker 1 handle all the records between id 1 and 9999 and worker 2 + # handle from 10000 and beyond by setting the +:start+ and +:finish+ + # option on each worker. + # + # # Let's process from record 10_000 on. + # Person.find_each(start: 10_000) do |person| # person.party_all_night! # end # @@ -51,8 +55,8 @@ module ActiveRecord # work. This also means that this method only works when the primary key is # orderable (e.g. an integer or string). # - # NOTE: You can't set the limit either, that's used to control - # the batch sizes. + # NOTE: By its nature, batch processing is subject to race conditions if + # other processes are modifying the database. def find_each(start: nil, finish: nil, batch_size: 1000, error_on_ignore: nil) if block_given? find_in_batches(start: start, finish: finish, batch_size: batch_size, error_on_ignore: error_on_ignore) do |records| @@ -89,15 +93,19 @@ module ActiveRecord # * <tt>:start</tt> - Specifies the primary key value to start from, inclusive of the value. # * <tt>:finish</tt> - Specifies the primary key value to end at, inclusive of the value. # * <tt>:error_on_ignore</tt> - Overrides the application config to specify if an error should be raised when - # the order and limit have to be ignored due to batching. + # an order is present in the relation. + # + # Limits are honored, and if present there is no requirement for the batch + # size, it can be less than, equal, or greater than the limit. # - # This is especially useful if you want multiple workers dealing with - # the same processing queue. You can make worker 1 handle all the records - # between id 0 and 10,000 and worker 2 handle from 10,000 and beyond - # (by setting the +:start+ and +:finish+ option on each worker). + # The options +start+ and +finish+ are especially useful if you want + # multiple workers dealing with the same processing queue. You can make + # worker 1 handle all the records between id 1 and 9999 and worker 2 + # handle from 10000 and beyond by setting the +:start+ and +:finish+ + # option on each worker. # - # # Let's process the next 2000 records - # Person.find_in_batches(start: 2000, batch_size: 2000) do |group| + # # Let's process from record 10_000 on. + # Person.find_in_batches(start: 10_000) do |group| # group.each { |person| person.party_all_night! } # end # @@ -106,8 +114,8 @@ module ActiveRecord # work. This also means that this method only works when the primary key is # orderable (e.g. an integer or string). # - # NOTE: You can't set the limit either, that's used to control - # the batch sizes. + # NOTE: By its nature, batch processing is subject to race conditions if + # other processes are modifying the database. def find_in_batches(start: nil, finish: nil, batch_size: 1000, error_on_ignore: nil) relation = self unless block_given? @@ -149,17 +157,19 @@ module ActiveRecord # * <tt>:start</tt> - Specifies the primary key value to start from, inclusive of the value. # * <tt>:finish</tt> - Specifies the primary key value to end at, inclusive of the value. # * <tt>:error_on_ignore</tt> - Overrides the application config to specify if an error should be raised when - # the order and limit have to be ignored due to batching. + # an order is present in the relation. + # + # Limits are honored, and if present there is no requirement for the batch + # size, it can be less than, equal, or greater than the limit. # - # This is especially useful if you want to work with the - # ActiveRecord::Relation object instead of the array of records, or if - # you want multiple workers dealing with the same processing queue. You can - # make worker 1 handle all the records between id 0 and 10,000 and worker 2 - # handle from 10,000 and beyond (by setting the +:start+ and +:finish+ - # option on each worker). + # The options +start+ and +finish+ are especially useful if you want + # multiple workers dealing with the same processing queue. You can make + # worker 1 handle all the records between id 1 and 9999 and worker 2 + # handle from 10000 and beyond by setting the +:start+ and +:finish+ + # option on each worker. # - # # Let's process the next 2000 records - # Person.in_batches(of: 2000, start: 2000).update_all(awesome: true) + # # Let's process from record 10_000 on. + # Person.in_batches(start: 10_000).update_all(awesome: true) # # An example of calling where query method on the relation: # @@ -179,19 +189,25 @@ module ActiveRecord # consistent. Therefore the primary key must be orderable, e.g an integer # or a string. # - # NOTE: You can't set the limit either, that's used to control the batch - # sizes. + # NOTE: By its nature, batch processing is subject to race conditions if + # other processes are modifying the database. def in_batches(of: 1000, start: nil, finish: nil, load: false, error_on_ignore: nil) relation = self unless block_given? return BatchEnumerator.new(of: of, start: start, finish: finish, relation: self) end - if arel.orders.present? || arel.taken.present? - act_on_order_or_limit_ignored(error_on_ignore) + if arel.orders.present? + act_on_ignored_order(error_on_ignore) + end + + batch_limit = of + if limit_value + remaining = limit_value + batch_limit = remaining if remaining < batch_limit end - relation = relation.reorder(batch_order).limit(of) + relation = relation.reorder(batch_order).limit(batch_limit) relation = apply_limits(relation, start, finish) batch_relation = relation @@ -199,11 +215,11 @@ module ActiveRecord if load records = batch_relation.records ids = records.map(&:id) - yielded_relation = self.where(primary_key => ids) + yielded_relation = where(primary_key => ids) yielded_relation.load_records(records) else ids = batch_relation.pluck(primary_key) - yielded_relation = self.where(primary_key => ids) + yielded_relation = where(primary_key => ids) end break if ids.empty? @@ -213,7 +229,20 @@ module ActiveRecord yield yielded_relation - break if ids.length < of + break if ids.length < batch_limit + + if limit_value + remaining -= ids.length + + if remaining == 0 + # Saves a useless iteration when the limit is a multiple of the + # batch size. + break + elsif remaining < batch_limit + relation = relation.limit(remaining) + end + end + batch_relation = relation.where(arel_attribute(primary_key).gt(primary_key_offset)) end end @@ -230,13 +259,13 @@ module ActiveRecord "#{quoted_table_name}.#{quoted_primary_key} ASC" end - def act_on_order_or_limit_ignored(error_on_ignore) - raise_error = (error_on_ignore.nil? ? self.klass.error_on_ignored_order_or_limit : error_on_ignore) + def act_on_ignored_order(error_on_ignore) + raise_error = (error_on_ignore.nil? ? self.klass.error_on_ignored_order : error_on_ignore) if raise_error - raise ArgumentError.new(ORDER_OR_LIMIT_IGNORED_MESSAGE) + raise ArgumentError.new(ORDER_IGNORE_MESSAGE) elsif logger - logger.warn(ORDER_OR_LIMIT_IGNORED_MESSAGE) + logger.warn(ORDER_IGNORE_MESSAGE) end end end diff --git a/activerecord/test/cases/batches_test.rb b/activerecord/test/cases/batches_test.rb index db71840658..8a4c1bd615 100644 --- a/activerecord/test/cases/batches_test.rb +++ b/activerecord/test/cases/batches_test.rb @@ -68,10 +68,26 @@ class EachTest < ActiveRecord::TestCase end end - def test_warn_if_limit_scope_is_set - assert_called(ActiveRecord::Base.logger, :warn) do - Post.limit(1).find_each { |post| post } + test 'find_each should honor limit if passed a block' do + limit = @total - 1 + total = 0 + + Post.limit(limit).find_each do |post| + total += 1 + end + + assert_equal limit, total + end + + test 'find_each should honor limit if no block is passed' do + limit = @total - 1 + total = 0 + + Post.limit(limit).find_each.each do |post| + total += 1 end + + assert_equal limit, total end def test_warn_if_order_scope_is_set @@ -84,7 +100,7 @@ class EachTest < ActiveRecord::TestCase previous_logger = ActiveRecord::Base.logger ActiveRecord::Base.logger = nil assert_nothing_raised do - Post.limit(1).find_each { |post| post } + Post.order('comments_count DESC').find_each { |post| post } end ensure ActiveRecord::Base.logger = previous_logger @@ -172,26 +188,26 @@ class EachTest < ActiveRecord::TestCase def test_find_in_batches_should_not_error_if_config_overridden # Set the config option which will be overridden - prev = ActiveRecord::Base.error_on_ignored_order_or_limit - ActiveRecord::Base.error_on_ignored_order_or_limit = true + prev = ActiveRecord::Base.error_on_ignored_order + ActiveRecord::Base.error_on_ignored_order = true assert_nothing_raised do PostWithDefaultScope.find_in_batches(error_on_ignore: false){} end ensure # Set back to default - ActiveRecord::Base.error_on_ignored_order_or_limit = prev + ActiveRecord::Base.error_on_ignored_order = prev end def test_find_in_batches_should_error_on_config_specified_to_error # Set the config option - prev = ActiveRecord::Base.error_on_ignored_order_or_limit - ActiveRecord::Base.error_on_ignored_order_or_limit = true + prev = ActiveRecord::Base.error_on_ignored_order + ActiveRecord::Base.error_on_ignored_order = true assert_raise(ArgumentError) do PostWithDefaultScope.find_in_batches(){} end ensure # Set back to default - ActiveRecord::Base.error_on_ignored_order_or_limit = prev + ActiveRecord::Base.error_on_ignored_order = prev end def test_find_in_batches_should_not_error_by_default @@ -249,6 +265,28 @@ class EachTest < ActiveRecord::TestCase end end + test 'find_in_batches should honor limit if passed a block' do + limit = @total - 1 + total = 0 + + Post.limit(limit).find_in_batches do |batch| + total += batch.size + end + + assert_equal limit, total + end + + test 'find_in_batches should honor limit if no block is passed' do + limit = @total - 1 + total = 0 + + Post.limit(limit).find_in_batches.each do |batch| + total += batch.size + end + + assert_equal limit, total + end + def test_in_batches_should_not_execute_any_query assert_no_queries do assert_kind_of ActiveRecord::Batches::BatchEnumerator, Post.in_batches(of: 2) @@ -486,4 +524,94 @@ class EachTest < ActiveRecord::TestCase assert_equal 1, Post.find_in_batches(:batch_size => 10_000).size end end + + [true, false].each do |load| + test "in_batches should return limit records when limit is less than batch size and load is #{load}" do + limit = 3 + batch_size = 5 + total = 0 + + Post.limit(limit).in_batches(of: batch_size, load: load) do |batch| + total += batch.count + end + + assert_equal limit, total + end + + test "in_batches should return limit records when limit is greater than batch size and load is #{load}" do + limit = 5 + batch_size = 3 + total = 0 + + Post.limit(limit).in_batches(of: batch_size, load: load) do |batch| + total += batch.count + end + + assert_equal limit, total + end + + test "in_batches should return limit records when limit is a multiple of the batch size and load is #{load}" do + limit = 6 + batch_size = 3 + total = 0 + + Post.limit(limit).in_batches(of: batch_size, load: load) do |batch| + total += batch.count + end + + assert_equal limit, total + end + + test "in_batches should return no records if the limit is 0 and load is #{load}" do + limit = 0 + batch_size = 1 + total = 0 + + Post.limit(limit).in_batches(of: batch_size, load: load) do |batch| + total += batch.count + end + + assert_equal limit, total + end + + test "in_batches should return all if the limit is greater than the number of records when load is #{load}" do + limit = @total + 1 + batch_size = 1 + total = 0 + + Post.limit(limit).in_batches(of: batch_size, load: load) do |batch| + total += batch.count + end + + assert_equal @total, total + end + end + + test '.error_on_ignored_order_or_limit= is deprecated' do + begin + prev = ActiveRecord::Base.error_on_ignored_order + assert_deprecated 'Please use error_on_ignored_order= instead.' do + ActiveRecord::Base.error_on_ignored_order_or_limit = true + end + assert ActiveRecord::Base.error_on_ignored_order + ensure + ActiveRecord::Base.error_on_ignored_order = prev + end + end + + test '.error_on_ignored_order_or_limit is deprecated' do + expected = ActiveRecord::Base.error_on_ignored_order + actual = assert_deprecated 'Please use error_on_ignored_order instead.' do + ActiveRecord::Base.error_on_ignored_order_or_limit + end + assert_equal expected, actual + end + + test '#error_on_ignored_order_or_limit is deprecated' do + expected = ActiveRecord::Base.error_on_ignored_order + actual = assert_deprecated 'Please use error_on_ignored_order instead.' do + Post.new.error_on_ignored_order_or_limit + end + assert_equal expected, actual + end end 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/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 06c2f10dc6..6acfec0621 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -26,7 +26,7 @@ class NumericData < ActiveRecord::Base end class CalculationsTest < ActiveRecord::TestCase - fixtures :companies, :accounts, :topics, :speedometers, :minivans + fixtures :companies, :accounts, :topics, :speedometers, :minivans, :books def test_should_sum_field assert_equal 318, Account.sum(:credit_limit) @@ -796,7 +796,6 @@ class CalculationsTest < ActiveRecord::TestCase end def test_group_by_attribute_with_custom_type - Book.create!(status: :published) - assert_equal({ "published" => 1 }, Book.group(:status).count) + assert_equal({ "proposed" => 2, "published" => 2 }, Book.group(:status).count) end end diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb index 09e7848bda..2bdabaee62 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -341,6 +341,18 @@ module ActiveRecord end end + def test_connection_notification_is_called + payloads = [] + subscription = ActiveSupport::Notifications.subscribe('!connection.active_record') do |name, started, finished, unique_id, payload| + 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] + ensure + ActiveSupport::Notifications.unsubscribe(subscription) if subscription + end + def test_pool_sets_connection_schema_cache connection = pool.checkout schema_cache = SchemaCache.new connection diff --git a/activerecord/test/cases/fixtures_test.rb b/activerecord/test/cases/fixtures_test.rb index 9455d4886c..a06aa4b247 100644 --- a/activerecord/test/cases/fixtures_test.rb +++ b/activerecord/test/cases/fixtures_test.rb @@ -622,6 +622,46 @@ class TransactionalFixturesOnCustomConnectionTest < ActiveRecord::TestCase end end +class TransactionalFixturesOnConnectionNotification < ActiveRecord::TestCase + self.use_transactional_tests = true + self.use_instantiated_fixtures = false + + def test_transaction_created_on_connection_notification + connection = stub(:transaction_open? => false) + connection.expects(:begin_transaction).with(joinable: false) + fire_connection_notification(connection) + end + + def test_notification_established_transactions_are_rolled_back + # Mocha is not thread-safe so define our own stub to test + connection = Class.new do + attr_accessor :rollback_transaction_called + def transaction_open?; true; end + def begin_transaction(*args); end + def rollback_transaction(*args) + @rollback_transaction_called = true + end + end.new + fire_connection_notification(connection) + teardown_fixtures + assert(connection.rollback_transaction_called, "Expected <mock connection>#rollback_transaction to be called but was not") + end + + private + + def fire_connection_notification(connection) + ActiveRecord::Base.connection_handler.stubs(:retrieve_connection).with(Book).returns(connection) + message_bus = ActiveSupport::Notifications.instrumenter + payload = { + class_name: 'Book', + config: nil, + connection_id: connection.object_id + } + + message_bus.instrument('!connection.active_record', payload) {} + end +end + class InvalidTableNameFixturesTest < ActiveRecord::TestCase fixtures :funny_jokes # Set to false to blow away fixtures cache and ensure our fixtures are loaded diff --git a/activerecord/test/cases/migration/references_statements_test.rb b/activerecord/test/cases/migration/references_statements_test.rb index 70c64f3e71..5dddb35c4c 100644 --- a/activerecord/test/cases/migration/references_statements_test.rb +++ b/activerecord/test/cases/migration/references_statements_test.rb @@ -35,7 +35,7 @@ module ActiveRecord assert_not index_exists?(table_name, :user_id) end - def test_create_reference_id_index_even_if_index_option_is_passed + def test_create_reference_id_index_even_if_index_option_is_not_passed add_reference table_name, :user assert index_exists?(table_name, :user_id) end diff --git a/activerecord/test/cases/relation/mutation_test.rb b/activerecord/test/cases/relation/mutation_test.rb index ffb2da7a26..36cb898010 100644 --- a/activerecord/test/cases/relation/mutation_test.rb +++ b/activerecord/test/cases/relation/mutation_test.rb @@ -44,8 +44,8 @@ module ActiveRecord end test "#_select!" do - assert relation.public_send("_select!", :foo).equal?(relation) - assert_equal [:foo], relation.public_send("select_values") + assert relation._select!(:foo).equal?(relation) + assert_equal [:foo], relation.select_values end test '#order!' do |