Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Inverse after find or initialize #12375

Merged
merged 1 commit into from

3 participants

@arthurnn
Collaborator

Problem

When calling firm.clients_of_firm.find_or_initialize_by_client_of(id), and the id was found on the db, the inverse relation was not being set.

How so?

As find_or_initialize_by_* will only yield the block if the entry is new, the collection_proxy was never calling add_to_target, which make sense as the entry is on the target already, however the set_inverse_instance happen inside the add_to_target which in the case of a found entry was never happening.

Solution

Always call set_inverse_instance after find_or_instantiator_by_attributes

review :green_heart: @rafaelfranca @carlosantoniodasilva

I am pretty much sure this is fixed on 4.0 as there is no method_missing thing anymore. (anyways I will double check and submit a patch to 4.0 if the issue happens in there.)

@arthurnn arthurnn fix inverse_of when find_or_initialize_by_*
inverse_of relation was not being set when calling find_or_initialize_by_ and the entry was
found on the db.
fed6ac9
@egilburg egilburg commented on the diff
...rd/lib/active_record/associations/collection_proxy.rb
@@ -77,10 +77,12 @@ def respond_to?(name, include_private = false)
def method_missing(method, *args, &block)
match = DynamicFinderMatch.match(method)
if match && match.instantiator?
- send(:find_or_instantiator_by_attributes, match, match.attribute_names, *args) do |r|
- proxy_association.send :set_owner_attributes, r
- proxy_association.send :add_to_target, r
- yield(r) if block_given?
+ send(:find_or_instantiator_by_attributes, match, match.attribute_names, *args) do |record|
+ proxy_association.send :set_owner_attributes, record
+ proxy_association.send :add_to_target, record
+ yield(record) if block_given?
+ end.tap do |record|
+ proxy_association.send :set_inverse_instance, record

Could you just add this line after (or before) the yield statement, before closing the original block and without having to tap into a new one?

@arthurnn Collaborator

Actually not. because the issue in here is that the block:

do |record|
  proxy_association.send :set_owner_attributes, record
  proxy_association.send :add_to_target, record
  yield(record) if block_given?
end

will never get call, when find_or_instantiator_by_attributes actual find the object on the db.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca merged commit 54c05ac into from
@arthurnn arthurnn deleted the branch
@arthurnn
Collaborator

:heart: this will save us some extra queries =)

@rafaelfranca

Maybe this is still present in 4.0.0 when using activerecord-deprecated_finders

@arthurnn
Collaborator

good catch.. i will double check that.(probably on monday) ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 26, 2013
  1. @arthurnn

    fix inverse_of when find_or_initialize_by_*

    arthurnn authored
    inverse_of relation was not being set when calling find_or_initialize_by_ and the entry was
    found on the db.
This page is out of date. Refresh to see the latest.
View
5 activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## unreleased ##
+* When calling the method .find_or_initialize_by_* from a collection_proxy
+ it should set the inverse_of relation even when the entry was found on the db.
+
+ *arthurnn*
+
* Callbacks on has_many should access the in memory parent if a inverse_of is set.
*arthurnn*
View
10 activerecord/lib/active_record/associations/collection_proxy.rb
@@ -77,10 +77,12 @@ def respond_to?(name, include_private = false)
def method_missing(method, *args, &block)
match = DynamicFinderMatch.match(method)
if match && match.instantiator?
- send(:find_or_instantiator_by_attributes, match, match.attribute_names, *args) do |r|
- proxy_association.send :set_owner_attributes, r
- proxy_association.send :add_to_target, r
- yield(r) if block_given?
+ send(:find_or_instantiator_by_attributes, match, match.attribute_names, *args) do |record|
+ proxy_association.send :set_owner_attributes, record
+ proxy_association.send :add_to_target, record
+ yield(record) if block_given?
+ end.tap do |record|
+ proxy_association.send :set_inverse_instance, record

Could you just add this line after (or before) the yield statement, before closing the original block and without having to tap into a new one?

@arthurnn Collaborator

Actually not. because the issue in here is that the block:

do |record|
  proxy_association.send :set_owner_attributes, record
  proxy_association.send :add_to_target, record
  yield(record) if block_given?
end

will never get call, when find_or_instantiator_by_attributes actual find the object on the db.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
elsif target.respond_to?(method) || (!proxy_association.klass.respond_to?(method) && Class.respond_to?(method))
View
11 activerecord/test/cases/associations/has_many_associations_test.rb
@@ -662,6 +662,17 @@ def test_inverse_on_before_validate
end
end
+ def test_inverse_after_find_or_initialize
+ firm = companies(:first_firm)
+ client = firm.clients_of_firm.find_or_initialize_by_client_of(firm.id)
+ assert_no_queries do
+ assert_equal firm, client.firm
+ end
+
+ firm.name = "A new firm"
+ assert_equal firm.name, client.firm.name
+ end
+
def test_new_aliased_to_build
company = companies(:first_firm)
new_client = assert_no_queries { company.clients_of_firm.new("name" => "Another Client") }
Something went wrong with that request. Please try again.