Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

ActiveRecord::Relation#to_sql does not match SQL executed when using includes #6132

Closed
mikelikesbikes opened this Issue · 32 comments
@mikelikesbikes
1.9.3p194 :009 > Todo.includes(:user).where("users.id" => 1)
  SQL (0.5ms)  SELECT "todos"."id" AS t0_r0, "todos"."name" AS t0_r1, "todos"."user_id" AS t0_r2, "todos"."created_at" AS t0_r3, "todos"."updated_at" AS t0_r4, "users"."id" AS t1_r0, "users"."name" AS t1_r1, "users"."created_at" AS t1_r2, "users"."updated_at" AS t1_r3 FROM "todos" LEFT OUTER JOIN "users" ON "users"."id" = "todos"."user_id" WHERE "users"."id" = 1
 => [#<Todo id: 2, name: "Wash the car", user_id: 1, created_at: "2012-05-02 22:15:55", updated_at: "2012-05-02 22:15:55">, #<Todo id: 3, name: "Get a spare key made", user_id: 1, created_at: "2012-05-02 22:16:05", updated_at: "2012-05-02 22:16:05">] 
1.9.3p194 :010 > Todo.includes(:user).where("users.id" => 1).to_sql
 => "SELECT \"todos\".* FROM \"todos\"  WHERE \"users\".\"id\" = 1" 

In looking at Issue #5990, @isaacsanders and I stumbled upon this issue.

We were attempting to make #pluck generate the same query that is used in the standard finder, but discovered that it doesn't use that query. We think this is the issue at the heart of #5990.

Thoughts?

cc @josevalim @tenderlove @steveklabnik

@isaacsanders

Yah, this one was weird.

@steveklabnik
Collaborator

... wow. That is odd.

@armstrjare

This is because eager loading is applied late at query execution and generates another Relation internally, referencing the join tables etc - which to_sql doesn't account for.

One option could be to alter to_sql to refer to a 'final' version of the relation to be used come query time, taking into account the logic from exec_queries (i.e. to use construct_relation_for_association_find in that case)

Flow diverges here:
https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/relation.rb#L171

Query with eager loaded association(s) generates internal Relation via here:
https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/relation/finder_methods.rb#L208-215

Relation#to_sql is doesn't currently account for this, as seen here:
https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/relation.rb#L456-458

EDIT:
Relation arel used in to_sql is built here, and doesn't consider eager loaded associations: https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/relation/query_methods.rb#L263-288

@isaacsanders

Do you know how to fix this?

@rafaelfranca

They guys, I fixed #5990 in master at a7e715e. Could you test if this issue still accours?

@rafaelfranca rafaelfranca was assigned
@frodsan

The issue is still present in master:

>> Todo.includes(:user).where("users.id" => 1)
  SQL (0.9ms)  SELECT "todos"."id" AS t0_r0, "todos"."name" AS t0_r1, "todos"."user_id" AS t0_r2, "todos"."created_at" AS t0_r3, "todos"."updated_at" AS t0_r4, "users"."id" AS t1_r0, "users"."name" AS t1_r1, "users"."created_at" AS t1_r2, "users"."updated_at" AS t1_r3 FROM "todos" LEFT OUTER JOIN "users" ON "users"."id" = "todos"."user_id" WHERE "users"."id" = 1
=> #<ActiveRecord::Relation []>
>> Todo.includes(:user).where("users.id" => 1).to_sql
=> "SELECT \"todos\".* FROM \"todos\"  WHERE \"users\".\"id\" = 1"
@rcrogers

This is a major pain. Does anyone know a way I can force ActiveRecord to do two separate queries instead of a join for queries like these?

@alexeymuranov

I've noticed this too. An inconvenience for debugging.

@cmer

:+1: Actually causing me problems in prod, not just for debugging.

@alexeymuranov

I do not know if this is related, but sometimes select accepts invalid or incomplete SQL. For example, the following works just fine:

User.select(%(SELECT "users".* FROM "users"  WHERE "dogs"."hot" = 't')).count

The table definition is the following:

ActiveRecord::Schema.define do
  create_table :users

  create_table :dogs do |t|
    t.integer :user_id
    t.boolean :hot
  end
end

and the models are associated accordingly.

@rafaelfranca

I nerver understand why someone use to_sql to execute queries in the application O_O.

@alexeymuranov

@rafaelfranca what do you mean? to_sql is to see a query, not to execute. About allowing to select by invalid SQL, maybe i am missing something, but i think this can become a problem if many people start relying on this behavior, and it will become hard to change.

@rafaelfranca

Yes, I know it is to see query, but if this behaviour is breaking application on production is because people is doing:

Model.connection.execute(Model.includes(:user).where(foo: 1).to_sql)
@alexeymuranov

Well, i am not convinced that it is ok for this line to break an application. Maybe i am still missing something :).

@rafaelfranca

Of course it is not ok to break the application, but there is not point to use this code.

However, this issue still need to be fixed for sure.

@cmer

Actually, that's not what I'm doing. I'm crazy but not that crazy.

We have implemented a clever way to generate cache keys for scopes in addition to the standard cache keys on single objects.

This way, we can do User.where(gender:'m').cache_key to see if something has changed in the database before fetching all the records. This is useful for russian doll caching and allows us to cache at a much higher level since we can now do cache(scope) in our views.

@oleander

@cmer I've been looking for a way to do something similar. Do you mind sharing your code related to ActiveRecord::Relation#cache_key? Thanks.

@cmer

@oleander I'll wrap it into a gem and keep you posted. I should have done this a long time ago anyways!

@cmer

@oleander Here it is: https://github.com/cmer/scope_cache_key

We've been using this code in prod for ~6 months and it works really well. I just made the gem 5 minutes ago so that part hasn't been tested properly, but it should just work. I literally just copied the code and specs over.

@fxn
Owner

Just for the record, it might interest you to have a look at the implementation of the explain method. It collects queries a posteriori, AR notifies them.

@gwcoffey

I may be missing something, but this isn't just a problem for debugging. It breaks queries like this:

Model.where(:id => {relation})

If {relation} has includes that generate outer joins, the above will fail, while this version will work:

Model.where(:id => {relation}.pluck(:model_id))

Since the where(:id => {relation}) syntax works in other cases, it ought to work here too. I'm working around it right now by plucking the IDs, but that is arguably less efficient.

@nathanl

@rafaelfranca - I agree that it would be silly to do

Model.connection.execute(Model.includes(:user).where(foo: 1).to_sql)

But that doesn't mean it's always silly to execute the generated SQL.

For example, Dossier is a reporting gem that uses SQL to generate reports. We specifically documented that you can hand-write your SQL or generate it with something like ActiveRecord.

https://github.com/adamhunter/dossier#basic-reports

This is intended to let you use powerful tools like AR's query generation when they suit you, or hand-write as insane a query as you want. It won't work, however, if AR misreports the SQL it would use.

@neerajdotname
Collaborator

I looked into it and I have a fix for it. However for that fix to work disable_implicit_join_references must be true. Since 5 tests which are testing for the deprecated message is failing.

I guess those deprecated items will be removed when Rails 4 is finally shipped.

@tfliss

In the meantime, I'm having some success with a monkey patch (based on @armstrjare 's comment):

module ActiveRecord
  module FinderMethods
    def monkey_patch_associations
      join_dependency = construct_join_dependency_for_association_find
      relation = construct_relation_for_association_find(join_dependency)
    end
  end
end

To be used like:

Model.connection.execute(Model.includes(:user).where(foo: 1).monkey_patch_associations().to_sql)

So far it seems to be displaying the same SQL as ActiveRecord::Base.logger for some pretty complex queries.

I'm finding .count() to be unreliable with .select(), so I needed this for a workaround.

Also a query of the form Model.select().include().limit().offset() has odd behavior when there are one-to-many relations involved. Model.connection.execute(Model.select().include().limit().offset().to_sql) returns a Result object that makes a lot more sense for paging if you're going to display a table anyway. I'd be glad to know if there's a better way to get a Result directly in ActiveRecord, but I'm not aware of one. So that's another situation where I (grudgingly) use to_sql and need the correct behavior.

@neerajdotname
Collaborator

@tfliss Can you check with master. This issue is fixed there.

@tfliss

@neerajdotname My application's on 3.2 (I should have pointed that out for the code snippet), but I was able to run some tests and see the difference when using master. to_sql does appear to print the same query as the logger, which is the issue at hand. So yes, this issue is fixed on master.

I'm still seeing interactions in master with eager loading and .select() that aren't appropriate for my use case (getting a PG::Result from a query built with ActiveRecord .select().include().limit().offset() - substitute .join() for .include() and things are predictable across varying data and parameters). That's all probably for another issue, though.

@joxxoxo

@rcrogers

This is a major pain. Does anyone know a way I can force ActiveRecord to do two separate queries instead of a join for queries like these?

In case anyone is interested take a look into

#will load associated in separate queries, 
#you may need to joins(...).preload(...) if using it in conditions
relation.preload(...)  

#will load associated in single ugly query
relation.eager_load(...) 

"includes" uses one of these according to applied conditions

@rafaelfranca

Fixed in master.

@shekibobo

This is still a problem in 4.0.4. Is it fixed in 4.0-stable?

@shekibobo

It appears that Arel actually uses #to_sql in certain cases:

I've created a gist that illustrates this happening.

Should I open a new issue for this?

@armstrjare

@shekibobo I think so - a new issue and maybe reference this one?

@Sinjo Sinjo referenced this issue in afair/postgresql_cursor
Merged

V0.5 rails 4.0 fixes #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.