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

Already on GitHub? Sign in to your account

Chaining (merging) scopes in inherited models #9869

Closed
khustochka opened this Issue Mar 22, 2013 · 6 comments

Comments

Projects
None yet
4 participants
Contributor

khustochka commented Mar 22, 2013

I saw there are a lot of issues around chaning/merging scopes and conditions. My use case broke on Rails master (worked on 4.0.0.beta1 and 3.2.13).

The code:

class Observation < ActiveRecord::Base
  scope :identified, lambda { where("species_id <> 0") }
end

class MyObservation < Observation
  scope :mine_identified, lambda { where(mine: true).identified }
end

MyObservation.mine_identified.to_sql
#=> "SELECT \"observations\".* FROM \"observations\"  WHERE (species_id <> 0)"
# FAIL!!

I did pretty lots of testing different variants. What can I say: it does not matter whether it is string or hash condition. But the order matters:

class MyObservation < Observation
  scope :mine_identified, lambda { identified.where(mine: true)}
end

MyObservation.mine_identified.to_sql
#=> "SELECT \"observations\".* FROM \"observations\"  WHERE \"observations\".\"mine\" = 't' AND (species_id <> 0)"
# PASS!!

And the most interesting is that failure happens only in the child model, if I move the combined scope into parent model, it is ok:

class Observation < ActiveRecord::Base
  scope :identified, lambda { where("species_id <> 0") }
  scope :mine_identified, lambda { identified.where(mine: true)}
end

Observation.mine_identified.to_sql
#=> "SELECT \"observations\".* FROM \"observations\"  WHERE \"observations\".\"mine\" = 't' AND (species_id <> 0)"
# PASS!!

P.S. I will try to dig into AR tests to see if I can create a failing test case.

Member

claudiob commented Mar 23, 2013

This change in the behavior was caused by cd26b6a.

If you use any version of Rails after that commit, you get

> MyObservation.mine_identified
# SELECT "observations".* FROM "observations" WHERE (species_id <> 0)

If you use the commit before that one (de4a60c), then you get

> MyObservation.mine_identified
# SELECT "observations".* FROM "observations" WHERE "observations"."mine" = 't' AND (species_id <> 0)

So it might be worth referring to the original issue #7365 and to cc @neerajdotname

Member

neerajdotname commented Mar 23, 2013

@claudiob thanks for narrowing it down to that commit. I'll look into this issue.

Contributor

khustochka commented Mar 23, 2013

@claudiob thank you

@ghost ghost assigned neerajdotname Mar 29, 2013

Owner

dhh commented Mar 29, 2013

Fixed in d593fa4.

@dhh dhh closed this Mar 29, 2013

Member

neerajdotname commented Mar 29, 2013

@dhh the fix is not yet applied. Here is the PR. #9929

It is causing one test to fail. See PR for my comments regarding that failing test.

Owner

dhh commented Mar 29, 2013

Set the milestone for 4.0 on that one instead.

@neerajdotname neerajdotname pushed a commit that referenced this issue Apr 5, 2013

@jonleighton Neeraj Singh + jonleighton failing test for #9869 f029fb0

@jonleighton jonleighton added a commit that referenced this issue Apr 5, 2013

@jonleighton jonleighton Fix scope chaining + STI
See #9869 and #9929.

The problem arises from the following example:

    class Project < ActiveRecord::Base
      scope :completed, -> { where completed: true }
    end

    class MajorProject < Project
    end

When calling:

    MajorProject.where(tasks_count: 10).completed

This expands to:

    MajorProject.where(tasks_count: 10).scoping {
      MajorProject.completed
    }

However the lambda for the `completed` scope is defined on Project. This
means that when it is called, `self` is Project rather than
MajorProject. So it expands to:

    MajorProject.where(tasks_count: 10).scoping {
      Project.where(completed: true)
    }

Since the scoping was applied on MajorProject, and not Project, this
fails to apply the tasks_count condition.

The solution is to make scoping apply across STI classes. I am slightly
concerned about the possible side-effects of this, but no tests fail and
it seems ok. I guess we'll see.
8606a7f

@tenderlove tenderlove added a commit that referenced this issue Apr 5, 2013

@tenderlove tenderlove Merge branch 'master' into railstest
* master: (44 commits)
  Improve the changelog entry [ci skip]
  Fix explicit names on multiple file fields
  Correctly parse bigint defaults in PostgreSQL
  Move changelog to the top [ci skip]
  Fix indent and remove extra white spaces
  Fix scope chaining + STI
  failing test for #9869
  Improve `belongs_to touch: true` timestamp test
  Sort modules in alphabetical order.
  Avoid an attempt to fetch old record when id was not present in touch callback
  Use the correct pk field from the reflected class to find the old record
  Refactor mail_to to not generate intermediate hashes when adding href
  Ensure mail_to helper does not modify the given html options hash
  Use inspect when writing the foreign key from the reflection
  Use a space after the comment sign when showing the result of commands
  Exclude template files for rdoc API [ci skip]
  template should have generic name
  use | to have more intent revealing code
  Revert "Merge pull request #10034 from benofsky/fix_skipping_object_callback_filters"
  stop depending on callbacks
  ...

Conflicts:
	railties/test/application/rake_test.rb
01034d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment