[SERVER-38442] Date_t::min() is incorrect Created: 06/Dec/18  Updated: 29/Oct/23  Resolved: 18/Dec/18

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: 4.1.7

Type: Task Priority: Major - P3
Reporter: Matthew Saltz (Inactive) Assignee: Billy Donahue
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Minor Change
Sprint: Dev Tools 2018-12-17, Dev Tools 2018-12-31
Participants:

 Description   

Date_t::min() right now actually just gives 0 milliseconds since the epoch, which is incorrect.



 Comments   
Comment by Githook User [ 18/Dec/18 ]

Author:

{'username': 'BillyDonahue', 'email': 'billy.donahue@mongodb.com', 'name': 'Billy Donahue'}

Message: SERVER-38442: Date_t::min() has the wrong value

Comment by Billy Donahue [ 10/Dec/18 ]

Done.

Comment by Matthew Saltz (Inactive) [ 10/Dec/18 ]

Sounds good. As part of the CR would you mind removing the TODOs I have for this ticket here?

Comment by Billy Donahue [ 10/Dec/18 ]

I will try to update the docs for Date_t::min() to at least make this situation understandable to a reader.

https://mongodbcr.appspot.com/251510001/

I think it's risky to change `Date_t::min()`, but we should clarify what it means.
negative values are supported, but they can throw or trigger invariants on you when you try to use them.

Comment by Matthew Saltz (Inactive) [ 10/Dec/18 ]

If the intention is that you can only represent times since the epoch, then the current implementation is correct. I guess the problem is that right now nothing prevents you from creating a Date_t for before the epoch, with e.g. Date_t::fromMillisSinceEpoch(-20) or just the constructor that takes a number of milliseconds. I filed this ticket when I was writing a test for overflow of Date_t addition and noticed that Date_t::min() + Milliseconds(-1) works just fine, though I expected it to throw.

Generally speaking I assume that anything with SomeType::min() or SomeType::max() like this should be defined such that min() is the value such that all values of SomeType will be considered greater than or equal to it, and vice versa (though that may not be a fair assumption). So again, if the time at the epoch (Date_t(0)) is supposed to be the minimum, that's fine, but I think we should prevent negative values from being created in that case.

And also - I don't know that this is really a problem as is, it doesn't seem to be used much in prod code. However, here's an example of a case with some subtlety since the check is for == Date_t::min() and not <= Date_t::min() (which could technically occur), but I don't know if that's a problem in practice.

Tagging schwerin and redbeard0531 since we discussed this last week.

Comment by Billy Donahue [ 10/Dec/18 ]

Is the objection here that Date_t holds a private signed long long millis and could be used to represent times before the Unix epoch?

We know the Unix epoch represented by time_t cannot be negative, since mktime returns -1 to indicate error. We could consider the signedness an implementation detail.

Date_t claims to represent dates representable by BSON.

https://github.com/mongodb/mongo/blob/d899a205ef66d916b071dd42fbb619775561a523/src/mongo/util/time_support.h#L55-L58

/**
 * Representation of a point in time, with millisecond resolution and capable
 * of representing all times representable by the BSON Date type.
 *
 * The epoch used for this type is the Posix Epoch (1970-01-01T00:00:00Z).
 */

And bsonspec.org writes:
"\x09" e_name int64 UTC datetime
"UTC datetime - The int64 is UTC milliseconds since the Unix epoch."

Whevever Date_t::min() is called, it's used as a singular value.
If we change it, we may have to deal with version skew issues as heterogeneous programs communicate by packing and unpacking Date_t::min() values into BSON objects. Changing this thing feels risky.
Maybe we should just document that it isn't negative and move on?

Generated at Thu Feb 08 04:48:58 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.