[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: CXX-879 Remove int64_t constructor for b_date
Branch: master
https://github.com/mongodb/mongo-cxx-driver/commit/67968af46a9dd3e7fd78866019162d9e4a9bb15a

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.

Generated at Wed Feb 07 22:00:39 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.