has_many :through broken for belongs_to on 2.0 preview

I figured out how to fix the issue and have attached a patch for the file activerecord/lib/active_record/associations/has_many_through_association.rb

This is not a defect. You are just using the associations incorrectly. has_many :through can only go through a has_many association, not a belongs_to. Please read the association docs or take this to the email list - Trac is not a support forum.

I would then like to reopen this ticket as an enhancement since I have already done the work. This should be a valid use for a has_many :through association. There is no reason that the only use has to be a replacements for a has_and_belongs_to_many with join table attributes.

On the surface it looks fine, but what's wrong with either of these approaches?

class App < ActiveRecord::Base
  belongs_to :client
  delegate :users, :to => :client
end

@app.client.users
@app.users # using delegation

More cases to associations add a lot more complexity, and I just don't think it's worth it in this case just to save a single primary-key indexed SQL query.

There's a lot more work that needs to be done to submit this as an enhancement patch. You need a full set of test cases for the new functionality. You also need to update the association documentation. And I suspect there will need to be other code changes to work with features like eager loading and using << to create the join model objects.

As Rick showed above, there are simple alternatives that get you most of what you want. I agree that adding complexity to the association code isn't worth the benefit here. But if you want to add tests and docs and complete the work for the patch, go ahead and it will be evaluated on its merits. You can reopen the ticket then.

I concur with Rick and Josh, but I do applaud you for taking the time to attempt to improve Rails. Just because this patch might not be deemed worth it in the end, it's still a good first step on your path to becoming a successful Rails contributor. So kudos to you for giving it a shot.

All right here I go for round 2.

The reason delegate will not work for my situation is that I need to extend the association. I also have a permissions system setup and I want to be able to do @app.users.with_access which would give me the users with access to the specific application.

Next you were right I did break eager loading and <<. I found in testing this that eager loading was already broken for a has_many :through that used a source that was an sti association using a non standard foreign_key. I can separate out a patch for that specific issue for quicker application if desired. I managed to get both eager loading and << working.

In running the tests I found out that all.sh is not running any of the tests in the associations folder. I am not sure if this was intentional but I have also included a fix for that in my patch. That I can also separate out into a another patch.

Lastly I updated all the related documentation that I could find.

Diff against trunk/activerecord revision 7892

There seems to be an inconsistency in the documentation:

+    #   class App < ActiveRecord::Base
+    #     belongs_to :client

+    #   @app = App.find :first
+    #   @app.clients.collect { |c| c.users }.flatten # select all the users for a given application

"@app.clients" does not make sense when App belongs_to :client.

You are correct. I have corrected the documentation and updated the patch to be against revision 7917.

Diff against trunk/activerecord revision 7917

What is the status on this enhancement?

I just ran into the same issue and using the delegate method has definite limitations, in particular where you have built an app around the whole associations idea. For example ActiveScaffold needs a whole bunch of workaround code because the delegate isn't a proper association that it can scan and figure out. As albus522 pointed out it also removes the ability to extend the association and add methods around it (eg security)

What are the blocking issues preventing this from being committed?

I've tried the patch and it seems to work. It solves the issue described in this ticket, is there any to reason to hold back the patch?