Add new Rails/ArelStar cop

[flanger001]

This is a new cop that checks for usage of arel_table["*"] and MyModel["*"] calls. arel_table quotes anything a user enters in the brackets and this can lead to some unexpected consequences:

class MyModel < ApplicationRecord
  scope :my_arel_scope, -> {
    select(arel_table["*"])
  }
end

puts MyModel.my_arel_scope.to_sql

# (Assuming MySQL)
#=> SELECT `my_models`.`*` FROM `my_models` 

This is correct-looking but actually incorrect SQL because a quoted asterisk is treated as a literal column name by MySQL and Postgres. I have not tried this in SQLite but I imagine it does the same thing.

Thanks for taking a look!

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• If this is a new cop, consider making a corresponding update to the Rails Style Guide.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[andyw8]

Might this be considered as a bug in Arel?

[flanger001]

@andyw8 I think you could make the argument either way honestly. Arel should probably not quote asterisks, but the core team will also likely say that Arel should not be used publicly. What do you think?

[dvandersluis]

Since * is a valid column identifier on both MySQL and Postgres (regardless of whether it should be), it's probably not a bug, Arel or not. However, behaviour did change in MySQL 8.0.21, and anyone upgrading to that will suddenly have their queries break (we experienced this at work).

[flanger001]

Actually, upon further consideration, I am not so sure this could be considered a bug. Having an asterisk as a column name is an extraordinarily bad idea, but there is nothing technically incorrect about it. These are both valid DDL for PostgreSQL and MySQL respectively:

CREATE TABLE foos ("*" varchar(255));
CREATE TABLE foos (`*` varchar(255));

So the arel_table["*"] usage would be correct in these cases, however abhorrent they might be.

[flanger001]

However, behaviour did change in MySQL 8.0.21, and anyone upgrading to that will suddenly have their queries break (we experienced this at work).

This is what happened to me as well, and indeed it was the impetus for this cop.

[andyw8]

I think there's also a policy decision to be made here. Since Arel is still considered a private internal API (recent discussion) should it be in scope for rubocop-rails?

[dvandersluis]

I was involved with that discussion and despite what the rails team has said, there are quite a few developers using Arel. No need to relitigate whether or not Arel should be public here, but since developers are using it and this is a real thing that can be encountered, I think the cop makes sense here.

[flanger001]

Hey, I'm all over that discussion!

Again, I think you could make the case either way. I personally think it should be in scope. arel_table in particular is used very frequently, and it not being a public API does not have any effect on the very real usage of it in the wild.

[flanger001]

Also it appears @dvandersluis and I are hitting the same exact beats here, but I would imagine this is the majority sentiment.

[flanger001]

@andyw8 Any further thoughts on this?

[andyw8]

I'm still on the fence, but if another maintainer gives it a 👍 then let's go ahead.

[flanger001]

Cool, thank you.

[flanger001]

I'm happy to close this if there's no interest but any chance I can get another set of eyes on this?

[koic]

Can you rebase with the latest master branch and move this entry to New features section?

[koic]

Can you write a reason to description of the cop why it is unsafe?

[flanger001]

Sure. I will add this description in here:

This cop is unsafe because it checks for ["*"].

hsh = { "*" => true }
hsh["*"] # This would get corrected to `hsh[Arel.star]`

[koic]

I get it, thank you for the address!

[flanger001]

Based on @eugeneius's review here #344 (review) I removed this description. I think it is still a good idea to call this an unsafe cop, but I'm open to feedback here.

[koic]

Thank you for the update. If my understanding is correct, there seems to be concern about incompatible behavior rather than false positives. If so, SafeAutoCorrect: false would be more eligible than Safe: false.

-Safe: false
+SafeAutoCorrect: false

https://docs.rubocop.org/rubocop/0.92/usage/auto_correct.html#safe-auto-correct

[flanger001]

@koic sounds good to me, and thanks for the clarification. I've pushed this up!

[koic]

Please remove this due to when Safe: false then SafeAutocorrect is also false.

[koic]

Is there any reason to set this?

[flanger001]

I set this to not autocorrect by default because it is unsafe.

[koic]

IMO, I think this setting is unnecessary due to Safe: false is not auto-corrected by default rubocop -a safe option. (In that case, users will explicitly use rubocop -A unsefe option.)

[eugeneius]

{_ :arel_table} means "anything or :arel_table", which will always match.

Similarly, the entire {const {_ :arel_table}} expression matches any node, and is equivalent to _.

I guess perhaps you meant to write:

Suggested change

	(send {const {_ :arel_table}} :[] $(str "*"))
	(send {const (send _ :arel_table)} :[] $(str "*"))

...which would resolve the false positive case mentioned in #344 (comment).

[flanger001]

@eugeneius This is a good change, thank you!

[koic]

Thank you!