faker-ruby / faker

A library for generating fake data such as names, addresses, and phone numbers.
MIT License
11.22k stars 3.18k forks source link

Date.today causes timezone issues in Rails. #2093

Closed schwern closed 1 year ago

schwern commented 4 years ago

Describe the bug

Any Faker method which uses Date.today in Ruby On Rails is potentially off by a day. There are similar potential issues for Time.now and DateTime.now.

To Reproduce

In Rails console, set a time zone which is in a different day. For example, right now it is July 27th in my local time zone, but July 28th in UTC. Date.today will return July 27th (local date), but Date.current will return July 28th (application's date).

[47] pry(main)> Time.zone = "UTC"
=> "UTC"
[53] pry(main)> Date.today
=> Mon, 27 Jul 2020
[54] pry(main)> Date.current
=> Tue, 28 Jul 2020

Now functions like Faker::Date.forward will be off by one with respect to the application's time zone.

[60] pry(main)> Faker::Date.forward(days: 1)
=> Tue, 28 Jul 2020

Expected behavior

I expect Faker::Date.forward(days: 1) to return Wed, 29 Jul 2020 which is 1 day forward in the application's time zone.

Additional context

This has been causing mysterious edge-case problems when using Faker data to test Rails validations. For example, Faker::Date.birthday(min_age: 15, max_age: 18) will occassionally fall outside the validation range of on_or_before: -> { 15.years.ago } causing a false test failure.

See https://thoughtbot.com/blog/its-about-time-zones for more detail about which Time and Date methods are safe under Rails.

Perhaps a config option could be added to have Faker use Rails-safe methods such as Date.current and Time.current. Or it could auto-detect a Rails environment.

Selectus2 commented 3 years ago

If I make the changes as mentioned in the resource given above i.e. if I change Date.today to Date.current will you accept my PR

robinator commented 1 year ago

I'm wondering why this was marked as completed when it's still an issue with the gem? Is there workaround for rails users or something I should be doing instead? I am also happy to put a patch up if that's better.

thdaraujo commented 1 year ago

If I make the changes as mentioned in the resource given above i.e. if I change Date.today to Date.current will you accept my PR (@Selectus2)

I think this is not the way to go because we don't assume you have Rails installed.

But we do use the to_date ActiveSupport method when available here: https://github.com/faker-ruby/faker/blob/82d7a86832601508932dcb727b4965afc5b9b6a5/lib/faker/default/date.rb#L148

And .to_date should take into consideration the timezone.

So maybe Faker::Date.forward should call get_date_object instead of Date.today and the problem would be solved.

I'm wondering why this was marked as completed when it's still an issue with the gem? Is there workaround for rails users or something I should be doing instead? I am also happy to put a patch up if that's better. (@robinator)

I believe this issue was closed because it was stale.

A possible workaround would be to set the TZ variable on an initializer. Ruby will then pull the timezone from the OS, for example:

> Faker::Date.forward(days: 1).to_time
=> 2023-04-13 00:00:00 -0700
> ENV['TZ'] = Time.zone = 'UTC'
=> "UTC"
> Faker::Date.forward(days: 1).to_time
=> 2023-04-13 00:00:00 +0000

But feel free to submit a patch with reproduction steps or a test and I'll review it! Thanks!

thdaraujo commented 1 year ago

reopening for further investigation - see https://github.com/faker-ruby/faker/issues/2093#issuecomment-1505535860

stefannibrasil commented 1 year ago

thanks @thdaraujo for helping with this!

I'm wondering why this was marked as completed when it's still an issue with the gem?

@robinator I closed it without pasting the comment, my bad. This is the message that was supposed to have been shared:

"Hey, folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the faker-ruby team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this issue.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this issue, then please let us know so that we can reprioritize it. Thanks!"

thdaraujo commented 1 year ago

assigned to @luciagirasoles