PATCH: Implement value_to_json for single-datum conversion

Hi all

Whenever I try to work with the new json types I trip over the lack of a
function to escape text to json. The attached patch against master
provides one by exposing the existing datum_to_json function to SQL.
I've used the name value_to_json, but I'm not sure it's necessarily the
right choice of name.

Please consider this for the 9.2 branch as well as to HEAD, as IMO it's
very important for basic usability of the json functionality. It applies
to 9.2 fine and passes "make check". I know it's late in the game, but
it's also a very small change and it's very hard to build up JSON data
structures other than simple rows or arrays without at minimum a way of
escaping `text' to json strings.

This feels basic enough that I'm wondering if there's a reason it wasn't
included from the start, but I don't see any comments in json.c talking
about anything like this, nor did I find any -hackers discussion about
it. I suspect it's just an oversight.

As value_to_json directly wraps datum_to_json it actually accepts record
and array types too. I didn't see any reason to prevent that and force
the user to instead use row_to_json or array_to_json for those cases. If
you don't want to accept this, I can provide a wrapper for escape_json
that only accepts a text argument instead, but I think *some* way to
escape text to JSON is vital to have in 9.2.

A docs patch will follow shortly if you're happy that this patch is
reasonable.

--
Craig Ringer

Whoops. It actually looks like the posted patch muffed up opr_sanity
checks. I'm totally new to pg_proc.h wrangling so I'm not sure why yet,
looking.

Sorry, not sure how I missed that. I'll follow up shortly.

--
Craig Ringer

OK, opr_sanity was failing because I added the value_to_json(text) alias
to ensure that:

value_to_json('some_literal')

worked, following the same approach as quote_literal(anyelement) and
quote_literal(text). That should be reasonable, right? The comments on
the affected check in opr_sanity say that it's not necessarily wrong so
long as the called function is prepared to handle the different
arguments its self - which it is, since it's already accepting anyelement.

The test comment reads:

Note that the expected output of this part of the test will
need to be modified whenever new pairs of types are made
binary-equivalent,
or when new polymorphic built-in functions are added

so that seems reasonable.

postgres=# \df quote_literal
List of functions
Schema | Name | Result data type | Argument data types |
Type
------------+---------------+------------------+---------------------+--------
pg_catalog | quote_literal | text | anyelement | normal
pg_catalog | quote_literal | text | text | normal
(2 rows)

postgres=# \df value_to_json
List of functions
Schema | Name | Result data type | Argument data types |
Type
------------+---------------+------------------+---------------------+--------
pg_catalog | value_to_json | json | anyelement | normal
pg_catalog | value_to_json | json | text | normal
(2 rows)

Revised patch that tweaks the expected result of opr_sanity attached.

--
Craig Ringer

Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> writes:
> OK, opr_sanity was failing because I added the value_to_json(text) alias
> to ensure that:

> value_to_json('some_literal')

> worked, following the same approach as quote_literal(anyelement) and
> quote_literal(text). That should be reasonable, right?

No, it isn't. What you're proposing is to let opr_sanity think that
text and anyelement are interchangeable to C functions, which is so
far from reality as to be ludicrous. That would be seriously damaging
to its ability to detect errors.

But more to the point, your analogy to quote_literal is faulty anyway.
If you looked at that, what you'd find is that only quote_literal(text)
is a C function. The other one is a SQL wrapper around a coercion to
text followed by the C function. I rather imagine that the definition
as you have it would crash on, say, value_to_json(42).

regards, tom lane

Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> writes:
> Whenever I try to work with the new json types I trip over the lack of a
> function to escape text to json. The attached patch against master
> provides one by exposing the existing datum_to_json function to SQL.
> ...
> This feels basic enough that I'm wondering if there's a reason it wasn't
> included from the start,

There actually was a previous thread about this:
http://archives.postgresql.org/pgsql-hackers/2012-05/msg00001.php
Note in particular Andrew's comment:

Second, RFC 4627 is absolutely clear: a valid JSON value can
only be an object or an array, so this thing about converting
arbitrary datum values to JSON is a fantasy. If anything, we
should adjust the JSON input routines to disallow anything else,
rather than start to output what is not valid JSON.

It's possible he's misread the spec, but I think we ought to tread very
carefully before adding "obvious" conversions we might regret later.

> Please consider this for the 9.2 branch as well as to HEAD, as IMO it's
> very important for basic usability of the json functionality. It applies
> to 9.2 fine and passes "make check". I know it's late in the game,

It's several months too late for feature additions to 9.2, especially
ones that would require an initdb to install. We can look at this for
9.3, but I'm still worried about the spec-compliance question.

regards, tom lane

On 08/13/2012 12:48 PM, Tom Lane wrote:
> There actually was a previous thread about this:
> http://archives.postgresql.org/pgsql-hackers/2012-05/msg00001.php
> Note in particular Andrew's comment:
>
> Second, RFC 4627 is absolutely clear: a valid JSON value can
> only be an object or an array, so this thing about converting
> arbitrary datum values to JSON is a fantasy. If anything, we
> should adjust the JSON input routines to disallow anything else,
> rather than start to output what is not valid JSON.

Thanks for taking a look. That makes sense. I guess these are similar
issues to those the XML type faces, where working with fragments is a
problem. The spec requires a single root element, but you don't always
have that when you're *building* XML, hence the addition of `IS DOCUMENT'.

I was hoping to find a low-impact way to allow SQL-level construction of
more complex JSON objects with correct text escaping, but it sounds like
this isn't the right route. I don't currently see any way to achieve the
kind of on-the-fly building you can do with XML's xmlelement(),
xmlconcat(), xmlforest() etc; nor equivalent to hstore's
hstore(text[],text[]), and I was hoping to improve that.

I have a half-finished JSON object constructor
json_object_from_arrays(text[], json[]) in the same style as
hstore(text[],text[]) . It won't work without the notion of json-typed
scalars, though, as the values of keys could then only be arrays or
objects, which isn't very useful. I can't usefully accept `anyarray' as
a values argument since arrays are of homogeneous type. Accepting text[]
would be a bug-magnet even if there was some kind of `text
json_escape(text)' function.

Would it be reasonable to add a separate json_element type, one that's
binary-equivalent to `json' but not constrained by the requirement to be
an array or object/dict? Or a `jsobject' ?

As for the value_to_json crashing, works for me:

postgres=# SELECT value_to_json(42);
value_to_json
---------------
42
(1 row)

... since datum_to_json is happy to accept anything you throw at it
using output function lookups, and value_to_json its self doesn't care
about the argument type at all. That was all in the regression tests.

Purely so I understand what the correct handling of the anyelement+text
overload would've been: In light of your comments on opr_sanity would
the right approach be to add a second C function like text_to_json that
only accepts 'text' to avoid confusing the sanity check? So the SQL
"value_to_json(anyelement)" would point to the C "value_to_json" and the
SQL "value_to_json(text)" would point to the C "text_to_json" ?

Anyway, clearly the value_to_json approach is out.

--
Craig Ringer

Craig Ringer <ringerc(at)ringerc(dot)id(dot)au> writes:
> As for the value_to_json crashing, works for me:

> postgres=# SELECT value_to_json(42);
> value_to_json
> ---------------
> 42
> (1 row)

Oh, right, because there actually is support for anyelement in the
underlying C function. There is not in the quote_literal case.

> Purely so I understand what the correct handling of the anyelement+text
> overload would've been: In light of your comments on opr_sanity would
> the right approach be to add a second C function like text_to_json that
> only accepts 'text' to avoid confusing the sanity check?

Actually, given the above, what did you need value_to_json(text) for at
all? Wouldn't value_to_json(anyelement) have covered it?

But yeah, the general approach to suppressing complaints from that
opr_sanity test is to make more C entry points. The point of it,
in some sense, is that if you want to make an assumption that two
types are binary-equivalent then it's better to have that assumption
in C code than embedded in the pg_proc entries. The cases that we
let pass via the "expected" outputs are only ones where binary
equivalence seems pretty well assured, like text vs varchar.

regards, tom lane

On 08/13/2012 01:55 PM, Tom Lane wrote:
> Actually, given the above, what did you need value_to_json(text) for at
> all? Wouldn't value_to_json(anyelement) have covered it?

Usability. Without the version accepting text an explicit cast to text
is required to disambiguate a literal argument like
value_to_json('something') .

> But yeah, the general approach to suppressing complaints from that
> opr_sanity test is to make more C entry points. The point of it,
> in some sense, is that if you want to make an assumption that two
> types are binary-equivalent then it's better to have that assumption
> in C code than embedded in the pg_proc entries. The cases that we
> let pass via the "expected" outputs are only ones where binary
> equivalence seems pretty well assured, like text vs varchar.

Thanks. I appreciate the explanation, and sorry for the newbie error.

On the JSON stuff, I can see it's not as simple as adding a simple
escape function. For my needs during the 9.2 timeframe I'll bundle up an
extension with the functionality I need and deal with the need to port
to whatever 9.3 includes. Hopefully a "json_value" or "javascript_value"
or similar can be introduced for 9.3, given comments like:

http://archives.postgresql.org/pgsql-hackers/2012-05/msg00030.php
http://archives.postgresql.org/pgsql-hackers/2012-05/msg00065.php

.. expressing not only the need for json scalars, but the fact that
they're already commonplace in pretty much everything else.

Given this:

http://archives.postgresql.org/pgsql-hackers/2012-05/msg00040.php

it does seem that `json' should be a whole document not a fragment, but
IMO a way to work with individual JSON values is going to be *necessary*
to get the most out of the json support - and to stop people who use
JSON in the real world complaining that Pg's JSON support is broken
because it follows the standard not real-world practice.

Personally the lack of json scalars has prevented me from using JSON
support in two different places already, though it's proving very useful
in many others.

--
Craig Ringer