This project is archived and is in readonly mode.

#2329 ✓resolved
James Le Cuirot

Default to :all if :select contains comma when counting

Reported by James Le Cuirot | March 24th, 2009 @ 11:01 PM | in 3.0.2

I just upgraded my application to 2.3 and pagination started generating invalid SQL. Admittedly 2.2 hadn't been calculating the pages properly before either but I hadn't noticed because this was the first time it had caused an error.

It fails because the scope I'm applying defines :select with a calculated column. In fact, if you just specify more than one column of any kind, it fails because it tries to do a query like this.

SELECT COUNT(foo, bar) FROM baz

I can't simply remove the calculated column when counting in my application code because the process of fetching the records and counting the number of pages is combined in plugins such as will_paginate.

This patch adds a single line that changes column_name to :all if it contains a comma. This shouldn't break anything that isn't broken already. A test is included.

Comments and changes to this ticket

  • CancelProfileIsBroken

    CancelProfileIsBroken August 5th, 2009 @ 03:47 PM

    • Tag changed from “2.3, 2.3.x, activerecord, active_record, calculations, patch, select, test” to “2.3, 2.3.x, activerecord, active_record, bugmash, calculations, patch, select, test”
  • Rajesh

    Rajesh August 8th, 2009 @ 10:10 PM

    Could not reproduce on the 2-3-stable. The given test case is passing without any changes to the code.

  • Rajesh

    Rajesh August 8th, 2009 @ 10:15 PM

    -1 Please read my comments above

  • James Le Cuirot

    James Le Cuirot August 8th, 2009 @ 10:39 PM

    There's a reason for that. The change that allowed scoped :select to be passed through to count created problems with has_many :through associations and was recently reverted as noted in ticket #2189. I was a little disappointed by this decision as I was relying on this feature. I haven't seen the discussion on rails-core and I would have to see that before commenting further. Time for bed now though. ;) Sorry for not commenting on this earlier but I've been away and just got back.

  • dira

    dira August 9th, 2009 @ 11:41 AM

    +1

  • dira

    dira August 9th, 2009 @ 02:27 PM

    +1 - I used paging for queries with calculated columns in a project.

  • Rizwan Reza

    Rizwan Reza August 9th, 2009 @ 03:25 PM

    verified

    The patch works fine under master but doesn't apply to 2-3-stable. All tests pass for master as well. Good work!

  • Elad Meidar

    Elad Meidar August 9th, 2009 @ 04:32 PM

    +1 on idea, -1 on patch, does not apply "Patch does not have a valid e-mail address."

  • James Le Cuirot

    James Le Cuirot August 9th, 2009 @ 04:48 PM

    It won't fix anything on the latest 2-3-stable anyway as I have already said because the feature was reverted. I have now read the rails-core discussion and I don't think the feature was reverted in master, hence why it still works there.

    The issue is related to ticket #2189 but this patch alone won't fix that and so a more general fix needs to be considered. Please read around these issues before simply stating "it doesn't apply" or "it works".

  • Rizwan Reza

    Rizwan Reza January 20th, 2010 @ 11:08 AM

    • State changed from “new” to “resolved”
    • Tag changed from “2.3, 2.3.x, activerecord, active_record, bugmash, calculations, patch, select, test” to “2.3, 2.3.x, activerecord, active_record, calculations, patch, select, test”
    • Milestone cleared.
  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to “3.0.2”
    • Importance changed from “” to “”

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