relation.merge removes conditions in 1.0.6 #146

Closed
the8472 opened this Issue Jul 2, 2012 · 6 comments

Comments

Projects
None yet
3 participants

the8472 commented Jul 2, 2012

This seems to be a pretty serious issue, not sure why i haven't run into this earlier:

1.0.1:

JobApplication.where(:state => "interview").where(:state => "received").merge(JobApplication.where(:state => nil)).debug_sql
SELECT "applications".* FROM "applications"  WHERE "applications"."state" = 'interview' AND "applications"."state" = 'received' AND "applications"."state" IS NULL

1.0.6

SELECT "applications".* FROM "applications"  WHERE "applications"."state" IS NULL

the8472 commented Jul 2, 2012

Ok, this seems to be intentional as per spec.

it 'uses the given equality condition in the case of a conflicting where from a default scope in AR >= 3.1' do
            relation = PersonNamedBill.where{name == 'Ernie'}
            sql = relation.to_sql
            sql.should_not match /Bill/
            sql.should match /Ernie/
          end unless ::ActiveRecord::VERSION::MINOR == 0

But when combined with scopes then it leads to the issue described in rails/rails#3052 because scopes use relation.merge

def scope(name, scope_options = {})
   ...
   relation = scoped.merge(options)
   ...
end

This seems... counterintuitive to me since wheres chained to each other behave differently to wheres inside scopes chained to other wheres.

I'm seeing this with combinations of equality and in. For example:

fund_request = FundRequest.create
#<FundRequest id: 1>
allocation = fund_request.allocations.create
#<Allocation id: 1>
fund_request.allocations.where{fund_request_id.in( FundRequest.completed.select{id} )}

I expect:

SELECT * FROM allocations WHERE fund_request_id = 1 AND fund_request_id IN (SELECT id FROM fund_requests WHERE completed  = 1)

Instead, I get the following rather different result:

SELECT * FROM allocations WHERE fund_request_id IN (SELECT id FROM fund_requests WHERE completed  = 1)

With subquery conditions this is a particularly harrowing bug because depending on the subquery outcome, this may or may not depend on the first part. All my specs pass, but I can't trust the code.

This does not seem to happen without squeel so I think there's a squeel-specific issue. For example, the following non-squeel code works as expected

class Owner < ActiveRecord::Base
  attr_accessible :completed

  has_many :properties
end
class Property < ActiveRecord::Base
  belongs_to :owner
  # attr_accessible :title, :body
end
o=Owner.create(completed: true)
o.properties.where(owner_id: [4,3]).to_sql
SELECT "properties".* FROM "properties"  WHERE "properties"."owner_id" = 1 AND "properties"."owner_id" IN (4, 3)

the8472 commented Jul 10, 2012

What really bugs me about this are the security implications. If scopes don't always guarantee narrowing, depend on the order they are applied and you're enforcing authorization through scopes (e.g. by using the cancan gem) then this could under some conditions grant a user access to more than he should have.

Owner

ernie commented Jul 11, 2012

@aepstein I'd be happy to take a look at this. There's a fair amount of discussion here so if you could please just verify that I have it right:

If I have a has_many relationship, and I do a query from the association's scope which adds an "in" condition against the belongs_to key, we lose the belongs_to equality condition and are left with only the in. Correct?

Owner

ernie commented Jul 11, 2012

Following up: I think there's something else going on in your code that isn't showing up here, @aepstein. The following spec passes for me:

          it 'maintains equality condition from an association scope if an in is added via #where' do
            relation = Person.first.articles.where{person_id.in [1,2,3]}
            sql = relation.to_sql
            relation.all.should eq Person.find(1).articles
            sql.should match /"person_id" = 1/
            sql.should match /"person_id" IN \(1, 2, 3\)/
          end

I also tried via a subquery with success.

@the8472 is correct that it's intentional that equalities, however, overwrite. Closing this as I'm unable to duplicate the equality/in condition. Will reopen if new information shows up.

ernie closed this Jul 11, 2012

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