Skip to content

Commit

Permalink
Mark Rails/UniqBeforePluck cops as unsafe
Browse files Browse the repository at this point in the history
`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.
  • Loading branch information
kunitoo committed Aug 6, 2020
1 parent ee5c3e0 commit e69b8ea
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 4 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
* [#312](https://github.com/rubocop-hq/rubocop-rails/pull/312): Mark `Rails/MailerName` as unsafe for auto-correct. ([@eugeneius][])
* [#294](https://github.com/rubocop-hq/rubocop-rails/pull/294): Update `Rails/ReversibleMigration` to register offenses for `remove_columns` and `remove_index`. ([@philcoggins][])
* [#310](https://github.com/rubocop-hq/rubocop-rails/issues/310): Add `EnforcedStyle` to `Rails/PluckInWhere`. By default, it does not register an offense if `pluck` method's receiver is a variable. ([@koic][])
* [#320](https://github.com/rubocop-hq/rubocop-rails/pull/320): Mark `Rails/UniqBeforePluck` as unsafe. ([@kunitoo][])

## 2.7.1 (2020-07-26)

Expand Down Expand Up @@ -262,3 +263,4 @@
[@taylorthurlow]: https://github.com/taylorthurlow
[@krim]: https://github.com/krim
[@philcoggins]: https://github.com/philcoggins
[@kunitoo]: https://github.com/kunitoo
3 changes: 2 additions & 1 deletion config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -657,11 +657,12 @@ Rails/UniqBeforePluck:
Description: 'Prefer the use of uniq or distinct before pluck.'
Enabled: true
VersionAdded: '0.40'
VersionChanged: '2.6'
VersionChanged: '2.8'
EnforcedStyle: conservative
SupportedStyles:
- conservative
- aggressive
Safe: false
AutoCorrect: false

Rails/UniqueValidationWithoutIndex:
Expand Down
8 changes: 5 additions & 3 deletions docs/modules/ROOT/pages/cops_rails.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -3977,10 +3977,10 @@ Time.at(timestamp).in_time_zone
| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

| Enabled
| Yes
| Yes
| No
| Yes (Unsafe)
| 0.40
| 2.6
| 2.8
|===

Prefer the use of distinct, before pluck instead of after.
Expand All @@ -3998,6 +3998,8 @@ as the cop cannot distinguish between calls to pluck on an
ActiveRecord::Relation vs a call to pluck on an
ActiveRecord::Associations::CollectionProxy.

This cop is unsafe because the behavior may change depending on the
database collation.
Autocorrect is disabled by default for this cop since it may generate
false positives.

Expand Down
2 changes: 2 additions & 0 deletions lib/rubocop/cop/rails/uniq_before_pluck.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ module Rails
# ActiveRecord::Relation vs a call to pluck on an
# ActiveRecord::Associations::CollectionProxy.
#
# This cop is unsafe because the behavior may change depending on the
# database collation.
# Autocorrect is disabled by default for this cop since it may generate
# false positives.
#
Expand Down

0 comments on commit e69b8ea

Please sign in to comment.