Time data type should have interval from 00:00 to 24:00 instead to 23:59 #7125

Closed
radeno opened this Issue Jul 21, 2012 · 22 comments

Comments

Projects
None yet
8 participants

radeno commented Jul 21, 2012

Time date type column does not record any statement about "day".

That is not problem, when time is between 00:00 to 23:59.
Problem is when you want calculate difference for whole day.

00:00 (begin midgnit) to 24:00 (end midnight) is 24 hours / 1 day.

But when is 24:00 saved as 00:00 difference is 0 which is bad. Time object send 24:00 correctly send to 00:00 of next day!.
Time column only saves 00:00 (as same day).

Practice problem is when we have opening and close hours. If opening hours is 8:00 and close is 0:00 (should be 24:00), it is closed earlier than is opened.

Rails saves time column type as Time object, and saves only time.
2000-01-01T00:00:00Z for 00:00
vs.
2000-01-02T00:00:00Z for 24:00

Member

steveklabnik commented Jul 21, 2012

This is pretty much your standard CS101 'off by one' error. You can't have 24:00, or else you wouldn't be able to have 0:00...

radeno commented Jul 21, 2012

Ok, take back. Is there any way save to database 24:00 ? Some switch in Rails or some hint?

Because ISO 8601:2004 says:

4.2.3 Midnight
The complete representations in basic and extended format for midnight, in accordance with 4.2.2, shall be
expressed in either of the two following ways:
Basic format Extended format
a) 000000 00:00:00 (the beginning of a calendar day)
b) 240000 24:00:00 (the end of a calendar day)
The representations may have reduced accuracy in accordance with 4.2.2.3 or may be designated as a time expression in accordance with 4.2.2.5. To represent midnight the representations may be expanded with a decimal fraction containing only zeros in accordance with 4.2.2.4.
NOTE 1 Midnight will normally be represented as [00:00] or [24:00].
NOTE 2 The end of one calendar day [24:00] coincides with [00:00] at the start of the next calendar day, e.g. [24:00] on 12 April 1985 is the same as [00:00] on 13 April 1985. If there is no association with a date or a time interval both a) and b) represent the same local time in the 24-hour timekeeping system.
NOTE 3 The choice of representation a) or b) will depend upon any association with a date, or a time interval.
Representations where [hh] has the value [24] are only preferred to represent the end of a time interval in accordance with
4.4 or recurring time interval in accordance with 4.5.

So if you want have two columns as interval, you can't, because Rails do not allow this.

Between 00:00 and 24:00 is 24 hour different. If you will have date representation then yes, 00:00 and 24:00 is same, because date is different. But when you have only time representation, 00:00 and 24:00 are different. 00:00 is begin of the day, 24:00 is 24 hours from day begin.
Another example: 00:00 to 24:00 represents nonstop. 00:00 to 00:00 is represents as same time, so closed or off.

radeno commented Jul 21, 2012

Changed issue title to be more specific.

Contributor

avit commented Jul 21, 2012

This is not a rails issue. If this is a situation that needs exactly 24 hours and not 23:59:59, then you might consider handling these as straight numeric fields instead. Another option might be to use datetime columns and consider Time.new(2012,01,02) - Time.new(2012,01,01) == 1.day # (== 86400.seconds).

Also, Time.parse('24:00') returns 00:00 tomorrow, so this should still work for interval arithmetic, even though it displays as 00:00.

Contributor

avit commented Jul 21, 2012

I think I see your point though, since SELECT TIME('24:00') is a valid SQL return value, ActiveRecord will parse that as 00:00 tomorrow (correct). But when you try to persist it back, it will cast the ruby Time back into the column type as '00:00' (incorrect).

radeno commented Jul 21, 2012

I think it is Rails issue, because time column (postgresql, mysql, ...) does not restrict value (24:00) which is always valid by ISO.
As you said Time.parse working correct because have time within date. But after save, you can't back represent correct time, because date representation missing. So 24:00 always represents next day, 00:00 not.

Contributor

avit commented Jul 21, 2012

Time::DATE_FORMATS[:db] takes care of how Time objects get quoted for the database so this could be handled with a custom format. The problem is that Time.now.beginning_of_day and Time.now.end_of_day + 1 are ambiguous since they're both 00:00, we can't assume which one should be serialized as 24.

2 things could resolve this ambiguity:

  • Implicitly Time.parse('24:00') flags it as a 24 for values that come from params. (also Time.new(2012,07,21,24))
  • Explicitly Time.new.beginning_of_day would need a method to tell it that it's a 24 and not a zero. Like this:
t = Time.now.beginning_of_day.twenty_four # => 2012-07-21 00:00:00 +0000
t.twenty_four? # => true

I'm not sure of the edge cases, I only played with it a little bit, but something like this should work:

EDIT: see attached commit below.

Is it too crazy for including into ActiveSupport?

Contributor

nfm commented Jul 22, 2012

If I understand your issue correctly, this is still a problem if your opening time is 0800, and your closing time is 0300. You can't subtract times in that manner without assuming they never cross the 2400/0000 barrier.

Storing 2400 only helps you in the case where that's the exact end time. If you assume that when start == end, they're 24 hours apart, you can solve this problem more generally and handle all the cases, rather than trying to store 2400.

Contributor

avit commented Jul 22, 2012

@nfm If there's any need for crossing midnight, then I agree this makes NO sense and you should be using a datetime column or else count seconds as integer or something else.

I can see the case for SQL TIME('24:00') in a time column though. Depends what it's needed for... the database has this facility, it's just a question of whether to use it, or work around it with other data types for the sake of the ORM.

If you assume that when start == end, they're 24 hours apart

What if you want both 0 and 24, i.e. end - start == 0 and end - start == 24.hours? Ruby Time natively supports this:

Time.parse('24:00') - Time.parse('00:00') == 1.day

However, the database (or at least MySQL) can also take unbounded hours in a time field, e.g. "72:00:00". Ruby will NOT natively parse this (argument out of range).

Although the monkey patch I posted above could easily be made to handle it too, I think this needs a decision on whether ActiveRecord should try to support the SQL ISO spec quoted above.

radeno commented Jul 22, 2012

@nfm don't see on my problem with opening/close our. See on standardized behavior by ISO 8061. It says that 00:00 and 24:00 are two different times when you have interval.
You said start == end is 24 hours isn't true. If you take data from database and both have 00:00 you can't reproduce time difference without ugly hack. Time.parse('00:00') and second Time.parse('00:00') different is 0, not 24 hours.

Main problem is closing midnight is bad saved to database. When is defined 24:00 from hour selecting, Ruby correctly parse it as 00:00 tomorrow, but Rails incorrect save only time component without information that is next day. And with reverse reproduction you take incorrect information. Value '24:00' (or 23:59:60) is standardized value by ISO/UTC (see section 5.3 Time of the day) and Rails broke this mechanism because saves data incomplete.

@avit avit added a commit to avit/rails that referenced this issue Jul 22, 2012

@avit avit Proposed solution for SQL TIME('24:00') roundtrips
This extension can be loaded optionally by manual require "active_support/core_ext/time/twenty_four"
Please review and comment #7125.
65958d3
Contributor

nfm commented Jul 23, 2012

@radeno I'm not disputing that being able to store 2400 is valid SQL, just that I don't think it will really solve the problem you're having. You can't sensibly subtract times unless you mandate that the end time will be greater than the start time, and never cross the midnight barrier.

There might be other cases where being able to store 2400 is important, but I think you'd be better off storing a start time, and a duration. That way, 2400 issues are averted, and you can have 0s durations, durations that cross the midnight barrier, and > 24h durations.

radeno commented Jul 23, 2012

@nfm I fear that this is a trivialization of the problem than it actually is and to tell use another date type or use duration is don't care about problem and ignores day time interval (from -- to) which is not the same as time duration, because duration could be longer than 1 day, when day time interval can't.

Backward reproduce data what is saved to database are incorrect. There are lost one secod from whole day. 00:00 to 23:59:59 is not day interval what time data type in Rails is considered. It is not about validity of SQL, it is about ISO/UTC specification what of course SQL (MySQL, Postgre, MS SQL) implement. Diff between 24:00 and 00:00 absolutely interval day, diff between 00:00 and 00:00 is zero.

So why standard ruby time class has implement both forms, 24:00 and 23:59:60 and correctly roll over to 00:00 but of next day. Then diff 00:00 and 00:00+1day is 24 hours. I don't tell about that everything is wrong and bad. I just pointed to situation that when is day time interval.(out of interval is valid only 00:00 to 23:59:59) Rails broke only one solution and mechanism how to correctly backward reproduce right time. I can't get after commit right data, one valid and one invalid time objects, because Rails trim additional information what is needed. It is not acceptable.

A little bit exaggerated it is like NASA and Locheed Martin, NASA used metric system and Lockeed Martin used english imperial units, and 125 million dollar Mars orbiter are lost :)

Contributor

avit commented Jul 23, 2012

I played with my naive Time hack, and quickly realized this isn't going to work as a wrapper around the core ruby classes. With timezones and everything else, there is absolutely no fixed idea of "midnight".

@radeno, What you need is a value object that counts past 24:00:00 without rolling over to zero, and you can easily patch that into your model with composed_of and a wrapper around something like ChronicDuration.

Unless there's a case for adding a built-in duration value class in ActiveSupport, I vote to close.

Member

steveklabnik commented Jul 23, 2012

You can't use composed_of, as it was removed in Rails 4.

Contributor

avit commented Jul 23, 2012

ah sorry, edgeapi must be out of date.

Note that the example code in the pull request #6742 commit message uses attribute accessors which are already cast to ruby classes (Time, etc.). Just a reminder: you may need to access the raw value from @attributes or read_attribute_before_type_cast when implementing your own wrappers around database values.

radeno commented Jul 23, 2012

I see that this issue does not be resolved to good of all. I thinks that this should be open topic. Some material this: http://dotat.at/tmp/ISO_8601-2004_E.pdf section 4.2

For now i try to implement some before_save filter but string value 24:00:00 is still converted to Time object. I want to define that data type "time" save as string, to save 24:00 not 00:00 because data integrity is really necessary. Exist some way to say AR that one column change data type virtually or save raw data?

Contributor

avit commented Jul 26, 2012

Exist some way to say AR that one column change data type virtually or save raw data?

I thought this might be easy, but I noticed that attributes_before_type_cast already returns instances of Time objects... I do wonder how you might get the raw string values from the database.

radeno commented Jul 26, 2012

It looks like all timestamps (and derivations) are always type casted after find. I found solution on internet but without success. So i must overwrite _before_type_cast for specific attributes like this:

def ends_before_type_cast
  return '24:00:00' if (ends - begins.beginning_of_day) == 1.day
  ends.strftime('%H:%M:%S')
end

that is not problem. But how to tell AR that it must be saved as "24:00:00" and not "00:00:00". It is bigger issue for me.

Contributor

jeremyf commented Apr 14, 2013

When you are working with date ranges, I would recommend treating the beginning of the range as inclusive and the end point of the range as exclusive. Thus Mon 12:00am to Tues 12:00am would be 24 hours, 0 minutes, 0 seconds. This ties to what @steveklabnik wrote.

I believe this issue should be closed. The solution could be packaged as a gem.

Owner

rafaelfranca commented May 12, 2013

Agree

espen commented May 14, 2013

I find it strange that Rails do not want to handle proper SQL data type values. I also fail to see the reason behind some of the examples that mix Time and DateTime. When applying Time (24:00) to a DateTime then I do not see the problem that it returns 00:00 with the next day. I do however have a problem with db value 24:00:00 returning correct before type cast and 00:00 after type cast.

I'd agree this is a problem on the Rails side. If you plug rails into an existing postgres database that has values as 24:00, loading them and then saving them will change that to 00:00. I think this is unacceptable considering it's only ISO8601 compliance that is being requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment