New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delegate to `scope` rather than `merge!` for collection proxy #25877

Merged
merged 4 commits into from Feb 20, 2017

Conversation

Projects
None yet
@kamipo
Member

kamipo commented Jul 18, 2016

merge! association.scope(nullify: false) is expensive but most methods
do not need the merge.

@rails-bot

This comment has been minimized.

rails-bot commented Jul 18, 2016

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@matthewd

This comment has been minimized.

Member

matthewd commented Jul 18, 2016

@kamipo kamipo force-pushed the kamipo:delegate_to_scope_rather_than_merge branch Jul 18, 2016

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 20, 2016

r? @matthewd since you have the context.

@Tietew

This comment has been minimized.

Tietew commented Jul 20, 2016

Includes #25786, fixes #25732

@kamipo kamipo force-pushed the kamipo:delegate_to_scope_rather_than_merge branch 4 times, most recently Jul 20, 2016

@kamipo kamipo force-pushed the kamipo:delegate_to_scope_rather_than_merge branch 2 times, most recently Jul 28, 2016

@kamipo kamipo force-pushed the kamipo:delegate_to_scope_rather_than_merge branch 2 times, most recently Aug 7, 2016

@kamipo kamipo force-pushed the kamipo:delegate_to_scope_rather_than_merge branch 3 times, most recently Aug 17, 2016

serggl added a commit to serggl/rails that referenced this pull request Oct 14, 2016

@serggl

This comment has been minimized.

serggl commented Dec 5, 2016

@rafaelfranca @matthewd: what are the plans on reviewing/merging these changes? Are there any hidden blockers?
This patch is helping A LOT in our production app that heavily uses eager loading, so I would love to see this merged

kamipo added some commits Jul 18, 2016

Delegate to `scope` rather than `merge!` for collection proxy
`merge! association.scope(nullify: false)` is expensive but most methods
do not need the merge.
No need to cache collection proxies separately
Because merging the association scope was removed.

@kamipo kamipo force-pushed the kamipo:delegate_to_scope_rather_than_merge branch to 673ed05 Dec 24, 2016

@kaspth

This comment has been minimized.

Member

kaspth commented Jan 8, 2017

Fixes #27582

guillaume-winddle added a commit to guillaume-winddle/rails that referenced this pull request Jan 9, 2017

serggl added a commit to serggl/rails that referenced this pull request Jan 20, 2017

@kamipo

This comment has been minimized.

Member

kamipo commented Feb 20, 2017

Any chance this improvement is merged in 5.1?

proxy_association.reset
proxy_association.reset_scope
self
end
def respond_to?(name, include_private = false)
super || scope.respond_to?(name, include_private)

This comment has been minimized.

@matthewd

This comment has been minimized.

@kamipo

kamipo Feb 20, 2017

Member

These respond_to? and method_missing are needed for supporting extending association.
e.g.

def find_most_recent
order("id DESC").first
end
def newest
created.last
end
def the_association
proxy_association
end

https://github.com/rails/rails/blob/6c520b75286aa5969de427c5aa417061bd7c58bc/activerecord/test/cases/associations/extension_test.rb

diff --git a/activerecord/lib/active_record/associations/collection_proxy.rb b/activerecord/lib/active_record/associations/collection_proxy.rb
index 4b24e7a018..de871f169b 100644
--- a/activerecord/lib/active_record/associations/collection_proxy.rb
+++ b/activerecord/lib/active_record/associations/collection_proxy.rb
@@ -1116,10 +1116,6 @@ def reset
         self
       end
 
-      def respond_to?(name, include_private = false)
-        super || scope.respond_to?(name, include_private)
-      end
-
       delegate_methods = [
         QueryMethods,
         SpawnMethods,
@@ -1152,14 +1148,6 @@ def find_from_target?
         def exec_queries
           load_target
         end
-
-        def method_missing(method, *args, &block)
-          if scope.respond_to?(method)
-            scope.public_send(method, *args, &block)
-          else
-            super
-          end
-        end
     end
   end
 end

Result:

% be rake test_sqlite3 --verbose
Use TESTOPTS="--verbose" to pass --verbose, etc. to runners.
/Users/kamipo/.rbenv/versions/2.4.0/bin/ruby -w -I"lib:test" -I"/Users/kamipo/src/github.com/rails/rails/vendor/bundle/ruby/2.4.0/gems/rake-12.0.0/lib" "/Users/kamipo/src/github.com/rails/rails/vendor/bundle/ruby/2.4.0/gems/rake-12.0.0/lib/rake/rake_test_loader.rb" "test/cases/adapter_test.rb" "test/cases/aggregations_test.rb" "test/cases/ar_schema_test.rb" "test/cases/associations/association_scope_test.rb" "test/cases/associations/belongs_to_associations_test.rb" "test/cases/associations/bidirectional_destroy_dependencies_test.rb" "test/cases/associations/callbacks_test.rb" "test/cases/associations/cascaded_eager_loading_test.rb" "test/cases/associations/eager_load_includes_full_sti_class_test.rb" "test/cases/associations/eager_load_nested_include_test.rb" "test/cases/associations/eager_singularization_test.rb" "test/cases/associations/eager_test.rb" "test/cases/associations/extension_test.rb" "test/cases/associations/has_and_belongs_to_many_associations_test.rb" "test/cases/associations/has_many_associations_test.rb" "test/cases/associations/has_many_through_associations_test.rb" "test/cases/associations/has_one_associations_test.rb" "test/cases/associations/has_one_through_associations_test.rb" "test/cases/associations/inner_join_association_test.rb" "test/cases/associations/inverse_associations_test.rb" "test/cases/associations/join_model_test.rb" "test/cases/associations/left_outer_join_association_test.rb" "test/cases/associations/nested_through_associations_test.rb" "test/cases/associations/required_test.rb" "test/cases/associations_test.rb" "test/cases/attribute_decorators_test.rb" "test/cases/attribute_methods/read_test.rb" "test/cases/attribute_methods_test.rb" "test/cases/attribute_set_test.rb" "test/cases/attribute_test.rb" "test/cases/attributes_test.rb" "test/cases/autosave_association_test.rb" "test/cases/base_test.rb" "test/cases/batches_test.rb" "test/cases/binary_test.rb" "test/cases/bind_parameter_test.rb" "test/cases/cache_key_test.rb" "test/cases/calculations_test.rb" "test/cases/callbacks_test.rb" "test/cases/clone_test.rb" "test/cases/coders/json_test.rb" "test/cases/coders/yaml_column_test.rb" "test/cases/collection_cache_key_test.rb" "test/cases/column_alias_test.rb" "test/cases/column_definition_test.rb" "test/cases/comment_test.rb" "test/cases/connection_adapters/adapter_leasing_test.rb" "test/cases/connection_adapters/connection_handler_test.rb" "test/cases/connection_adapters/connection_specification_test.rb" "test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb" "test/cases/connection_adapters/mysql_type_lookup_test.rb" "test/cases/connection_adapters/schema_cache_test.rb" "test/cases/connection_adapters/type_lookup_test.rb" "test/cases/connection_management_test.rb" "test/cases/connection_pool_test.rb" "test/cases/connection_specification/resolver_test.rb" "test/cases/core_test.rb" "test/cases/counter_cache_test.rb" "test/cases/custom_locking_test.rb" "test/cases/database_statements_test.rb" "test/cases/date_test.rb" "test/cases/date_time_precision_test.rb" "test/cases/date_time_test.rb" "test/cases/defaults_test.rb" "test/cases/dirty_test.rb" "test/cases/disconnected_test.rb" "test/cases/dup_test.rb" "test/cases/enum_test.rb" "test/cases/errors_test.rb" "test/cases/explain_subscriber_test.rb" "test/cases/explain_test.rb" "test/cases/finder_respond_to_test.rb" "test/cases/finder_test.rb" "test/cases/fixture_set/file_test.rb" "test/cases/fixtures_test.rb" "test/cases/forbidden_attributes_protection_test.rb" "test/cases/habtm_destroy_order_test.rb" "test/cases/hot_compatibility_test.rb" "test/cases/i18n_test.rb" "test/cases/inheritance_test.rb" "test/cases/integration_test.rb" "test/cases/invalid_connection_test.rb" "test/cases/invertible_migration_test.rb" "test/cases/json_serialization_test.rb" "test/cases/locking_test.rb" "test/cases/log_subscriber_test.rb" "test/cases/migration/change_schema_test.rb" "test/cases/migration/change_table_test.rb" "test/cases/migration/column_attributes_test.rb" "test/cases/migration/column_positioning_test.rb" "test/cases/migration/columns_test.rb" "test/cases/migration/command_recorder_test.rb" "test/cases/migration/compatibility_test.rb" "test/cases/migration/create_join_table_test.rb" "test/cases/migration/foreign_key_test.rb" "test/cases/migration/index_test.rb" "test/cases/migration/logger_test.rb" "test/cases/migration/pending_migrations_test.rb" "test/cases/migration/references_foreign_key_test.rb" "test/cases/migration/references_index_test.rb" "test/cases/migration/references_statements_test.rb" "test/cases/migration/rename_table_test.rb" "test/cases/migration_test.rb" "test/cases/migrator_test.rb" "test/cases/mixin_test.rb" "test/cases/modules_test.rb" "test/cases/multiparameter_attributes_test.rb" "test/cases/multiple_db_test.rb" "test/cases/nested_attributes_test.rb" "test/cases/nested_attributes_with_callbacks_test.rb" "test/cases/persistence_test.rb" "test/cases/pooled_connections_test.rb" "test/cases/primary_keys_test.rb" "test/cases/query_cache_test.rb" "test/cases/quoting_test.rb" "test/cases/readonly_test.rb" "test/cases/reaper_test.rb" "test/cases/reflection_test.rb" "test/cases/relation/delegation_test.rb" "test/cases/relation/merging_test.rb" "test/cases/relation/mutation_test.rb" "test/cases/relation/or_test.rb" "test/cases/relation/predicate_builder_test.rb" "test/cases/relation/record_fetch_warning_test.rb" "test/cases/relation/where_chain_test.rb" "test/cases/relation/where_clause_test.rb" "test/cases/relation/where_test.rb" "test/cases/relation_test.rb" "test/cases/relations_test.rb" "test/cases/reload_models_test.rb" "test/cases/result_test.rb" "test/cases/sanitize_test.rb" "test/cases/schema_dumper_test.rb" "test/cases/schema_loading_test.rb" "test/cases/scoping/default_scoping_test.rb" "test/cases/scoping/named_scoping_test.rb" "test/cases/scoping/relation_scoping_test.rb" "test/cases/secure_token_test.rb" "test/cases/serialization_test.rb" "test/cases/serialized_attribute_test.rb" "test/cases/statement_cache_test.rb" "test/cases/store_test.rb" "test/cases/suppressor_test.rb" "test/cases/tasks/database_tasks_test.rb" "test/cases/tasks/mysql_rake_test.rb" "test/cases/tasks/postgresql_rake_test.rb" "test/cases/tasks/sqlite_rake_test.rb" "test/cases/test_fixtures_test.rb" "test/cases/time_precision_test.rb" "test/cases/timestamp_test.rb" "test/cases/touch_later_test.rb" "test/cases/transaction_callbacks_test.rb" "test/cases/transaction_isolation_test.rb" "test/cases/transactions_test.rb" "test/cases/type/adapter_specific_registry_test.rb" "test/cases/type/date_time_test.rb" "test/cases/type/integer_test.rb" "test/cases/type/string_test.rb" "test/cases/type/type_map_test.rb" "test/cases/type/unsigned_integer_test.rb" "test/cases/type_test.rb" "test/cases/types_test.rb" "test/cases/unconnected_test.rb" "test/cases/validations/absence_validation_test.rb" "test/cases/validations/association_validation_test.rb" "test/cases/validations/i18n_generate_message_validation_test.rb" "test/cases/validations/i18n_validation_test.rb" "test/cases/validations/length_validation_test.rb" "test/cases/validations/presence_validation_test.rb" "test/cases/validations/uniqueness_validation_test.rb" "test/cases/validations_test.rb" "test/cases/view_test.rb" "test/cases/yaml_serialization_test.rb" "test/cases/adapters/sqlite3/collation_test.rb" "test/cases/adapters/sqlite3/copy_table_test.rb" "test/cases/adapters/sqlite3/explain_test.rb" "test/cases/adapters/sqlite3/legacy_migration_test.rb" "test/cases/adapters/sqlite3/quoting_test.rb" "test/cases/adapters/sqlite3/sqlite3_adapter_test.rb" "test/cases/adapters/sqlite3/sqlite3_create_folder_test.rb" "test/cases/adapters/sqlite3/statement_pool_test.rb" 
/Users/kamipo/src/github.com/rails/rails/activesupport/lib/active_support/core_ext/enumerable.rb:20: warning: method redefined; discarding old sum
Using sqlite3
Run options: --seed 21502

# Running:

............................................................................................................................................................................................................................................................................................................................................................................................EEEE.E.EEE.................................................................E...................................................................................................E..................................................................E......................................................................................................................................................E.......................................................................................................................................................................................................................................................................................E............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................S......................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................S.................................................................................................................................................................................................................................................../Users/kamipo/src/github.com/rails/rails/vendor/bundle/ruby/2.4.0/gems/builder-3.2.2/lib/builder/xchar.rb:111: warning: constant ::Fixnum is deprecated
......................................................................................................................S.............................................................................................................................E......................................................................................................................................................................................................................................................................................................................................................................................................................................................S..........................................................................................................................................

Finished in 81.682485s, 59.2906 runs/s, 166.1678 assertions/s.

  1) Error:
AssociationsExtensionsTest#test_proxy_association_after_scoped:
NoMethodError: undefined method `the_association' for #<Comment::ActiveRecord_Associations_CollectionProxy:0x007fe271d0c5b8>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/extension_test.rb:72:in `test_proxy_association_after_scoped'


  2) Error:
AssociationsExtensionsTest#test_extension_on_has_many:
NoMethodError: undefined method `find_most_recent' for #<Comment::ActiveRecord_Associations_CollectionProxy:0x007fe26d5900b8>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/extension_test.rb:13:in `test_extension_on_has_many'


  3) Error:
AssociationsExtensionsTest#test_named_extension_on_habtm:
NoMethodError: undefined method `find_most_recent' for #<Project::ActiveRecord_Associations_CollectionProxy:0x007fe271cee3b0>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/extension_test.rb:21:in `test_named_extension_on_habtm'


  4) Error:
AssociationsExtensionsTest#test_marshalling_named_extensions:
NoMethodError: undefined method `find_most_recent' for #<Project::ActiveRecord_Associations_CollectionProxy:0x007fe271cd5e28>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/extension_test.rb:54:in `test_marshalling_named_extensions'


  5) Error:
AssociationsExtensionsTest#test_named_extension_and_block_on_habtm:
NoMethodError: undefined method `find_most_recent' for #<Project::ActiveRecord_Associations_CollectionProxy:0x007fe27391e698>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/extension_test.rb:30:in `test_named_extension_and_block_on_habtm'


  6) Error:
AssociationsExtensionsTest#test_marshalling_extensions:
NoMethodError: undefined method `find_most_recent' for #<Project::ActiveRecord_Associations_CollectionProxy:0x007fe2738e4740>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/extension_test.rb:41:in `test_marshalling_extensions'


  7) Error:
AssociationsExtensionsTest#test_extension_on_habtm:
NoMethodError: undefined method `find_most_recent' for #<Project::ActiveRecord_Associations_CollectionProxy:0x007fe2738c4198>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/extension_test.rb:17:in `test_extension_on_habtm'


  8) Error:
AssociationsExtensionsTest#test_named_two_extensions_on_habtm:
NoMethodError: undefined method `find_most_recent' for #<Project::ActiveRecord_Associations_CollectionProxy:0x007fe27389a230>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/extension_test.rb:25:in `test_named_two_extensions_on_habtm'


  9) Error:
HasAndBelongsToManyAssociationsTest#test_association_with_extend_option:
NoMethodError: undefined method `category' for #<Project::ActiveRecord_Associations_CollectionProxy:0x007fe271d15348>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb:620:in `test_association_with_extend_option'


 10) Error:
HasManyAssociationsTest#test_association_with_extend_option:
NoMethodError: undefined method `author' for #<Comment::ActiveRecord_Associations_CollectionProxy:0x007fe26cb54310>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/has_many_associations_test.rb:2247:in `block in <class:HasManyAssociationsTest>'


 11) Error:
HasManyAssociationsTest#test_association_with_extend_option_with_multiple_extensions:
NoMethodError: undefined method `author' for #<Comment::ActiveRecord_Associations_CollectionProxy:0x007fe273a06a60>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/has_many_associations_test.rb:2253:in `block in <class:HasManyAssociationsTest>'


 12) Error:
HasManyThroughAssociationsTest#test_merge_join_association_with_has_many_through_association_proxy:
NoMethodError: undefined method `ratings' for #<Comment::ActiveRecord_Associations_CollectionProxy:0x007fe270a8be20>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/has_many_through_associations_test.rb:794:in `block in test_merge_join_association_with_has_many_through_association_proxy'
    /Users/kamipo/src/github.com/rails/rails/activesupport/lib/active_support/testing/assertions.rb:30:in `assert_nothing_raised'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/has_many_through_associations_test.rb:794:in `test_merge_join_association_with_has_many_through_association_proxy'


 13) Error:
AssociationsJoinModelTest#test_polymorphic_has_many_going_through_join_model_with_custom_select_and_joins:
NoMethodError: undefined method `add_joins_and_select' for #<Tag::ActiveRecord_Associations_CollectionProxy:0x007fe271c4b5e8>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/associations/join_model_test.rb:90:in `test_polymorphic_has_many_going_through_join_model_with_custom_select_and_joins'


 14) Error:
NamedScopingTest#test_scopes_to_get_newest:
NoMethodError: undefined method `newest' for #<Comment::ActiveRecord_Associations_CollectionProxy:0x007fe271da1690>
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:124:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:94:in `method_missing'
    /Users/kamipo/src/github.com/rails/rails/activerecord/test/cases/scoping/named_scoping_test.rb:519:in `test_scopes_to_get_newest'

4843 runs, 13573 assertions, 0 failures, 14 errors, 4 skips

You have skipped tests. Run with --verbose for details.
rake aborted!
Command failed with status (1): [ruby -w -I"lib:test" -I"/Users/kamipo/src/github.com/rails/rails/vendor/bundle/ruby/2.4.0/gems/rake-12.0.0/lib" "/Users/kamipo/src/github.com/rails/rails/vendor/bundle/ruby/2.4.0/gems/rake-12.0.0/lib/rake/rake_test_loader.rb" "test/cases/adapter_test.rb" "test/cases/aggregations_test.rb" "test/cases/ar_schema_test.rb" "test/cases/associations/association_scope_test.rb" "test/cases/associations/belongs_to_associations_test.rb" "test/cases/associations/bidirectional_destroy_dependencies_test.rb" "test/cases/associations/callbacks_test.rb" "test/cases/associations/cascaded_eager_loading_test.rb" "test/cases/associations/eager_load_includes_full_sti_class_test.rb" "test/cases/associations/eager_load_nested_include_test.rb" "test/cases/associations/eager_singularization_test.rb" "test/cases/associations/eager_test.rb" "test/cases/associations/extension_test.rb" "test/cases/associations/has_and_belongs_to_many_associations_test.rb" "test/cases/associations/has_many_associations_test.rb" "test/cases/associations/has_many_through_associations_test.rb" "test/cases/associations/has_one_associations_test.rb" "test/cases/associations/has_one_through_associations_test.rb" "test/cases/associations/inner_join_association_test.rb" "test/cases/associations/inverse_associations_test.rb" "test/cases/associations/join_model_test.rb" "test/cases/associations/left_outer_join_association_test.rb" "test/cases/associations/nested_through_associations_test.rb" "test/cases/associations/required_test.rb" "test/cases/associations_test.rb" "test/cases/attribute_decorators_test.rb" "test/cases/attribute_methods/read_test.rb" "test/cases/attribute_methods_test.rb" "test/cases/attribute_set_test.rb" "test/cases/attribute_test.rb" "test/cases/attributes_test.rb" "test/cases/autosave_association_test.rb" "test/cases/base_test.rb" "test/cases/batches_test.rb" "test/cases/binary_test.rb" "test/cases/bind_parameter_test.rb" "test/cases/cache_key_test.rb" "test/cases/calculations_test.rb" "test/cases/callbacks_test.rb" "test/cases/clone_test.rb" "test/cases/coders/json_test.rb" "test/cases/coders/yaml_column_test.rb" "test/cases/collection_cache_key_test.rb" "test/cases/column_alias_test.rb" "test/cases/column_definition_test.rb" "test/cases/comment_test.rb" "test/cases/connection_adapters/adapter_leasing_test.rb" "test/cases/connection_adapters/connection_handler_test.rb" "test/cases/connection_adapters/connection_specification_test.rb" "test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb" "test/cases/connection_adapters/mysql_type_lookup_test.rb" "test/cases/connection_adapters/schema_cache_test.rb" "test/cases/connection_adapters/type_lookup_test.rb" "test/cases/connection_management_test.rb" "test/cases/connection_pool_test.rb" "test/cases/connection_specification/resolver_test.rb" "test/cases/core_test.rb" "test/cases/counter_cache_test.rb" "test/cases/custom_locking_test.rb" "test/cases/database_statements_test.rb" "test/cases/date_test.rb" "test/cases/date_time_precision_test.rb" "test/cases/date_time_test.rb" "test/cases/defaults_test.rb" "test/cases/dirty_test.rb" "test/cases/disconnected_test.rb" "test/cases/dup_test.rb" "test/cases/enum_test.rb" "test/cases/errors_test.rb" "test/cases/explain_subscriber_test.rb" "test/cases/explain_test.rb" "test/cases/finder_respond_to_test.rb" "test/cases/finder_test.rb" "test/cases/fixture_set/file_test.rb" "test/cases/fixtures_test.rb" "test/cases/forbidden_attributes_protection_test.rb" "test/cases/habtm_destroy_order_test.rb" "test/cases/hot_compatibility_test.rb" "test/cases/i18n_test.rb" "test/cases/inheritance_test.rb" "test/cases/integration_test.rb" "test/cases/invalid_connection_test.rb" "test/cases/invertible_migration_test.rb" "test/cases/json_serialization_test.rb" "test/cases/locking_test.rb" "test/cases/log_subscriber_test.rb" "test/cases/migration/change_schema_test.rb" "test/cases/migration/change_table_test.rb" "test/cases/migration/column_attributes_test.rb" "test/cases/migration/column_positioning_test.rb" "test/cases/migration/columns_test.rb" "test/cases/migration/command_recorder_test.rb" "test/cases/migration/compatibility_test.rb" "test/cases/migration/create_join_table_test.rb" "test/cases/migration/foreign_key_test.rb" "test/cases/migration/index_test.rb" "test/cases/migration/logger_test.rb" "test/cases/migration/pending_migrations_test.rb" "test/cases/migration/references_foreign_key_test.rb" "test/cases/migration/references_index_test.rb" "test/cases/migration/references_statements_test.rb" "test/cases/migration/rename_table_test.rb" "test/cases/migration_test.rb" "test/cases/migrator_test.rb" "test/cases/mixin_test.rb" "test/cases/modules_test.rb" "test/cases/multiparameter_attributes_test.rb" "test/cases/multiple_db_test.rb" "test/cases/nested_attributes_test.rb" "test/cases/nested_attributes_with_callbacks_test.rb" "test/cases/persistence_test.rb" "test/cases/pooled_connections_test.rb" "test/cases/primary_keys_test.rb" "test/cases/query_cache_test.rb" "test/cases/quoting_test.rb" "test/cases/readonly_test.rb" "test/cases/reaper_test.rb" "test/cases/reflection_test.rb" "test/cases/relation/delegation_test.rb" "test/cases/relation/merging_test.rb" "test/cases/relation/mutation_test.rb" "test/cases/relation/or_test.rb" "test/cases/relation/predicate_builder_test.rb" "test/cases/relation/record_fetch_warning_test.rb" "test/cases/relation/where_chain_test.rb" "test/cases/relation/where_clause_test.rb" "test/cases/relation/where_test.rb" "test/cases/relation_test.rb" "test/cases/relations_test.rb" "test/cases/reload_models_test.rb" "test/cases/result_test.rb" "test/cases/sanitize_test.rb" "test/cases/schema_dumper_test.rb" "test/cases/schema_loading_test.rb" "test/cases/scoping/default_scoping_test.rb" "test/cases/scoping/named_scoping_test.rb" "test/cases/scoping/relation_scoping_test.rb" "test/cases/secure_token_test.rb" "test/cases/serialization_test.rb" "test/cases/serialized_attribute_test.rb" "test/cases/statement_cache_test.rb" "test/cases/store_test.rb" "test/cases/suppressor_test.rb" "test/cases/tasks/database_tasks_test.rb" "test/cases/tasks/mysql_rake_test.rb" "test/cases/tasks/postgresql_rake_test.rb" "test/cases/tasks/sqlite_rake_test.rb" "test/cases/test_fixtures_test.rb" "test/cases/time_precision_test.rb" "test/cases/timestamp_test.rb" "test/cases/touch_later_test.rb" "test/cases/transaction_callbacks_test.rb" "test/cases/transaction_isolation_test.rb" "test/cases/transactions_test.rb" "test/cases/type/adapter_specific_registry_test.rb" "test/cases/type/date_time_test.rb" "test/cases/type/integer_test.rb" "test/cases/type/string_test.rb" "test/cases/type/type_map_test.rb" "test/cases/type/unsigned_integer_test.rb" "test/cases/type_test.rb" "test/cases/types_test.rb" "test/cases/unconnected_test.rb" "test/cases/validations/absence_validation_test.rb" "test/cases/validations/association_validation_test.rb" "test/cases/validations/i18n_generate_message_validation_test.rb" "test/cases/validations/i18n_validation_test.rb" "test/cases/validations/length_validation_test.rb" "test/cases/validations/presence_validation_test.rb" "test/cases/validations/uniqueness_validation_test.rb" "test/cases/validations_test.rb" "test/cases/view_test.rb" "test/cases/yaml_serialization_test.rb" "test/cases/adapters/sqlite3/collation_test.rb" "test/cases/adapters/sqlite3/copy_table_test.rb" "test/cases/adapters/sqlite3/explain_test.rb" "test/cases/adapters/sqlite3/legacy_migration_test.rb" "test/cases/adapters/sqlite3/quoting_test.rb" "test/cases/adapters/sqlite3/sqlite3_adapter_test.rb" "test/cases/adapters/sqlite3/sqlite3_create_folder_test.rb" "test/cases/adapters/sqlite3/statement_pool_test.rb" ]
/Users/kamipo/src/github.com/rails/rails/vendor/bundle/ruby/2.4.0/gems/rake-12.0.0/exe/rake:27:in `<top (required)>'
/Users/kamipo/.rbenv/versions/2.4.0/bin/bundle:22:in `load'
/Users/kamipo/.rbenv/versions/2.4.0/bin/bundle:22:in `<main>'
Tasks: TOP => test_sqlite3 => test:sqlite3
(See full trace by running task with --trace)
bundle exec rake test_sqlite3 --verbose  64.12s user 6.51s system 78% cpu 1:29.94 total
@@ -1149,6 +1148,18 @@ def find_from_target?
def exec_queries
load_target
end
def respond_to_missing?(method, _)
scope.respond_to?(method) || super

This comment has been minimized.

@kamipo

kamipo Feb 20, 2017

Member

@matthewd As you pointed out, I fixed to define respond_to_missing? instead of respond_to?. Thanks!

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 23, 2017

Member

Should not _ be passed to respond_to?? Also I don't think super does something here.

This comment has been minimized.

@matthewd

matthewd Feb 23, 2017

Member

@rafaelfranca no; our method_missing checks respond_to? (without allowing privates) and then uses public_send: we don't respond to scope's private methods, even when invoked privately.

@matthewd matthewd merged commit eb41ac4 into rails:master Feb 20, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd

This comment has been minimized.

Member

matthewd commented Feb 20, 2017

❤️ 💚 💙 💛 💜

Thanks for persevering on this.. it got stuck for much longer than it should've.

@kamipo kamipo deleted the kamipo:delegate_to_scope_rather_than_merge branch Feb 20, 2017

@serggl

This comment has been minimized.

serggl commented Feb 20, 2017

Thank you!

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 20, 2017

Badass @kamipo ❤️

@Billiam

This comment has been minimized.

Billiam commented Mar 7, 2017

Is there any chance this fix will be backported to 5.0.x, or will it be 5.1+ only?

seanlinsley added a commit to modernmsg/rails that referenced this pull request Mar 11, 2017

Merge pull request rails#25877 from kamipo/delegate_to_scope_rather_t…
…han_merge

Delegate to `scope` rather than `merge!` for collection proxy

There was a conflict in activerecord/test/cases/associations/has_many_associations_test.rb
that I skipped over instead of properly resolving, but this still includes the behavior changes.
@yosiat

This comment has been minimized.

yosiat commented Mar 19, 2017

Is there any change this fix will be back ported to 4.2?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 4, 2017

Is there any change this fix will be back ported to 4.2?

4.2 is not supported anymore for anything besides security fixes.

firm.clients << Client.first
firm.save!
assert_equal firm.clients.count, firm.clients.update_all(description: "Great!")
assert_not_equal clients_proxy_id, firm.clients.object_id

This comment has been minimized.

@avokhmin

avokhmin May 19, 2017

Contributor

it was so easy to stub associations:

allow(foo.bars).to receive(:find) { baz }

but already no :(

This comment has been minimized.

@kamipo

kamipo May 19, 2017

Member

Will fix it in #29098 (2c3d99e).

This comment has been minimized.

@avokhmin

avokhmin May 19, 2017

Contributor

@kamipo Awesome! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment