[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:
| |||||||
| 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. | |||||||
| 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. Date_t claims to represent dates representable by BSON.
And bsonspec.org writes: Whevever Date_t::min() is called, it's used as a singular value. |