Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Time encoding #16

Merged
merged 1 commit into from
Apr 3, 2014
Merged

Fix Time encoding #16

merged 1 commit into from
Apr 3, 2014

Conversation

fbernier
Copy link
Contributor

@fbernier fbernier commented Mar 22, 2014

Hi,

This is a resulting of a yak shave for the following issue:
https://github.com/mongoid/mongoid/pull/3563

The previous code was causing rounding errors. The result for us was when using Mongoid::Timestamps in embedded documents, the query sent to mongodb when deleting documents is a $pullAll with its created_at and updated_at fields which were badly BSON encoded. For every documents where the rounding error occurred, they were never deleted with a delete_all or destroy_all method call.

The Time I used in the updated spec is one causing this rounding error. You can test the old line bson encoding line with it to see the test actually fail.

This also fixes DateTime as it is converted to a Time when to_bson is called on it.

@fbernier
Copy link
Contributor Author

No idea about the failing spec ... it works on my machine ...

I also just noticed the patch we made is exactly what Moped::BSON was doing here:
https://github.com/mongoid/moped/blob/436cae6fd039da0a61e24d017721c817a7ad2462/lib/moped/bson/extensions/time.rb#L10

prathe added a commit to demarque/bson-ruby that referenced this pull request Mar 24, 2014
@durran
Copy link
Member

durran commented Apr 3, 2014

I'm looking into the failure.

@durran
Copy link
Member

durran commented Apr 3, 2014

Ok making notes here with the issue from the existing and to why the fix should be correct...

obj.to_f loses the last bit of precision when dealing with the microseconds... In this case, when it should be 1395511505.505000 it is actually 1395511505.5049999, therefore the final value for millis gets rounded down in the final conversion, 1395511505505 vs 1395511505504.

The current is exactly what the existing ruby driver does, surprised no one has caught this yet - thanks for reporting. (https://github.com/mongodb/mongo-ruby-driver/blob/1.x-stable/lib/bson/bson_ruby.rb#L448)

@durran
Copy link
Member

durran commented Apr 3, 2014

And the failure is the c extension needs to also be changed. It's the second run with the extension enabled that is failing.

@durran durran merged commit 4d59e4a into mongodb:master Apr 3, 2014
@fbernier
Copy link
Contributor Author

fbernier commented Apr 3, 2014

@durran Ah it all makes sense now... Thanks for the fix and the release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants