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

Make t.timestamps with precision by default. #34970

Merged
merged 1 commit into from
Jan 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,7 @@ def change
t.string :message_id, null: false
t.string :message_checksum, null: false

if supports_datetime_with_precision?
t.timestamps precision: 6
else
t.timestamps
end
t.timestamps

t.index [ :message_id, :message_checksum ], name: "index_action_mailbox_inbound_emails_uniqueness", unique: true
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ def change
t.text :body, limit: 16777215
t.references :record, null: false, polymorphic: true, index: false

t.datetime :created_at, null: false
t.datetime :updated_at, null: false
t.timestamps

t.index [ :record_type, :record_id, :name ], name: "index_action_text_rich_texts_uniqueness", unique: true
end
Expand Down
12 changes: 6 additions & 6 deletions actiontext/test/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
t.text "body", limit: 16777215
t.string "record_type", null: false
t.integer "record_id", null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.datetime "created_at", precision: 6, null: false
t.datetime "updated_at", precision: 6, null: false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed to apply new default the same with Action Mailbox.

t.index ["record_type", "record_id", "name"], name: "index_action_text_rich_texts_uniqueness", unique: true
end

Expand All @@ -45,14 +45,14 @@

create_table "messages", force: :cascade do |t|
t.string "subject"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.datetime "created_at", precision: 6, null: false
t.datetime "updated_at", precision: 6, null: false
end

create_table "people", force: :cascade do |t|
t.string "name"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.datetime "created_at", precision: 6, null: false
t.datetime "updated_at", precision: 6, null: false
end

end
5 changes: 5 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
* Make `t.timestamps` with precision by default.

*Ryuta Kamizono*


## Rails 6.0.0.beta1 (January 18, 2019) ##

* Remove deprecated `#set_state` from the transaction object.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ class TableDefinition
deprecate :indexes=

def initialize(
conn,
name,
temporary: false,
if_not_exists: false,
Expand All @@ -271,6 +272,7 @@ def initialize(
comment: nil,
**
)
@conn = conn
@columns_hash = {}
@indexes = []
@foreign_keys = []
Expand Down Expand Up @@ -410,6 +412,10 @@ def foreign_key(table_name, options = {}) # :nodoc:
def timestamps(**options)
options[:null] = false if options[:null].nil?

if !options.key?(:precision) && @conn.supports_datetime_with_precision?
options[:precision] = 6
end

column(:created_at, :datetime, options)
column(:updated_at, :datetime, options)
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1129,6 +1129,10 @@ def columns_for_distinct(columns, orders) # :nodoc:
def add_timestamps(table_name, options = {})
options[:null] = false if options[:null].nil?

if !options.key?(:precision) && supports_datetime_with_precision?
options[:precision] = 6
end

add_column table_name, :created_at, :datetime, options
add_column table_name, :updated_at, :datetime, options
end
Expand Down Expand Up @@ -1290,7 +1294,7 @@ def schema_creation
end

def create_table_definition(*args)
TableDefinition.new(*args)
TableDefinition.new(self, *args)
end

def create_alter_table(name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,10 @@ def remove_index_for_alter(table_name, options = {})
def add_timestamps_for_alter(table_name, options = {})
options[:null] = false if options[:null].nil?

if !options.key?(:precision) && supports_datetime_with_precision?
options[:precision] = 6
end

[add_column_for_alter(table_name, :created_at, :datetime, options), add_column_for_alter(table_name, :updated_at, :datetime, options)]
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def schema_creation
end

def create_table_definition(*args)
MySQL::TableDefinition.new(*args)
MySQL::TableDefinition.new(self, *args)
end

def new_column_from_field(table_name, field)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ def schema_creation
end

def create_table_definition(*args)
PostgreSQL::TableDefinition.new(*args)
PostgreSQL::TableDefinition.new(self, *args)
end

def create_alter_table(name)
Expand Down Expand Up @@ -718,6 +718,10 @@ def change_column_null_for_alter(table_name, column_name, null, default = nil)
def add_timestamps_for_alter(table_name, options = {})
options[:null] = false if options[:null].nil?

if !options.key?(:precision) && supports_datetime_with_precision?
options[:precision] = 6
end

[add_column_for_alter(table_name, :created_at, :datetime, options), add_column_for_alter(table_name, :updated_at, :datetime, options)]
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def schema_creation
end

def create_table_definition(*args)
SQLite3::TableDefinition.new(*args)
SQLite3::TableDefinition.new(self, *args)
end

def new_column_from_field(table_name, field)
Expand Down
95 changes: 47 additions & 48 deletions activerecord/lib/active_record/migration/compatibility.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,55 @@ def self.find(version)
V6_0 = Current

class V5_2 < V6_0
module TableDefinition
def timestamps(**options)
options[:precision] ||= nil
super
end
end

module CommandRecorder
def invert_transaction(args, &block)
[:transaction, args, block]
end
end

def create_table(table_name, **options)
if block_given?
super { |t| yield compatible_table_definition(t) }
else
super
end
end

def change_table(table_name, **options)
if block_given?
super { |t| yield compatible_table_definition(t) }
else
super
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these similar overrides?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is to inject V5_2::TableDefinition to the table definition instance.

https://github.com/rails/rails/pull/34970/files#diff-2a8be25f82da6b3935cc6a41300a1b01R19`

If the change_table is removed, the test_timestamps_doesnt_set_precision_on_change_table is failed.

https://github.com/rails/rails/pull/34970/files#diff-ec048630132cce280a9445022318906eR140

end

def create_join_table(table_1, table_2, **options)
if block_given?
super { |t| yield compatible_table_definition(t) }
else
super
end
end

def add_timestamps(table_name, **options)
options[:precision] ||= nil
super
end

private
def compatible_table_definition(t)
class << t
prepend TableDefinition
end
t
end

def command_recorder
recorder = super
Expand Down Expand Up @@ -87,35 +129,12 @@ def create_table(table_name, options = {})
options[:id] = :integer
end

if block_given?
super do |t|
yield compatible_table_definition(t)
end
else
super
end
end

def change_table(table_name, options = {})
if block_given?
super do |t|
yield compatible_table_definition(t)
end
else
super
end
super
end

def create_join_table(table_1, table_2, column_options: {}, **options)
column_options.reverse_merge!(type: :integer)

if block_given?
super do |t|
yield compatible_table_definition(t)
end
else
super
end
super
end

def add_column(table_name, column_name, type, options = {})
Expand All @@ -136,7 +155,7 @@ def compatible_table_definition(t)
class << t
prepend TableDefinition
end
t
super
end
end

Expand All @@ -154,33 +173,13 @@ def timestamps(**options)
end
end

def create_table(table_name, options = {})
if block_given?
super do |t|
yield compatible_table_definition(t)
end
else
super
end
end

def change_table(table_name, options = {})
if block_given?
super do |t|
yield compatible_table_definition(t)
end
else
super
end
end

def add_reference(*, **options)
def add_reference(table_name, ref_name, **options)
options[:index] ||= false
super
end
alias :add_belongs_to :add_reference

def add_timestamps(_, **options)
def add_timestamps(table_name, **options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this not cause warnings of unused arguments?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently Ruby doesn't raise any warnings of unused method arguments for now.

https://travis-ci.org/rails/rails/jobs/481187001

options[:null] = true if options[:null].nil?
super
end
Expand Down
51 changes: 51 additions & 0 deletions activerecord/test/cases/ar_schema_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -157,4 +157,55 @@ def test_timestamps_without_null_set_null_to_false_on_add_timestamps
assert @connection.column_exists?(:has_timestamps, :created_at, null: false)
assert @connection.column_exists?(:has_timestamps, :updated_at, null: false)
end

if subsecond_precision_supported?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be supports_datetime_with_precision??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supports_datetime_with_precision? can't be used in the ActiveRecord::TestCase scope.
subsecond_precision_supported? is defined in the top level as a helper.

https://github.com/rails/rails/blob/5fcf8e98808c59bc1496a4048e133a7b32512765/activerecord/test/cases/helper.rb#L43-L45

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

assert @connection.column_exists?(:has_timestamps, :created_at, precision: 6, null: false)
assert @connection.column_exists?(:has_timestamps, :updated_at, precision: 6, null: false)
end

def test_timestamps_sets_presicion_on_change_table
ActiveRecord::Schema.define do
create_table :has_timestamps

change_table :has_timestamps do |t|
t.timestamps default: Time.now
end
end

assert @connection.column_exists?(:has_timestamps, :created_at, precision: 6, null: false)
assert @connection.column_exists?(:has_timestamps, :updated_at, precision: 6, null: false)
end

if ActiveRecord::Base.connection.supports_bulk_alter?
def test_timestamps_sets_presicion_on_change_table_with_bulk
ActiveRecord::Schema.define do
create_table :has_timestamps

change_table :has_timestamps, bulk: true do |t|
t.timestamps default: Time.now
end
end

assert @connection.column_exists?(:has_timestamps, :created_at, precision: 6, null: false)
assert @connection.column_exists?(:has_timestamps, :updated_at, precision: 6, null: false)
end
end

def test_timestamps_sets_presicion_on_add_timestamps
ActiveRecord::Schema.define do
create_table :has_timestamps
add_timestamps :has_timestamps, default: Time.now
end

assert @connection.column_exists?(:has_timestamps, :created_at, precision: 6, null: false)
assert @connection.column_exists?(:has_timestamps, :updated_at, precision: 6, null: false)
end
end
end
Loading