MongoMapper stores Time(/stamps) incorrectly. #308

Closed
sgonyea opened this Issue Aug 11, 2011 · 12 comments

5 participants

@sgonyea

MM is storing time in seconds form, not milliseconds as it claims to in the code. Looking at the code for BSON, it seems to persist Time objects for you... So I find it confusing that this is being done manually (and incorrectly). Also, Timecop really shouldn't be used in these specs.

@bkeepers

PDI :)

@jnunemaker

I'm thinking this was for a reason. Like Mongo internally only stores to the second for Times.

@sgonyea

Hmm, I replied by email but don't see it here.

BSON serializes Time objects into I64-form. It does so by effectively doing:

time = TIme.now
mili_time = (time.to_f * 1_000).to_i

Since it's already doing this conversion, it seems to me that MongoMapper shouldn't even care about Time. Just let BSON do the serialization / conversion details.

@jnunemaker
@sgonyea

Actually, I was thinking it... It wouldn't affect backwards compatibility. The date transform in BSON is here:

https://github.com/mongodb/mongo-ruby-driver/blob/master/lib/bson/bson_ruby.rb#L450-455

Effectively, it's always going to take the Date object and multiply it by 1_000. The way MongoMapper is serializing dates, it simply loses precision.

t = Time.now
# => Sat Aug 13 17:32:09 -0700 2011 

# This is how MongoMapper causes Time to be stored:
t.to_i * 1_000
# => 1313281929000 

# This is how BSON serializes Time:
> (t.to_f * 1_000).to_i
# => 1313281929493 

MongoMapper is basically taking the Time object, calling #to_i on it, then turning it back into a Time object. It'd be safe to simply skip that entire process, as you're never actually giving BSON the integer to store. Just a less precise Time instance.

Hopefully that all makes sense :).

Sorry for all the Shoulda whining, btw. I can make this change, but I didn't want to bastardize the tests too much. We could actually rip out most of the time stamp related tests. Those are BSON's problem.

@jnunemaker
@bkeepers

So what do you guys suggest we do to fix this?

@kpshek kpshek added a commit to kpshek/mongomapper that referenced this issue Feb 13, 2012
@kpshek kpshek Fix issue #308 - ensure milliseconds are preserved with time values b43549f
@bkeepers bkeepers closed this in 029dd45 Mar 30, 2012
@gaffneyc

Just ran into an issue with this commit today:

model.time = Time.now # => "2012-10-03 11:21:51 -0400" (1349277711.61238)
model.save

ModelClass.find_by_time(Time.parse("2012-10-03 11:21:51 -0400")) # => nil (1349277711.0)

The issue is now that they are being stored with millisecond precession we need to know the milliseconds to be able to find the record by the time.

@jnunemaker

@gaffneyc yeah, some work is being done here: #455 (comment)

@jnunemaker

Also, why would you ever find by exact time? Not sure I am understanding that exact use case. I would think times should always be range queried.

@gaffneyc

@jnunemaker the use case we had to do with report generation and it turned out we weren't normalizing the time correctly on our end (Time#change returns a new instance). We talked about it a bit in the office and we couldn't come up with a use case beyond some stuff dealing with tests.

@leifcr

@gaffneyc I agree that it's usually when doing tests. I've encountered this in a plugin that were checking time on newly created objects against either other existing objects. I also encountered it where I had to check the time on a created object for equalness of specific dates/times. However, it's quite easy to fix if you do the "ISO" conversion or convert to a string format before checking. Ideally MM would "change" the time to what it's stored in when saving the data, so it's correct on newly created objects. ( Done in pull request #455 )

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