first_or_create is applying erroneous scope to model callbacks #7853

Closed
uberllama opened this Issue Oct 5, 2012 · 12 comments

Comments

Projects
None yet
4 participants
@uberllama
Contributor

uberllama commented Oct 5, 2012

The following code does not result in a raise to the controller.

Wish.rb

  before_create :verify_wish_limit

  private

  def verify_wish_limit
    raise OverLimitException # if blah blah
  end

wishes_controller.rb

  def create
    @wish = @user.wishes.where(:product_id => @product.id).first_or_create
  rescue Wish::OverLimitException
    # handle error
  end

Changing the creation line to the following does:

@wish = @user.wishes.find_or_create_by_product_id(@product.id)
@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Oct 6, 2012

Member

I'll investigate.

Member

senny commented Oct 6, 2012

I'll investigate.

senny added a commit to senny/rails that referenced this issue Oct 6, 2012

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Oct 6, 2012

Member

I tried to reproduce the error in the test-case mentioned above but as I see it, everything works as expected:

class Topic
  before_create  :default_written_on

  def default_written_on
    self.written_on = Time.now unless attribute_present?("written_on")
  end
end
  def test_first_or_create_with_callbacks
    # make sure, that the attribute is not set without the callback beeing run
    assert_equal nil, Topic.new.written_on
    topic = Topic.where(:title => 'new topic').first_or_create
    assert_not_equal nil, topic.written_on
  end

@uberllama If my test-case represents your situation I need some more inputs how to proceed. could it be related to your if condition? Did you make sure that the callback does not run or did you just check if the exception is not raised? What rails version did you use?

Member

senny commented Oct 6, 2012

I tried to reproduce the error in the test-case mentioned above but as I see it, everything works as expected:

class Topic
  before_create  :default_written_on

  def default_written_on
    self.written_on = Time.now unless attribute_present?("written_on")
  end
end
  def test_first_or_create_with_callbacks
    # make sure, that the attribute is not set without the callback beeing run
    assert_equal nil, Topic.new.written_on
    topic = Topic.where(:title => 'new topic').first_or_create
    assert_not_equal nil, topic.written_on
  end

@uberllama If my test-case represents your situation I need some more inputs how to proceed. could it be related to your if condition? Did you make sure that the callback does not run or did you just check if the exception is not raised? What rails version did you use?

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Oct 6, 2012

Member

@steveklabnik could you tag this with needs feedback?

Member

senny commented Oct 6, 2012

@steveklabnik could you tag this with needs feedback?

@uberllama

This comment has been minimized.

Show comment Hide comment
@uberllama

uberllama Oct 6, 2012

Contributor

Senny, the problem specifically occurs with exceptions. Changing the controller code from a first_or_create to a find_or_create_by results in the exception being caught and handled.

Contributor

uberllama commented Oct 6, 2012

Senny, the problem specifically occurs with exceptions. Changing the controller code from a first_or_create to a find_or_create_by results in the exception being caught and handled.

@uberllama

This comment has been minimized.

Show comment Hide comment
@uberllama

uberllama Oct 6, 2012

Contributor

Sorry, Rails 3.2.8.

In further model testing I found that the wish is actually being created with the first_or_create call, despite an exception being raised in before_create.

# creates record when it shouldn't
Wish.where(:product_id => 44444).first_or_create

# raises exception and does not create record
Wish.find_or_create_by_product_id(44444)
Contributor

uberllama commented Oct 6, 2012

Sorry, Rails 3.2.8.

In further model testing I found that the wish is actually being created with the first_or_create call, despite an exception being raised in before_create.

# creates record when it shouldn't
Wish.where(:product_id => 44444).first_or_create

# raises exception and does not create record
Wish.find_or_create_by_product_id(44444)
@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Oct 6, 2012

Member

I don't see why it should behave differently when exceptions are involved. I modified my test case to raise an exception in the before_create callback and I could catch that exception in the test case. Also the record was not created.

@uberllama can you test it against master? Feel free to fork my copy of rails and modify the test-case I created to reveal the bug. Also does your before_create callback run or not when you use first_or_create?

Member

senny commented Oct 6, 2012

I don't see why it should behave differently when exceptions are involved. I modified my test case to raise an exception in the before_create callback and I could catch that exception in the test case. Also the record was not created.

@uberllama can you test it against master? Feel free to fork my copy of rails and modify the test-case I created to reveal the bug. Also does your before_create callback run or not when you use first_or_create?

@uberllama

This comment has been minimized.

Show comment Hide comment
@uberllama

uberllama Oct 6, 2012

Contributor

This is definitely a confounding issue. I'm looking deeper into my actual conditional that throws the exception and will report back.

Contributor

uberllama commented Oct 6, 2012

This is definitely a confounding issue. I'm looking deeper into my actual conditional that throws the exception and will report back.

@uberllama

This comment has been minimized.

Show comment Hide comment
@uberllama

uberllama Oct 6, 2012

Contributor

I've looked at the log, and it seems like there's a deeper scoping issue at play. Let me try to explain, with clearer code.

Wish.rb

belongs_to :user
before_create :verify_wish_limit
 def verify_wish_limit
   raise OverLimitException unless user.wishes.count < 5
 end

Now, assuming the user has reached their limit in this case, look at these logs:

user.wishes.where(:product_id => 12345).first_or_create:

SELECT "wishes".* FROM "wishes" WHERE "wishes"."user_id" = 1 AND "wishes"."product_id" = 12345 LIMIT 1
SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1
SELECT COUNT(*) FROM "wishes" WHERE "wishes"."product_id" = 12345 AND "wishes"."user_id" = 5
INSERT INTO "wishes"...

Note the last select. Its erroneously applying the original where condition to the user.wishes.count call in Wish#verify_wish_limit

user.wishes.find_or_create_by_product_id(12345):

SELECT "wishes".* FROM "wishes" WHERE "wishes"."user_id" = 1 AND "wishes"."product_id" = 12345 LIMIT 1
SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1
SELECT COUNT(*) FROM "wishes" WHERE "wishes"."user_id" = 1

This is correct behaviour.

Contributor

uberllama commented Oct 6, 2012

I've looked at the log, and it seems like there's a deeper scoping issue at play. Let me try to explain, with clearer code.

Wish.rb

belongs_to :user
before_create :verify_wish_limit
 def verify_wish_limit
   raise OverLimitException unless user.wishes.count < 5
 end

Now, assuming the user has reached their limit in this case, look at these logs:

user.wishes.where(:product_id => 12345).first_or_create:

SELECT "wishes".* FROM "wishes" WHERE "wishes"."user_id" = 1 AND "wishes"."product_id" = 12345 LIMIT 1
SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1
SELECT COUNT(*) FROM "wishes" WHERE "wishes"."product_id" = 12345 AND "wishes"."user_id" = 5
INSERT INTO "wishes"...

Note the last select. Its erroneously applying the original where condition to the user.wishes.count call in Wish#verify_wish_limit

user.wishes.find_or_create_by_product_id(12345):

SELECT "wishes".* FROM "wishes" WHERE "wishes"."user_id" = 1 AND "wishes"."product_id" = 12345 LIMIT 1
SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1
SELECT COUNT(*) FROM "wishes" WHERE "wishes"."user_id" = 1

This is correct behaviour.

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Oct 6, 2012

Member

@uberllama can you adjust the issue title to reflect the actual issue?

Member

senny commented Oct 6, 2012

@uberllama can you adjust the issue title to reflect the actual issue?

@uberllama

This comment has been minimized.

Show comment Hide comment
@uberllama

uberllama Oct 6, 2012

Contributor

Done!

Contributor

uberllama commented Oct 6, 2012

Done!

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 7, 2012

Owner

I'm not sure but when you call user.wishes.where(:product_id => 12345) it is mutating the wishes relation in the scope of the create. This is why it is using the same where clause in the count.

@jonleighton could you take a look on this one?

Owner

rafaelfranca commented Oct 7, 2012

I'm not sure but when you call user.wishes.where(:product_id => 12345) it is mutating the wishes relation in the scope of the create. This is why it is using the same where clause in the count.

@jonleighton could you take a look on this one?

@ghost ghost assigned jonleighton Oct 7, 2012

@jonleighton

This comment has been minimized.

Show comment Hide comment
@jonleighton

jonleighton Oct 19, 2012

Member

This:

@user.wishes.where(:product_id => @product.id).first_or_create

Expands to:

scope = @user.wishes.where(:product_id => @product.id)
scope.first || scope.create

Which expands to:

scope = @user.wishes.where(:product_id => @product.id)
scope.first || scope.scoping { Wish.create }

Therefore, you can easily see that the create callbacks will run within the scope.

I think the only solution is to add a find_or_create_by method to Relation:

@user.wishes.find_or_create_by(:product_id => @product.id)

This actually reads better to me than first_or_create, tbh. And it's consistent with the new Relation#find_by method.

So I'm gonna do that.

Member

jonleighton commented Oct 19, 2012

This:

@user.wishes.where(:product_id => @product.id).first_or_create

Expands to:

scope = @user.wishes.where(:product_id => @product.id)
scope.first || scope.create

Which expands to:

scope = @user.wishes.where(:product_id => @product.id)
scope.first || scope.scoping { Wish.create }

Therefore, you can easily see that the create callbacks will run within the scope.

I think the only solution is to add a find_or_create_by method to Relation:

@user.wishes.find_or_create_by(:product_id => @product.id)

This actually reads better to me than first_or_create, tbh. And it's consistent with the new Relation#find_by method.

So I'm gonna do that.

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