Re: inconsistent behaviour of json_to_record and friends with embedded json

Jeff Janes <jeff.janes@gmail.com> writes:
> On Thu, May 30, 2019 at 10:41 AM Robert Vollmert <rob@vllmrt.net> wrote:
>> 1. select * from json_to_record('{"out": "{\"key\": 1}"}') as (out json);
>> Postgres 10/11:
>> 1. gives
>> ERROR:  invalid input syntax for type json
>> DETAIL:  Token "key" is invalid.
>> CONTEXT:  JSON data, line 1: "{"key...

> This is caused by commit:

> commit cf35346e813e5a1373f308d397bb0a8f3f21d530
> Author: Andrew Dunstan <andrew@dunslane.net>
> Date:   Thu Apr 6 22:11:21 2017 -0400
>
>     Make json_populate_record and friends operate recursively

> As far as I can tell, this was not an intended change, so I agree it is
> probably a bug.

I agree.  It looks to me like the problem is this over-optimistic
assumption:

            /*
             * Add quotes around string value (should be already escaped) if
             * converting to json/jsonb.
             */

No, it's *not* already escaped.  Fixing the code to use escape_json()
is a bit tedious, because for some reason that function wasn't designed
to support non-null-terminated input, but with the attached patch we get
what seems to me like sane behavior:

regression=# select * from json_to_record('{"out": "{\"key\": 1}"}') as (out json);
      out
----------------
 "{\"key\": 1}"
(1 row)

regression=# select * from json_to_record('{"out": {"key": 1}}') as (out json);
    out
------------
 {"key": 1}
(1 row)

i.e. what you get is either a string or an object, the same as it was in
the input (same for all combinations of json/jsonb in/out).

Not terribly surprisingly, this patch changes no existing regression test
cases.  We should add some, but I've not done so here.

Also, modulo this bug for the json-input case, this *is* an intentional
behavioral change from 9.6, but it seems fairly poorly documented to
me.  Should we try to improve that?

            regards, tom lane

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 28bdc72..9e7035c 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -2803,26 +2803,7 @@ populate_scalar(ScalarIOData *io, Oid typid, int32 typmod, JsValue *jsv)

         json = jsv->val.json.str;
         Assert(json);
-
-        /* already done the hard work in the json case */
-        if ((typid == JSONOID || typid == JSONBOID) &&
-            jsv->val.json.type == JSON_TOKEN_STRING)
-        {
-            /*
-             * Add quotes around string value (should be already escaped) if
-             * converting to json/jsonb.
-             */
-
-            if (len < 0)
-                len = strlen(json);
-
-            str = palloc(len + sizeof(char) * 3);
-            str[0] = '"';
-            memcpy(&str[1], json, len);
-            str[len + 1] = '"';
-            str[len + 2] = '\0';
-        }
-        else if (len >= 0)
+        if (len >= 0)
         {
             /* Need to copy non-null-terminated string */
             str = palloc(len + 1 * sizeof(char));
@@ -2830,7 +2811,21 @@ populate_scalar(ScalarIOData *io, Oid typid, int32 typmod, JsValue *jsv)
             str[len] = '\0';
         }
         else
-            str = json;            /* null-terminated string */
+            str = json;            /* string is already null-terminated */
+
+        /* If converting to json/jsonb, make string into valid JSON literal */
+        if ((typid == JSONOID || typid == JSONBOID) &&
+            jsv->val.json.type == JSON_TOKEN_STRING)
+        {
+            StringInfoData buf;
+
+            initStringInfo(&buf);
+            escape_json(&buf, str);
+            /* free temporary buffer */
+            if (str != json)
+                pfree(str);
+            str = buf.data;
+        }
     }
     else
     {