Add incorrect Postgres timestamp type check #229
Conversation
|
|
||
| ActiveRecord::Base.establish_connection :primary | ||
|
|
||
| # Enable pgcrypto extension for PostgreSQL if needed (for gen_random_uuid) |
There was a problem hiding this comment.
Is this needed by the changes submitted in this PR? If not, let's remove this.
| *.gem | ||
| Gemfile.lock | ||
| gemfiles/**.lock | ||
| /.idea |
There was a problem hiding this comment.
This seems to be tool-specific, not project specific, and thus belongs to the system-wide or user-level gitignore, not a project .gitignore. Let's remove this entry.
|
|
||
| def test_timestamp_without_time_zone_is_error | ||
| Context.create_table(:events) do |t| | ||
| t.column :occurred_at, :timestamp, null: false |
There was a problem hiding this comment.
Is null: false needed here? If not, let's get rid of it as it's irrelevant to what's being tested.
|
|
||
| def test_timestamp_with_time_zone_is_ok | ||
| Context.create_table(:events) do |t| | ||
| t.timestamptz :occurred_at, null: false |
|
|
||
| def test_ignore_tables | ||
| Context.create_table(:events) do |t| | ||
| t.column :occurred_at, :timestamp, null: false |
|
|
||
| each_table(except: config(:ignore_tables)) do |table| | ||
| each_column(table, except: config(:ignore_columns)) do |column| | ||
| next unless timestamp_column?(column) |
There was a problem hiding this comment.
Is this method call necessary, given sql_type is checked below?
| check_rails_default_timezone | ||
| check_sql_adapter |
There was a problem hiding this comment.
We don't have config checkers now, so I suggest removing them for now. I'd like to think a bit more about how to make them work.
| class ActiveRecordDoctor::Detectors::IncorrectTimestampTypeTest < Minitest::Test | ||
| def setup | ||
| skip unless postgresql? | ||
| ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.datetime_type = :timestamptz |
There was a problem hiding this comment.
Could we remove that and use timestamp or timestamptz in all test cases below?
| # Simulate column.sql_type as 'timestamp without time zone' | ||
| ActiveRecord::Base.connection.change_column :events, :occurred_at, "timestamp without time zone" |
There was a problem hiding this comment.
Let's pass the right type when the column is being created.
| 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 |
There was a problem hiding this comment.
Let's remove configuration-related tests along with the corresponding production code.
2 participants
By default, even with Rails 8, a new Rails installation will not use timestamps
with timezoneby default. They are (arguably wrongly)without timezone. There is some discussion of that here: rails/rails#41084It is agreed in that issue that it is Postgres best practice to use timezones, and Rails only uses
without timezonedefaults for legacy reasons. rails/rails#53687eg, use these settings:
This is my first submission, I understand if they styling isn't perfect 👍 .
Also, this is the first check (?) that looks at Rails config, so I wasn't quite sure how to do that part.
Resolves: #228