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

Updates the mongo-style raw encoding to match v2 of the Extended JSON spec #247

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Updates the mongo-style raw encoding to match v2 of the Extended JSON spec #247

wants to merge 1 commit into from

Conversation

atheriel
Copy link

@atheriel atheriel commented Aug 1, 2018

The $binary format has changed as of May 2017 (see commit). Before, we had

{"x" : {"$binary" : "//8=", "$type" : "00"}}

and now we have

{"x" : { "$binary" : {"base64" : "//8=", "subType" : "00"}}}

This commit should implement the new format. I believe this may be a breaking change for some users, but since they are likely interacting with MongoDB in some way they'll have to change at some point anyway.

The "$binary" format has changed as of May 2017. Before, we had e.g.

{"x" : {"$binary" : "//8=", "$type" : "00"}}

and now we have e.g.

{"x" : { "$binary" : {"base64" : "//8=", "subType" : "00"}}}

Signed-off-by: Aaron Jacobs <[email protected]>
@jeroen
Copy link
Owner

jeroen commented Aug 1, 2018

Looking at the reference manual it still describes the current format:

screen shot 2018-08-01 at 11 15 09 pm

@atheriel
Copy link
Author

atheriel commented Aug 1, 2018

Well, I didn't expect that.

@codecov-io
Copy link

Codecov Report

Merging #247 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #247   +/-   ##
=======================================
  Coverage   63.82%   63.82%           
=======================================
  Files          88       88           
  Lines        2764     2764           
=======================================
  Hits         1764     1764           
  Misses       1000     1000
Impacted Files Coverage Δ
R/asJSON.raw.R 57.14% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a68e1d4...f7892d4. Read the comment docs.

@atheriel
Copy link
Author

atheriel commented Aug 1, 2018

The Q&A section of the spec suggests that implementations could support both (it terms the older version "Legacy"). Perhaps we could have a raw = "mongo2" option?

@jeroen
Copy link
Owner

jeroen commented Aug 1, 2018

Why do you need this new format? I would much prefer to stick with the mongo default behavior.

@jeroen
Copy link
Owner

jeroen commented Aug 1, 2018

I am running mongodb 4.0 and get this:

m <- mongolite::mongo()
m$insert(list(foo=charToRaw('bar')))
m$export()
{ "_id" : { "$oid" : "5b62271647a302483976d756" }, "foo" : { "$binary" : "YmFy", "$type" : "05" } }

So if neither the documentation nor the implementation of mongodb has changed, I don't think jsonlite should either.

@atheriel
Copy link
Author

atheriel commented Aug 1, 2018

I don't need it -- and certainly don't mind holding off on changing it. I only noticed the issue when I tried to run the mongolite spec test suite.

As for your example, I noticed that export() eventually calls down into libbson's bson_as_json() function, which the inline API documentation refers to as returning "libbson's legacy JSON format". I think if it used bson_as_canonical_extended_json() it would return the expected string, if I'm reading the sources correctly. Maybe that's an issue to deal with in mongolite, though -- I'd be happy to submit something coordinated with this change to jsonlite.

Finally, there appears to be an open issue to update the mongodb docs, which clears up some of the mystery.

@jeroen
Copy link
Owner

jeroen commented Aug 1, 2018

OK thanks for the clarification. I think I should first aks clarification from the mongo devs if I should be switching to bson_as_canonical_extended_json() in mongolite, before merging the change here.

@atheriel
Copy link
Author

atheriel commented Aug 1, 2018

Sounds reasonable to me!

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

Successfully merging this pull request may close these issues.

3 participants