diff options
49 files changed, 569 insertions, 3 deletions
diff --git a/changelogs/unreleased/32054-rails-should-use-timestamptz-database-type-for-postgresql.yml b/changelogs/unreleased/32054-rails-should-use-timestamptz-database-type-for-postgresql.yml new file mode 100644 index 00000000000..7fc9e0a4f0e --- /dev/null +++ b/changelogs/unreleased/32054-rails-should-use-timestamptz-database-type-for-postgresql.yml @@ -0,0 +1,4 @@ +--- +title: Add database helpers 'add_timestamps_with_timezone' and 'timestamps_with_timezone' +merge_request: 11229 +author: @blackst0ne diff --git a/config/initializers/active_record_data_types.rb b/config/initializers/active_record_data_types.rb new file mode 100644 index 00000000000..beb97c6fce0 --- /dev/null +++ b/config/initializers/active_record_data_types.rb @@ -0,0 +1,24 @@ +# ActiveRecord custom data type for storing datetimes with timezone information. +# See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11229 + +if Gitlab::Database.postgresql? + require 'active_record/connection_adapters/postgresql_adapter' + + module ActiveRecord + module ConnectionAdapters + class PostgreSQLAdapter + NATIVE_DATABASE_TYPES.merge!(datetime_with_timezone: { name: 'timestamptz' }) + end + end + end +elsif Gitlab::Database.mysql? + require 'active_record/connection_adapters/mysql2_adapter' + + module ActiveRecord + module ConnectionAdapters + class AbstractMysqlAdapter + NATIVE_DATABASE_TYPES.merge!(datetime_with_timezone: { name: 'timestamp' }) + end + end + end +end diff --git a/config/initializers/active_record_table_definition.rb b/config/initializers/active_record_table_definition.rb new file mode 100644 index 00000000000..4f59e35f4da --- /dev/null +++ b/config/initializers/active_record_table_definition.rb @@ -0,0 +1,24 @@ +# ActiveRecord custom method definitions with timezone information. +# See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11229 + +require 'active_record/connection_adapters/abstract/schema_definitions' + +# Appends columns `created_at` and `updated_at` to a table. +# +# It is used in table creation like: +# create_table 'users' do |t| +# t.timestamps_with_timezone +# end +module ActiveRecord + module ConnectionAdapters + class TableDefinition + def timestamps_with_timezone(**options) + options[:null] = false if options[:null].nil? + + [:created_at, :updated_at].each do |column_name| + column(column_name, :datetime_with_timezone, options) + end + end + end + end +end diff --git a/db/migrate/20160314114439_add_requested_at_to_members.rb b/db/migrate/20160314114439_add_requested_at_to_members.rb index 273819d4cd8..76c8b8a1a24 100644 --- a/db/migrate/20160314114439_add_requested_at_to_members.rb +++ b/db/migrate/20160314114439_add_requested_at_to_members.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class AddRequestedAtToMembers < ActiveRecord::Migration def change add_column :members, :requested_at, :datetime diff --git a/db/migrate/20160415062917_create_personal_access_tokens.rb b/db/migrate/20160415062917_create_personal_access_tokens.rb index ce0b33f32bd..c7b49870bf7 100644 --- a/db/migrate/20160415062917_create_personal_access_tokens.rb +++ b/db/migrate/20160415062917_create_personal_access_tokens.rb @@ -1,3 +1,5 @@ +# rubocop:disable Migration/Datetime +# rubocop:disable Migration/Timestamps class CreatePersonalAccessTokens < ActiveRecord::Migration def change create_table :personal_access_tokens do |t| diff --git a/db/migrate/20160610204157_add_deployments.rb b/db/migrate/20160610204157_add_deployments.rb index cb144ea8a6d..0e7e6e747a3 100644 --- a/db/migrate/20160610204157_add_deployments.rb +++ b/db/migrate/20160610204157_add_deployments.rb @@ -1,6 +1,7 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Datetime class AddDeployments < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160610204158_add_environments.rb b/db/migrate/20160610204158_add_environments.rb index e1c71d173c4..699cee2b246 100644 --- a/db/migrate/20160610204158_add_environments.rb +++ b/db/migrate/20160610204158_add_environments.rb @@ -1,6 +1,7 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Datetime class AddEnvironments < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb index f27295524e1..97aaaf9d2c8 100644 --- a/db/migrate/20160705054938_add_protected_branches_push_access.rb +++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb @@ -1,6 +1,7 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Timestamps class AddProtectedBranchesPushAccess < ActiveRecord::Migration DOWNTIME = false diff --git a/db/migrate/20160705054952_add_protected_branches_merge_access.rb b/db/migrate/20160705054952_add_protected_branches_merge_access.rb index 32adfa266cd..51a52a5ac17 100644 --- a/db/migrate/20160705054952_add_protected_branches_merge_access.rb +++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb @@ -1,6 +1,7 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Timestamps class AddProtectedBranchesMergeAccess < ActiveRecord::Migration DOWNTIME = false diff --git a/db/migrate/20160724205507_add_resolved_to_notes.rb b/db/migrate/20160724205507_add_resolved_to_notes.rb index b8ebcdbd156..3aca272a3f7 100644 --- a/db/migrate/20160724205507_add_resolved_to_notes.rb +++ b/db/migrate/20160724205507_add_resolved_to_notes.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class AddResolvedToNotes < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160727163552_create_user_agent_details.rb b/db/migrate/20160727163552_create_user_agent_details.rb index ed4ccfedc0a..3eb36f8464f 100644 --- a/db/migrate/20160727163552_create_user_agent_details.rb +++ b/db/migrate/20160727163552_create_user_agent_details.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateUserAgentDetails < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160727191041_create_boards.rb b/db/migrate/20160727191041_create_boards.rb index 56afbd4e030..9ec8df1b8e8 100644 --- a/db/migrate/20160727191041_create_boards.rb +++ b/db/migrate/20160727191041_create_boards.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateBoards < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160727193336_create_lists.rb b/db/migrate/20160727193336_create_lists.rb index 61d501215f2..3fd95dc8cfc 100644 --- a/db/migrate/20160727193336_create_lists.rb +++ b/db/migrate/20160727193336_create_lists.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateLists < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb index 30d98a0124e..404c253e18b 100644 --- a/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb +++ b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime # rubocop:disable RemoveIndex class AddDeletedAtToNamespaces < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160824124900_add_table_issue_metrics.rb b/db/migrate/20160824124900_add_table_issue_metrics.rb index e9bb79b3c62..30d35ef1db2 100644 --- a/db/migrate/20160824124900_add_table_issue_metrics.rb +++ b/db/migrate/20160824124900_add_table_issue_metrics.rb @@ -1,6 +1,8 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Datetime +# rubocop:disable Migration/Timestamps class AddTableIssueMetrics < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160825052008_add_table_merge_request_metrics.rb b/db/migrate/20160825052008_add_table_merge_request_metrics.rb index e01cc5038b9..56b39634dfd 100644 --- a/db/migrate/20160825052008_add_table_merge_request_metrics.rb +++ b/db/migrate/20160825052008_add_table_merge_request_metrics.rb @@ -1,6 +1,8 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Datetime +# rubocop:disable Migration/Timestamps class AddTableMergeRequestMetrics < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20160831214002_create_project_features.rb b/db/migrate/20160831214002_create_project_features.rb index 343953826f0..7ac6c8ec654 100644 --- a/db/migrate/20160831214002_create_project_features.rb +++ b/db/migrate/20160831214002_create_project_features.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateProjectFeatures < ActiveRecord::Migration DOWNTIME = false diff --git a/db/migrate/20160915042921_create_merge_requests_closing_issues.rb b/db/migrate/20160915042921_create_merge_requests_closing_issues.rb index 94874a853da..10c5604bb5c 100644 --- a/db/migrate/20160915042921_create_merge_requests_closing_issues.rb +++ b/db/migrate/20160915042921_create_merge_requests_closing_issues.rb @@ -1,6 +1,7 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Timestamps class CreateMergeRequestsClosingIssues < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20161014173530_create_label_priorities.rb b/db/migrate/20161014173530_create_label_priorities.rb index 2c22841c28a..28937c81e02 100644 --- a/db/migrate/20161014173530_create_label_priorities.rb +++ b/db/migrate/20161014173530_create_label_priorities.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateLabelPriorities < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20161113184239_create_user_chat_names_table.rb b/db/migrate/20161113184239_create_user_chat_names_table.rb index 97b597654f7..62ccb599f2e 100644 --- a/db/migrate/20161113184239_create_user_chat_names_table.rb +++ b/db/migrate/20161113184239_create_user_chat_names_table.rb @@ -1,3 +1,5 @@ +# rubocop:disable Migration/Datetime +# rubocop:disable Migration/Timestamps class CreateUserChatNamesTable < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20161124111402_add_routes_table.rb b/db/migrate/20161124111402_add_routes_table.rb index a02e046a18e..f5241d906d1 100644 --- a/db/migrate/20161124111402_add_routes_table.rb +++ b/db/migrate/20161124111402_add_routes_table.rb @@ -1,6 +1,7 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Timestamps class AddRoutesTable < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20161221152132_add_last_used_at_to_key.rb b/db/migrate/20161221152132_add_last_used_at_to_key.rb index fb2b15817de..86dc7870247 100644 --- a/db/migrate/20161221152132_add_last_used_at_to_key.rb +++ b/db/migrate/20161221152132_add_last_used_at_to_key.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class AddLastUsedAtToKey < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20161223034646_create_timelogs_ce.rb b/db/migrate/20161223034646_create_timelogs_ce.rb index 66d9cd823fb..1e894cc9161 100644 --- a/db/migrate/20161223034646_create_timelogs_ce.rb +++ b/db/migrate/20161223034646_create_timelogs_ce.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateTimelogsCe < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20161228124936_change_expires_at_to_date_in_personal_access_tokens.rb b/db/migrate/20161228124936_change_expires_at_to_date_in_personal_access_tokens.rb index af1bac897cc..16f7cc487ce 100644 --- a/db/migrate/20161228124936_change_expires_at_to_date_in_personal_access_tokens.rb +++ b/db/migrate/20161228124936_change_expires_at_to_date_in_personal_access_tokens.rb @@ -1,6 +1,7 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. +# rubocop:disable Migration/Datetime class ChangeExpiresAtToDateInPersonalAccessTokens < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170120131253_create_chat_teams.rb b/db/migrate/20170120131253_create_chat_teams.rb index 7995d383986..52208821911 100644 --- a/db/migrate/20170120131253_create_chat_teams.rb +++ b/db/migrate/20170120131253_create_chat_teams.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateChatTeams < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170130221926_create_uploads.rb b/db/migrate/20170130221926_create_uploads.rb index 6f06c5dd840..4d9fa0bb692 100644 --- a/db/migrate/20170130221926_create_uploads.rb +++ b/db/migrate/20170130221926_create_uploads.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class CreateUploads < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170222143317_drop_ci_projects.rb b/db/migrate/20170222143317_drop_ci_projects.rb index 4db8658f36f..9973e53501c 100644 --- a/db/migrate/20170222143317_drop_ci_projects.rb +++ b/db/migrate/20170222143317_drop_ci_projects.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class DropCiProjects < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb b/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb index 69dd15b8b4e..1a77d5934a3 100644 --- a/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb +++ b/db/migrate/20170301205639_remove_unused_ci_tables_and_columns.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class RemoveUnusedCiTablesAndColumns < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170309173138_create_protected_tags.rb b/db/migrate/20170309173138_create_protected_tags.rb index 796f3c90344..4684c9964c4 100644 --- a/db/migrate/20170309173138_create_protected_tags.rb +++ b/db/migrate/20170309173138_create_protected_tags.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateProtectedTags < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170314082049_create_system_note_metadata.rb b/db/migrate/20170314082049_create_system_note_metadata.rb index dd1e6cf8172..fee47e96053 100644 --- a/db/migrate/20170314082049_create_system_note_metadata.rb +++ b/db/migrate/20170314082049_create_system_note_metadata.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateSystemNoteMetadata < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170315194013_add_closed_at_to_issues.rb b/db/migrate/20170315194013_add_closed_at_to_issues.rb index 1326118cc8d..34a1bd7ca8c 100644 --- a/db/migrate/20170315194013_add_closed_at_to_issues.rb +++ b/db/migrate/20170315194013_add_closed_at_to_issues.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class AddClosedAtToIssues < ActiveRecord::Migration DOWNTIME = false diff --git a/db/migrate/20170322013926_create_container_repository.rb b/db/migrate/20170322013926_create_container_repository.rb index 91540bc88bd..242f7b8d17d 100644 --- a/db/migrate/20170322013926_create_container_repository.rb +++ b/db/migrate/20170322013926_create_container_repository.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateContainerRepository < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170329095907_create_ci_trigger_schedules.rb b/db/migrate/20170329095907_create_ci_trigger_schedules.rb index cfcfa27ebb5..06a2010db23 100644 --- a/db/migrate/20170329095907_create_ci_trigger_schedules.rb +++ b/db/migrate/20170329095907_create_ci_trigger_schedules.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class CreateCiTriggerSchedules < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170425112128_create_pipeline_schedules_table.rb b/db/migrate/20170425112128_create_pipeline_schedules_table.rb index 3612a796ae8..57df47f5f42 100644 --- a/db/migrate/20170425112128_create_pipeline_schedules_table.rb +++ b/db/migrate/20170425112128_create_pipeline_schedules_table.rb @@ -1,3 +1,5 @@ +# rubocop:disable Migration/Datetime +# rubocop:disable Migration/Timestamps class CreatePipelineSchedulesTable < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/migrate/20170427215854_create_redirect_routes.rb b/db/migrate/20170427215854_create_redirect_routes.rb index 2bf086b3e30..6db508e5db4 100644 --- a/db/migrate/20170427215854_create_redirect_routes.rb +++ b/db/migrate/20170427215854_create_redirect_routes.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateRedirectRoutes < ActiveRecord::Migration # Set this constant to true if this migration requires downtime. DOWNTIME = false diff --git a/db/migrate/20170503004125_add_last_repository_updated_at_to_projects.rb b/db/migrate/20170503004125_add_last_repository_updated_at_to_projects.rb index 00c685cf342..2ea49f62742 100644 --- a/db/migrate/20170503004125_add_last_repository_updated_at_to_projects.rb +++ b/db/migrate/20170503004125_add_last_repository_updated_at_to_projects.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class AddLastRepositoryUpdatedAtToProjects < ActiveRecord::Migration DOWNTIME = false diff --git a/db/migrate/20170523121229_create_conversational_development_index_metrics.rb b/db/migrate/20170523121229_create_conversational_development_index_metrics.rb index 9f9ec526055..7026a867ae1 100644 --- a/db/migrate/20170523121229_create_conversational_development_index_metrics.rb +++ b/db/migrate/20170523121229_create_conversational_development_index_metrics.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreateConversationalDevelopmentIndexMetrics < ActiveRecord::Migration DOWNTIME = false diff --git a/db/migrate/20170525132202_create_pipeline_stages.rb b/db/migrate/20170525132202_create_pipeline_stages.rb index 25656f2a2c2..825993aa41e 100644 --- a/db/migrate/20170525132202_create_pipeline_stages.rb +++ b/db/migrate/20170525132202_create_pipeline_stages.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Timestamps class CreatePipelineStages < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/db/post_migrate/20170425130047_drop_ci_trigger_schedules_table.rb b/db/post_migrate/20170425130047_drop_ci_trigger_schedules_table.rb index 24750c58ef0..159b533eaaa 100644 --- a/db/post_migrate/20170425130047_drop_ci_trigger_schedules_table.rb +++ b/db/post_migrate/20170425130047_drop_ci_trigger_schedules_table.rb @@ -1,3 +1,4 @@ +# rubocop:disable Migration/Datetime class DropCiTriggerSchedulesTable < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 77ba2a5fd87..161d2544169 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -122,7 +122,7 @@ limit can vary from installation to installation. As a result it's recommended you do not use more than 32 threads in a single migration. Usually 4-8 threads should be more than enough. -## Removing indices +## Removing indexes When removing an index make sure to use the method `remove_concurrent_index` instead of the regular `remove_index` method. The `remove_concurrent_index` method @@ -142,7 +142,7 @@ class MyMigration < ActiveRecord::Migration end ``` -## Adding indices +## Adding indexes If you need to add a unique index please keep in mind there is the possibility of existing duplicates being present in the database. This means that should @@ -222,6 +222,41 @@ add_column_with_default(:projects, :foo, :integer, default: 10, limit: 8) add_column(:projects, :foo, :integer, default: 10, limit: 8) ``` +## Timestamp column type + +By default, Rails uses the `timestamp` data type that stores timestamp data without timezone information. +The `timestamp` data type is used by calling either the `add_timestamps` or the `timestamps` method. +Also Rails converts the `:datetime` data type to the `timestamp` one. + +Example: + +```ruby +# timestamps +create_table :users do |t| + t.timestamps +end + +# add_timestamps +def up + add_timestamps :users +end + +# :datetime +def up + add_column :users, :last_sign_in, :datetime +end +``` + +Instead of using these methods one should use the following methods to store timestamps with timezones: + +* `add_timestamps_with_timezone` +* `timestamps_with_timezone` + +This ensures all timestamps have a time zone specified. This in turn means existing timestamps won't +suddenly use a different timezone when the system's timezone changes. It also makes it very clear which +timezone was used in the first place. + + ## Testing Make sure that your migration works with MySQL and PostgreSQL with data. An diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index a412bb6dbd2..cd85f961242 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -1,6 +1,39 @@ module Gitlab module Database module MigrationHelpers + # Adds `created_at` and `updated_at` columns with timezone information. + # + # This method is an improved version of Rails' built-in method `add_timestamps`. + # + # Available options are: + # default - The default value for the column. + # null - When set to `true` the column will allow NULL values. + # The default is to not allow NULL values. + def add_timestamps_with_timezone(table_name, options = {}) + options[:null] = false if options[:null].nil? + + [:created_at, :updated_at].each do |column_name| + if options[:default] && transaction_open? + raise '`add_timestamps_with_timezone` with default value cannot be run inside a transaction. ' \ + 'You can disable transactions by calling `disable_ddl_transaction!` ' \ + 'in the body of your migration class' + end + + # If default value is presented, use `add_column_with_default` method instead. + if options[:default] + add_column_with_default( + table_name, + column_name, + :datetime_with_timezone, + default: options[:default], + allow_null: options[:null] + ) + else + add_column(table_name, column_name, :datetime_with_timezone, options) + end + end + end + # Creates a new index, concurrently when supported # # On PostgreSQL this method creates an index concurrently, on MySQL this diff --git a/rubocop/cop/migration/add_timestamps.rb b/rubocop/cop/migration/add_timestamps.rb new file mode 100644 index 00000000000..08ddd91e54d --- /dev/null +++ b/rubocop/cop/migration/add_timestamps.rb @@ -0,0 +1,25 @@ +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that checks if 'add_timestamps' method is called with timezone information. + class AddTimestamps < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = 'Do not use `add_timestamps`, use `add_timestamps_with_timezone` instead'.freeze + + # Check methods. + def on_send(node) + return unless in_migration?(node) + + add_offense(node, :selector) if method_name(node) == :add_timestamps + end + + def method_name(node) + node.children[1] + end + end + end + end +end diff --git a/rubocop/cop/migration/datetime.rb b/rubocop/cop/migration/datetime.rb new file mode 100644 index 00000000000..651935dd53e --- /dev/null +++ b/rubocop/cop/migration/datetime.rb @@ -0,0 +1,36 @@ +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that checks if datetime data type is added with timezone information. + class Datetime < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = 'Do not use the `datetime` data type, use `datetime_with_timezone` instead'.freeze + + # Check methods in table creation. + def on_def(node) + return unless in_migration?(node) + + node.each_descendant(:send) do |send_node| + add_offense(send_node, :selector) if method_name(send_node) == :datetime + end + end + + # Check methods. + def on_send(node) + return unless in_migration?(node) + + node.each_descendant do |descendant| + add_offense(node, :expression) if descendant.type == :sym && descendant.children.last == :datetime + end + end + + def method_name(node) + node.children[1] + end + end + end + end +end diff --git a/rubocop/cop/migration/timestamps.rb b/rubocop/cop/migration/timestamps.rb new file mode 100644 index 00000000000..71a9420cc3b --- /dev/null +++ b/rubocop/cop/migration/timestamps.rb @@ -0,0 +1,27 @@ +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # Cop that checks if 'timestamps' method is called with timezone information. + class Timestamps < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = 'Do not use `timestamps`, use `timestamps_with_timezone` instead'.freeze + + # Check methods in table creation. + def on_def(node) + return unless in_migration?(node) + + node.each_descendant(:send) do |send_node| + add_offense(send_node, :selector) if method_name(send_node) == :timestamps + end + end + + def method_name(node) + node.children[1] + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index dae30969abf..6b8d127dde6 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -8,7 +8,10 @@ require_relative 'cop/migration/add_column_with_default_to_large_table' require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_index' require_relative 'cop/migration/add_index' +require_relative 'cop/migration/add_timestamps' +require_relative 'cop/migration/datetime' require_relative 'cop/migration/remove_concurrent_index' require_relative 'cop/migration/remove_index' require_relative 'cop/migration/reversible_add_column_with_default' +require_relative 'cop/migration/timestamps' require_relative 'cop/migration/update_column_in_batches' diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 3fdafd867da..30aa463faf8 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -7,7 +7,42 @@ describe Gitlab::Database::MigrationHelpers, lib: true do ) end - before { allow(model).to receive(:puts) } + before do + allow(model).to receive(:puts) + end + + describe '#add_timestamps_with_timezone' do + before do + allow(model).to receive(:transaction_open?).and_return(false) + end + + context 'using PostgreSQL' do + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + allow(model).to receive(:disable_statement_timeout) + end + + it 'adds "created_at" and "updated_at" fields with the "datetime_with_timezone" data type' do + expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, { null: false }) + expect(model).to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, { null: false }) + + model.add_timestamps_with_timezone(:foo) + end + end + + context 'using MySQL' do + before do + allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + + it 'adds "created_at" and "updated_at" fields with "datetime_with_timezone" data type' do + expect(model).to receive(:add_column).with(:foo, :created_at, :datetime_with_timezone, { null: false }) + expect(model).to receive(:add_column).with(:foo, :updated_at, :datetime_with_timezone, { null: false }) + + model.add_timestamps_with_timezone(:foo) + end + end + end describe '#add_concurrent_index' do context 'outside a transaction' do diff --git a/spec/rubocop/cop/migration/add_timestamps_spec.rb b/spec/rubocop/cop/migration/add_timestamps_spec.rb new file mode 100644 index 00000000000..18df62dec3e --- /dev/null +++ b/spec/rubocop/cop/migration/add_timestamps_spec.rb @@ -0,0 +1,90 @@ +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/migration/add_timestamps' + +describe RuboCop::Cop::Migration::AddTimestamps do + include CopHelper + + subject(:cop) { described_class.new } + let(:migration_with_add_timestamps) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column(:users, :username, :text) + add_timestamps(:users) + end + end + ) + end + + let(:migration_without_add_timestamps) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column(:users, :username, :text) + end + end + ) + end + + let(:migration_with_add_timestamps_with_timezone) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column(:users, :username, :text) + add_timestamps_with_timezone(:users) + end + end + ) + end + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + it 'registers an offense when the "add_timestamps" method is used' do + inspect_source(cop, migration_with_add_timestamps) + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([7]) + end + end + + it 'does not register an offense when the "add_timestamps" method is not used' do + inspect_source(cop, migration_without_add_timestamps) + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + + it 'does not register an offense when the "add_timestamps_with_timezone" method is used' do + inspect_source(cop, migration_with_add_timestamps_with_timezone) + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + end + + context 'outside of migration' do + it 'registers no offense' do + inspect_source(cop, migration_with_add_timestamps) + inspect_source(cop, migration_without_add_timestamps) + inspect_source(cop, migration_with_add_timestamps_with_timezone) + + expect(cop.offenses.size).to eq(0) + end + end +end diff --git a/spec/rubocop/cop/migration/datetime_spec.rb b/spec/rubocop/cop/migration/datetime_spec.rb new file mode 100644 index 00000000000..388b086ce6a --- /dev/null +++ b/spec/rubocop/cop/migration/datetime_spec.rb @@ -0,0 +1,90 @@ +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/migration/datetime' + +describe RuboCop::Cop::Migration::Datetime do + include CopHelper + + subject(:cop) { described_class.new } + let(:migration_with_datetime) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column(:users, :username, :text) + add_column(:users, :last_sign_in, :datetime) + end + end + ) + end + + let(:migration_without_datetime) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column(:users, :username, :text) + end + end + ) + end + + let(:migration_with_datetime_with_timezone) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column(:users, :username, :text) + add_column(:users, :last_sign_in, :datetime_with_timezone) + end + end + ) + end + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + it 'registers an offense when the ":datetime" data type is used' do + inspect_source(cop, migration_with_datetime) + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([7]) + end + end + + it 'does not register an offense when the ":datetime" data type is not used' do + inspect_source(cop, migration_without_datetime) + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + + it 'does not register an offense when the ":datetime_with_timezone" data type is used' do + inspect_source(cop, migration_with_datetime_with_timezone) + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + end + + context 'outside of migration' do + it 'registers no offense' do + inspect_source(cop, migration_with_datetime) + inspect_source(cop, migration_without_datetime) + inspect_source(cop, migration_with_datetime_with_timezone) + + expect(cop.offenses.size).to eq(0) + end + end +end diff --git a/spec/rubocop/cop/migration/timestamps_spec.rb b/spec/rubocop/cop/migration/timestamps_spec.rb new file mode 100644 index 00000000000..cdf1423d0c5 --- /dev/null +++ b/spec/rubocop/cop/migration/timestamps_spec.rb @@ -0,0 +1,99 @@ +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/migration/timestamps' + +describe RuboCop::Cop::Migration::Timestamps do + include CopHelper + + subject(:cop) { described_class.new } + let(:migration_with_timestamps) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + create_table :users do |t| + t.string :username, null: false + t.timestamps null: true + t.string :password + end + end + end + ) + end + + let(:migration_without_timestamps) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + create_table :users do |t| + t.string :username, null: false + t.string :password + end + end + end + ) + end + + let(:migration_with_timestamps_with_timezone) do + %q( + class Users < ActiveRecord::Migration + DOWNTIME = false + + def change + create_table :users do |t| + t.string :username, null: false + t.timestamps_with_timezone null: true + t.string :password + end + end + end + ) + end + + context 'in migration' do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + it 'registers an offense when the "timestamps" method is used' do + inspect_source(cop, migration_with_timestamps) + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([8]) + end + end + + it 'does not register an offense when the "timestamps" method is not used' do + inspect_source(cop, migration_without_timestamps) + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + + it 'does not register an offense when the "timestamps_with_timezone" method is used' do + inspect_source(cop, migration_with_timestamps_with_timezone) + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + end + + context 'outside of migration' do + it 'registers no offense' do + inspect_source(cop, migration_with_timestamps) + inspect_source(cop, migration_without_timestamps) + inspect_source(cop, migration_with_timestamps_with_timezone) + + expect(cop.offenses.size).to eq(0) + end + end +end |