Subject: Re: PATCH: recursive json_populate_record()
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
 Andres Freund <andres@anarazel.de>
Cc: David Steele <david@pgmasters.net>,
 Michael Paquier <michael.paquier@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 Aleksander Alekseev <a.alekseev@postgrespro.ru>,
 PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
References: <687e19a7-14e2-d0f5-2bff-35d62bc5c05a@postgrespro.ru>
 <934.1485111512@sss.pgh.pa.us>
 <9e21a39c-c1d7-b9b5-44a0-c5345a5029f6@postgrespro.ru>
 <13020.1485377905@sss.pgh.pa.us>
 <295ad4a5-2923-1834-5140-35d0b9a5818c@postgrespro.ru>
 <16179.1485380983@sss.pgh.pa.us>
 <CAB7nPqSGyCxh9CqPXp5TTE+A0svyWvbO1sp3BT87VtDxtT4tLQ@mail.gmail.com>
 <9bf14a7c-0794-f90c-c012-8dd0335ab364@pgmasters.net>
 <bcb6edee-f124-d5f4-3314-09b5274bd328@pgmasters.net>
 <754780af-d905-9704-cf4d-b64e632b86a3@2ndQuadrant.com>
 <20170403211741.7ommm5dssfv3abo6@alap3.anarazel.de>
 <45c0aa55-1b62-c622-31fd-cb21e017d5cd@2ndQuadrant.com>
 <8fcd9e65-acb0-8221-ef23-01e5c2fe90e4@2ndQuadrant.com>
 <ad7bc08c-b5af-e4e7-cc61-0ea326f5aa42@2ndQuadrant.com>
From: Nikita Glukhov <n.gluhov@postgrespro.ru>
Organization: Postgres Professional
Message-ID: <fbd1d566-bba0-a3de-d6d0-d3b1d7c24ff2@postgrespro.ru>
Date: Mon, 22 May 2017 22:19:37 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.1
MIME-Version: 1.0
In-Reply-To: <ad7bc08c-b5af-e4e7-cc61-0ea326f5aa42@2ndQuadrant.com>
Content-Type: multipart/mixed;
 boundary="------------97DFA12F8ADB1DDEA068F484"
Content-Language: en-US
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

This is a multi-part message in MIME format.
--------------97DFA12F8ADB1DDEA068F484
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

Attached two small fixes for the previous committed patch:

1. I've noticed a difference in behavior between json_populate_record()
and  jsonb_populate_record() if we are trying to populate record from a
non-JSON-object: json function throws an error but jsonb function returns
a record with NULL fields. So I think it would be better to throw an error
in jsonb case too, but I'm not sure.

2. Also I've found a some kind of thinko in JsGetObjectSize() macro, but it
seems that this obvious mistake can not lead to incorrect behavior.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company


--------------97DFA12F8ADB1DDEA068F484
Content-Type: text/x-patch;
 name="fix-thinko-in-JsObjectSize-macro.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="fix-thinko-in-JsObjectSize-macro.patch"

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index ab9a745..a98742f 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -308,12 +308,11 @@ typedef struct JsObject
 	((jsv)->is_json ? (jsv)->val.json.type == JSON_TOKEN_STRING \
 		: ((jsv)->val.jsonb && (jsv)->val.jsonb->type == jbvString))
 
-#define JsObjectSize(jso) \
+#define JsObjectIsEmpty(jso) \
 	((jso)->is_json \
-		? hash_get_num_entries((jso)->val.json_hash) \
-		: !(jso)->val.jsonb_cont || JsonContainerSize((jso)->val.jsonb_cont))
-
-#define JsObjectIsEmpty(jso) (JsObjectSize(jso) == 0)
+		? hash_get_num_entries((jso)->val.json_hash) == 0 \
+		: (!(jso)->val.jsonb_cont || \
+		   JsonContainerSize((jso)->val.jsonb_cont) == 0))
 
 #define JsObjectFree(jso) do { \
 		if ((jso)->is_json) \

--------------97DFA12F8ADB1DDEA068F484
Content-Type: text/x-patch;
 name="fix-jsonb_populate_record-behavior-on-non-objects.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="fix-jsonb_populate_record-behavior-on-non-objects.patch"

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index a98742f..083982c 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -2683,7 +2683,20 @@ JsValueToJsObject(JsValue *jsv, JsObject *jso)
 			JsonContainerIsObject(jbv->val.binary.data))
 			jso->val.jsonb_cont = jbv->val.binary.data;
 		else
+		{
+			bool		is_scalar = IsAJsonbScalar(jbv) ||
+				(jbv->type == jbvBinary &&
+				 JsonContainerIsScalar(jbv->val.binary.data));
+
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg(is_scalar
+							? "cannot call %s on a scalar"
+							: "cannot call %s on an array",
+							"populate_composite")));
+
 			jso->val.jsonb_cont = NULL;
+		}
 	}
 }
 
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index f199eb4..7954703 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -2276,17 +2276,9 @@ SELECT jsa FROM jsonb_populate_record(NULL::jsbrec, '{"jsa": ["aaa", null, [1, 2
 (1 row)
 
 SELECT rec FROM jsonb_populate_record(NULL::jsbrec, '{"rec": 123}') q;
- rec  
-------
- (,,)
-(1 row)
-
+ERROR:  cannot call populate_composite on a scalar
 SELECT rec FROM jsonb_populate_record(NULL::jsbrec, '{"rec": [1, 2]}') q;
- rec  
-------
- (,,)
-(1 row)
-
+ERROR:  cannot call populate_composite on an array
 SELECT rec FROM jsonb_populate_record(NULL::jsbrec, '{"rec": {"a": "abc", "c": "01.02.2003", "x": 43.2}}') q;
                 rec                
 -----------------------------------
@@ -2303,11 +2295,7 @@ SELECT reca FROM jsonb_populate_record(NULL::jsbrec, '{"reca": 123}') q;
 ERROR:  expected json array
 HINT:  see the value of key "reca"
 SELECT reca FROM jsonb_populate_record(NULL::jsbrec, '{"reca": [1, 2]}') q;
-      reca       
------------------
- {"(,,)","(,,)"}
-(1 row)
-
+ERROR:  cannot call populate_composite on a scalar
 SELECT reca FROM jsonb_populate_record(NULL::jsbrec, '{"reca": [{"a": "abc", "b": 456}, null, {"c": "01.02.2003", "x": 43.2}]}') q;
                           reca                          
 --------------------------------------------------------

--------------97DFA12F8ADB1DDEA068F484
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--------------97DFA12F8ADB1DDEA068F484--