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

Add new `Rails/NotNullColumn` cop #3415

Merged
merged 1 commit into from Aug 17, 2016

Conversation

@pocke
Copy link
Member

pocke commented Aug 14, 2016

I've added a new Rails/NotNullColumn cop.

Target Problem

RDBMS doesn't allow to add a new not null column.
So, in a migration file of Rails, rake db:migrate crashes if a column with NOT NULL constraint is added.

For example

db/migrate/20160814100049_add_not_null_column.rb

class AddNotNullColumn < ActiveRecord::Migration[5.0]
  def change
    create_table :users do |t|
      t.timestamps null: false
    end

    add_column :users, :name, :string, null: false
  end
end
$ bin/rake db:migrate:reset
Dropped database 'db/development.sqlite3'
Dropped database 'db/test.sqlite3'
Created database 'db/development.sqlite3'
Created database 'db/test.sqlite3'
== 20160814100049 AddNotNullColumn: migrating =================================
-- create_table(:users)
   -> 0.0010s
-- add_column(:users, :name, :string, {:null=>false})
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

SQLite3::SQLException: Cannot add a NOT NULL column with default value NULL: ALTER TABLE "users" ADD "name" varchar NOT NULL
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `initialize'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `new'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `prepare'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:134:in `execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:232:in `block in execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract_adapter.rb:566:in `block in log'
/home/pocke/.gem/ruby/2.3.0/gems/activesupport-5.0.0.1/lib/active_support/notifications/instrumenter.rb:21:in `instrument'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract_adapter.rb:560:in `log'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:232:in `execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/schema_statements.rb:547:in `add_column'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:373:in `add_column'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:845:in `block in method_missing'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:814:in `block in say_with_time'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:814:in `say_with_time'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:834:in `method_missing'
/tmp/tmp.0s18esP231/hoge/db/migrate/20160814100049_add_not_null_column.rb:7:in `change'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:788:in `exec_migration'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:772:in `block (2 levels) in migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:771:in `block in migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/connection_pool.rb:398:in `with_connection'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:770:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:950:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1211:in `block in execute_migration_in_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1279:in `block in ddl_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `block in transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/transaction.rb:189:in `within_new_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/transactions.rb:211:in `transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1279:in `ddl_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1210:in `execute_migration_in_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1183:in `block in migrate_without_lock'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1182:in `each'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1182:in `migrate_without_lock'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1133:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1005:in `up'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:983:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/tasks/database_tasks.rb:161:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/railties/databases.rake:58:in `block (2 levels) in <top (required)>'
ActiveRecord::StatementInvalid: SQLite3::SQLException: Cannot add a NOT NULL column with default value NULL: ALTER TABLE "users" ADD "name" varchar NOT NULL
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `initialize'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `new'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `prepare'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:134:in `execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:232:in `block in execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract_adapter.rb:566:in `block in log'
/home/pocke/.gem/ruby/2.3.0/gems/activesupport-5.0.0.1/lib/active_support/notifications/instrumenter.rb:21:in `instrument'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract_adapter.rb:560:in `log'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:232:in `execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/schema_statements.rb:547:in `add_column'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:373:in `add_column'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:845:in `block in method_missing'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:814:in `block in say_with_time'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:814:in `say_with_time'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:834:in `method_missing'
/tmp/tmp.0s18esP231/hoge/db/migrate/20160814100049_add_not_null_column.rb:7:in `change'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:788:in `exec_migration'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:772:in `block (2 levels) in migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:771:in `block in migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/connection_pool.rb:398:in `with_connection'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:770:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:950:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1211:in `block in execute_migration_in_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1279:in `block in ddl_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `block in transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/transaction.rb:189:in `within_new_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/transactions.rb:211:in `transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1279:in `ddl_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1210:in `execute_migration_in_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1183:in `block in migrate_without_lock'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1182:in `each'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1182:in `migrate_without_lock'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1133:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1005:in `up'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:983:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/tasks/database_tasks.rb:161:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/railties/databases.rake:58:in `block (2 levels) in <top (required)>'
SQLite3::SQLException: Cannot add a NOT NULL column with default value NULL
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `initialize'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `new'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:91:in `prepare'
/home/pocke/.gem/ruby/2.3.0/gems/sqlite3-1.3.11/lib/sqlite3/database.rb:134:in `execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:232:in `block in execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract_adapter.rb:566:in `block in log'
/home/pocke/.gem/ruby/2.3.0/gems/activesupport-5.0.0.1/lib/active_support/notifications/instrumenter.rb:21:in `instrument'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract_adapter.rb:560:in `log'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:232:in `execute'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/schema_statements.rb:547:in `add_column'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/sqlite3_adapter.rb:373:in `add_column'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:845:in `block in method_missing'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:814:in `block in say_with_time'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:814:in `say_with_time'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:834:in `method_missing'
/tmp/tmp.0s18esP231/hoge/db/migrate/20160814100049_add_not_null_column.rb:7:in `change'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:788:in `exec_migration'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:772:in `block (2 levels) in migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:771:in `block in migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/connection_pool.rb:398:in `with_connection'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:770:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:950:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1211:in `block in execute_migration_in_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1279:in `block in ddl_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `block in transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/transaction.rb:189:in `within_new_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/connection_adapters/abstract/database_statements.rb:232:in `transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/transactions.rb:211:in `transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1279:in `ddl_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1210:in `execute_migration_in_transaction'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1183:in `block in migrate_without_lock'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1182:in `each'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1182:in `migrate_without_lock'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1133:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:1005:in `up'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/migration.rb:983:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/tasks/database_tasks.rb:161:in `migrate'
/home/pocke/.gem/ruby/2.3.0/gems/activerecord-5.0.0.1/lib/active_record/railties/databases.rake:58:in `block (2 levels) in <top (required)>'
Tasks: TOP => db:migrate:reset => db:migrate
(See full trace by running task with --trace)

Feature

The new cop detects the above problem.

$ rubocop db/migrate/20160814100049_add_not_null_column.rb --rails --only Rails/NotNullColumn
Inspecting 1 file
C

Offenses:

db/migrate/20160814100049_add_not_null_column.rb:7:40: C: Do not add a NOT NULL column
    add_column :users, :name, :string, null: false
                                       ^^^^^^^^^^^

1 file inspected, 1 offense detected

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).
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
@pocke pocke force-pushed the pocke:add-Rails/NotNullColumn-cop branch from 6961400 to f78c16f Aug 14, 2016
# # good
# add_column :users, :name, :string, null: true
class NotNullColumn < Cop
MSG = 'Do not add a NOT NULL column'.freeze

This comment has been minimized.

Copy link
@mikegee

mikegee Aug 15, 2016

Contributor

Maybe the message should be: 'Do not add a NOT NULL column without a default value'

This comment has been minimized.

Copy link
@backus

backus Aug 15, 2016

Contributor

I agree with @mikegee. When I first scanned through this PR I thought this was a cop that simply encouraged people to not have NOT NULL columns and I only realized this wasn't the case when I read through the cop's source. This message would very likely confuse end users.

This comment has been minimized.

Copy link
@pocke

pocke Aug 16, 2016

Author Member

@mikegee @backus

Thanks for your feedback!
I've fixed the message.

@pocke pocke force-pushed the pocke:add-Rails/NotNullColumn-cop branch from f78c16f to 3edfa8c Aug 16, 2016
@@ -1430,6 +1430,10 @@ Rails/HasAndBelongsToMany:
StyleGuide: 'https://github.com/bbatsov/rails-style-guide#has-many-through'
Enabled: true

Rails/NotNullColumn:
Description: 'Do not add a NOT NULL column'

This comment has been minimized.

Copy link
@mikegee

mikegee Aug 16, 2016

Contributor

Please change this to mention default value, too.

@pocke pocke force-pushed the pocke:add-Rails/NotNullColumn-cop branch from 3edfa8c to 7a41890 Aug 17, 2016
@pocke

This comment has been minimized.

Copy link
Member Author

pocke commented Aug 17, 2016

Oh, I forgot the description.
I've fixed it. Thanks!
#3415 (comment)

@bbatsov bbatsov merged commit 344156a into rubocop-hq:master Aug 17, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pocke pocke deleted the pocke:add-Rails/NotNullColumn-cop branch Aug 17, 2016
Neodelf added a commit to Neodelf/rubocop that referenced this pull request Oct 15, 2016
@ixti

This comment has been minimized.

Copy link

ixti commented Jan 13, 2017

NOT NULL is a constraint. And does not depends on whenever DEFAULT value is set for column in all RDBMSes. So it's a 100% valid and works as expected in MySQL and PostgreSQL:

CREATE TABLE foo ( name text NOT NULL );

Also which version of the SQLite fails for you? Because the SQL above works like a charm for me:

$ sqlite3 --version
3.15.2 2016-11-28 19:13:37 bbd85d235f7037c6a033a9690534391ffeacecc8

$ sqlite3 /tmp/test.db
SQLite version 3.15.2 2016-11-28 19:13:37
Enter ".help" for usage hints.
sqlite> CREATE TABLE foo ( name text NOT NULL );
sqlite> INSERT INTO foo ( name ) VALUES ( "x" );
sqlite> INSERT INTO foo ( name ) VALUES ( NULL );
Error: NOT NULL constraint failed: foo.name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.