Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

incorrect scope in associated collection methods

#6252

Just found a recent change may lead to incorrect semantic: https://github.com/rails/rails/commit/31d101879f1acae604d24d831a4b8...

It's hard to describe it so I attached a test case .. They fail in edge rails but will pass if you revert to a commit prior to 31d101879f1acae604d24d831a4b82a4482acf31.

For those who don't want to read the attachment:

Suppose you a model, Brand, acts as a tree:

class Brand < ActiveRecord::Base
  belongs_to :parent, :class_name => 'Brand'
  has_many :children, :class_name => 'Brand', :foreign_key => :parent_id

  before_validation :find_parent_and_do_something

  private

    def find_parent_and_do_something
      if self.parent_id
        Brand.find(parent_id) # and do sth. # break line
      end
    end
end

Then code like

b = Brand.create!
b.children.create! # break line

will raise a RecordNotFound excpetion on the line "break line", because Brand has incorrect scope (:parent_id => 123) in associate collection #create.

Reported by Jan Xie · January 5th, 2011 @ 06:46 AM

State: resolved
Milestone: none
Assigned to: Jon Leighton Jon Leighton
Importance: Low

Activity

  1. Jan Xie
  2. Jon Leighton
    Jon Leighton
    • State changed from new to open
    • Assigned user set to Jon Leighton
    • Importance changed from to Low

    Hi,

    Thanks for the report. This is on my list to look at.

    Jon

    January 7th, 2011 @ 08:05 PM

  3. hemant
    hemant

    Hi Jon,

    I could replicate the issue using the tests provided by Jan Xie.

    In association_collection.rb I've removed scoped.scoping around build_record(attrs, &block) as
    the scopes are already being injected in following method populate_with_current_scope_attributes present in base.rb

      def populate_with_current_scope_attributes
        if scope = self.class.send(:current_scoped_methods)
          create_with = scope.scope_for_create
          create_with.each { |att,value|
            respond_to?("#{att}=") && send("#{att}=", value)
          }
        end
      end
    

    This change fix all the tests though but I might be missing/overlooking something crucial here by removing scoping.
    Please comment.

    Thanks,
    Hemant

    January 19th, 2011 @ 10:08 PM

  4. Jon Leighton
    Jon Leighton

    Hi hemant,

    Thanks for the comment. I haven't had a chance to look into this properly yet but I haven't forgotten and I promise I will get to it :)

    Cheers,
    Jon

    January 19th, 2011 @ 10:12 PM

  5. Repository
    Repository
    • State changed from open to resolved

    (from [63c73dd0214188dc91442db538e141e30ec3b1b9]) We shouldn't be using scoped.scoping { ... } to build associated records, as this can affect validations/callbacks/etc inside the record itself [#6252 state:resolved] https://github.com/rails/rails/commit/63c73dd0214188dc91442db538e14...

    January 30th, 2011 @ 08:32 PM