Links do not expire

[threadedstream]

Here's the simple request responsible for generating expiry links,which simply delegates the work to shortener api.

    def form_expiry_link
      link = Shortener::ShortenedUrl.generate('https://facebook.com', expires_at: DateTime.now)
      render json: link
    end

However, when i head over to localhost:3000/ endpoint, it redirects me to the facebook.com, although, for obvious reason, it shouldn't. What am i doing wrong?

[threadedstream]

I finally managed to find an issue. Had to dig into the source code. It turns out that unexpired entries are filtered relative to the UTC time(assuming that ::Time.current.to_s(:db) returns time in UTC format). ::Time.now fixed an issue. Is it considered as a bug? Should i submit pr?

[fschwahn]

Yes please, a PR with a failing test demonstrating the issue and a fix would be great.

[fschwahn]

Hm, after looking at the PR, I don't think that's the problem. Time.current is timezone aware, and replacing with Time.now will likely introduce bugs. So let's try to troubleshoot first:

Do you see the problem if you use Time.current instead of DateTime.now? In rails you usually should not use DateTime at all, as all of these tasks can be fulfilled with Time or ActiveSupport::TimeWithZone.

If the problem persists, can you post the output of DateTime.now as well as Time.current?

[threadedstream]

Thanks for an answer. My apologies for a great delay. Well, problem still persists even after changing to Time.current. Here's an output of Time.current: 2021-05-14 10:23:59 UTC, and also DateTime.now: 2021-05.14T10:24:23+03:00. Outputs seem to be correct. Problem hides in the fact that unexpired scope uses Time.current.to_s(:db) function, which outputs incorrect time relative to my country.

[fschwahn]

Ok, I think your problem is that there's no timezone set. Time.current returns UTC, while DateTime.now returns a time in a timezone. So these times are actually 3 hours apart. Do you set a timezone in your application.rb-file? Something like config.time_zone = "Minsk"?

In any case, you shouldn't mix DateTime with Time, as only Time is made time zone aware by rails.

[threadedstream]

I looked into application.rb file and found that this setting is already set to a proper value

[fschwahn]

To what value is it set? Time.current should not return UTC, if config.timezone is set to something else. I think you need to investigate what's going on there.

[threadedstream]

It's set to Moscow. Well, but what Time.current.to_s(:db) is supposed to output then? Does it output correct time irrespective of timezone one's country sticks to? I mean, in case if config.timezone is set to whatsoever non-default value?

[fschwahn]

Time.current is supposed to return current time in the respective time zone, Time.now just returns your system time (without timezone information). In your case Time.current returns your system time as though it was in UTC, which is wrong. It's hard to say why this is happening, but when you fix this issue, your problem will go away. I don't believe this is a bug in shortener.

An example what is returned on my system:

Time.current
# => Fri, 14 May 2021 10:28:07 CEST +02:00
Time.current.to_s(:db)
# => "2021-05-14 08:28:10"
Time.zone
# => #<ActiveSupport::TimeZone:0x00007ff33ff6bcf8 @name="Berlin", @utc_offset=nil, @tzinfo=#<TZInfo::DataTimezone: Europe/Berlin>>

[jpmcgrath]

Coming in a bit late to the convo after reviewing @ThreadedStream's PR https://github.com/jpmcgrath/shortener/pull/144

It seems to me that this issue is that by converting to a string, you'd drop the timezone info. When that is passed to the db as a string, the db will assume the time info is in the db's timezone, which in the case of ThreadedStream is probably different to their application timezone. This difference results in the string being converted to a time some hours different than what was intended.

Time.now works because it is a time class, which is really just a wrapper that is representing a number since unix time (which is measured from UTC). This means the db doesn't have to guess the timezone for the requested time.

I have reviewed the PR and have made suggestions on how better to "prove" that this change fixes what it sets out to fix.

Thanks again for the contribution.

[threadedstream]

I apologize for a huge delay. @jpmcgrath, thank you very much for spending some time reviewing my pr. Upon analyzing the whole conversation, my thoughts led me to the state of dilemma. On one hand, i should fix the issues pointed out in pr, and on the other, just settle it all by tuning rails timezone settings(although it's already set to a proper value). Well, how do we solve such a conundrum?