Inconsistency found in NullConstaintChecker

[hanzongyu]

Hi everybody,

I enjoyed using this gem very much, and I was just wondering whether anyone face this similar issue.

I've created a User Migration to create a user table with the following columns:

class CreateUsers < ActiveRecord::Migration[6.0]
  def change
    create_table :users do |t|
      t.string :name, null: false
      t.boolean :admin, null: false
      t.timestamps
    end
  end
end

This is my current user model:

class User < ApplicationRecord
end

Running the bundle exec database_consistency throws the following error messages:

NullConstraintChecker fail User name column is required in the database but do not have presence validator
NullConstraintChecker fail User admin column is required in the database but do not have presence validator

Following rectifications were made to the model:

class User < ApplicationRecord
  validates :admin, presence: true
  validates :name, presence: true
end

Running the bundle exec database_consistency no longer shows the error messages but the suggestion of including of presence validator for User admin column result in an incorrect behaviour.

In the rails console:

User.create(admin:false).save
 => false 

Throws an error: Validation failed: Admin can't be blank, Name can't be blank (ActiveRecord::RecordInvalid) This behaviour is not the correct as a boolean of value false does not mean that it is blank.

I believe that it should be inclusion validation instead:

class User < ApplicationRecord
  validates :admin, inclusion: [true,false]
  validates :name, presence: true
end

My suggestion would be to Update the error message to be more specific to target this edge case. Instead of NullConstraintChecker fail User admin column is required in the database but do not have presence validator, it could be NullConstraintChecker fail User admin column is required in the database but do not have inclusion validator.

What do you think 🙂

[djezzzl]

Hi @hanzongyu,

Thank you for spotting the issue! What do you think if we make the message more generic to cover all cases and let an engineer decide what should be used?

The message could be: "NullConstraintChecker fail User admin column is required in the database but do not have a validator that disallows nil values."

[djezzzl]

I have improved it with my proposal here: https://github.com/djezzzl/database_consistency/pull/178. Please let me know if you disagree so that we can discuss it.

[djezzzl]

The fix was released in 1.7.3. Please try it out and let me know if that helped. Thank you again, and have a great day!

Feel free to reopen the issue if needed.