diff options
39 files changed, 252 insertions, 281 deletions
diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb index 7556f984f2..754ac144cc 100644 --- a/actionpack/test/controller/caching_test.rb +++ b/actionpack/test/controller/caching_test.rb @@ -381,19 +381,14 @@ class CollectionCacheController < ActionController::Base render 'index' end - def index_explicit_render + def index_explicit_render_in_controller @customers = [Customer.new('david', 1)] - render partial: 'customers/customer', collection: @customers + render partial: 'customers/customer', collection: @customers, cached: true end def index_with_comment @customers = [Customer.new('david', 1)] - render partial: 'customers/commented_customer', collection: @customers, as: :customer - end - - def index_with_callable_cache_key - @customers = [Customer.new('david', 1)] - render @customers, cache: -> customer { 'cached_david' } + render partial: 'customers/commented_customer', collection: @customers, as: :customer, cached: true end end @@ -404,7 +399,7 @@ class AutomaticCollectionCacheTest < ActionController::TestCase @controller.perform_caching = true @controller.partial_rendered_times = 0 @controller.cache_store = ActiveSupport::Cache::MemoryStore.new - ActionView::PartialRenderer.collection_cache = @controller.cache_store + ActionView::PartialRenderer.collection_cache = ActiveSupport::Cache::MemoryStore.new end def test_collection_fetches_cached_views @@ -427,7 +422,7 @@ class AutomaticCollectionCacheTest < ActionController::TestCase end def test_explicit_render_call_with_options - get :index_explicit_render + get :index_explicit_render_in_controller assert_select ':root', "david, 1" end @@ -440,12 +435,6 @@ class AutomaticCollectionCacheTest < ActionController::TestCase assert_equal 1, @controller.partial_rendered_times end - def test_caching_with_callable_cache_key - get :index_with_callable_cache_key - assert_customer_cached 'cached_david', 'david, 1' - assert_customer_cached 'david/1', 'david, 1' - end - private def assert_customer_cached(key, content) assert_match content, diff --git a/actionpack/test/fixtures/collection_cache/index.html.erb b/actionpack/test/fixtures/collection_cache/index.html.erb index 521b1450df..853e501ab4 100644 --- a/actionpack/test/fixtures/collection_cache/index.html.erb +++ b/actionpack/test/fixtures/collection_cache/index.html.erb @@ -1 +1 @@ -<%= render @customers %>
\ No newline at end of file +<%= render partial: 'customers/customer', collection: @customers, cached: true %> diff --git a/actionview/lib/action_view/helpers/cache_helper.rb b/actionview/lib/action_view/helpers/cache_helper.rb index 401f398721..e842d8e31c 100644 --- a/actionview/lib/action_view/helpers/cache_helper.rb +++ b/actionview/lib/action_view/helpers/cache_helper.rb @@ -126,44 +126,16 @@ module ActionView # # Now all you have to do is change that timestamp when the helper method changes. # - # === Automatic Collection Caching + # === Collection Caching # - # When rendering collections such as: + # When rendering a collection of objects that each use the same partial, a `cached` + # option can be passed. + # For collections rendered such: # - # <%= render @notifications %> - # <%= render partial: 'notifications/notification', collection: @notifications %> + # <%= render partial: 'notifications/notification', collection: @notifications, cached: true %> # - # If the notifications/_notification partial starts with a cache call as: - # - # <% cache notification do %> - # <%= notification.name %> - # <% end %> - # - # The collection can then automatically use any cached renders for that - # template by reading them at once instead of one by one. - # - # See ActionView::Template::Handlers::ERB.resource_cache_call_pattern for - # more information on what cache calls make a template eligible for this - # collection caching. - # - # The automatic cache multi read can be turned off like so: - # - # <%= render @notifications, cache: false %> - # - # === Explicit Collection Caching - # - # If the partial template doesn't start with a clean cache call as - # mentioned above, you can still benefit from collection caching by - # adding a special comment format anywhere in the template, like: - # - # <%# Template Collection: notification %> - # <% my_helper_that_calls_cache(some_arg, notification) do %> - # <%= notification.name %> - # <% end %> - # - # The pattern used to match these is <tt>/# Template Collection: (\S+)/</tt>, - # so it's important that you type it out just so. - # You can only declare one collection in a partial template file. + # The `cached: true` will make Action Views rendering issue a `read_multi` to + # the cache store instead of reading from it for every partial. def cache(name = {}, options = {}, &block) if controller.respond_to?(:perform_caching) && controller.perform_caching name_options = options.slice(:skip_digest, :virtual_path) diff --git a/actionview/lib/action_view/log_subscriber.rb b/actionview/lib/action_view/log_subscriber.rb index 9047dbdd85..aa38db2a3a 100644 --- a/actionview/lib/action_view/log_subscriber.rb +++ b/actionview/lib/action_view/log_subscriber.rb @@ -20,7 +20,15 @@ module ActionView end end alias :render_partial :render_template - alias :render_collection :render_template + + def render_collection(event) + identifier = event.payload[:identifier] || 'templates' + + info do + " Rendered collection of #{from_rails_root(identifier)}" \ + " #{render_count(event.payload)} (#{event.duration.round(1)}ms)" + end + end def logger ActionView::Base.logger @@ -38,6 +46,14 @@ module ActionView def rails_root @root ||= "#{Rails.root}/" end + + def render_count(payload) + if payload[:cache_hits] + "[#{payload[:cache_hits]} / #{payload[:count]} cache hits]" + else + "[#{payload[:count]} times]" + end + end end end diff --git a/actionview/lib/action_view/renderer/abstract_renderer.rb b/actionview/lib/action_view/renderer/abstract_renderer.rb index aa77a77acf..23e672a95f 100644 --- a/actionview/lib/action_view/renderer/abstract_renderer.rb +++ b/actionview/lib/action_view/renderer/abstract_renderer.rb @@ -35,8 +35,12 @@ module ActionView end end - def instrument(name, options={}) - ActiveSupport::Notifications.instrument("render_#{name}.action_view", options){ yield } + def instrument(name, **options) + options[:identifier] ||= (@template && @template.identifier) || @path + + ActiveSupport::Notifications.instrument("render_#{name}.action_view", options) do |payload| + yield payload + end end def prepend_formats(formats) diff --git a/actionview/lib/action_view/renderer/partial_renderer.rb b/actionview/lib/action_view/renderer/partial_renderer.rb index a9bd257e76..514804b08e 100644 --- a/actionview/lib/action_view/renderer/partial_renderer.rb +++ b/actionview/lib/action_view/renderer/partial_renderer.rb @@ -294,7 +294,7 @@ module ActionView def render(context, options, block) setup(context, options, block) - identifier = (@template = find_partial) ? @template.identifier : @path + @template = find_partial @lookup_context.rendered_format ||= begin if @template && @template.formats.present? @@ -305,11 +305,9 @@ module ActionView end if @collection - instrument(:collection, :identifier => identifier || "collection", :count => @collection.size) do - render_collection - end + render_collection else - instrument(:partial, :identifier => identifier) do + instrument(:partial) do render_partial end end @@ -318,15 +316,17 @@ module ActionView private def render_collection - return nil if @collection.blank? + instrument(:collection, count: @collection.size) do |payload| + return nil if @collection.blank? - if @options.key?(:spacer_template) - spacer = find_template(@options[:spacer_template], @locals.keys).render(@view, @locals) - end + if @options.key?(:spacer_template) + spacer = find_template(@options[:spacer_template], @locals.keys).render(@view, @locals) + end - cache_collection_render do - @template ? collection_with_template : collection_without_template - end.join(spacer).html_safe + cache_collection_render(payload) do + @template ? collection_with_template : collection_without_template + end.join(spacer).html_safe + end end def render_partial @@ -428,8 +428,6 @@ module ActionView layout = find_template(layout, @template_keys) end - collection_cache_eligible = automatic_cache_eligible? - partial_iteration = PartialIteration.new(@collection.size) locals[iteration] = partial_iteration @@ -438,11 +436,6 @@ module ActionView locals[counter] = partial_iteration.index content = template.render(view, locals) - - if collection_cache_eligible - collection_cache_rendered_partial(content, object) - end - content = layout.render(view, locals) { content } if layout partial_iteration.iterate! content diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb index 4860f00243..f7deba94ce 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -1,5 +1,3 @@ -require 'active_support/core_ext/object/try' - module ActionView module CollectionCaching # :nodoc: extend ActiveSupport::Concern @@ -11,40 +9,25 @@ module ActionView end private - def cache_collection_render - return yield unless cache_collection? + def cache_collection_render(instrumentation_payload) + return yield unless @options[:cached] keyed_collection = collection_by_cache_keys - cached_partials = collection_cache.read_multi(*keyed_collection.keys) + cached_partials = collection_cache.read_multi(*keyed_collection.keys) + instrumentation_payload[:cache_hits] = cached_partials.size @collection = keyed_collection.reject { |key, _| cached_partials.key?(key) }.values rendered_partials = @collection.empty? ? [] : yield index = 0 - keyed_collection.map do |cache_key, _| - cached_partials.fetch(cache_key) do - rendered_partials[index].tap { index += 1 } - end + fetch_or_cache_partial(cached_partials, order_by: keyed_collection.each_key) do + rendered_partials[index].tap { index += 1 } end end - def cache_collection? - @options.fetch(:cache, automatic_cache_eligible?) - end - - def automatic_cache_eligible? - @template && @template.eligible_for_collection_caching?(as: @options[:as]) - end - - def callable_cache_key? - @options[:cache].respond_to?(:call) - end - def collection_by_cache_keys - seed = callable_cache_key? ? @options[:cache] : ->(i) { i } - @collection.each_with_object({}) do |item, hash| - hash[expanded_cache_key(seed.call(item))] = item + hash[expanded_cache_key(item)] = item end end @@ -53,10 +36,13 @@ module ActionView key.frozen? ? key.dup : key # #read_multi & #write may require mutability, Dalli 2.6.0. end - def collection_cache_rendered_partial(rendered_partial, key_by) - if callable_cache_key? - key = expanded_cache_key(@options[:cache].call(key_by)) - collection_cache.write(key, rendered_partial, @options[:cache_options]) + def fetch_or_cache_partial(cached_partials, order_by:) + order_by.map do |cache_key| + cached_partials.fetch(cache_key) do + yield.tap do |rendered_partial| + collection_cache.write(cache_key, rendered_partial) + end + end end end end diff --git a/actionview/lib/action_view/template.rb b/actionview/lib/action_view/template.rb index 15fc2b71a3..169ee55fdc 100644 --- a/actionview/lib/action_view/template.rb +++ b/actionview/lib/action_view/template.rb @@ -130,7 +130,6 @@ module ActionView @source = source @identifier = identifier @handler = handler - @cache_name = extract_resource_cache_name @compiled = false @original_encoding = nil @locals = details[:locals] || [] @@ -166,10 +165,6 @@ module ActionView @type ||= Types[@formats.first] if @formats.first end - def eligible_for_collection_caching?(as: nil) - @cache_name == (as || inferred_cache_name).to_s - end - # Receives a view object and return a template similar to self by using @virtual_path. # # This method is useful if you have a template object but it does not contain its source @@ -355,23 +350,5 @@ module ActionView ActiveSupport::Notifications.instrument("#{action}.action_view".freeze, payload, &block) end end - - EXPLICIT_COLLECTION = /# Template Collection: (?<resource_name>\w+)/ - - def extract_resource_cache_name - if match = @source.match(EXPLICIT_COLLECTION) || resource_cache_call_match - match[:resource_name] - end - end - - def resource_cache_call_match - if @handler.respond_to?(:resource_cache_call_pattern) - @source.match(@handler.resource_cache_call_pattern) - end - end - - def inferred_cache_name - @inferred_cache_name ||= @virtual_path.split('/'.freeze).last.sub('_'.freeze, ''.freeze) - end end end diff --git a/actionview/lib/action_view/template/handlers/erb.rb b/actionview/lib/action_view/template/handlers/erb.rb index 1f8459c24b..85a100ed4c 100644 --- a/actionview/lib/action_view/template/handlers/erb.rb +++ b/actionview/lib/action_view/template/handlers/erb.rb @@ -123,31 +123,6 @@ module ActionView ).src end - # Returns Regexp to extract a cached resource's name from a cache call at the - # first line of a template. - # The extracted cache name is captured as :resource_name. - # - # <% cache notification do %> # => notification - # - # The pattern should support templates with a beginning comment: - # - # <%# Still extractable even though there's a comment %> - # <% cache notification do %> # => notification - # - # But fail to extract a name if a resource association is cached. - # - # <% cache notification.event do %> # => nil - def resource_cache_call_pattern - /\A - (?:<%\#.*%>)* # optional initial comment - \s* # followed by optional spaces or newlines - <%\s*cache[\(\s] # followed by an ERB call to cache - \s* # followed by optional spaces or newlines - (?<resource_name>\w+) # capture the cache call argument as :resource_name - [\s\)] # followed by a space or close paren - /xm - end - private def valid_encoding(string, encoding) diff --git a/actionview/test/template/log_subscriber_test.rb b/actionview/test/template/log_subscriber_test.rb index 4776c18b0b..93a0701dcc 100644 --- a/actionview/test/template/log_subscriber_test.rb +++ b/actionview/test/template/log_subscriber_test.rb @@ -86,7 +86,7 @@ class AVLogSubscriberTest < ActiveSupport::TestCase wait assert_equal 1, @logger.logged(:info).size - assert_match(/Rendered test\/_customer.erb/, @logger.logged(:info).last) + assert_match(/Rendered collection of test\/_customer.erb \[2 times\]/, @logger.logged(:info).last) end end @@ -96,7 +96,7 @@ class AVLogSubscriberTest < ActiveSupport::TestCase wait assert_equal 1, @logger.logged(:info).size - assert_match(/Rendered customers\/_customer\.html\.erb/, @logger.logged(:info).last) + assert_match(/Rendered collection of customers\/_customer\.html\.erb \[2 times\]/, @logger.logged(:info).last) end end @@ -106,7 +106,21 @@ class AVLogSubscriberTest < ActiveSupport::TestCase wait assert_equal 1, @logger.logged(:info).size - assert_match(/Rendered collection/, @logger.logged(:info).last) + assert_match(/Rendered collection of templates/, @logger.logged(:info).last) + end + end + + def test_render_collection_with_cached_set + Rails.stub(:root, File.expand_path(FIXTURE_LOAD_PATH)) do + def @view.view_cache_dependencies; []; end + def @view.fragment_cache_key(*); 'ahoy `controller` dependency'; end + + @view.render(partial: 'customers/customer', collection: [ Customer.new('david'), Customer.new('mary') ], cached: true, + locals: { greeting: 'hi' }) + wait + + assert_equal 1, @logger.logged(:info).size + assert_match(/Rendered collection of customers\/_customer\.html\.erb \[0 \/ 2 cache hits\]/, @logger.logged(:info).last) end end end diff --git a/actionview/test/template/render_test.rb b/actionview/test/template/render_test.rb index bf811abdd0..3561114d90 100644 --- a/actionview/test/template/render_test.rb +++ b/actionview/test/template/render_test.rb @@ -628,56 +628,59 @@ class LazyViewRenderTest < ActiveSupport::TestCase end end -class CachedCollectionViewRenderTest < CachedViewRenderTest +class CachedCollectionViewRenderTest < ActiveSupport::TestCase class CachedCustomer < Customer; end - teardown do - ActionView::PartialRenderer.collection_cache.clear - end + include RenderTestCases - test "with custom key" do - customer = Customer.new("david") - key = cache_key([customer, 'key'], "test/_customer") + # Ensure view path cache is primed + setup do + view_paths = ActionController::Base.view_paths + assert_equal ActionView::OptimizedFileSystemResolver, view_paths.first.class - ActionView::PartialRenderer.collection_cache.write(key, 'Hello') + ActionView::PartialRenderer.collection_cache = ActiveSupport::Cache::MemoryStore.new - assert_equal "Hello", - @view.render(partial: "test/customer", collection: [customer], cache: ->(item) { [item, 'key'] }) + setup_view(view_paths) end - test "with caching with custom key and rendering with different key" do - customer = Customer.new("david") - key = cache_key([customer, 'key'], "test/_customer") + teardown do + GC.start + I18n.reload! + end - ActionView::PartialRenderer.collection_cache.write(key, 'Hello') + test "collection caching does not cache by default" do + customer = Customer.new("david", 1) + key = cache_key(customer, "test/_customer") - assert_equal "Hello: david", - @view.render(partial: "test/customer", collection: [customer], cache: ->(item) { [item, 'another_key'] }) + ActionView::PartialRenderer.collection_cache.write(key, 'Cached') + + assert_not_equal "Cached", + @view.render(partial: "test/customer", collection: [customer]) end - test "automatic caching with inferred cache name" do - customer = CachedCustomer.new("david") - key = cache_key(customer, "test/_cached_customer") + test "collection caching with partial that doesn't use fragment caching" do + customer = Customer.new("david", 1) + key = cache_key(customer, "test/_customer") ActionView::PartialRenderer.collection_cache.write(key, 'Cached') assert_equal "Cached", - @view.render(partial: "test/cached_customer", collection: [customer]) + @view.render(partial: "test/customer", collection: [customer], cached: true) end - test "automatic caching with as name" do - customer = CachedCustomer.new("david") - key = cache_key(customer, "test/_cached_customer_as") + test "collection caching with cached true" do + customer = CachedCustomer.new("david", 1) + key = cache_key(customer, "test/_cached_customer") ActionView::PartialRenderer.collection_cache.write(key, 'Cached') assert_equal "Cached", - @view.render(partial: "test/cached_customer_as", collection: [customer], as: :buyer) + @view.render(partial: "test/cached_customer", collection: [customer], cached: true) end private - def cache_key(names, virtual_path) + def cache_key(*names, virtual_path) digest = ActionView::Digestor.digest name: virtual_path, finder: @view.lookup_context, dependencies: [] - @view.fragment_cache_key([ *Array(names), digest ]) + @view.fragment_cache_key([ *names, digest ]) end end diff --git a/actionview/test/template/template_test.rb b/actionview/test/template/template_test.rb index 921011b073..533c1c3219 100644 --- a/actionview/test/template/template_test.rb +++ b/actionview/test/template/template_test.rb @@ -192,38 +192,6 @@ class TestERBTemplate < ActiveSupport::TestCase assert_match(/\xFC/, e.message) end - def test_not_eligible_for_collection_caching_without_cache_call - [ - "<%= 'Hello' %>", - "<% cache_customer = 42 %>", - "<% cache customer.name do %><% end %>", - "<% my_cache customer do %><% end %>" - ].each do |body| - template = new_template(body, virtual_path: "test/foo/_customer") - assert_not template.eligible_for_collection_caching?, "Template #{body.inspect} should not be eligible for collection caching" - end - end - - def test_eligible_for_collection_caching_with_cache_call_or_explicit - [ - "<% cache customer do %><% end %>", - "<% cache(customer) do %><% end %>", - "<% cache( customer) do %><% end %>", - "<% cache( customer ) do %><% end %>", - "<%cache customer do %><% end %>", - "<% cache customer do %><% end %>", - " <% cache customer do %>\n<% end %>\n", - "<%# comment %><% cache customer do %><% end %>", - "<%# comment %>\n<% cache customer do %><% end %>", - "<%# comment\n line 2\n line 3 %>\n<% cache customer do %><% end %>", - "<%# comment 1 %>\n<%# comment 2 %>\n<% cache customer do %><% end %>", - "<%# comment 1 %>\n<%# Template Collection: customer %>\n<% my_cache customer do %><% end %>" - ].each do |body| - template = new_template(body, virtual_path: "test/foo/_customer") - assert template.eligible_for_collection_caching?, "Template #{body.inspect} should be eligible for collection caching" - end - end - def with_external_encoding(encoding) old = Encoding.default_external Encoding::Converter.new old, encoding if old != encoding diff --git a/activemodel/lib/active_model/validator.rb b/activemodel/lib/active_model/validator.rb index 1d2888a818..21339b628e 100644 --- a/activemodel/lib/active_model/validator.rb +++ b/activemodel/lib/active_model/validator.rb @@ -167,6 +167,13 @@ module ActiveModel def should_validate?(record) # :nodoc: !record.persisted? || record.changed? || record.marked_for_destruction? end + + # Always validate the record if the attribute is a virtual attribute. + # We have no way of knowing that the record was changed if the attribute + # does not exist in the database. + def unknown_attribute?(record, attribute) # :nodoc: + !record.attributes.include?(attribute.to_s) + end end # +BlockValidator+ is a special +EachValidator+ which receives a block on initialization diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 641e9195c6..c18403865f 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,11 @@ +* Ensure that mutations of the array returned from `ActiveRecord::Relation#to_a` + do not affect the original relation, by returning a duplicate array each time. + + This brings the behavior in line with `CollectionProxy#to_a`, which was + already more careful. + + *Matthew Draper* + * Fixed `where` for polymorphic associations when passed an array containing different types. Fixes #17011. diff --git a/activerecord/lib/active_record/association_relation.rb b/activerecord/lib/active_record/association_relation.rb index ee0bb8fafe..c18e88e4cf 100644 --- a/activerecord/lib/active_record/association_relation.rb +++ b/activerecord/lib/active_record/association_relation.rb @@ -10,7 +10,7 @@ module ActiveRecord end def ==(other) - other == to_a + other == records end def build(*args, &block) diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb index 2a9627a474..b9aed05135 100644 --- a/activerecord/lib/active_record/associations/collection_proxy.rb +++ b/activerecord/lib/active_record/associations/collection_proxy.rb @@ -979,6 +979,10 @@ module ActiveRecord end alias_method :to_a, :to_ary + def records # :nodoc: + load_target + end + # Adds one or more +records+ to the collection by setting their foreign keys # to the association's primary key. Returns +self+, so several appends may be # chained together. diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb index c7cc48ba16..f913f0852a 100644 --- a/activerecord/lib/active_record/associations/singular_association.rb +++ b/activerecord/lib/active_record/associations/singular_association.rb @@ -45,7 +45,7 @@ module ActiveRecord end def get_records - return scope.limit(1).to_a if skip_statement_cache? + return scope.limit(1).records if skip_statement_cache? conn = klass.connection sc = reflection.association_scope_cache(conn, owner) do diff --git a/activerecord/lib/active_record/null_relation.rb b/activerecord/lib/active_record/null_relation.rb index 0b500346bc..1ab4e0404f 100644 --- a/activerecord/lib/active_record/null_relation.rb +++ b/activerecord/lib/active_record/null_relation.rb @@ -1,7 +1,7 @@ module ActiveRecord module NullRelation # :nodoc: def exec_queries - @records = [] + @records = [].freeze end def pluck(*column_names) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 7e842668c6..09afdc6c69 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -253,17 +253,21 @@ module ActiveRecord # Converts relation objects to Array. def to_a + records.dup + end + + def records # :nodoc: load @records end # Serializes the relation objects Array. def encode_with(coder) - coder.represent_seq(nil, to_a) + coder.represent_seq(nil, records) end def as_json(options = nil) #:nodoc: - to_a.as_json(options) + records.as_json(options) end # Returns size of the records. @@ -298,13 +302,13 @@ module ActiveRecord # Returns true if there is exactly one record. def one? return super if block_given? - limit_value ? to_a.one? : size == 1 + limit_value ? records.one? : size == 1 end # Returns true if there is more than one record. def many? return super if block_given? - limit_value ? to_a.many? : size > 1 + limit_value ? records.many? : size > 1 end # Returns a cache key that can be used to identify the records fetched by @@ -418,7 +422,7 @@ module ActiveRecord if id.is_a?(Array) id.map.with_index { |one_id, idx| update(one_id, attributes[idx]) } elsif id == :all - to_a.each { |record| record.update(attributes) } + records.each { |record| record.update(attributes) } else if ActiveRecord::Base === id id = id.id @@ -457,7 +461,7 @@ module ActiveRecord MESSAGE where(conditions).destroy_all else - to_a.each(&:destroy).tap { reset } + records.each(&:destroy).tap { reset } end end @@ -587,7 +591,7 @@ module ActiveRecord def reset @last = @to_sql = @order_clause = @scope_for_create = @arel = @loaded = nil @should_eager_load = @join_dependency = nil - @records = [] + @records = [].freeze @offsets = {} self end @@ -654,21 +658,21 @@ module ActiveRecord def ==(other) case other when Associations::CollectionProxy, AssociationRelation - self == other.to_a + self == other.records when Relation other.to_sql == to_sql when Array - to_a == other + records == other end end def pretty_print(q) - q.pp(self.to_a) + q.pp(self.records) end # Returns true if relation is blank. def blank? - to_a.blank? + records.blank? end def values @@ -676,7 +680,7 @@ module ActiveRecord end def inspect - entries = to_a.take([limit_value, 11].compact.min).map!(&:inspect) + entries = records.take([limit_value, 11].compact.min).map!(&:inspect) entries[10] = '...' if entries.size == 11 "#<#{self.class.name} [#{entries.join(', ')}]>" @@ -685,14 +689,14 @@ module ActiveRecord protected def load_records(records) - @records = records + @records = records.freeze @loaded = true end private def exec_queries - @records = eager_loading? ? find_with_associations : @klass.find_by_sql(arel, bound_attributes) + @records = eager_loading? ? find_with_associations.freeze : @klass.find_by_sql(arel, bound_attributes).freeze preload = preload_values preload += includes_values unless eager_loading? diff --git a/activerecord/lib/active_record/relation/batches.rb b/activerecord/lib/active_record/relation/batches.rb index de005e2810..243ef0eae9 100644 --- a/activerecord/lib/active_record/relation/batches.rb +++ b/activerecord/lib/active_record/relation/batches.rb @@ -187,7 +187,7 @@ module ActiveRecord loop do if load - records = batch_relation.to_a + records = batch_relation.records ids = records.map(&:id) yielded_relation = self.where(primary_key => ids) yielded_relation.load_records(records) diff --git a/activerecord/lib/active_record/relation/batches/batch_enumerator.rb b/activerecord/lib/active_record/relation/batches/batch_enumerator.rb index c6e39814dd..13393dc605 100644 --- a/activerecord/lib/active_record/relation/batches/batch_enumerator.rb +++ b/activerecord/lib/active_record/relation/batches/batch_enumerator.rb @@ -35,7 +35,7 @@ module ActiveRecord return to_enum(:each_record) unless block_given? @relation.to_enum(:in_batches, of: @of, start: @start, finish: @finish, load: true).each do |relation| - relation.to_a.each { |record| yield record } + relation.records.each { |record| yield record } end end diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb index 2b1ac42395..f2578f5f96 100644 --- a/activerecord/lib/active_record/relation/delegation.rb +++ b/activerecord/lib/active_record/relation/delegation.rb @@ -38,7 +38,7 @@ module ActiveRecord delegate :to_xml, :to_yaml, :length, :collect, :map, :each, :all?, :include?, :to_ary, :join, :[], :&, :|, :+, :-, :sample, :reverse, :compact, :in_groups, :in_groups_of, - :shuffle, :split, to: :to_a + :shuffle, :split, to: :records delegate :table_name, :quoted_table_name, :primary_key, :quoted_primary_key, :connection, :columns_hash, :to => :klass diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 16563515f6..0037398554 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -506,7 +506,7 @@ module ActiveRecord def find_some_ordered(ids) ids = ids.slice(offset_value || 0, limit_value || ids.size) || [] - result = except(:limit, :offset).where(primary_key => ids).to_a + result = except(:limit, :offset).where(primary_key => ids).records if result.size == ids.size pk_type = @klass.type_for_attribute(primary_key) @@ -522,7 +522,7 @@ module ActiveRecord if loaded? @records.first else - @take ||= limit(1).to_a.first + @take ||= limit(1).records.first end end @@ -573,7 +573,7 @@ module ActiveRecord end def find_last(limit) - limit ? to_a.last(limit) : to_a.last + limit ? records.last(limit) : records.last end end end diff --git a/activerecord/lib/active_record/relation/spawn_methods.rb b/activerecord/lib/active_record/relation/spawn_methods.rb index 67d7f83cb4..d5c18a2a4a 100644 --- a/activerecord/lib/active_record/relation/spawn_methods.rb +++ b/activerecord/lib/active_record/relation/spawn_methods.rb @@ -29,7 +29,7 @@ module ActiveRecord # This is mainly intended for sharing common conditions between multiple associations. def merge(other) if other.is_a?(Array) - to_a & other + records & other elsif other spawn.merge!(other) else diff --git a/activerecord/lib/active_record/validations/absence.rb b/activerecord/lib/active_record/validations/absence.rb index 2e19e6dc5c..376d743c92 100644 --- a/activerecord/lib/active_record/validations/absence.rb +++ b/activerecord/lib/active_record/validations/absence.rb @@ -2,7 +2,7 @@ module ActiveRecord module Validations class AbsenceValidator < ActiveModel::Validations::AbsenceValidator # :nodoc: def validate_each(record, attribute, association_or_value) - return unless should_validate?(record) + return unless should_validate?(record) || unknown_attribute?(record, attribute) if record.class._reflect_on_association(attribute) association_or_value = Array.wrap(association_or_value).reject(&:marked_for_destruction?) end diff --git a/activerecord/lib/active_record/validations/length.rb b/activerecord/lib/active_record/validations/length.rb index 69e048eef1..fe34e4875c 100644 --- a/activerecord/lib/active_record/validations/length.rb +++ b/activerecord/lib/active_record/validations/length.rb @@ -2,7 +2,7 @@ module ActiveRecord module Validations class LengthValidator < ActiveModel::Validations::LengthValidator # :nodoc: def validate_each(record, attribute, association_or_value) - return unless should_validate?(record) || associations_are_dirty?(record) + return unless should_validate?(record) || unknown_attribute?(record, attribute) || associations_are_dirty?(record) if association_or_value.respond_to?(:loaded?) && association_or_value.loaded? association_or_value = association_or_value.target.reject(&:marked_for_destruction?) end diff --git a/activerecord/lib/active_record/validations/presence.rb b/activerecord/lib/active_record/validations/presence.rb index 7e85ed43ac..e34d2d70ab 100644 --- a/activerecord/lib/active_record/validations/presence.rb +++ b/activerecord/lib/active_record/validations/presence.rb @@ -2,7 +2,7 @@ module ActiveRecord module Validations class PresenceValidator < ActiveModel::Validations::PresenceValidator # :nodoc: def validate_each(record, attribute, association_or_value) - return unless should_validate?(record) + return unless should_validate?(record) || unknown_attribute?(record, attribute) if record.class._reflect_on_association(attribute) association_or_value = Array.wrap(association_or_value).reject(&:marked_for_destruction?) end diff --git a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb index bae4197aaf..9096cbc0ab 100644 --- a/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb @@ -146,7 +146,7 @@ class HasAndBelongsToManyAssociationsTest < ActiveRecord::TestCase assert_equal 1, country.treaties.count end - def test_join_table_composit_primary_key_should_not_warn + def test_join_table_composite_primary_key_should_not_warn country = Country.new(:name => 'India') country.country_id = 'c1' country.save! diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 090b885dd5..4c15ab6644 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1273,6 +1273,16 @@ class RelationTest < ActiveRecord::TestCase assert posts.loaded? end + def test_to_a_should_dup_target + posts = Post.all + + original_size = posts.size + removed = posts.to_a.pop + + assert_equal original_size, posts.size + assert_includes posts.to_a, removed + end + def test_build posts = Post.all diff --git a/activerecord/test/cases/validations/absence_validation_test.rb b/activerecord/test/cases/validations/absence_validation_test.rb index dd43ee358c..180acbcb6a 100644 --- a/activerecord/test/cases/validations/absence_validation_test.rb +++ b/activerecord/test/cases/validations/absence_validation_test.rb @@ -72,4 +72,18 @@ class AbsenceValidationTest < ActiveRecord::TestCase assert man.valid? end end + + def test_validates_absence_of_virtual_attribute_on_model + repair_validations(Interest) do + Interest.send(:attr_accessor, :token) + Interest.validates_absence_of(:token) + + interest = Interest.create!(topic: 'Thought Leadering') + assert interest.valid? + + interest.token = 'tl' + + assert interest.invalid? + end + end end diff --git a/activerecord/test/cases/validations/length_validation_test.rb b/activerecord/test/cases/validations/length_validation_test.rb index c5d8f8895c..624aeaaddb 100644 --- a/activerecord/test/cases/validations/length_validation_test.rb +++ b/activerecord/test/cases/validations/length_validation_test.rb @@ -74,4 +74,20 @@ class LengthValidationTest < ActiveRecord::TestCase assert owner.valid? assert pet.valid? end + + def test_validates_presence_of_virtual_attribute_on_model + repair_validations(Pet) do + Pet.send(:attr_accessor, :nickname) + Pet.validates_length_of(:name, minimum: 1) + Pet.validates_length_of(:nickname, minimum: 1) + + pet = Pet.create!(name: 'Fancy Pants', nickname: 'Fancy') + + assert pet.valid? + + pet.nickname = '' + + assert pet.invalid? + end + end end diff --git a/activerecord/test/cases/validations/presence_validation_test.rb b/activerecord/test/cases/validations/presence_validation_test.rb index 6f8ad06ab6..691f10a635 100644 --- a/activerecord/test/cases/validations/presence_validation_test.rb +++ b/activerecord/test/cases/validations/presence_validation_test.rb @@ -80,4 +80,19 @@ class PresenceValidationTest < ActiveRecord::TestCase assert man.valid? end end + + def test_validates_presence_of_virtual_attribute_on_model + repair_validations(Interest) do + Interest.send(:attr_accessor, :abbreviation) + Interest.validates_presence_of(:topic) + Interest.validates_presence_of(:abbreviation) + + interest = Interest.create!(topic: 'Thought Leadering', abbreviation: 'tl') + assert interest.valid? + + interest.abbreviation = '' + + assert interest.invalid? + end + end end diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index f4c324803c..db8d279cff 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,7 @@ +* Make `benchmark('something', silence: true)` actually work + + *DHH* + * Add `#on_weekday?` method to `Date`, `Time`, and `DateTime`. `#on_weekday?` returns `true` if the receiving date/time does not fall on a Saturday diff --git a/activesupport/lib/active_support/benchmarkable.rb b/activesupport/lib/active_support/benchmarkable.rb index 805b7a714f..3988b147ac 100644 --- a/activesupport/lib/active_support/benchmarkable.rb +++ b/activesupport/lib/active_support/benchmarkable.rb @@ -38,7 +38,7 @@ module ActiveSupport options[:level] ||= :info result = nil - ms = Benchmark.ms { result = options[:silence] ? silence { yield } : yield } + ms = Benchmark.ms { result = options[:silence] ? logger.silence { yield } : yield } logger.send(options[:level], '%s (%.1fms)' % [ message, ms ]) result else diff --git a/activesupport/lib/active_support/cache.rb b/activesupport/lib/active_support/cache.rb index 610105f41c..1c63e8a93f 100644 --- a/activesupport/lib/active_support/cache.rb +++ b/activesupport/lib/active_support/cache.rb @@ -333,21 +333,19 @@ module ActiveSupport options = names.extract_options! options = merged_options(options) - instrument_multi(:read, names, options) do |payload| - results = {} - names.each do |name| - key = normalize_key(name, options) - entry = read_entry(key, options) - if entry - if entry.expired? - delete_entry(key, options) - else - results[name] = entry.value - end + results = {} + names.each do |name| + key = normalize_key(name, options) + entry = read_entry(key, options) + if entry + if entry.expired? + delete_entry(key, options) + else + results[name] = entry.value end end - results end + results end # Fetches data from the cache, using the given keys. If there is data in @@ -555,17 +553,6 @@ module ActiveSupport ActiveSupport::Notifications.instrument("cache_#{operation}.active_support", payload){ yield(payload) } end - def instrument_multi(operation, keys, options = nil) - log do - formatted_keys = keys.map { |k| "- #{k}" }.join("\n") - "Caches multi #{operation}:\n#{formatted_keys}#{options.blank? ? "" : " (#{options.inspect})"}" - end - - payload = { key: keys } - payload.merge!(options) if options.is_a?(Hash) - ActiveSupport::Notifications.instrument("cache_#{operation}_multi.active_support", payload) { yield(payload) } - end - def log return unless logger && logger.debug? && !silence? logger.debug(yield) diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb index 174913365a..2ca4b51efa 100644 --- a/activesupport/lib/active_support/cache/mem_cache_store.rb +++ b/activesupport/lib/active_support/cache/mem_cache_store.rb @@ -96,16 +96,14 @@ module ActiveSupport options = names.extract_options! options = merged_options(options) - instrument_multi(:read, names, options) do - keys_to_names = Hash[names.map{|name| [normalize_key(name, options), name]}] - raw_values = @data.get_multi(keys_to_names.keys, :raw => true) - values = {} - raw_values.each do |key, value| - entry = deserialize_entry(value) - values[keys_to_names[key]] = entry.value unless entry.expired? - end - values + keys_to_names = Hash[names.map{|name| [normalize_key(name, options), name]}] + raw_values = @data.get_multi(keys_to_names.keys, :raw => true) + values = {} + raw_values.each do |key, value| + entry = deserialize_entry(value) + values[keys_to_names[key]] = entry.value unless entry.expired? end + values end # Increment a cached value. This method uses the memcached incr atomic diff --git a/activesupport/test/benchmarkable_test.rb b/activesupport/test/benchmarkable_test.rb index 04d4f5e503..5af041f458 100644 --- a/activesupport/test/benchmarkable_test.rb +++ b/activesupport/test/benchmarkable_test.rb @@ -41,6 +41,20 @@ class BenchmarkableTest < ActiveSupport::TestCase assert_last_logged 'test_run' end + def test_with_silence + assert_difference 'buffer.count', +2 do + benchmark('test_run') do + logger.info "SOMETHING" + end + end + + assert_difference 'buffer.count', +1 do + benchmark('test_run', silence: true) do + logger.info "NOTHING" + end + end + end + def test_within_level logger.level = ActiveSupport::Logger::DEBUG benchmark('included_debug_run', :level => :debug) { } diff --git a/activesupport/test/caching_test.rb b/activesupport/test/caching_test.rb index 4a299429f3..39fa3cedbe 100644 --- a/activesupport/test/caching_test.rb +++ b/activesupport/test/caching_test.rb @@ -1151,15 +1151,6 @@ class CacheStoreLoggerTest < ActiveSupport::TestCase @cache.mute { @cache.fetch('foo') { 'bar' } } assert @buffer.string.blank? end - - def test_multi_read_loggin - @cache.write 'hello', 'goodbye' - @cache.write 'world', 'earth' - - @cache.read_multi('hello', 'world') - - assert_match "Caches multi read:\n- hello\n- world", @buffer.string - end end class CacheEntryTest < ActiveSupport::TestCase diff --git a/railties/test/application/per_request_digest_cache_test.rb b/railties/test/application/per_request_digest_cache_test.rb index 210646c7c0..dfe3fc9354 100644 --- a/railties/test/application/per_request_digest_cache_test.rb +++ b/railties/test/application/per_request_digest_cache_test.rb @@ -29,6 +29,8 @@ class PerRequestDigestCacheTest < ActiveSupport::TestCase app_file 'app/controllers/customers_controller.rb', <<-RUBY class CustomersController < ApplicationController + self.perform_caching = true + def index render [ Customer.new('david', 1), Customer.new('dingus', 2) ] end |