Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

[PATCH] Refactoring AssociationProxy and subclasses to avoid @finder_sql, @counter_sql, etc

#5838

This is another refactoring patch extracted from my work on nested through associations (see #1152).

Currently there is a rather confusing mix of variables such as @finder_sql, @counter_sql, etc. They are used in different ways at different times, and due to the use of inheritance it's quite hard to follow what is getting set or used where.

The patch implements a level of consistency across all AssociationProxy subclasses:

  • AssociationProxy calls construct_scope on initialization, which calls construct_find_scope and construct_create_scope and assigns the results in a @scope variable
  • construct_find_scope and construct_create_scope are then implemented by subclasses
  • The @scope variable is used by the subclasses when performing queries
  • In AssociationCollection, where :finder_sql and :counter_sql are used, they are dealt with by the new methods custom_finder_sql and custom_counter_sql (previously they would be assigned to @finder_sql or whatever, but depending on the situation @finder_sql would either be passed to the :conditions of the scope, or passed directly to find_by_sql)

Hope this makes sense, let me know if further explanation is needed.

(You can see the commit in the nested through associations fork here: http://github.com/bjeanes/rails/commit/78b8c51cb3b0c629152f3bbaf6d8...)

Reported by Jon Leighton · October 19th, 2010 @ 11:12 AM

State: committed
Milestone: 3.1
Assigned to: Aaron Patterson Aaron Patterson
Importance: Low

Activity

  1. Jon Leighton
    Jon Leighton
    • Tag changed from active_record, associations, refactoring to active_record, associations, patch, refactoring

    October 19th, 2010 @ 02:48 PM

  2. Aaron Patterson
    Aaron Patterson
    • State changed from new to incomplete
    • Milestone set to 3.1
    • Importance changed from to Low

    I tried out this patch, but the AR tests fail miserably. Can you make sure everything works, then submit the patch again. Thanks.

    October 21st, 2010 @ 12:47 AM

  3. Jon Leighton
    Jon Leighton

    Hi Aaron,

    Damn! Thanks for taking the time to look at this and really sorry that I messed it up. It looks like a teeny bit of my nested associations patch slipped through the net here. I don't know how that happened. Sorry.

    It was a one line fix which I've applied - updated patch attached. I've run all the tests and they work.

    Cheers,
    Jon

    October 21st, 2010 @ 05:54 PM

  4. Jon Leighton
    Jon Leighton

    Hiya,

    Is it possible to re-mark this as "open" as I have fixed the patch? I've re-applied and tested it today, it still applies cleanly and works. Just don't want it to get buried and forgotten, especially as I'm thinking that this needs to get in before the overall nested through associations patch can be looked at easily.

    Thanks very much,
    Jon

    October 28th, 2010 @ 03:00 PM

  5. Aaron Patterson
    Aaron Patterson
    • State changed from incomplete to open

    Hey! I've marked it open again, and I'll take a look. Thanks!

    October 28th, 2010 @ 11:03 PM

  6. Aaron Patterson
    Aaron Patterson
    • State changed from open to committed

    Sorry I took so long, but it's merged in now. Thanks!

    October 30th, 2010 @ 02:37 PM

  7. Jon Leighton