Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove column restrictions for `#count`, let the database raise if the SQL is invalid. #10710

Merged

Conversation

@senny
Copy link
Member

senny commented May 21, 2013

Fixes #5554

Previously the code path for #count used some validation logic and used to fall back on #count(:all). This prevented the execution of some specific count queries.

This patch gets rid of the validation logic and let's the database decide if the query is valid or not. One backwards incompatible result of this patch is the following:

relation = User.select("id, name")

# later in the code
relation.count

This count will now blow up because it you can't count on two columns. The solution is to specify the counting column explicitly:

relation.count(:all)
@senny
Copy link
Member Author

senny commented May 21, 2013

@jonleighton this is the patch I asked you about. What do you think?

/cc @rafaelfranca @carlosantoniodasilva

@senny
Copy link
Member Author

senny commented May 29, 2013

@neerajdotname maybe you could give me your thoughts on this one?

@neerajdotname
Copy link
Member

neerajdotname commented May 29, 2013

@senny PR looks good to me. +1.

@jonleighton
Copy link
Member

jonleighton commented Jun 7, 2013

@senny sorry for my slowness. I am happy to merge this, but it needs a rebase. Also could you expand the changelog message to note that the previous behaviour was untested and surprising, which is why we're making this change. Also would be good to note in changelog that people can do count(:all), as explained in your message above.

@senny
Copy link
Member Author

senny commented Jun 9, 2013

@jonleighton I rebased the PR and extended the CHANGELOG to include the pieces that you mentioned. Can you take another look?

jonleighton added a commit that referenced this pull request Jun 9, 2013
…ounts

Remove column restrictions for `#count`, let the database raise if the SQL is invalid.
@jonleighton jonleighton merged commit f5e133e into rails:master Jun 9, 2013
jonleighton added a commit that referenced this pull request Jun 9, 2013
…ounts

Remove column restrictions for `#count`, let the database raise if the SQL is invalid.
Conflicts:
	activerecord/CHANGELOG.md
jonleighton added a commit that referenced this pull request Jun 9, 2013
…ounts

Remove column restrictions for `#count`, let the database raise if the SQL is invalid.
Conflicts:
	activerecord/CHANGELOG.md
dhh added a commit that referenced this pull request Jun 11, 2013
…ise_on_counts".

This commit causes certain associations to no longer be able to be found through includes (polymorphic belongs_to).

This reverts commit 433e75f.
@prathamesh-sonpatki

This comment has been minimized.

Copy link
Member

prathamesh-sonpatki commented on da9b5d4 Jun 12, 2013

Will this be backported to 3.2-stable?

This comment has been minimized.

Copy link
Member Author

senny replied Jun 12, 2013

I don't think so, it changes the behavior and as such should not go back to older releases.

This comment has been minimized.

Copy link
Contributor

ernie replied Jul 14, 2013

This should (imho) be reverted before 4.0.1 is released. Calling join on select_values makes assumptions that the values in select_values are strings. This is not always the case, and will break if Arel nodes are used in the select.

If we must introduce the behavior, we should keep select_for_count in place so libraries have a place to hook in and prevent this sort of assumption from causing breakage. For more, see https://github.com/ernie/squeel/blob/master/lib/squeel/adapters/active_record/relation_extensions.rb#L76-L103.

This comment has been minimized.

Copy link
Member Author

senny replied Jul 14, 2013

@ernie I see your point but I don't think the old functionality was reasonable behavior either. Let me bring select_for_count back but keep the current behavior. Is that ok?

This comment has been minimized.

Copy link
Member Author

senny replied Jul 14, 2013

In the long term we should refactor the code to not make the same assumptions about select_values.

This comment has been minimized.

Copy link
Member Author

senny replied Jul 14, 2013

@ernie would that work for you: senny@f147092

This comment has been minimized.

Copy link
Contributor

ernie replied Jul 14, 2013

This comment has been minimized.

Copy link
Member Author

senny replied Jul 14, 2013

@ernie the method is back on master and 4-0-stable. Please ping me if you have the time to work on a PR to make the count code path not completely string focused.

rafaelfranca added a commit that referenced this pull request Oct 1, 2013
…ise_on_counts"

This reverts commit b8e2978.

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/relation/calculations.rb

Reason: This change is not backward compatible and it was reverted
before 4.0.0 at 2ad168e so we can't include in 4-0-stable.

Closes #12417
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented on db519c0 Nov 1, 2013

I think this commit message is wrong right?

This comment has been minimized.

Copy link
Member Author

senny replied Nov 2, 2013

Judging from the commit message it looks very wrong! The strange part is, that I remember issues with count where we didn't specify :all. This was due to a refactoring on how count determined to column when select and stuff was involved.

After investigating I found that this line was already completely replaced. See 4ba9c50

This comment has been minimized.

Copy link
Member

rafaelfranca replied Nov 4, 2013

I see. I was investigating the failing tests on master and this is the first commit that introduced the failure and the commit message was very strange 😄. count(:all) or exists? are not working with polymorphic includes. I'm continue the investigation

This comment has been minimized.

Copy link
Member Author

senny replied Nov 4, 2013

@rafaelfranca thanks and sorry for the confusion 😓

@devkrd
Copy link

devkrd commented Apr 27, 2018

Could you tell me where exactly to change the path to the file, the version of rails 5.0.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.