Merge :joins instead of clobbering them

The DISTINCT is for when you're joining a has_many or habtm association - e.g. a post joined with comments:

Post.find(:all, :joins => :comments, :conditions => 'comments.approved = 1')

Ticket #46 with_scope clobbers result attributes when :joins key is specified scopes the select to the posts table (i.e. 'posts.*') but if a post has more than one comment approved you'll get back duplicate post instances. Adding 'DISTINCT posts.*' eliminates these duplicate records.

There may by some circumstances where you don't want the DISTINCT in which case you can override it with :select, but the majority of cases would need it so that's why I've added it by default.

I agree with Andrew that DISTINCT would make sense in a lot of cases. However, using DISTINCT can easily become a performance problem if the query has LIMIT, ORDER BY etc. Apart from that, it'll also change the existing behavior, which can possibly break some stuff. So I'm not sure if it'll be a sensible default or not.

How about adding :distinct option to find ?

Interested in hearing more thoughts on this.

We could do a :distinct option and then set it true by default if we detect a has_many or habtm join being specified. This would only be when using the new style join syntax limiting any backward compatibility problems.

I'm not convinced about setting it to default.

If we don't set it as the default when the destination join has more than one matching row then we'll get duplicate instances of the parent. I can't think of a use case where this would correct or useful.

However looking at count it sets :distinct => true for :include and leaves it out when using :joins, so I guess the precedent is to make the developer specify it.

I've modified the patch to only merge :joins and not bother about making the select statement distinct. I'll create a separate ticket and patch for adding a distinct option to find.

One question regarding the distinct option for find - which of the following would be better:

Post.find :all, :distinct => true, :joins => :comments

or

Post.find :distinct, :joins => :comments

"Post.find :all, :distinct => true, :joins => :comments"

We can always do "Post.all, :distinct => true, :joins => :comments" once that's done.

The patch looks good. Could you rebase against master?

Updated patch rebased against master

It is used in with_scope - chaining multiple named scopes works fine for me. Do you have an example that doesn't work?

What happened to the patch for DISTINCT to work?