Subject: Re: json_query conditional wrapper bug

From: Peter Eisentraut <[email-masked]>
Date: Wed, 4 Sep 2024 12:16:31 +0200

On 28.08.24 11:21, Peter Eisentraut wrote:
> These are ok:
> 
> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without wrapper);
>   json_query
> ------------
>   42
> 
> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with 
> unconditional wrapper);
>   json_query
> ------------
>   [42]
> 
> But this appears to be wrong:
> 
> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with conditional 
> wrapper);
>   json_query
> ------------
>   [42]
> 
> This should return an unwrapped 42.

If I make the code change illustrated in the attached patch, then I get 
the correct result here.  And various regression test results change, 
which, to me, all look more correct after this patch.  I don't know what 
the code I removed was supposed to accomplish, but it seems to be wrong 
somehow.  In the current implementation, the WITH CONDITIONAL WRAPPER 
clause doesn't appear to work correctly in any case I could identify.
From ea8a36584abf6cea0a05ac43ad8d101012b14313 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Wed, 4 Sep 2024 12:09:35 +0200
Subject: [PATCH] WIP: Fix JSON_QUERY WITH CONDITIONAL WRAPPER

---
 src/backend/utils/adt/jsonpath_exec.c       |  5 +----
 src/test/regress/sql/sqljson_queryfuncs.sql | 10 +++++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index e3ee0093d4d..a55a34685f0 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -3957,10 +3957,7 @@ JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty,
 	else if (wrapper == JSW_UNCONDITIONAL)
 		wrap = true;
 	else if (wrapper == JSW_CONDITIONAL)
-		wrap = count > 1 ||
-			IsAJsonbScalar(singleton) ||
-			(singleton->type == jbvBinary &&
-			 JsonContainerIsScalar(singleton->val.binary.data));
+		wrap = count > 1;
 	else
 	{
 		elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);
diff --git a/src/test/regress/sql/sqljson_queryfuncs.sql b/src/test/regress/sql/sqljson_queryfuncs.sql
index 21ff7787a28..a7342416a9c 100644
--- a/src/test/regress/sql/sqljson_queryfuncs.sql
+++ b/src/test/regress/sql/sqljson_queryfuncs.sql
@@ -146,11 +146,11 @@ CREATE DOMAIN rgb AS rainbow CHECK (VALUE IN ('red', 'green', 'blue'));
 SELECT JSON_VALUE(NULL::jsonb, '$');
 
 SELECT
-	JSON_QUERY(js, '$'),
-	JSON_QUERY(js, '$' WITHOUT WRAPPER),
-	JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER),
-	JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER),
-	JSON_QUERY(js, '$' WITH ARRAY WRAPPER)
+	JSON_QUERY(js, '$') AS "unspec",
+	JSON_QUERY(js, '$' WITHOUT WRAPPER) AS "without",
+	JSON_QUERY(js, '$' WITH CONDITIONAL WRAPPER) AS "with cond",
+	JSON_QUERY(js, '$' WITH UNCONDITIONAL ARRAY WRAPPER) AS "with uncond",
+	JSON_QUERY(js, '$' WITH ARRAY WRAPPER) AS "with"
 FROM
 	(VALUES
 		(jsonb 'null'),
-- 
2.46.0

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 4 Sep 2024 16:10:37 -0400

On 2024-09-04 We 6:16 AM, Peter Eisentraut wrote:
>> conditional wrapper);
> If I make the code change illustrated in the attached patch, then I 
> get the correct result here.  And various regression test results 
> change, which, to me, all look more correct after this patch.  I don't 
> know what the code I removed was supposed to accomplish, but it seems 
> to be wrong somehow.  In the current implementation, the WITH 
> CONDITIONAL WRAPPER clause doesn't appear to work correctly in any 
> case I could identify.

Agree the code definitely looks wrong. If anything the test should 
probably be reversed:

         wrap = count > 1  || !(
             IsAJsonbScalar(singleton) ||
             (singleton->type == jbvBinary &&
JsonContainerIsScalar(singleton->val.binary.data)));

i.e. in the count = 1 case wrap unless it's a scalar or a binary 
wrapping a scalar. The code could do with a comment about the logic.

I know we're very close to release but we should fix this as it's a new 
feature.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Amit Langote <[email-masked]>
Date: Tue, 10 Sep 2024 17:00:17 +0900

Sorry for missing this report and thanks Andrew for the offlist heads up.

> clause doesn't appear to work correctly in any case I could identify.

Agreed that this looks wrong.

I've wondered why the condition was like that but left it as-is,
because I thought at one point that that's needed to ensure that the
returned single scalar SQL/JSON item is valid jsonb.

I've updated your patch to include updated test outputs and a nearby
code comment expanded.  Do you intend to commit it or do you prefer
that I do?

-- 
Thanks, Amit Langote

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 10 Sep 2024 22:15:55 +0200

> code comment expanded.  Do you intend to commit it or do you prefer

This change looks unrelated:

-ERROR:  new row for relation "test_jsonb_constraints" violates check 
constraint "test_jsonb_constraint4"
+ERROR:  new row for relation "test_jsonb_constraints" violates check 
constraint "test_jsonb_constraint5"

Is this some randomness in the way these constraints are evaluated?