-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add new Rails/BulkChangeTable
cop
#5881
Conversation
config/default.yml
Outdated
SupportedDatabases: | ||
- mysql | ||
- postgresql | ||
- '...' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry if my understanding was wrong.
Is this to change this default.yml to match the connection adapter of Rails application that user is targeting?
I tried this cop. I ran the connection adapter as "Oracle", but false positive will occur if I run with this default setting. Perhaps the same is true when using "SQLite3" (Other than "MySQL", "PostgreSQL").
If it is need to change depending on the connection adapter users are using, it seems not to be the default.
The connection adapter used by Rails application can be determined by adapter
in Rails.root/config/database.yml. The core adapters managed by Rails is as follows.
- MySQL => mysql2
- PostgreSQL => postgresql
- SQLite3 => sqlite3
Rather than preparing the default.yml setting, one option is to automatically decide based on adapter
specified in Rails.root/config/database.yml.
However, this proposal is not silver bullet, connection to multiple databases using establish_connection
etc seems difficult to detect by static analysis.
In any case, it seems that it is not good for false positives to appear depending on the dependent connection adapter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your reviewing!
Is this to change this default.yml to match the connection adapter of Rails application that user is targeting?
Yes. For example, if using MySQL, it is needed to edit .rubocop.yml
as follows:
Rails/BulkChangeTable:
Database: mysql
For that reason, I think it will occur false positives on the other adapters by default.
I think there are two options to this Cop.
- If the adapter is not set, this Cop will not detect offenses
- Automatically determine adapter from
database.yml
The former will be occur false negatives on the MySQL adapter or the PostgreSQL adapter. And also, it will also behave the same as disabled by default.
The latter looks good, but as @koic says, there are some difficulties. For example, how can this Cop determine Rails.root
? What options should be provided to users using multiple databases?
Currently I'm going to fix this Cop based on the latter, but I'd like to hear many opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, how can this Cop determine Rails.root?
I think that the root of repository can be regarded as Rails.root
. Of course, if it can be changed in configuration, then it can correspond to the unexpected configuration, but first I think that it is better to start with support of CoC configuration that ride on Rails way (I think it is better to start with the smallest case) .
What options should be provided to users using multiple databases?
Since the regular configuration of Rails (5.2 or lower) doesn't yet support multiple databases, I think that this cop don't need to support yet.
📝 Rails 6 introduces Part 1 implementation of multiple databases.
rails/rails#32274
# Add bulk alter support for PostgreSQL in 5.2.0 | ||
# @see https://github.com/rails/rails/pull/31331 | ||
target_rails_version >= 5.2 | ||
when nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since bulk migration is only supported by mysql and postgres, shouldn't this return false?
5a18785
to
828d1ce
Compare
return nil unless File.exist?('config/database.yml') | ||
yaml = YAML.load_file('config/database.yml') | ||
return nil unless yaml.is_a? Hash | ||
config = yaml['development'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Cop detect a database adapter when adapter
is set as development
in config/database.yml
.
In other words, it cannot detect an adapter if using DATABASE_URL
, other configuration file location and more.
However, you can change it by setting Database
with .rubocop.yml
.
If set `Database` option, `SupportedDatabases` is also used as configurable values.
828d1ce
to
58a1d9f
Compare
This Cop checks whether alter queries are combinable. If combinable queries are detected, it suggests to you to use `change_table` with `bulk: true` instead. When use this method, make combinable alter queries a bulk alter query. The `bulk` option is only supported on the MySQL and the PostgreSQL (5.2 later) adapter; thus it will automatically detect an adapter from `development` environment in `config/database.yml` when the `Database` option is not set. If the adapter is not `mysql2` or `postgresql`, this Cop ignores offenses. For example, the following migration executes 4 SQLs: ```ruby class BulkExample < ActiveRecord::Migration[5.2] def change // Output SQL ActiveRecord::Base.logger = Logger.new(STDOUT) add_reference :users, :team add_column :users, :name, :string, null: false add_column :users, :nickname, :string end end ``` ``` == 20180513070748 BulkExample: migrating ====================================== -- add_reference(:users, :team) D, [2018-05-13T16:19:09.247167 #88149] DEBUG -- : (173.7ms) ALTER TABLE `users` ADD `team_id` bigint D, [2018-05-13T16:19:09.248363 #88149] DEBUG -- : ↳ db/migrate/20180513070748_bulk_example.rb:5 D, [2018-05-13T16:19:09.279004 #88149] DEBUG -- : (29.2ms) CREATE INDEX `index_users_on_team_id` ON `users` (`team_id`) D, [2018-05-13T16:19:09.279818 #88149] DEBUG -- : ↳ db/migrate/20180513070748_bulk_example.rb:5 -> 0.2073s -- add_column(:users, :name, :string, {:null=>false}) D, [2018-05-13T16:19:09.339642 #88149] DEBUG -- : (59.1ms) ALTER TABLE `users` ADD `name` varchar(255) NOT NULL D, [2018-05-13T16:19:09.339814 #88149] DEBUG -- : ↳ db/migrate/20180513070748_bulk_example.rb:6 -> 0.0596s -- add_column(:users, :nickname, :string) D, [2018-05-13T16:19:09.386546 #88149] DEBUG -- : (46.2ms) ALTER TABLE `users` ADD `nickname` varchar(255) D, [2018-05-13T16:19:09.387489 #88149] DEBUG -- : ↳ db/migrate/20180513070748_bulk_example.rb:7 -> 0.0476s == 20180513070748 BulkExample: migrated (0.3151s) ============================= ``` If you use `bulk: true`, it combines into 3 SQLs. ```ruby class BulkExample < ActiveRecord::Migration[5.2] def change ActiveRecord::Base.logger = Logger.new(STDOUT) add_reference :users, :team change_table :users, bulk: true do |t| t.string :name, null: false t.string :nickname end end end ``` ``` == 20180513070748 BulkExample: migrating ====================================== -- add_reference(:users, :team) D, [2018-05-13T16:26:35.937416 #89967] DEBUG -- : (70.5ms) ALTER TABLE `users` ADD `team_id` bigint D, [2018-05-13T16:26:35.937644 #89967] DEBUG -- : ↳ db/migrate/20180513070748_bulk_example.rb:5 D, [2018-05-13T16:26:35.974410 #89967] DEBUG -- : (34.1ms) CREATE INDEX `index_users_on_team_id` ON `users` (`team_id`) D, [2018-05-13T16:26:35.974609 #89967] DEBUG -- : ↳ db/migrate/20180513070748_bulk_example.rb:5 -> 0.1083s -- change_table(:users, {:bulk=>true}) D, [2018-05-13T16:26:36.030428 #89967] DEBUG -- : (52.1ms) ALTER TABLE `users` ADD `name` varchar(255) NOT NULL, ADD `nickname` varchar(255) D, [2018-05-13T16:26:36.030584 #89967] DEBUG -- : ↳ db/migrate/20180513070748_bulk_example.rb:6 -> 0.0557s == 20180513070748 BulkExample: migrated (0.1644s) ============================= ``` By the way, this Cop ignores non-combinable queries such as `add_references` because including these in bulk changes will cause an error like the following: ``` == 20180513070748 BulkExample: migrating ====================================== -- change_table(:users, {:bulk=>true}) D, [2018-05-13T16:30:52.348856 #90767] DEBUG -- : (0.4ms) SELECT RELEASE_LOCK('5518437444551514000') D, [2018-05-13T16:30:52.349985 #90767] DEBUG -- : ↳ /Users/watanabekazuma/.rbenv/versions/2.4.1/bin/rake:22 rake aborted! StandardError: An error has occurred, all later migrations canceled: Unknown method called : add_reference_for_alter([:team, {}]) ```
58a1d9f
to
5ace4d6
Compare
This Cop checks whether alter queries are combinable. | ||
If combinable queries are detected, it suggests to you | ||
to use `change_table` with `bulk: true` instead. | ||
When use this method, make combinable alter queries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence doesn't seem to make sense to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. This is a bit buried in a merged PR, so I've made an issue of it #6158
See also (Japanese): rubocop/rubocop-jp#29 Thanks @meganemura
This Cop checks whether alter queries are combinable. If combinable queries are detected, it suggests to you to use
change_table
withbulk: true
instead.When use this method, make combinable alter queries a bulk alter query.
The
bulk
option is only supported on the MySQL and the PostgreSQL (5.2 later) adapter; thus it will automatically detect an adapter fromdevelopment
environment inconfig/database.yml
when theDatabase
option is not set. If the adapter is notmysql2
orpostgresql
, this Cop ignores offenses.For example, the following migration generates 4 queries:
If you use
bulk: true
, it combines into 3 queries.By the way, this Cop ignores non-combinable queries such as
add_references
because including these in bulk changes will cause an error like the following:Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).and description in grammatically correct, complete sentences.
rake default
orrake parallel
. It executes all tests and RuboCop for itself, and generates the documentation.