Backport #25364 to 4-2-stable

[kamipo]

This PR is backport #25364 to 4-2-stable.

Currently Type::Date#serialize does not cast a value to a date object.
It should be cast to a date object for finding by date column correctly
working.

Fixes #25354.

r? @rafaelfranca

[rails-bot]

Warning

• Pull requests are usually filed against the master branch for this repo, but this one is against 4-2-stable. Please double check that you specified the right target!

[rafaelfranca]

Tests are failing with sqlite3, but I don't think it is related to this. I'll investigate.

[rafaelfranca]

Yes, seems it is related.

[kamipo]

Failing test was removed at 19537ec on master.
Can we remove the test on 4-2-stable?

[rafaelfranca]

Weird. But they are passing in 4-2-stable. Why they are broken in your branch?

[kamipo]

Actually this assert is unexpected behavior, I think.
Previously type casting against Type::Date was not worked.

[rafaelfranca]

Oh, now I understand it. So those tests still make sense because the column information is still being used differently from 5.0, but this assertion needs to be changed or removed

[kamipo]

Changed this assertion to expected behavior!

[tmertens]

@kamipo I'm not sure this is sufficient to fix the underlying problem. I tried this same solution last night (as module included onto ActiveRecord::Type::Date to monkey patch it), and I am seeing this method called in some cases, such as when persisting records; however, when calling where on the model, it is still coercing to a UTC date time string. Here is an example (rspec) test:

describe 'querying dates with times works around day boundaries' do
    let(:model_class) do
      model_class = Class.new(ActiveRecord::Base)
      model_class.table_name = 'date_serialization_test'
      model_class
    end

    let!(:old_record)       { model_class.create!(a_date_field: Date.new(2016, 6, 8)) }
    let!(:boundary_record)  { model_class.create!(a_date_field: Date.new(2016, 6, 9)) }
    let!(:newer_record)     { model_class.create!(a_date_field: Date.new(2016, 6, 10)) }

    before(:context) do
      ActiveRecord::Migration.new.create_table(:date_serialization_test) do |t|
        t.date :a_date_field
      end
    end

    after(:context) do
      ActiveRecord::Migration.new.drop_table(:date_serialization_test)
    end

    it 'coerces TimeWithZone to Date in current locale' do
      date = Time.new(2016, 6, 9, 23, 0, 0, '-05:00').in_time_zone
      byebug
      query = model_class.where('a_date_field >= ?', date)

      expect(query).to include boundary_record, newer_record
      expect(query).not_to include old_record
    end

    it 'coerces Time to Date in current locale' do
      date = Time.new(2016, 6, 9, 23, 0, 0, '-05:00')
      query = model_class.where('a_date_field >= ?', date)

      expect(query).to include boundary_record, newer_record
      expect(query).not_to include old_record
    end

    it 'coerces DateTime to Date in current locale' do
      date = DateTime.new(2016, 6, 9, 23, 0, 0, '-5')
      query = model_class.where('a_date_field >= ?', date)
      binding.pry
      expect(query).to include boundary_record, newer_record
      expect(query).not_to include old_record
    end

    it 'queries still work correctly using a Date object' do
      date = Date.new(2016, 6, 9)
      query = model_class.where('a_date_field >= ?', date)

      expect(query).to include boundary_record, newer_record
      expect(query).not_to include old_record
    end
  end

I suspect that because the where clause uses an SQL string, it may not be able to determine the column type? I am finding that my issue may not be quite the same as the case this and the relate rails 5 PR solves.

[kamipo]

I suspect that because the where clause uses an SQL string, it may not be able to determine the column type?

Right, AR cannot determine the column type if using where with an SQL string for now.

[tmertens]

Yes, after further investigation I determined we need to just type cast the values ourselves. Thanks @kamipo