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

Mark Rails/UniqBeforePluck cops as unsafe #320

Merged
merged 1 commit into from
Aug 7, 2020

Conversation

kunitoo
Copy link
Contributor

@kunitoo kunitoo commented Aug 6, 2020

Rails/UniqBeforePluck is unsafe depending on the database collation.
Here is an example of an unsafe case for the three databases that Rails supports(MySQL, SQLite, PostgreSQL).

In the case of MySQL

The default collation is utf8mb4_general_ci(in Rails), so it is unsafe.

An example is as follows.

  • Create table
class CreateUsers < ActiveRecord::Migration[6.0]
  def change
    create_table :users do |t|
      t.string :name
    end
  end
end
  • Create data
User.create!(name: 'A')
User.create!(name: 'a')

before autocorrect

User.pluck(:name).uniq #=> ["A", "a"]

after autocorrect

User.distinct.pluck(:name) #=> ["A"]

In the case of SQLite

The default collation is BINARY, so it is safe.
If you change the collation to NOCASE, the collation is unsafe.

The following is an example.

  • Create table
class CreateUsers < ActiveRecord::Migration[6.0]
  def change
    create_table :users do |t|
      t.string :name, collation: 'NOCASE'
    end
  end
end

Other than the above, it is the same as MySQL.

In the case of PostgreSQL

The default collation is safe.
If you create a custom collation and specify it, the collation will be unsafe.

A concrete example is as follows.

  • Create custom collation
CREATE COLLATION case_insensitive (
  provider = icu,
  locale = 'und-u-ks-level2',
  deterministic = false
);
  • Create table
class CreateUsers < ActiveRecord::Migration[6.0]
  def change
    create_table :users do |t|
      t.string :name, collation: 'case_insensitive'
    end
  end
end

Other than the above, it is the same as MySQL.

Replace this text with a summary of the changes in your PR.
The more detailed you are, the better.


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.

`Rails/UniqBeforePluck` is unsafe depending on the database collation.
Here is an example of an unsafe case for the three databases that Rails supports(MySQL, SQLite, PostgreSQL).

### In the case of MySQL

The default collation is `utf8mb4_general_ci`(in Rails), so it is unsafe.

An example is as follows.
- Create table
```
class CreateUsers < ActiveRecord::Migration[6.0]
  def change
    create_table :users do |t|
      t.string :name
    end
  end
end
```

- Create data
```
User.create!(name: 'A')
User.create!(name: 'a')
```

before autocorrect
```
User.pluck(:name).uniq #=> ["A", "a"]
```

after autocorrect
```
User.distinct.pluck(:name) #=> ["A"]
```

### In the case of SQLite

The default collation is `BINARY`, so it is safe.
If you change the collation to `NOCASE`, the collation is unsafe.

The following is an example.

- Create table
```
class CreateUsers < ActiveRecord::Migration[6.0]
  def change
    create_table :users do |t|
      t.string :name, collation: 'NOCASE'
    end
  end
end
```

Other than the above, it is the same as MySQL.

### In the case of PostgreSQL

The default collation is safe.
If you create a custom collation and specify it, the collation will be unsafe.

A concrete example is as follows.

- Create custom collation
```
CREATE COLLATION case_insensitive (
  provider = icu,
  locale = 'und-u-ks-level2',
  deterministic = false
);
```

- Create table
```
class CreateUsers < ActiveRecord::Migration[6.0]
  def change
    create_table :users do |t|
      t.string :name, collation: 'case_insensitive'
    end
  end
end
```

Other than the above, it is the same as MySQL.
@koic koic merged commit dc888a3 into rubocop:master Aug 7, 2020
@koic
Copy link
Member

koic commented Aug 7, 2020

Good catch! Thank you for the great investigation and explanation!

@kunitoo kunitoo deleted the uniq_before_pluck_to_unsafe branch August 7, 2020 15:20
koic added a commit to koic/rubocop-rails that referenced this pull request Aug 17, 2020
Follow rubocop/rubocop#8529 and rubocop#320.

This PR changes the mark of `Rails/UniqBeforePluck` from `Safe: false`
to `SafeAutoCorrect: false`.

False positives to detection are not a concern. Only worry about
incompatibilities with auto-correction.

Below is an excerpt from the official document.

> * Safe (true/false) - indicates whether the cop can yield false
> positives (by design) or not.
>
> *SafeAutoCorrect (true/false) - indicates whether the auto-correct a cop
> does is safe (equivalent) by design. If a cop is unsafe its auto-correct
> automatically becomes unsafe as well.

https://docs.rubocop.org/rubocop/0.89/usage/auto_correct.html#safe-auto-correct
koic added a commit to koic/rubocop-rails that referenced this pull request Aug 17, 2020
Follow rubocop#320 and rubocop/rubocop#8529.

This PR changes the mark of `Rails/UniqBeforePluck` from `Safe: false`
to `SafeAutoCorrect: false`.

False positives to detection are not a concern. Only worry about
incompatibilities with auto-correction.

Below is an excerpt from the official document.

> * Safe (true/false) - indicates whether the cop can yield false
> positives (by design) or not.
>
> * SafeAutoCorrect (true/false) - indicates whether the auto-correct a cop
> does is safe (equivalent) by design. If a cop is unsafe its auto-correct
> automatically becomes unsafe as well.

https://docs.rubocop.org/rubocop/0.89/usage/auto_correct.html#safe-auto-correct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants