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.

AssociationCollection.count Returns Incorrect Value

#2118

AssociationCollection.count can return an incorrect value if a finder_sql is a multiline string and there is a new line character immediately following "SELECT". This issue affects HasManyAssociations, HasAndBelongToManyAssociations, and HasManyThroughAssociations.

The root of the problem is that when construct_sql generates the counter_sql from the finder_sql it uses a regex that includes a space after the SELECT. This is the current regex:


/SELECT (\/\*.*?\*\/ )?(.*)\bFROM\b/im

The solution is to match any word break character after the SELECT


/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im

I added tests to show the problem to both the HasManyAssociationsTest and HasAndBelongsToManyAssociationsTest. I did not add a test to the HasManyThroughAssociationsTest because there were no existing tests for using finder_sql and counter_sql.

NOTES ON THE PATCH:

To show the issue in the HasManyAssociationsTest I needed to add an additional client fixture. This is because of the way that AssociationsCollection count uses connection.select_value to execute the counter_sql. select_value returns a single value from a record. If there are multiple values in the record, it returns the first element of the hash. In this case, that was the rating for companies(:another_client) and the rating happened to be 1, which was the expected count. I added another company that was a client of companies(:another_firm) so that the expected count would be 2. This required changing several tests to reflect that there now was another company and largely accounts for the size of my patch.

In addition to changing the regular expression to respect line breaks immediately following the SELECT statement in finder_sql, I also refactored the code that sets the counter_sql into a method, construct_counter_sql, on the base AssociationCollection. I did this because the code to generate the counter_sql was identical in HasManyCollection, HasAndBelongsToManyCollection, and HasManyThroughCollection. Modifying the regex in 3 separate places felt dirty.

Reported by Patrick Joyce · March 3rd, 2009 @ 03:33 PM

State: resolved
Milestone: 3.0.2
Assigned to: nobody
Importance: Medium

Activity

  1. Michael Koziarski
    Michael Koziarski

    Can you rebase this down into a single patch please?

    March 9th, 2009 @ 08:03 AM

  2. Patrick Joyce
    Patrick Joyce

    Rebased the patch down to a single patch.

    March 9th, 2009 @ 03:22 PM

  3. Repository
    Repository
    • State changed from new to resolved

    (from [4851ca9e13a4317342df02ae25b1929340523f7a]) Generate proper :counter_sql from :finder_sql when there is a newline character immediately following 'SELECT' [#2118 state:resolved]

    Signed-off-by: Pratik Naik pratiknaik@gmail.com
    http://github.com/rails/rails/commit/4851ca9e13a4317342df02ae25b192...

    June 21st, 2009 @ 10:10 PM

  4. Yehuda Katz (wycats)
    Yehuda Katz (wycats)
    • State changed from resolved to open

    This patch doesn't work on Postgres because the test you added (test_count_with_finder_sql) doesn't work on Postgres, even in the single line version. Can we make this work?

    June 22nd, 2009 @ 08:06 PM

  5. Yehuda Katz (wycats)
  6. Repository
    Repository
    • State changed from open to resolved

    (from [80f1f863cd0f9cba89079511282de5710a2e1832]) Revert "Generate proper :counter_sql from :finder_sql when there is a newline character immediately following 'SELECT' [#2118 state:resolved]"

    This reverts commit 4851ca9e13a4317342df02ae25b1929340523f7a.

    The tests do not pass for postgresql.
    http://github.com/rails/rails/commit/80f1f863cd0f9cba89079511282de5...

    June 22nd, 2009 @ 08:07 PM

  7. Yehuda Katz (wycats)
    Yehuda Katz (wycats)
    • State changed from resolved to open

    June 22nd, 2009 @ 11:29 PM

  8. Repository
    Repository
    • State changed from open to resolved

    (from [33dc4eb9ebe7d1a577aa7edf7215b0c14352205d]) Generate proper :counter_sql from :finder_sql when there is a newline character immediately following 'SELECT' [#2118 state:resolved]

    Signed-off-by: Pratik Naik pratiknaik@gmail.com
    http://github.com/rails/rails/commit/33dc4eb9ebe7d1a577aa7edf7215b0...

    June 23rd, 2009 @ 10:50 PM

  9. Repository
    Repository

    (from [89e4ccfae9fd2b845464336e0b6ab63ae9a18827]) Revert "Generate proper :counter_sql from :finder_sql when there is a newline character immediately following 'SELECT' [#2118 state:resolved]"

    This reverts commit 4851ca9e13a4317342df02ae25b1929340523f7a.

    The tests do not pass for postgresql.
    http://github.com/rails/rails/commit/89e4ccfae9fd2b845464336e0b6ab6...

    June 23rd, 2009 @ 10:50 PM

  10. Patrick Joyce
    Patrick Joyce

    i'll fix the patch to work with Postgres but it will probably have to wait until the weekend.

    June 24th, 2009 @ 02:30 PM

  11. Repository
    Repository

    (from [45e6f19925f23c3db257c15371d8f512cca843cd]) Revert "Revert "Generate proper :counter_sql from :finder_sql when there is a newline character immediately following 'SELECT' [#2118 state:resolved]""

    This reverts commit 80f1f863cd0f9cba89079511282de5710a2e1832.

    The feature doesn't work on Postgres, so don't test it on Postgres.
    Also, Postgres compatibility is irrelevant to the ticket/patch in question.
    http://github.com/rails/rails/commit/45e6f19925f23c3db257c15371d8f5...

    July 1st, 2009 @ 12:07 AM

  12. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2
    • Importance changed from to Medium

    October 15th, 2010 @ 11:01 PM