Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Merge pull request #16481 from sgrif/sg-change-default-timestamps
Change the default `null` value for timestamps
  • Loading branch information
dhh committed Aug 18, 2014
2 parents eced6f8 + ea3ba34 commit 7ee0550
Show file tree
Hide file tree
Showing 14 changed files with 105 additions and 27 deletions.
Expand Up @@ -56,6 +56,19 @@ def default_primary_key
end
end

module TimestampDefaultDeprecation # :nodoc:
def emit_warning_if_null_unspecified(options)
return if options.key?(:null)

ActiveSupport::Deprecation.warn(<<-MESSAGE.strip_heredoc)
`timestamp` was called without specifying an option for `null`. In Rails
5.0, this behavior will change to `null: false`. You should manually
specify `null: true` to prevent the behavior of your existing migrations
from changing.
MESSAGE
end
end

# Represents the schema of an SQL table in an abstract way. This class
# provides methods for manipulating the schema representation.
#
Expand All @@ -77,6 +90,8 @@ def default_primary_key
# The table definitions
# The Columns are stored as a ColumnDefinition in the +columns+ attribute.
class TableDefinition
include TimestampDefaultDeprecation

# An array of ColumnDefinition objects, representing the column changes
# that have been defined.
attr_accessor :indexes
Expand Down Expand Up @@ -276,6 +291,7 @@ def index(column_name, options = {})
# <tt>:updated_at</tt> to the table.
def timestamps(*args)
options = args.extract_options!
emit_warning_if_null_unspecified(options)
column(:created_at, :datetime, options)
column(:updated_at, :datetime, options)
end
Expand Down Expand Up @@ -405,6 +421,8 @@ def add_column(name, type, options)
# end
#
class Table
include TimestampDefaultDeprecation

def initialize(table_name, base)
@table_name = table_name
@base = base
Expand Down Expand Up @@ -452,8 +470,9 @@ def rename_index(index_name, new_index_name)
# Adds timestamps (+created_at+ and +updated_at+) columns to the table. See SchemaStatements#add_timestamps
#
# t.timestamps
def timestamps
@base.add_timestamps(@table_name)
def timestamps(options = {})
emit_warning_if_null_unspecified(options)
@base.add_timestamps(@table_name, options)
end

# Changes the column's definition according to the new options.
Expand Down Expand Up @@ -559,6 +578,5 @@ def native
@base.native_database_types
end
end

end
end
Expand Up @@ -839,9 +839,9 @@ def columns_for_distinct(columns, orders) #:nodoc:
#
# add_timestamps(:suppliers)
#
def add_timestamps(table_name)
add_column table_name, :created_at, :datetime
add_column table_name, :updated_at, :datetime
def add_timestamps(table_name, options = {})
add_column table_name, :created_at, :datetime, options
add_column table_name, :updated_at, :datetime, options
end

# Removes the timestamp columns (+created_at+ and +updated_at+) from the table definition.
Expand Down
Expand Up @@ -764,8 +764,8 @@ def remove_index_sql(table_name, options = {})
"DROP INDEX #{index_name}"
end

def add_timestamps_sql(table_name)
[add_column_sql(table_name, :created_at, :datetime), add_column_sql(table_name, :updated_at, :datetime)]
def add_timestamps_sql(table_name, options = {})
[add_column_sql(table_name, :created_at, :datetime, options), add_column_sql(table_name, :updated_at, :datetime, options)]
end

def remove_timestamps_sql(table_name)
Expand Down
Expand Up @@ -9,7 +9,7 @@ def change
<% end -%>
<% end -%>
<% if options[:timestamps] %>
t.timestamps
t.timestamps null: false
<% end -%>
end
<% attributes_with_index.each do |attribute| -%>
Expand Down
Expand Up @@ -105,7 +105,7 @@ def test_remove_timestamps
with_real_execute do
begin
ActiveRecord::Base.connection.create_table :delete_me do |t|
t.timestamps
t.timestamps null: true
end
ActiveRecord::Base.connection.remove_timestamps :delete_me
assert !column_present?('delete_me', 'updated_at', 'datetime')
Expand Down
Expand Up @@ -105,7 +105,7 @@ def test_remove_timestamps
with_real_execute do
begin
ActiveRecord::Base.connection.create_table :delete_me do |t|
t.timestamps
t.timestamps null: true
end
ActiveRecord::Base.connection.remove_timestamps :delete_me
assert !column_present?('delete_me', 'updated_at', 'datetime')
Expand Down
8 changes: 4 additions & 4 deletions activerecord/test/cases/adapters/postgresql/timestamp_test.rb
Expand Up @@ -87,15 +87,15 @@ def test_timestamp_data_type_with_precision

def test_timestamps_helper_with_custom_precision
ActiveRecord::Base.connection.create_table(:foos) do |t|
t.timestamps :precision => 4
t.timestamps :null => true, :precision => 4
end
assert_equal 4, activerecord_column_option('foos', 'created_at', 'precision')
assert_equal 4, activerecord_column_option('foos', 'updated_at', 'precision')
end

def test_passing_precision_to_timestamp_does_not_set_limit
ActiveRecord::Base.connection.create_table(:foos) do |t|
t.timestamps :precision => 4
t.timestamps :null => true, :precision => 4
end
assert_nil activerecord_column_option("foos", "created_at", "limit")
assert_nil activerecord_column_option("foos", "updated_at", "limit")
Expand All @@ -104,14 +104,14 @@ def test_passing_precision_to_timestamp_does_not_set_limit
def test_invalid_timestamp_precision_raises_error
assert_raises ActiveRecord::ActiveRecordError do
ActiveRecord::Base.connection.create_table(:foos) do |t|
t.timestamps :precision => 7
t.timestamps :null => true, :precision => 7
end
end
end

def test_postgres_agrees_with_activerecord_about_precision
ActiveRecord::Base.connection.create_table(:foos) do |t|
t.timestamps :precision => 4
t.timestamps :null => true, :precision => 4
end
assert_equal '4', pg_datetime_precision('foos', 'created_at')
assert_equal '4', pg_datetime_precision('foos', 'updated_at')
Expand Down
57 changes: 57 additions & 0 deletions activerecord/test/cases/ar_schema_test.rb
Expand Up @@ -14,6 +14,7 @@ def setup
@connection.drop_table :fruits rescue nil
@connection.drop_table :nep_fruits rescue nil
@connection.drop_table :nep_schema_migrations rescue nil
@connection.drop_table :has_timestamps rescue nil
ActiveRecord::SchemaMigration.delete_all rescue nil
end

Expand Down Expand Up @@ -88,5 +89,61 @@ def test_normalize_version
assert_equal "017", ActiveRecord::SchemaMigration.normalize_migration_number("0017")
assert_equal "20131219224947", ActiveRecord::SchemaMigration.normalize_migration_number("20131219224947")
end

def test_timestamps_without_null_is_deprecated_on_create_table
assert_deprecated do
ActiveRecord::Schema.define do
create_table :has_timestamps do |t|
t.timestamps
end
end
end
end

def test_timestamps_without_null_is_deprecated_on_change_table
assert_deprecated do
ActiveRecord::Schema.define do
create_table :has_timestamps

change_table :has_timestamps do |t|
t.timestamps
end
end
end
end

def test_no_deprecation_warning_from_timestamps_on_create_table
assert_not_deprecated do
ActiveRecord::Schema.define do
create_table :has_timestamps do |t|
t.timestamps null: true
end

drop_table :has_timestamps

create_table :has_timestamps do |t|
t.timestamps null: false
end
end
end
end

def test_no_deprecation_warning_from_timestamps_on_change_table
assert_not_deprecated do
ActiveRecord::Schema.define do
create_table :has_timestamps
change_table :has_timestamps do |t|
t.timestamps null: true
end

drop_table :has_timestamps

create_table :has_timestamps
change_table :has_timestamps do |t|
t.timestamps null: false, default: Time.now
end
end
end
end
end
end
7 changes: 5 additions & 2 deletions activerecord/test/cases/migration/change_schema_test.rb
Expand Up @@ -176,8 +176,11 @@ def test_create_table_raises_when_redefining_custom_primary_key_column
end

def test_create_table_with_timestamps_should_create_datetime_columns
connection.create_table table_name do |t|
t.timestamps
# FIXME: Remove the silence when we change the default `null` behavior
ActiveSupport::Deprecation.silence do
connection.create_table table_name do |t|
t.timestamps
end
end
created_columns = connection.columns(table_name)

Expand Down
4 changes: 2 additions & 2 deletions activerecord/test/cases/migration/change_table_test.rb
Expand Up @@ -88,8 +88,8 @@ def test_remove_references_column_type_with_polymorphic_and_type

def test_timestamps_creates_updated_at_and_created_at
with_change_table do |t|
@connection.expect :add_timestamps, nil, [:delete_me]
t.timestamps
@connection.expect :add_timestamps, nil, [:delete_me, null: true]
t.timestamps null: true
end
end

Expand Down
2 changes: 1 addition & 1 deletion activerecord/test/cases/migration/helper.rb
Expand Up @@ -22,7 +22,7 @@ def setup
super
@connection = ActiveRecord::Base.connection
connection.create_table :test_models do |t|
t.timestamps
t.timestamps null: true
end

TestModel.reset_column_information
Expand Down
2 changes: 1 addition & 1 deletion activerecord/test/cases/migration_test.rb
Expand Up @@ -561,7 +561,7 @@ def test_adding_multiple_columns
t.string :qualification, :experience
t.integer :age, :default => 0
t.date :birthdate
t.timestamps
t.timestamps null: true
end
end

Expand Down
10 changes: 5 additions & 5 deletions activerecord/test/schema/schema.rb
Expand Up @@ -138,7 +138,7 @@ def except(adapter_names_to_exclude)
t.integer :engines_count
t.integer :wheels_count
t.column :lock_version, :integer, null: false, default: 0
t.timestamps
t.timestamps null: false
end

create_table :categories, force: true do |t|
Expand Down Expand Up @@ -537,7 +537,7 @@ def except(adapter_names_to_exclude)
t.references :best_friend_of
t.integer :insures, null: false, default: 0
t.timestamp :born_at
t.timestamps
t.timestamps null: false
end

create_table :peoples_treasures, id: false, force: true do |t|
Expand All @@ -548,7 +548,7 @@ def except(adapter_names_to_exclude)
create_table :pets, primary_key: :pet_id, force: true do |t|
t.string :name
t.integer :owner_id, :integer
t.timestamps
t.timestamps null: false
end

create_table :pirates, force: true do |t|
Expand Down Expand Up @@ -726,13 +726,13 @@ def except(adapter_names_to_exclude)
t.string :parent_title
t.string :type
t.string :group
t.timestamps
t.timestamps null: true
end

create_table :toys, primary_key: :toy_id, force: true do |t|
t.string :name
t.integer :pet_id, :integer
t.timestamps
t.timestamps null: false
end

create_table :traffic_lights, force: true do |t|
Expand Down
2 changes: 1 addition & 1 deletion railties/test/generators/model_generator_test.rb
Expand Up @@ -222,7 +222,7 @@ def test_model_with_polymorphic_belongs_to_attribute_generates_belongs_to_associ

def test_migration_with_timestamps
run_generator
assert_migration "db/migrate/create_accounts.rb", /t.timestamps/
assert_migration "db/migrate/create_accounts.rb", /t.timestamps null: false/
end

def test_migration_timestamps_are_skipped
Expand Down

0 comments on commit 7ee0550

Please sign in to comment.