Primary key should be NOT NULL
#23622
Conversation
|
r? @schneems (@rails-bot has picked a reviewer for you, use r? to override) |
kamipo
force-pushed
the
primary_key_should_be_not_null
branch
from
af583a8
to
2c91c68
Compare
|
Given that this only affects SQLite, I would prefer an implementation that only touches SQLite related code. Is there a reason we can't just emit |
|
This change is also needed for #23553. Should we affect only sqlite3 and mysql2 adapters? |
|
Why would this affect the mysql2 adapter? I thought primary keys were implicitly not null in mysql? |
|
AR's column definition is default nullable. But the native timestamp type in MySQL is default not null.
|
kamipo
force-pushed
the
primary_key_should_be_not_null
branch
from
2c91c68
to
0faa08b
Compare
|
|
||
| def column_options(o) | ||
| options = super | ||
| options[:null] = false if o.primary_key |
There was a problem hiding this comment.
I fixed to only affect to SQLite, thanks!
|
r? @sgrif |
kamipo
force-pushed
the
primary_key_should_be_not_null
branch
4 times, most recently
from
5bc47b2
to
23f6452
Compare
| @@ -229,7 +229,7 @@ def test_any_type_primary_key | |||
| assert_equal "code", Barcode.primary_key | |||
|
|
|||
| column = Barcode.column_for_attribute(Barcode.primary_key) | |||
| assert_not column.null unless current_adapter?(:SQLite3Adapter) | |||
| assert_not column.null | |||
There was a problem hiding this comment.
Added the test first at #23959.
This PR fixes the wrong behavior for sqlite3 adapter.
kamipo
force-pushed
the
primary_key_should_be_not_null
branch
3 times, most recently
from
62b3e49
to
0ea47eb
Compare
|
@rafaelfranca What do you think about this? |
Follow up to rails#18228. In MySQL and PostgreSQL, primary key is to be `NOT NULL` implicitly. But in SQLite it must be specified `NOT NULL` explicitly.
kamipo
force-pushed
the
primary_key_should_be_not_null
branch
from
0ea47eb
to
98fb374
Compare
|
@jeremy What do you think about this? |
Primary key should be `NOT NULL`
Follow up to #18228.
In MySQL and PostgreSQL, primary key is to be
NOT NULLimplicitly.But in SQLite it must be specified
NOT NULLexplicitly.lib/active_record/connection_adapters/sqlite3_adapter.rb#L56