From: David G. Johnston <david.g.johnston@gmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Bug List <pgsql-bugs@lists.postgresql.org>
Subject: Re: Bug in jsonb_path_exists (maybe _match) one-element scalar/variable jsonpath handling
Date: Wed, 7 Dec 2022 15:52:20 -0700
Message-ID: <CAKFQuwZ_A_JFpBaHiNvR3Ti5X5L9Ut6csyARPygO53Chd_vcUA@mail.gmail.com> (raw)
In-Reply-To: <CAPpHfdtweZwYrf0uTXRWiFRW6_L0eAWMBQFH9fM-SCbwqikJUA@mail.gmail.com>
References: <CAKFQuwbeytffJkVnEqDyLZ=rQsznoTh1OgDoOF3VmOMkxcTMjA@mail.gmail.com>
<CAPpHfdvuHTjZLOWy1BEr0b1qw=mNXqw0OEndnLsdaw85ftDc-A@mail.gmail.com>
<CAPpHfdtweZwYrf0uTXRWiFRW6_L0eAWMBQFH9fM-SCbwqikJUA@mail.gmail.com>
On Mon, Dec 5, 2022 at 4:58 PM Alexander Korotkov <aekorotkov@gmail.com>
wrote:
> On Fri, Dec 2, 2022 at 3:18 PM Alexander Korotkov <aekorotkov@gmail.com>
> wrote:
> > Draft patch fixing the issue is attached. Let me know what you think
> > about this.
>
> Revised patch is attached, wrong pfree() is fixed. I was intended to
> backpatch it. But the behavior change makes me uneasy.
>
> select * from jsonb_path_query('{"a": 10}', '$ ? (@.a < $value)');
>
> Currently, this query generates an error because of missing "value"
> variable. The patch suppress this error. I'm not sure this error
> should be suppressed. Especially, I'm sure this should be
> backpatched.
>
> Should we fix only existence checking behaviour and let other cases
> throw an error? Thoughts?
>
>
I've attached some additional regression test changes to formally document
what it is we are affecting here. The "false" ones seems like it can
stand-in for all of the types left behind when the variable one got moved
to its own case.
The regressions.diffs file is the changes made by the 0001 patch.
Instead of making everything that today correctly produces a "could not
find jsonpath variable" error behave in a non-error way we need to make
_exists produce the exact same error. Aside from seemingly being correct
on its own merits, it is superior to turning what was a true outcome to a
false outcome, which is much more likely to go unnoticed and cause people
grief.
I feel like we are not adequately testing the "jspGetNext" true outcome of
the variable path but I still haven't fully gotten my head around the code.
The behavior of the introduced constant false jsonpath expression seems
internally consistent. Fixing the documentation to make it clear how such
an unusual but acceptable jsonpath expression behaves is material for a
separate patch.
David J.
Attachments:
regression.diffs (application/octet-stream, 1.8K) download0000-v1-Add-regression-tests-demonstrating-scalar-jsonpath-b.patch (application/octet-stream, 4.8K) download
From 48bfa0fd1c4c45d727b29e1dfd0f2a9ce3d39afa Mon Sep 17 00:00:00 2001
From: "David G. Johnston" <David.G.Johnston@Gmail.com>
Date: Wed, 7 Dec 2022 22:28:46 +0000
Subject: [PATCH] Add regression tests demonstrating scalar jsonpath behavior
---
src/test/regress/expected/jsonb_jsonpath.out | 36 ++++++++++++++++++++
src/test/regress/sql/jsonb_jsonpath.sql | 8 +++++
2 files changed, 44 insertions(+)
diff --git a/src/test/regress/expected/jsonb_jsonpath.out b/src/test/regress/expected/jsonb_jsonpath.outindex 508ddd797e..5fbbc8519d 100644--- a/src/test/regress/expected/jsonb_jsonpath.out+++ b/src/test/regress/expected/jsonb_jsonpath.out@@ -2212,6 +2212,14 @@ SELECT jsonb_path_query('[{"a": 1}, {"a": 2}]', '$[*] ? (@.a > 10)');
------------------
(0 rows)
+SELECT jsonb_path_query('[{"a": 1}]', '$undefined_var');+ERROR: could not find jsonpath variable "undefined_var"+SELECT jsonb_path_query('[{"a": 1}]', 'false');+ jsonb_path_query +------------------+ false+(1 row)+
SELECT jsonb_path_query_array('[{"a": 1}, {"a": 2}, {}]', 'strict $[*].a');
ERROR: JSON object does not contain key "a"
SELECT jsonb_path_query_array('[{"a": 1}, {"a": 2}]', '$[*].a');
@@ -2282,6 +2290,14 @@ SELECT jsonb_path_query_first('[{"a": 1}, {"a": 2}, {"a": 3}, {"a": 5}]', '$[*].
(1 row)
+SELECT jsonb_path_query_first('[{"a": 1}]', '$undefined_var');+ERROR: could not find jsonpath variable "undefined_var"+SELECT jsonb_path_query_first('[{"a": 1}]', 'false');+ jsonb_path_query_first +------------------------+ false+(1 row)+
SELECT jsonb '[{"a": 1}, {"a": 2}]' @? '$[*].a ? (@ > 1)';
?column?
----------
@@ -2312,6 +2328,18 @@ SELECT jsonb_path_exists('[{"a": 1}, {"a": 2}, {"a": 3}, {"a": 5}]', '$[*] ? (@.
f
(1 row)
+SELECT jsonb_path_exists('[{"a": 1}]', '$undefined_var');+ jsonb_path_exists +-------------------+ t+(1 row)++SELECT jsonb_path_exists('[{"a": 1}]', 'false');+ jsonb_path_exists +-------------------+ t+(1 row)+
SELECT jsonb_path_match('true', '$', silent => false);
jsonb_path_match
------------------
@@ -2374,6 +2402,14 @@ SELECT jsonb_path_match('[{"a": 1}, {"a": 2}]', '$[*].a > 1');
t
(1 row)
+SELECT jsonb_path_match('[{"a": 1}]', '$undefined_var');+ERROR: could not find jsonpath variable "undefined_var"+SELECT jsonb_path_match('[{"a": 1}]', 'false');+ jsonb_path_match +------------------+ f+(1 row)+
-- test string comparison (Unicode codepoint collation)
WITH str(j, num) AS
(
diff --git a/src/test/regress/sql/jsonb_jsonpath.sql b/src/test/regress/sql/jsonb_jsonpath.sqlindex 60f73cb059..bd025077d5 100644--- a/src/test/regress/sql/jsonb_jsonpath.sql+++ b/src/test/regress/sql/jsonb_jsonpath.sql@@ -532,6 +532,8 @@ set time zone default;
SELECT jsonb_path_query('[{"a": 1}, {"a": 2}]', '$[*]');
SELECT jsonb_path_query('[{"a": 1}, {"a": 2}]', '$[*] ? (@.a > 10)');
+SELECT jsonb_path_query('[{"a": 1}]', '$undefined_var');+SELECT jsonb_path_query('[{"a": 1}]', 'false');
SELECT jsonb_path_query_array('[{"a": 1}, {"a": 2}, {}]', 'strict $[*].a');
SELECT jsonb_path_query_array('[{"a": 1}, {"a": 2}]', '$[*].a');
@@ -547,12 +549,16 @@ SELECT jsonb_path_query_first('[{"a": 1}, {"a": 2}]', '$[*].a ? (@ == 1)');
SELECT jsonb_path_query_first('[{"a": 1}, {"a": 2}]', '$[*].a ? (@ > 10)');
SELECT jsonb_path_query_first('[{"a": 1}, {"a": 2}, {"a": 3}, {"a": 5}]', '$[*].a ? (@ > $min && @ < $max)', vars => '{"min": 1, "max": 4}');
SELECT jsonb_path_query_first('[{"a": 1}, {"a": 2}, {"a": 3}, {"a": 5}]', '$[*].a ? (@ > $min && @ < $max)', vars => '{"min": 3, "max": 4}');
+SELECT jsonb_path_query_first('[{"a": 1}]', '$undefined_var');+SELECT jsonb_path_query_first('[{"a": 1}]', 'false');
SELECT jsonb '[{"a": 1}, {"a": 2}]' @? '$[*].a ? (@ > 1)';
SELECT jsonb '[{"a": 1}, {"a": 2}]' @? '$[*] ? (@.a > 2)';
SELECT jsonb_path_exists('[{"a": 1}, {"a": 2}]', '$[*].a ? (@ > 1)');
SELECT jsonb_path_exists('[{"a": 1}, {"a": 2}, {"a": 3}, {"a": 5}]', '$[*] ? (@.a > $min && @.a < $max)', vars => '{"min": 1, "max": 4}');
SELECT jsonb_path_exists('[{"a": 1}, {"a": 2}, {"a": 3}, {"a": 5}]', '$[*] ? (@.a > $min && @.a < $max)', vars => '{"min": 3, "max": 4}');
+SELECT jsonb_path_exists('[{"a": 1}]', '$undefined_var');+SELECT jsonb_path_exists('[{"a": 1}]', 'false');
SELECT jsonb_path_match('true', '$', silent => false);
SELECT jsonb_path_match('false', '$', silent => false);
@@ -569,6 +575,8 @@ SELECT jsonb_path_match('[true, true]', '$[*]', silent => false);
SELECT jsonb '[{"a": 1}, {"a": 2}]' @@ '$[*].a > 1';
SELECT jsonb '[{"a": 1}, {"a": 2}]' @@ '$[*].a > 2';
SELECT jsonb_path_match('[{"a": 1}, {"a": 2}]', '$[*].a > 1');
+SELECT jsonb_path_match('[{"a": 1}]', '$undefined_var');+SELECT jsonb_path_match('[{"a": 1}]', 'false');
-- test string comparison (Unicode codepoint collation)
WITH str(j, num) AS
--
2.25.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: david.g.johnston@gmail.com, aekorotkov@gmail.com, tgl@sss.pgh.pa.us, pgsql-bugs@lists.postgresql.org
Subject: Re: Bug in jsonb_path_exists (maybe _match) one-element scalar/variable jsonpath handling
In-Reply-To: <CAKFQuwZ_A_JFpBaHiNvR3Ti5X5L9Ut6csyARPygO53Chd_vcUA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox