Skip to content
This repository has been archived by the owner on Aug 7, 2018. It is now read-only.

Support rails5.1 and rails 5.2 #7

Open
wants to merge 2 commits into
base: appfolio
Choose a base branch
from
Open

Support rails5.1 and rails 5.2 #7

wants to merge 2 commits into from

Conversation

pololee
Copy link

@pololee pololee commented Jul 12, 2018

fix active_record.time_zone_aware_types error happen in 5.1 and 5.2

DEPRECATION WARNING: Time columns will become time zone aware in Rails 5.1. This still causes `String`s to be parsed as if they were in `Time.zone`, and `Time`s to be converted to `Time.zone`.

To keep the old behavior, you must add the following to your initializer:

    config.active_record.time_zone_aware_types = [:datetime]

To silence this deprecation warning, add the following:

    config.active_record.time_zone_aware_types = [:datetime, :time]

What does this even mean? I found an article explain it well http://engineering.liefery.com/2017/10/25/times-in-rails-5.html
Here is a quick summary:

Here’s a good way of thinking about it: which columns in your application should return an ActiveSupport::TimeWithZone object?

To keep the old behaviour and only have datetime columns return an ActiveSupport::TimeWithZone object, use [:datetime].
To use the new behaviour and have time columns also return an ActiveSupport::TimeWithZone object, use [:datetime, :time].

Here is where I found how to config it without Rails.config http://api.rubyonrails.org/classes/ActiveRecord/Timestamp.html

As the testcases suggest, it expects the old behaviour, i.e. only datetime is time zone aware.

context 'for column attribute' do
it 'should be detected from column type' do
expect(klass.timeliness_attribute_timezone_aware?(:birth_date)).to be_falsey
expect(klass.timeliness_attribute_timezone_aware?(:birth_time)).to be_falsey
expect(klass.timeliness_attribute_timezone_aware?(:birth_datetime)).to be_truthy
end
end
context 'for non-column attribute' do
it 'should be detected from the validation type' do
expect(klass.timeliness_attribute_timezone_aware?(:some_date)).to be_falsey
expect(klass.timeliness_attribute_timezone_aware?(:some_time)).to be_falsey
expect(klass.timeliness_attribute_timezone_aware?(:some_datetime)).to be_truthy
end
end
end

I changed spec_helper.rb to set time_zone_aware_types, but I don't know how to enforce it in a Rails app. I am not familiar with gem and I am afraid this may break sth 😞

Polo added 2 commits July 9, 2018 15:07
Hello! The sqlite3-ruby gem has changed it's name to just sqlite3.  Rather than
installing `sqlite3-ruby`, you should install `sqlite3`.  Please update your
dependencies accordingly.

Thanks from the Ruby sqlite3 team!

<3 <3 <3 <3
- change gemspec
- appraisal
- generate gemfiles

---

- fix `active_record.time_zone_aware_types` error happen in 5.1 and 5.2

```
DEPRECATION WARNING: Time columns will become time zone aware in Rails 5.1. This still causes `String`s to be parsed as if they were in `Time.zone`, and `Time`s to be converted to `Time.zone`.

To keep the old behavior, you must add the following to your initializer:

    config.active_record.time_zone_aware_types = [:datetime]

To silence this deprecation warning, add the following:

    config.active_record.time_zone_aware_types = [:datetime, :time]
```

What does this even mean? I found an article explain it well http://engineering.liefery.com/2017/10/25/times-in-rails-5.html
Here is a quick summary:

```
Here’s a good way of thinking about it: which columns in your application should return an ActiveSupport::TimeWithZone object?

To keep the old behaviour and only have datetime columns return an ActiveSupport::TimeWithZone object, use [:datetime].
To use the new behaviour and have time columns also return an ActiveSupport::TimeWithZone object, use [:datetime, :time].
```
Here is where I found how to config it without `Rails.config` http://api.rubyonrails.org/classes/ActiveRecord/Timestamp.html

As the testcases suggest, it expects the old behaviour, i.e. only `datetime` is time zone aware.
https://github.com/appfolio/validates_timeliness/blob/4c80b403e500f36ca3d0d93cf312c0a30828b71c/spec/validates_timeliness/orm/active_record_spec.rb#L90-L105

I changed `spec_helper.rb` to set `time_zone_aware_types`, but I don't know how to enforce it in a Rails app.

---

In `lib/validates_timeliness/orm/active_record.rb`, the `timeliness_column_for_attribute` function depends on the internal implementation of ActiveRecord.
https://github.com/appfolio/validates_timeliness/blob/4c80b403e500f36ca3d0d93cf312c0a30828b71c/lib/validates_timeliness/orm/active_record.rb#L17-L33

It gave me a really hard time to figure out how to fix it. I read the active_record source and here is what I found. I put them here as the reference.
I didn't dig into the details why this funtion has to depend on the internal implementation of ActiveRecord. It's out of the scope of this upgrade.

I got the error when I run the test:

```
     NoMethodError:
       undefined method `new_column' for #<ActiveRecord::ConnectionAdapters::SQLite3Adapter:0x007fd0f11d4ad8>
     # ./lib/validates_timeliness/orm/active_record.rb:32:in `block in timeliness_column_for_attribute'
     # ./lib/validates_timeliness/orm/active_record.rb:18:in `fetch'
     # ./lib/validates_timeliness/orm/active_record.rb:18:in `timeliness_column_for_attribute'
     # ./lib/validates_timeliness/orm/active_record.rb:10:in `timeliness_attribute_timezone_aware?'
     # ./spec/validates_timeliness/orm/active_record_spec.rb:100:in `block (4 levels) in <top (required)>'
```

- rails https://github.com/rails/rails/releases/tag/v4.2.0.beta1

commit: introduce `new_column` method rails/rails@98a7dde

- rails https://github.com/rails/rails/releases/tag/v5.1.0.beta1

commit: refactor column initialization into `new_column_from_field `
rails/rails@3dce96a

```
abstract_mysql_adapter has `new_column`
https://github.com/rails/rails/blob/3dce96a6b1242452860e3a9560ccfb357cede0b1/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L172
postgresql doesn’t have `new_column` anymore
https://github.com/rails/rails/commit/3dce96a6b1242452860e3a9560ccfb357cede0b1#diff-6e377cf054cb4121e6f207bd7c14b492
sqlite3 just use base class `AbstractAdapter #new_column`
https://github.com/rails/rails/blob/3dce96a6b1242452860e3a9560ccfb357cede0b1/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb#L328
```

- rails https://github.com/rails/rails/releases/tag/v5.2.0.beta1

commit: Make internal methods to private
rails/rails@e4108fc

```
abstract_adapter doesn’t have `new_column` and `lookup_case_type` methods anymore
rails/rails@e4108fc#diff-c226a4680f86689c3c170d4bc5911e96L478

in connection_adapters/abstract/schema_statements.rb, it called `new_column_from_field`, but doesn't have definition for it
https://github.com/rails/rails/blob/e4108fc619e0f1c28cdec6049d31f2db01d56dfd/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L108-L114

mysql_adapter has `new_column_from_field` in its schema_statements file
https://github.com/rails/rails/blob/e4108fc619e0f1c28cdec6049d31f2db01d56dfd/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb#L14

sqlite3_adapter has `new_column_from_field` in its schema_statements file
rails/rails@e4108fc#diff-cf85d787d5dfda96e016b6121426460cR14

postgresql_adapter has `new_column_from_field` in its schema_statements file
rails/rails@e4108fc#diff-6e377cf054cb4121e6f207bd7c14b492R578

The `new_column_from_field` method in the above are private method
```

- rails 4.2.10
lookup_cast_type https://github.com/rails/rails/blob/v4.2.10/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L475

- rails 5.0
fetch_type_metadata https://github.com/rails/rails/blob/v5.0.0.beta1/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L857

- rails 5.1
fetch_type_metadata https://github.com/rails/rails/blob/v5.1.0/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L653

- rails 5.2
fetch_type_metadata https://github.com/rails/rails/blob/v5.2.0/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L1295
@pololee
Copy link
Author

pololee commented Jul 12, 2018

@coveralls
Copy link

coveralls commented Jul 12, 2018

Coverage Status

Coverage decreased (-0.4%) to 97.619% when pulling 089fe28 on supportRails52 into 4c80b40 on appfolio.

@pololee pololee requested a review from pkmiec July 12, 2018 20:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants