Rails/NotNullColumn: Why are defaults required?

[oharace]

I'd like to generate some discussion on why this cop is requiring defaults for not-nullable columns.

First:

$ bundle exec rubocop -V
0.76.0 (using Parser 2.6.5.0, running on ruby 2.6.5 x86_64-darwin19)

The code that made this cop fail was pretty basic:

# This makes RuboCop fail with:
# 'Do not add a NOT NULL column without a default value. (convention: Rails/NotNullColumn)'

add_reference :parent_table, :child_table, foreign_key: true, null: false

Requiring a default value for a non-nullable foreign key seems strange to me. The only scenario where I can see this being useful is in a structure where all child records should be using a base parent record except when the child record explicitly needs to use a different parent record.

A non-nullable foreign key shouldn't be required to have a default. From a data-integrity standpoint, it is very useful to prevent records from being saved if they lack the data we expect.

In the cop's other scenario where we are adding a non-nullable column that is not a foreign key, it is still useful to not require a default value. If the system needs the column to have a default value, then by all means, add a default! But it shouldn't be required.

To help further explain my thought process, let's say we need to add a new column that keeps track of a user's name. We make this column, :name, non-nullable. For our business purposes, we do not want to default it. What would we default it to anyway, an empty string? The application should handle the validation and make sure the user provides us their name. In the off-chance that something happens and that insert/update (without a value for their name) reaches the DB, we want the insert/update to fail. Requiring the column to have a default would allow this record to still be saved, and now we have a data-integrity issue since the record is missing data we expect or need. Should the application have caught it? Yes. But the database is the source of truth and a default shouldn't always be required. (If the :name column is non-nullable and has a default, is it truly required? Doesn't seem so.)

I'd like to see what people think about:

• Why should a default be required for non-nullable foreign keys?
• Is this cop needed in general?
• What scenarios could I be missing where requiring a default makes sense?

I'm here to learn, and I'm keen to see your answers. Thanks for discussing!

[andyw8]

Just for reference, here's the original PR: rubocop/rubocop#3415

[oharace]

@andyw8 Thank you! That helps a lot.

If I'm reading it correctly, it seems that RDBMS at the time required default values, so this cop makes sense. Perhaps this is an outdated cop (or is useful when using old database versions)? I've used mysql and postgresql lately and can add non-nullable columns without default values just fine.

EDIT: I see that the big use case is when adding non-nullable columns to existing tables. In that regard, I think making the migration fail is much more useful than requiring a default value. That way, the user can decide how they want to treat existing records, whether that be by adding a default or a multi-step process of adding the column, determining/assigning the values, and then making the column non-nullable. Requiring a default value can have future consequences like my example in the original post about receiving missing data but saving successfully anyway.

[lxxxvi]

I agree with @oharace.

Migrations can be difficult indeed, adding NOT NULL columns to an existing table sometimes require additional work.

I believe there are (at least) two ways to introduce a "not-null-no-default" column.

1. Add column as NOT NULL DEFAULT x ➡️ migrate data ➡️ remove default
2. Add column as NULL ➡️ migrate data ➡️ change column to NOT NULL

I usually go with option 2, because the process does not require "wrong" defaults, and it allows me to easily identify "non-migrated" rows.

Another thing that bothers me is that the documentation considers this example to be "good":

add_reference :products, :category, null: false, default: 1

Linter-wise that example may make sense, but only if the target is to prevent the migration from crashing.
Data-wise this approach is rather dangerous, as people may see it as common "good" way to handle such situation, which isn't.

EDIT:
May be we can rephrase the message to make it clearer what the intention of the cop is...?

[natematykiewicz]

I'm probably going to restate everything in this thread. But I figured another example might be helpful.

I was just working on a fresh Rails app. I generated a new migration using an example from https://guides.rubyonrails.org/active_record_migrations.html#writing-a-migration

bin/rails generate migration AddUserRefToProducts user:references

This generates a new migration setting null: false. This migration makes this cop fail. Seems odd to me that an example copied directly from the documentation would fail (though arguably, it's a bad generator).

I understand the problem that's at play -- if there's existing rows, this migration won't succeed. In my case, I didn't have existing rows so the migration ran fine but Rubocop is mad (which was confusing).

I think the worst part is the cop description (and a big complaint of mine regarding Rubocop in general). The description is: "Do not add a NOT NULL column without a default value.". That's incredibly unhelpful. A "why" would be much more helpful. Clearly the cop doesn't want me to add a not null column without a default, the cop is called "NotNullColumn" and it's erroring on this line. But WHY is that wrong? The answer to that question wasn't immediately obvious to me, especially since Rails generated this code. A description of "Do not add a NOT NULL column without a default value, as this will fail if there are existing rows. Set a default, or add the NOT NULL in a later migration after populating the column." would be much more helpful. There's a few other cops out there that have "what" descriptions without a "why", and it makes me wish there were some explanation of why a decision was made (is there some best practice I should know, instead of blindly following the cop?).

It still feels really weird that I can run a Rails generator command straight from the docs and get a Rubocop failure. 🙁

[oharace]

Thank you for your replies, you both bring up valuable points. I think this cop provides a valuable warning, albeit with some rewording, but it shouldn't be forcing the user to set a default value. Instead, maybe it could provide a more-detailed message providing the user with some solutions such as:

1. Set a permanent default value
2. Set a temporary default value, migrate data, remove the default value
3. Recommend removing the non-null constraint, migrate data, and add a non-null constraint

Most cops aren't very wordy, though. Are there any cops you can think of that have a similar strategy like this?

[jdufresne]

Why should a default be required for non-nullable foreign keys?

Could the cop ignore FK columns but work as-is for value columns?

Unlike string/int value columns, FKs often don't have a good default to fallback to. Choosing a random record from the referenced table is often wrong. Choosing 1 or 0 to to satisfy the cop also doesn't solve anything in practice except to silence the cop. If the recommendation is to add a bogus value, that is as good as no cop.

[paulodeon]

If you have data in the table where you're adding a reference you'll get an error like this:

ActiveRecord::NotNullViolation: PG::NotNullViolation: ERROR: column "covering_id" of relation "cover_requests" contains null values

You can fix the cop and run the migration by adding change_column_default:

class AddCoveringToCoverRequests < ActiveRecord::Migration[7.0]
  def change
    add_reference :cover_requests, :covering, null: false, default: 1, foreign_key: { to_table: :employments }
    change_column_default :cover_requests, :covering_id, from: 1, to: nil
  end
end

The case where you don't have any data in the table and you're adding a reference this cop is pretty annoying but I think the above is a reasonable solution.

Keep the cop as it is useful for the other cases and use the above whenever adding foreign keys.

[natematykiewicz]

@paulodeon The problem isn't that the cop exists, the problem is that the error message is not helpful.

Do not add a NOT NULL column without a default value.

That's not helpful. Tell the user why. I think that's my biggest complaint about a lot of cops. They don't explain a why, and so they just feel like an arbitrary set of rules. This one has a very clear why. So just say it.

Here's an example that would make this cop much clearer. I'm sure it could be wordsmithed better:

A migration that adds a non-nullable column without a default value will fail if there's rows in the table, as existing rows would otherwise have a null value on a non-nullable column.

Furthermore, adding a default is not always desirable. There's other ways to add a NOT NULL column without a default value. Strong Migrations explains how to add a NOT NULL to an existing column. So if you're wanting to add a NOT NULL column that does not have a default you can, but it's a multi-step process.

First you add the column as nullable with no default. Then you follow the steps here to make it be not nullable. Note, you will need to backfill existing rows before validating the check constraint:
https://github.com/ankane/strong_migrations#setting-not-null-on-an-existing-column

[paulodeon]

@natematykiewicz I do agree with you - especially on the broader point about rubocop

The example was to help others in the same boat without the need to change rubocop 😅