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

common/escape: do not escape / in json #14130

Merged
merged 2 commits into from Apr 8, 2017

Conversation

Projects
None yet
5 participants
@liewegas
Member

liewegas commented Mar 24, 2017

As far as I can tell '/' is perfectly valid for JSON
strings (keys or values).

Signed-off-by: Sage Weil sage@redhat.com

common/escape: do not escape / in json
As far as I can tell '/' is perfectly valid for JSON
strings (keys or values).

Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas requested review from yehudasa and dmick Mar 24, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 24, 2017

retest this please

@dmick

This comment has been minimized.

Member

dmick commented Mar 25, 2017

This was like the second thing I complained about when I started at Dreamhost. I led myself to believe that it was ambiguous through the various specs. http://www.json.org/ claims that it must be escaped, as does http://www.ietf.org/rfc/rfc4627.txt, but given that it's Just Javascript, it's not clear why. As I recall, the reason given somewhere was that, if you didn't escape /, you could conceivably confuse a parser by including </tag> in a string and thus causing the end of the tag.

Edit: see also
http://stackoverflow.com/questions/1580647/json-why-are-forward-slashes-escaped

It's annoying, but it seems to be the standard.

@dmick

There doesn't seem to be an explicit "disapprove", but sadly I think we're more compliant keeping this annoying escaping

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 25, 2017

So sad. Okay!

@liewegas liewegas closed this Mar 25, 2017

@jdurgin

This comment has been minimized.

Member

jdurgin commented Mar 27, 2017

this one drives me nuts.

I'd say we should omit the , since that's what python does by default:

>>> import json                                                                                                                                                                                                   
>>> json.dumps('foo/bar')                                                                                                                                                                                         
'"foo/bar"'  
>>> json.loads('"foo/bar"')
u'foo/bar'
>>> json.loads('"foo\/bar"')
u'foo/bar'

The only case where it matters is an embedded string in js. Parsing the json explicitly with js avoids that problem, so I don't see a reason for us to do this. If necessary, it could be transformed to having escaped '/'s by whatever is embedding it in html as well.

@jdurgin jdurgin reopened this Mar 27, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 27, 2017

if python's json doesn't escape '/' that's good enough for me!

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 27, 2017

Spec on http://json.org/ is not precise. It says:

A string is any unicode character except " or \ or 'control character'.
Then it lists examples of escapes which is a mix of: escaped control characters, escape ", escaped , escaped 4 hex digit unicode and escaped / .

This is a more precise spec: http://www.ietf.org/rfc/rfc4627.txt . It says:

The representation of strings is similar to conventions used in the C
family of programming languages. A string begins and ends with
quotation marks. All Unicode characters may be placed within the
quotation marks except for the characters that must be escaped:
quotation mark, reverse solidus, and the control characters (U+0000
through U+001F).
Note: "control characters" phrase is defined as ASCII characters between 00h and 1fh. In other words, it includes backspace, formfeed, newline, but stops before 20h (space). Forward slash isn't in the range of "control characters".

It seems a JSON encoder can choose to escape any character, it must escape certain ones. / falls in "choose to escape" section not in "must escape".

from esnme/ultrajson#110

@jdurgin

lgtm, guessing some cli tests will need fixups (rbd suite at least, maybe rgw too)

@dmick

This comment has been minimized.

Member

dmick commented Mar 27, 2017

It's not like I don't sympathize with the desire. Interesting that Python json.dump apparently doesn't even have an option to escape /.

@dmick

This comment has been minimized.

Member

dmick commented Mar 27, 2017

But, yes, the only reason I ever found was for embedding in HTML, which is of little import to us generally speaking. (also, while json.org is ambiguous, its right-column 'grammar' and railroad diagrams aren't.)

@liewegas

This comment has been minimized.

Member

liewegas commented Mar 27, 2017

@dmick

This comment has been minimized.

Member

dmick commented Mar 27, 2017

The BNF in rfc4627 is really odd. It shows a char is either unescaped or one of the following (including escaped forward slash), but then says unescaped = %x20-21 / %x23-5B / %x5D-10FFFF, which certainly includes %2f. O_o

I'm willing to step aside now.

@yehudasa

This comment has been minimized.

Member

yehudasa commented Mar 30, 2017

@liewegas does it pass teuthology? I'd be happier with having this one configurable somehow.

@cbodley

This comment has been minimized.

Contributor

cbodley commented Apr 4, 2017

qa/workunits/cephtool/test.sh: handle non-escaped-/ json
Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas merged commit 175e33b into ceph:master Apr 8, 2017

2 of 3 checks passed

default Build triggered. sha1 is merged.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details

@liewegas liewegas deleted the liewegas:wip-json-slash branch Apr 8, 2017

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