This project is archived and is in readonly mode.

#5838 ✓committed
Jon Leighton

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

Reported by Jon Leighton | October 19th, 2010 @ 11:12 AM | in 3.1

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...)

Comments and changes to this ticket

  • Jon Leighton

    Jon Leighton October 19th, 2010 @ 02:48 PM

    • Tag changed from “active_record, associations, refactoring” to “active_record, associations, patch, refactoring”
  • Aaron Patterson

    Aaron Patterson October 21st, 2010 @ 12:47 AM

    • 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.

  • Jon Leighton

    Jon Leighton October 21st, 2010 @ 05:54 PM

    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

  • Jon Leighton

    Jon Leighton October 28th, 2010 @ 03:00 PM

    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

  • Aaron Patterson

    Aaron Patterson October 28th, 2010 @ 11:03 PM

    • State changed from “incomplete” to “open”

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

  • Aaron Patterson

    Aaron Patterson October 30th, 2010 @ 02:37 PM

    • State changed from “open” to “committed”

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

  • Jon Leighton

    Jon Leighton October 30th, 2010 @ 05:51 PM

    Lovely, thanks.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages