Inverse of on build #12413

Merged
merged 1 commit into from Sep 30, 2013

Conversation

Projects
None yet
5 participants
Member

arthurnn commented Sep 30, 2013

Problem

When building a new relations(any type of relation) we need to setup the inverse_of association.
See #10371

Solution

Call .set_inverse_instance on the association parent. Also remove set_inverse_instance from add_to_target on collection, as we build it before, and we dont need that extra call.

review @rafaelfranca @robin850

Owner

rafaelfranca commented Sep 30, 2013

Was this fixed on 4+?

Member

arthurnn commented Sep 30, 2013

yep.. on rails 4, we do almost the same thing. https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/association.rb#L239 I can even refectory on 3.2 to pull those four calls in a method initialize_attributes like rails 4 does.

Owner

rafaelfranca commented Sep 30, 2013

I think is better to stick with the smaller patch.

rafaelfranca added a commit that referenced this pull request Sep 30, 2013

@rafaelfranca rafaelfranca merged commit ccd11d5 into rails:3-2-stable Sep 30, 2013

Owner

rafaelfranca commented Sep 30, 2013

❤️ 💚 💙 💛 💜

@arthurnn arthurnn deleted the arthurnn:inverse_of_on_build branch Sep 30, 2013

Member

arthurnn commented Sep 30, 2013

👍

Contributor

masterkain commented Oct 4, 2013

This breaks Devise for me. Using 3.2-stable. I have not conducted many tests because it got me a while to get to the culprit, however we saw exceptions after a bundle update on user registration.

What happens:

Started POST "/account" for 127.0.0.1 at 2013-10-04 03:09:22 +0200
Processing by Users::RegistrationsController#create as HTML
  Parameters: {"utf8"=>"✓", "authenticity_token"=>"9qsDa9jS9nMHNgkysAb7mn/6akG+Dp3tvF/iLazx5W4=", "user"=>{"email"=>"masterkain+456y3@gmail.com", "password"=>"[FILTERED]", "password_confirmation"=>"[FILTERED]", "personal_name"=>"", "tos"=>"1"}}
   (0.1ms)  BEGIN
  Account Exists (1.1ms)  SELECT 1 AS one FROM "accounts" WHERE "accounts"."email" = 'masterkain+456y3@gmail.com' LIMIT 1
  User Load (2.8ms)  SELECT "accounts".* FROM "accounts" WHERE "accounts"."deleted_at" IS NULL AND "accounts"."type" IN ('User') AND "accounts"."authentication_token" = 'QG32Cs1qPzPBubzdTy5L' LIMIT 1
  User Load (0.7ms)  SELECT "accounts".* FROM "accounts" WHERE "accounts"."deleted_at" IS NULL AND "accounts"."type" IN ('User') AND "accounts"."confirmation_token" = 'XVpypX2aa9sAqbftCh3e' LIMIT 1
  CACHE (0.0ms)  SELECT 1 AS one FROM "accounts" WHERE "accounts"."email" = 'masterkain+456y3@gmail.com' LIMIT 1
  User Load (0.7ms)  SELECT "accounts".* FROM "accounts" WHERE "accounts"."deleted_at" IS NULL AND "accounts"."type" IN ('User') AND "accounts"."confirmation_token" = 'qt99E6KyXmmjimkSmX57' LIMIT 1
  CACHE (0.0ms)  SELECT 1 AS one FROM "accounts" WHERE "accounts"."email" = 'masterkain+456y3@gmail.com' LIMIT 1
  User Load (0.5ms)  SELECT "accounts".* FROM "accounts" WHERE "accounts"."deleted_at" IS NULL AND "accounts"."type" IN ('User') AND "accounts"."confirmation_token" = 'FJpzpsHxyjqfLJSEF7xm' LIMIT 1
  CACHE (0.0ms)  SELECT 1 AS one FROM "accounts" WHERE "accounts"."email" = 'masterkain+456y3@gmail.com' LIMIT 1

and so forth, until it goes in SystemStackError.

It seems like it's looping on a validation or something, need to investigate more.

Member

arthurnn commented Oct 4, 2013

@masterkain Are you sure the error is from this commit, and not from Devise update or something like that? we are using this version on production in a fair big codebase and I didnt see this error yet.(which version of devise are you using?)

Contributor

masterkain commented Oct 4, 2013

Yes, I'm fairly sure, the error begins with this commit, devise is unchanged, I'm using 2.2.7.

I immediately suspected rails was involved, so I reverted the bundle update commit (which updated just few gems) and started to try all the rails commits I was missing (about 8), and this is the one generating the infinite loop.

Member

arthurnn commented Oct 4, 2013

@masterkain Can you put together a simple rails+devise app with a spec that illustrate this error? I cloned devise 2.2.7, pointed to rails 3-2-stable branch, and all tests are green.

Contributor

pacoguzman commented Oct 4, 2013

this commit broke our application's build, I'm studying the problem I'll try to provide a test application asap. But basically seems that (as an example).

class Post; has_many :comments; :inverse_of => :title; end
class Comment; belongs_to :post; end

post = Post.new(:comments => [Comment.new])
post.save

If you try to access to the post on the Comment class to validate something the post is nil.

Member

arthurnn commented Oct 4, 2013

Ok, I just found out where the error lies on. Submitting a PR with the fix in a few minutes.

Contributor

pacoguzman commented Oct 4, 2013

Awesome! thanks

Member

arthurnn commented Oct 4, 2013

@masterkain @pacoguzman could you guys update to latest 3.2 and let me know if the bug still happens.?
thanks

Contributor

pacoguzman commented Oct 5, 2013

Everything green now, thanks!

Contributor

masterkain commented Oct 7, 2013

I'm afraid this does not solve the problem for me, always on user registration.

Using revision 7ed5bdc

A SystemStackError occurred in registrations#create:

 stack level too deep
 /home/ubuntu/apps/myapp-rails/shared/bundle/ruby/2.0.0/bundler/gems/rails-7ed5bdc83447/activesupport/lib/active_support/notifications/instrumenter.rb:23

I have nothing else in the backtrace.
Better start looking at user's code, but nothing changed in that regard, only Rails.

Owner

rafaelfranca commented Oct 7, 2013

@masterkain we will need a way to reproduce your issue or we can't do anything.

Owner

rafaelfranca commented Oct 7, 2013

@masterkain could you help us with this?

Contributor

masterkain commented Oct 7, 2013

yeah I'm trying to see where the bug lies, the fact that the stacktrace flags the instrumentation and given I have callbacks using them I have some chance to assemble something for reproducing the behavior.

Contributor

masterkain commented Oct 7, 2013

Ok, the error appears to be related to creating an association directly in a before_create, like this:

class Account < ActiveRecord::Base
  has_one :account_stat, inverse_of: :account # counters, billing, etc.

  # Use directly the magic create_association Rails' method.
  before_create :create_account_stat!
end

class AccountStat < ActiveRecord::Base
  belongs_to :account, inverse_of: :account_stat
end

I'm going ahead in a few and try to assemble a reproducible version, but commenting out the before_create or the inverse_of makes the specs pass.

Member

arthurnn commented Oct 8, 2013

Interesting, I was able to simulate the SystemStackError

# Running:

E

Finished in 0.090370s, 11.0656 runs/s, 0.0000 assertions/s.

  1) Error:
FirstTest#test_relation:
SystemStackError: stack level too deep
    /Users/arthurnn/dev/rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:211

Which is happening in master as well. @rafaelfranca I will write tests for this and submit a patch for it.

arthurnn added a commit to arthurnn/rails that referenced this pull request Oct 8, 2013

make cycle detection on autosave works on a thread level
Fixes `stack level too deep` error when creating a children object on a parent callback.
This error was only happening when the inverse_of relation was set.

[related #12413]

rafaelfranca added a commit that referenced this pull request Oct 10, 2013

Revert "Merge pull request #12413 from arthurnn/inverse_of_on_build"
This reverts commit ccd11d5, reversing
changes made to 54c05ac.

Reason: This caused a regression when the associated record is created
in a before_create callback. See
#12413 (comment)
Owner

rafaelfranca commented Oct 10, 2013

@masterkain could you try 3-2-stable?

rafaelfranca added a commit that referenced this pull request Oct 10, 2013

Revert "Merge pull request #12413 from arthurnn/inverse_of_on_build"
This reverts commit ccd11d5, reversing
changes made to 54c05ac.

Reason: This caused a regression when the associated record is created
in a before_create callback. See
#12413 (comment)
Contributor

masterkain commented Oct 11, 2013

it seems to work, thanks, I'm going to run the spec suite over to make sure.

tenderlove added a commit that referenced this pull request Oct 16, 2013

Merge branch '3-2-15' into 3-2-sec
* 3-2-15:
  bumping to rc3
  Revert "Merge pull request #12413 from arthurnn/inverse_of_on_build"
  Revert "Merge pull request #12443 from arthurnn/add_inverse_of_add_target"
  bumping to rc2
  Merge pull request #12443 from arthurnn/add_inverse_of_add_target
  bumping version to 3.2.15.rc1
  Fix STI scopes using benolee's suggestion. Fixes #11939

tenderlove added a commit that referenced this pull request Oct 16, 2013

Merge branch '3-2-sec' into 3-2-stable
* 3-2-sec:
  updating changelogs
  bumping to 3.2.15
  bumping to rc3
  Revert "Merge pull request #12413 from arthurnn/inverse_of_on_build"
  Revert "Merge pull request #12443 from arthurnn/add_inverse_of_add_target"
  bumping to rc2
  Merge pull request #12443 from arthurnn/add_inverse_of_add_target
  bumping version to 3.2.15.rc1
  Remove the use of String#% when formatting durations in log messages

Conflicts:
	activerecord/CHANGELOG.md

This problem is fixed in commit #14030.

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