[CXX-879] Error-prone use of std::time_t in bsoncxx::types::b_date Created: 25/Mar/16 Updated: 08/Jan/24 Resolved: 15/Apr/16 |
|
| Status: | Closed |
| Project: | C++ Driver |
| Component/s: | BSON, Documentation, Implementation |
| Affects Version/s: | 3.0.0 |
| Fix Version/s: | 3.0.2 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Allan Bazinet | Assignee: | Samantha Ritter (Inactive) |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
We're in the process of testing conversion of our code base from the legacy C++ driver to the 3.0 driver. In the legacy driver, it's a common use case to use the BSONObjBuilder's appendTimeT() function, which accepts a time_t. The closest analog to this in the 3.0 driver appears to be bsoncxx::types::b_date class, which accepts either an int64 or a std::chrono::system_clock::time_point. Neither of these are actually what you want when you've got a std::time_t, but it's not unreasonable for someone performing this conversion to hand the class a time_t, which will compile without issue or warning, and which results in Bad Things happening. It might be advantageous to at the least note in the documentation or porting notes that if one is moving from the legacy driver, then use of std::chrono::system_clock::from_time_t to get a time_point is one option here. |
| Comments |
| Comment by Githook User [ 15/Apr/16 ] |
|
Author: {u'username': u'samantharitter', u'name': u'samantharitter', u'email': u'samantha.ritter@10gen.com'}Message: |
| Comment by Samantha Ritter (Inactive) [ 12/Apr/16 ] |
|
code review: https://github.com/mongodb/mongo-cxx-driver/pull/480 |
| Comment by Allan Bazinet [ 25/Mar/16 ] |
|
That seems reasonable to me in terms of attempting to prevent errors; it's not that difficult to obtain a time_point from a variety of other representations, and it would seem that you'd then be able to eliminate the 'no matching call to append' issue that the BSON stream insertion operator now issues when passed a time_point. |
| Comment by Mira Carey [ 25/Mar/16 ] |
|
alb, After a little bit of discussion, we're completely in agreement that leaving the integer constructor is a little too dangerous. I think the best way forward is to remove the int64_t constructor and only support system_clock::time_point's and chrono::milliseconds. That should make it obvious that time_t's need to be coerced to time_points via system_clock::from_time_t, and will require anyone with a random integer to tell us it's in milliseconds. Which is about as good as the type system can do for us. |
| Comment by Andrew Morrow (Inactive) [ 25/Mar/16 ] |
|
alb - Thanks for the bug report. I think the API of b_date does need some reconsideration. Our goal with the new interface is to make it difficult or impossible to do the wrong thing (unlike the old driver). The challenge here is that the BSON date time is explicitly defined to be a signed 64-bit integer, and we really do want to allow access to it in its raw form. But your point about subtle trouble when passing in a time_t is well taken. I'm also somewhat suspicious that we should be naming the clock here at all. Perhaps the solution is to only permit construction of a b_date from a std::chrono::milliseconds, but we allow the result to be read as either an int64_t or as a std::chrono::milliseconds. |