Guard against finder options or invalid-type column names in Relation#count

[rousisk]

This is what I came up with regarding #20434 . Keep in mind that the current implementation of count in 4.1 and 4.2 has two related but distinct issues:

1. When both a column name and an options hash are provided, the options are silently ignored.
2. When a Hash (with finder options or otherwise) is provided as the sole argument, it is silently ignored and a count(:all) is performed.

I introduced an explicit check for the first case, raising an ArgumentError whenever an options hash is specified. When the activerecord-deprecated_finders gem is present we can safely let that handle the finder options.

I handle the second issue by passing the argument to the DB directly and letting that to eventually raise an error. This is consistent with the approach taken in the current master when provided with an invalid type for a column name.

Providing both a column name and an options hash

These are the results of running User.count(:id, conditions: { username: 'rousis' }):

4-1-stable and 4-2-stable	4-1 and 4-2 with deprecated_finders	master	4-2-stable with thtis PR
SELECT COUNT("users"."id") FROM "users"	SELECT COUNT("users"."id") FROM "users" WHERE "users"."username" = $1 [["username", "rousis"]](plus deprecation warning)	ArgumentError: wrong number of arguments (2 for 0..1)	ArgumentError: Relation#count does not support finder options anymore. Please build a scope and then call count on it or use the activerecord-deprecated_finders gem to enable this functionality.

Providing a single hash argument

And these are the results of running User.count({ username: 'rousis' }):

4-1-stable and 4-2-stable	4-1 and 4-2 with deprecated_finders	master	4-2-stable with thtis PR
SELECT COUNT(*) FROM "users"	SELECT COUNT(*) FROM "users" WHERE "users"."username" = 'rousis' (plus deprecation warning)	ActiveRecord:: StatementInvalid	ActiveRecord:: StatementInvalid

This PR targets 4-2-stable but it might be a useful addition to 4-1-stable as well.

Any feedback on the concept or the implementation would be greatly appreciated.

[meinac]

I think we should just remove options argument from this method. Otherwise arity method will return wrong number of arguments.

[rousisk]

That would definitely simplify things @meinac and it would be closer to how this was finally dealt with on master.

But wouldn't it break existing code that relies on this feature with the help of the activerecord-deprecated_finders gem?

[meinac]

@rousisk IMHO in activerecord-deprecated_finders gem side we can create a count method to override original one and keep serving option to developers to use old behaviour. Handling this in rails side does not make sense to me.

[rousisk]

@meinac I totally agree with you - this would be a cleaner way.

This would require some kind of coordination in gem version dependencies, so that users don't find themselves with a newer rails version while still using the current activerecord-deprecated_finders, as this would break existing functionality.

Also this PR's code would never make it to master, which already took the better, cleaner, way. Not that this should be an excuse, of course, just another point in the general trade-off.

It would be nice if @matthewd, @rafaelfranca or @sgrif could weigh in.

[rousisk]

Hi @matthewd, @rafaelfranca, @sgrif. Is there any interest in this PR ?

[rousisk]

Hi @rafaelfranca, just rebased and pushed. Let me know if I can be of any help.

[kaspth]

Don't think we should add this code, as the deprecated finders won't be supported in Rails 5: https://github.com/rails/activerecord-deprecated_finders

[rousisk]

Hey @kaspth, this is targeted to 4.2 only, not master! Its purpose being to ease upgrades from 4.0 to more recent versions of 4.x. See also #20434 for background/context.

[kaspth]

Ah, right it is! So used to things being against master I didn't think twice about it. Sorry about that, and thanks for working on this 😁

[rousisk]

No problem, better safe than sorry so thanks for looking at it in the first place :)

[maclover7]

Please rebase.

[rousisk]

@maclover7 done!

[rousisk]

@maclover7 had to rebase again. Let me know if you need anything else.

[maclover7]

@rousisk I'm not a member of the Rails core team (only an Issue team member) so you may have to wait a little while to get a merge or more feedback...

[rafaelfranca]

Sorry for the delay. I think it is better to be handled in the gem that in Rails. We could just leave the code as it is here and as soon people upgrade the deprecated finders gem they will get the proper errors.

[rafaelfranca]

Ah, it still need to changes here. So yeah, we can change Rails to not accept the options argument. It is not different of the current implementation since it gives an argument error when the gem is not present.

[rousisk]

thanks for the feedback @rafaelfranca.

So you suggest changing the definition to def count(column_name = nil), as it's currently on master?

We won't have any more the descriptive error message for people that just upgraded, is that OK?