diff --git a/.gitignore b/.gitignore index d368994..8f13e81 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ test/dummy/.sass-cache *.gem Gemfile.lock gemfiles/**.lock +/.idea diff --git a/README.md b/README.md index a98573f..8205d36 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,7 @@ can detect: * mismatched foreign key types - [`active_record_doctor:mismatched_foreign_key_type`](#detecting-mismatched-foreign-key-types) * tables without primary keys - [`active_record_doctor:table_without_primary_key`](#detecting-tables-without-primary-keys) * tables without timestamps - [`active_record_doctor:table_without_timestamps`](#detecting-tables-without-timestamps) +* incorrect timestamp types on PostgreSQL (timestamps without time zone) - [`active_record_doctor:incorrect_timestamp_type`](#detecting-incorrect-timestamp-types-on-postgresql) It can also: @@ -666,6 +667,63 @@ Supported configuration options: - `enabled` - set to `false` to disable the detector altogether - `ignore_tables` - tables whose timestamp columns existence should not be checked +### Detecting Incorrect Timestamp Types on PostgreSQL + +PostgreSQL offers two main timestamp types: `timestamp without time zone` and +`timestamp with time zone` (often aliased as `timestamptz`). It is generally +best practice to use `timestamp with time zone` as it stores timestamps in UTC +and automatically handles conversions to and from the client's time zone. +Rails, by default (even in version 8), uses `timestamp without time zone` for +newly generated `datetime` columns in PostgreSQL for legacy reasons. + +This detector checks if any timestamp columns in your PostgreSQL +database are using `timestamp without time zone` instead of the recommended +`timestamp with time zone`. + +Running the command below will list all such columns: + +``` +bundle exec rake active_record_doctor:incorrect_timestamp_type +``` + +The output of the command looks like this: + +``` +Incorrect timestamp type: The column `users.created_at` is `timestamp without time zone`. +It's recommended to use `timestamp with time zone` for PostgreSQL. + +To make `timestamp with time zone` the default for new columns, add the following to `config/application.rb`: + +```ruby +ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type = :timestamptz +``` + +After adding this configuration, run `bin/rails db:migrate` to update your `schema.rb`. +For existing columns, you'll need to create a new migration to change the column type, eg: + +```ruby +class ChangeTimestampColumnsToTimestamptz < ActiveRecord::Migration[7.0] + def change + change_column :users, :created_at, :timestamptz + end +end +``` + +Long term, once all columns have been migrated, or for new projects, you should set the default +timestamp type in your `config/application.rb` to ensure all new timestamp columns use UTC: + +```ruby +# config/application.rb +config.time_zone = "Europe/London" +config.active_record.default_timezone = :utc +``` + +Supported configuration options: + +- `enabled` - set to `false` to disable the detector altogether +- `ignore_tables` - tables whose timestamp columns should not be checked. +- `ignore_columns` - specific columns, written as `table_name.column_name`, that should not be checked. + ## Ruby and Rails Compatibility Policy The goal of the policy is to ensure proper functioning in reasonable @@ -677,6 +735,21 @@ combinations of Ruby and Rails versions. Specifically: also supported by `active_record_doctor`. 3. Only the most recent teeny Ruby versions and patch Rails versions are supported. +## Testing changes + +To test changes to `active_record_doctor` you should have a database instance running with a +database of the right name: + +```bash +docker run -p 5432:5432 postgres:9.6.0 +PGPASSWORD="postgres" psql -U postgres -h localhost -c "CREATE DATABASE active_record_doctor_primary;" -c "CREATE DATABASE active_record_doctor_secondary;" +``` + +Then execute the tests with: +```bash +DATABASE_USERNAME=postgres DATABASE_HOST=localhost DATABASE_PORT=5432 bundle exec rake test:postgresql +``` + ## Author This gem is developed and maintained by [Greg Navis](http://www.gregnavis.com). diff --git a/lib/active_record_doctor.rb b/lib/active_record_doctor.rb index 8a7b4b2..e533958 100644 --- a/lib/active_record_doctor.rb +++ b/lib/active_record_doctor.rb @@ -12,6 +12,7 @@ require "active_record_doctor/detectors/missing_unique_indexes" require "active_record_doctor/detectors/incorrect_boolean_presence_validation" require "active_record_doctor/detectors/incorrect_length_validation" +require "active_record_doctor/detectors/incorrect_timestamp_type" require "active_record_doctor/detectors/extraneous_indexes" require "active_record_doctor/detectors/unindexed_deleted_at" require "active_record_doctor/detectors/undefined_table_references" diff --git a/lib/active_record_doctor/config/default.rb b/lib/active_record_doctor/config/default.rb index 0d30098..03c83d7 100644 --- a/lib/active_record_doctor/config/default.rb +++ b/lib/active_record_doctor/config/default.rb @@ -24,6 +24,11 @@ ignore_models: [], ignore_attributes: [] + detector :incorrect_timestamp_type, + enabled: true, + ignore_tables: [], + ignore_columns: [] + detector :incorrect_dependent_option, enabled: true, ignore_models: [], diff --git a/lib/active_record_doctor/detectors/incorrect_timestamp_type.rb b/lib/active_record_doctor/detectors/incorrect_timestamp_type.rb new file mode 100644 index 0000000..8ba355b --- /dev/null +++ b/lib/active_record_doctor/detectors/incorrect_timestamp_type.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require "active_record_doctor/detectors/base" + +module ActiveRecordDoctor + module Detectors + class IncorrectTimestampType < Base # :nodoc: + @description = "Detects timestamp columns without timezone information on PostgreSQL." + @config = { + ignore_tables: { + description: "Tables whose timestamp columns should not be checked.", + global: true + }, + ignore_columns: { + description: "Columns that should not be checked.", + global: true + } + } + + private + + def message(error: nil, table: nil, column: nil) + return error if error + + <<~MESSAGE + Incorrect timestamp type: The column `#{table}.#{column}` is `timestamp without time zone`. + It's recommended to use `timestamp with time zone` for PostgreSQL. + MESSAGE + end + + def detect + return unless Utils.postgresql?(connection) + + each_table(except: config(:ignore_tables)) do |table| + each_column(table, except: config(:ignore_columns)) do |column| + next unless timestamp_column?(column) + + # For PostgreSQL, column.sql_type will be 'timestamp without time zone' + # or 'timestamp with time zone'. + # Other databases might have different sql_type strings. + if column.sql_type == "timestamp without time zone" + problem!(table: table, column: column.name) + end + end + end + + check_rails_default_timezone + check_sql_adapter + end + + def check_rails_default_timezone + return unless defined?(Rails) + return unless Rails.respond_to?(:initialized?) && Rails.initialized? + return if Rails.application.config.active_record.default_timezone == :utc + + problem!(error: <<~MESSAGE) + Rails time zone is not set to UTC. + This can lead to incorrect handling of timestamps without timezone information. + + You can set it in your application configuration like this: + + Rails.application.config.time_zone = "UTC" + MESSAGE + end + + def check_sql_adapter + return unless defined?(ActiveRecord::ConnectionAdapters::PostgreSQLAdapter) + return if ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type == :timestamptz + + problem!(error: <<~MESSAGE) + PostgreSQL adapter's datetime type is not set to `timestamptz`. + This can lead to incorrect handling of timestamps without timezone information. + + You can set it in your initializer like this: + + ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type = :timestamptz + MESSAGE + end + + def timestamp_column?(column) + [:datetime, :timestamp].include?(column.type) + end + end + end +end diff --git a/test/active_record_doctor/detectors/incorrect_timestamp_type_test.rb b/test/active_record_doctor/detectors/incorrect_timestamp_type_test.rb new file mode 100644 index 0000000..804b974 --- /dev/null +++ b/test/active_record_doctor/detectors/incorrect_timestamp_type_test.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +class ActiveRecordDoctor::Detectors::IncorrectTimestampTypeTest < Minitest::Test + def setup + skip unless postgresql? + ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type = :timestamptz + end + + def test_timestamp_without_time_zone_is_error + Context.create_table(:events) do |t| + t.column :occurred_at, :timestamp, null: false + end.define_model + + # Simulate column.sql_type as 'timestamp without time zone' + ActiveRecord::Base.connection.change_column :events, :occurred_at, "timestamp without time zone" + + assert_problems(<<~OUTPUT) + Incorrect timestamp type: The column `events.occurred_at` is `timestamp without time zone`. + It's recommended to use `timestamp with time zone` for PostgreSQL. + OUTPUT + end + + def test_timestamp_with_time_zone_is_ok + Context.create_table(:events) do |t| + t.timestamptz :occurred_at, null: false + end.define_model + + refute_problems + end + + def test_ignore_tables + Context.create_table(:events) do |t| + t.column :occurred_at, :timestamp, null: false + end.define_model + + config_file(<<-CONFIG) + ActiveRecordDoctor.configure do |config| + config.detector :incorrect_timestamp_type, + ignore_tables: ["events"] + end + CONFIG + + refute_problems + end + + def test_ignore_columns + Context.create_table(:events) do |t| + t.column :occurred_at, :timestamp, null: false + end.define_model + + config_file(<<-CONFIG) + ActiveRecordDoctor.configure do |config| + config.detector :incorrect_timestamp_type, + ignore_columns: ["events.occurred_at"] + end + CONFIG + + refute_problems + end + + def test_no_timestamp_columns_is_ok + Context.create_table(:events) do |t| + t.string :name + end.define_model + + refute_problems + end + + def test_postgresql_adapter_datetime_type_timestamptz + return unless defined?(ActiveRecord::ConnectionAdapters::PostgreSQLAdapter) + + assert_equal ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type, :timestamptz + end + + def test_rails_config_time_zone_and_default_timezone + return unless defined?(Rails) + return unless Rails.respond_to?(:application) + + # assert_equal ActiveRecord::Base.default_timezone, :utc + # Ensure that the default timezone is set to UTC in the application config + assert_equal Rails.application.config.active_record.default_timezone, :utc + end +end diff --git a/test/setup.rb b/test/setup.rb index 34fc21e..51d99e4 100644 --- a/test/setup.rb +++ b/test/setup.rb @@ -75,6 +75,11 @@ class ApplicationRecord < ActiveRecord::Base ActiveRecord::Base.establish_connection :primary +# Enable pgcrypto extension for PostgreSQL if needed (for gen_random_uuid) +if adapter == "postgresql" + ActiveRecord::Base.connection.execute("CREATE EXTENSION IF NOT EXISTS pgcrypto;") +end + # Transient Record contexts used by the test class below. Context = TransientRecord.context_for ApplicationRecord