On 9/19/19 12:25 PM, Tom Lane wrote: > I wrote: >> I found a spot that seemed like a reasonable place, and added some >> coverage of the point. Updated patch attached. > > Doc patch pushed.
Thanks! I did not get to review them last night but upon review not too long ago, they looked great. >> It seems to me that there are some discrepancies between what the spec >> says and what jsonpath_scan.l actually does, so maybe we should take a >> hard look at that code too. The biggest issue is that jsonpath_scan.l >> seems to allow single- and double-quoted strings interchangeably, which is >> OK per ECMAScript, but then the SQL/JSON spec seems to be saying that only >> double-quoted strings are allowed. I'd rather be conservative about this >> than get out in front of the spec and use syntax space that they might do >> something else with someday. I agree with erring on the side of the spec vs. what ECMAScript does. In JSON, strings, identifiers, etc. are double-quoted. Anything that is single quoted with throw an error in a compliant JSON parser. Looking at the user documentation for how some other databases with SQL/JSON support, this seems to back up your analysis. > > The attached proposed patch makes these changes: > > 1. Remove support for single-quoted literals in jsonpath. > > 2. Treat an unrecognized escape (e.g., "\z") as meaning the escaped > character, rather than throwing an error. > > 3. A few cosmetic adjustments to make the jsonpath_scan code shorter and > clearer (IMHO). If this refers to s/any/other/, yes I would agree it's clearer. > As for #1, although the SQL/JSON tech report does reference ECMAScript > which allows both single- and double-quoted strings, it seems to me > that their intent is to allow only the double-quoted variant. They > specifically reference JSON string literals at one point, and of course > JSON only allows double-quoted. Also, all of their discussion and > examples use double-quoted. Plus you'd have to be pretty nuts to want > to use single-quoted when writing a jsonpath string literal inside a SQL > literal (and the tech report seems to contemplate that jsonpaths MUST be > string literals, though of course our implementation does not require > that). I agree with the above (though wrt single-quoting and literals, I have seen stranger things). > As for #2, the existing code throws an error, but this is contrary > to clear statements in every single one of the relevant standards. Makes sense. I looked at the patch, but did not test it. From what I can see, it looks good, but perhaps we add a test in it to show that single-quoted literals are unsupported? Jonathan
signature.asc
Description: OpenPGP digital signature