Bug in jsonb_path_exists (maybe _match) one-element scalar/variable jsonpath handling

Ping (+ cc'ing Alexander who committed this)

On Wed, Nov 23, 2022 at 10:31 PM David G. Johnston <
david.g.johnston@gmail.com> wrote:

Hi, David!

Thank you for the report.

On Thu, Nov 24, 2022 at 8:31 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

The following query produces an incorrect result. It should error (or at worse produce "false"), but it instead produces "true" (this applies to @? too)

select jsonb_path_exists('{"foo": true}'::jsonb, '$bar', '{}', false);

Yes, this definitely looks incorrect.

Specifically, since exists doesn't care about values, just presence, found is false, and since the variable is the only thing present, hasNext is also false. Thus we simply return jperOK without ever checking to see what the variable actually is. This results in the exists code producing a true result.

Looking at this more, it isn't just the variable case that ends up producing the wrong answer. Going by the principle that any function call of jsonb_path_exists that returns true should produce said match when executing jsonb_path_match, this is also broken for the rest (probably) of the matched types in the case group. And indeed, if the variable "bar" is defined the error in the match case just changes to "single boolean result is expected".

Variable case is definitely broken, but I don't think other cases are
broken. If we're checking for existence and there is a constant, we
can immediately return true because constant exists indeed. That
logic doesn't work for variable, which could be non-existent.

select jsonb_path_exists('{"foo": true}'::jsonb, '"bar"', '{}', false); -- true (bar in double quotes)
select jsonb_path_match('{"foo": true}'::jsonb, '"bar"', '{}', false); -- ERROR: single boolean result is expected
select jsonb_path_match('{"foo": true}'::jsonb, '$bar', '{"bar":"foo"}', false); -- same error as above, as expected

I expect the missing variable specification to produce jperError and the rest of the block to produce jperNotFound. The "single boolean result expected" error seems incorrect though I'm not sure where that is coming from. But I'm also not considering, or am even aware of, what the standard we are guided by here says should actually happen.

I think jsonb_path_match() behaves correctly, it expects jsonpatch
expression to return single boolend and throws an error otherwise.
BTW, do you mean something like this: jsonb_path_match() equivalent to
jsonb_path_match() expression?

select jsonb_path_match('{"foo": true}'::jsonb, 'exists($bar)',
'{"bar":"foo"}', false);

Draft patch fixing the issue is attached. Let me know what you think
about this.

------
Regards,
Alexander Korotkov

On Fri, Dec 2, 2022 at 5:18 AM Alexander Korotkov <aekorotkov@gmail.com>
wrote:

On Thu, Nov 24, 2022 at 8:31 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

The following query produces an incorrect result. It should error (or

at worse produce "false"), but it instead produces "true" (this applies to
@? too)

select jsonb_path_exists('{"foo": true}'::jsonb, '$bar', '{}', false);

Variable case is definitely broken, but I don't think other cases are
broken. If we're checking for existence and there is a constant, we
can immediately return true because constant exists indeed. That
logic doesn't work for variable, which could be non-existent.

select jsonb_path_exists('{"foo": true}'::jsonb, '"bar"', '{}', false);

-- true (bar in double quotes)

I think my issue with the constant is that the function itself is said to
return whether or not the provided path matches the input json. It is
impossible to match the input json if there is no reference to the input
json in the jsonpath expression. As the existing wording promises: "Checks
whether the JSON path returns any item for the specified JSON value" - the
word item is rightly taken to mean that the path at minimum references the
root (i.e., mandatory $) - and that any true result from exists will, if
the expression is used for _match, produce the "item for the specified JSON
value" that was found.

So I'll stand by my conclusion that the behavior of constants is buggy -
though I suppose fixing the bug is probably most readily accomplished by
changing the definition of what behavior we are promising and fixing up the
documentation to express that change. In short, it is really an error to
not specify "$" in your expression - but if you don't you will simply get a
true outcome for the existence test - for backward compatibility reasons.

select jsonb_path_match('{"foo": true}'::jsonb, '"bar"', '{}', false);

-- ERROR: single boolean result is expected

select jsonb_path_match('{"foo": true}'::jsonb, '$bar', '{"bar":"foo"}',

false); -- same error as above, as expected

I expect the missing variable specification to produce jperError and the

rest of the block to produce jperNotFound. The "single boolean result
expected" error seems incorrect though I'm not sure where that is coming
from. But I'm also not considering, or am even aware of, what the standard
we are guided by here says should actually happen.

I think jsonb_path_match() behaves correctly, it expects jsonpatch
expression to return single boolend and throws an error otherwise.

Yeah, I may have mis-interpreted the meaning of the error message.
Something like: "jsonpath expression must produce a single boolean result"
would be a bit more clear.

David J.

On Fri, Dec 2, 2022 at 5:24 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Fri, Dec 2, 2022 at 5:18 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:

On Thu, Nov 24, 2022 at 8:31 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

The following query produces an incorrect result. It should error (or at worse produce "false"), but it instead produces "true" (this applies to @? too)

select jsonb_path_exists('{"foo": true}'::jsonb, '$bar', '{}', false);

Variable case is definitely broken, but I don't think other cases are
broken. If we're checking for existence and there is a constant, we
can immediately return true because constant exists indeed. That
logic doesn't work for variable, which could be non-existent.

select jsonb_path_exists('{"foo": true}'::jsonb, '"bar"', '{}', false); -- true (bar in double quotes)

I think my issue with the constant is that the function itself is said to return whether or not the provided path matches the input json. It is impossible to match the input json if there is no reference to the input json in the jsonpath expression. As the existing wording promises: "Checks whether the JSON path returns any item for the specified JSON value" - the word item is rightly taken to mean that the path at minimum references the root (i.e., mandatory $) - and that any true result from exists will, if the expression is used for _match, produce the "item for the specified JSON value" that was found.

So I'll stand by my conclusion that the behavior of constants is buggy - though I suppose fixing the bug is probably most readily accomplished by changing the definition of what behavior we are promising and fixing up the documentation to express that change. In short, it is really an error to not specify "$" in your expression - but if you don't you will simply get a true outcome for the existence test - for backward compatibility reasons.

Thank you for explaining your point, but I can't agree with that.
Constant jsonpath expression is always returning item for the input
JSON value. Even despite the input value is ignored. This is
redundant case, but still correct.

select jsonb_path_match('{"foo": true}'::jsonb, '"bar"', '{}', false); -- ERROR: single boolean result is expected
select jsonb_path_match('{"foo": true}'::jsonb, '$bar', '{"bar":"foo"}', false); -- same error as above, as expected

I expect the missing variable specification to produce jperError and the rest of the block to produce jperNotFound. The "single boolean result expected" error seems incorrect though I'm not sure where that is coming from. But I'm also not considering, or am even aware of, what the standard we are guided by here says should actually happen.

I think jsonb_path_match() behaves correctly, it expects jsonpatch
expression to return single boolend and throws an error otherwise.

Yeah, I may have mis-interpreted the meaning of the error message. Something like: "jsonpath expression must produce a single boolean result" would be a bit more clear.

OK, I'm not a native English speaker and can't judge about this. I
propose this should be considered separately.

BTW, what do you think about the patch?

------
Regards,
Alexander Korotkov

On Fri, Dec 2, 2022 at 5:57 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:

On Fri, Dec 2, 2022 at 5:24 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Fri, Dec 2, 2022 at 5:18 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:

On Thu, Nov 24, 2022 at 8:31 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

The following query produces an incorrect result. It should error (or at worse produce "false"), but it instead produces "true" (this applies to @? too)

select jsonb_path_exists('{"foo": true}'::jsonb, '$bar', '{}', false);

Variable case is definitely broken, but I don't think other cases are
broken. If we're checking for existence and there is a constant, we
can immediately return true because constant exists indeed. That
logic doesn't work for variable, which could be non-existent.

select jsonb_path_exists('{"foo": true}'::jsonb, '"bar"', '{}', false); -- true (bar in double quotes)

I think my issue with the constant is that the function itself is said to return whether or not the provided path matches the input json. It is impossible to match the input json if there is no reference to the input json in the jsonpath expression. As the existing wording promises: "Checks whether the JSON path returns any item for the specified JSON value" - the word item is rightly taken to mean that the path at minimum references the root (i.e., mandatory $) - and that any true result from exists will, if the expression is used for _match, produce the "item for the specified JSON value" that was found.

So I'll stand by my conclusion that the behavior of constants is buggy - though I suppose fixing the bug is probably most readily accomplished by changing the definition of what behavior we are promising and fixing up the documentation to express that change. In short, it is really an error to not specify "$" in your expression - but if you don't you will simply get a true outcome for the existence test - for backward compatibility reasons.

Thank you for explaining your point, but I can't agree with that.
Constant jsonpath expression is always returning item for the input
JSON value. Even despite the input value is ignored. This is
redundant case, but still correct.

Let me explain more what I do mean. In the SQL SELECT statement there
is a WHERE clause. This clause should express the predicate, which
should match to rows. But you're writing "WHERE 1 = 1" or "WHERE
true" then all rows are matching even that no column is referenced.
This is how SQL is working. And I see no reason why jsonpath should
work in a different way.

------
Regards,
Alexander Korotkov

On Fri, Dec 2, 2022 at 10:47 AM Alexander Korotkov <aekorotkov@gmail.com>
wrote:

Thank you for explaining your point, but I can't agree with that.
Constant jsonpath expression is always returning item for the input
JSON value. Even despite the input value is ignored. This is
redundant case, but still correct.

Let me explain more what I do mean. In the SQL SELECT statement there
is a WHERE clause. This clause should express the predicate, which
should match to rows. But you're writing "WHERE 1 = 1" or "WHERE
true" then all rows are matching even that no column is referenced.
This is how SQL is working. And I see no reason why jsonpath should
work in a different way.

I like the analogy but it seems to support my conclusion moreso than yours:

Consider: select jsonb_path_exists('{"foo":"bar"}'::jsonb, 'false');

The analogous SQL query is: "SELECT * FROM table WHERE false" would
indeed produce an empty set - which EXISTS would interpret as false but you
want to evaluate to true

Or, "SELECT * FROM table WHERE 'banana';" which produces the same kind of
error that I wish jsonb_path_exists would produce when one writes a
similarly nonsensical path.

David J.

I'll probably get to a formal review of the patch - but actually I am
hoping someone else more comfortable in the codebase chimes in here with an
opinion. Though as I said, I'm willing to concede that the behavior should
probably stay unchanged, for compatibility reasons, and we just need to
decide on how to correctly document this.

On Fri, Dec 2, 2022 at 10:40 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Fri, Dec 2, 2022 at 10:47 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:

Thank you for explaining your point, but I can't agree with that.
Constant jsonpath expression is always returning item for the input
JSON value. Even despite the input value is ignored. This is
redundant case, but still correct.

Let me explain more what I do mean. In the SQL SELECT statement there
is a WHERE clause. This clause should express the predicate, which
should match to rows. But you're writing "WHERE 1 = 1" or "WHERE
true" then all rows are matching even that no column is referenced.
This is how SQL is working. And I see no reason why jsonpath should
work in a different way.

I like the analogy but it seems to support my conclusion moreso than yours:

Consider: select jsonb_path_exists('{"foo":"bar"}'::jsonb, 'false');

The analogous SQL query is: "SELECT * FROM table WHERE false" would indeed produce an empty set - which EXISTS would interpret as false but you want to evaluate to true

Or, "SELECT * FROM table WHERE 'banana';" which produces the same kind of error that I wish jsonb_path_exists would produce when one writes a similarly nonsensical path.

I think this is cross-analogy existing to matching, which doesn't
work. jsonb_path_exists() has existence symantic, while simple where
clause doesn't.

I think
"select jsonb_path_match('{"foo":"bar"}'::jsonb, 'false');"
is equivalent to
"SELECT * FROM table WHERE false;"

"select jsonb_path_exists('{"foo":"bar"}'::jsonb, '"match"');"
is equivalent to
"SELECT * FROM table WHERE EXISTS (SELECT 'match');"

------
Regards,
Alexander Korotkov

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?

------
Regards,
Alexander Korotkov

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.

On Thu, Dec 8, 2022 at 1:52 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

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.

This makes sense to me. See the attached patch implementing this.
I'm going to push and backpatch it if no objections.

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.

I would appreciate if you could work on such patch. If so, feel free
to post it.

------
Regards,
Alexander Korotkov

On Sun, Jan 8, 2023 at 2:19 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:

On Thu, Dec 8, 2022 at 1:52 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

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.

This makes sense to me. See the attached patch implementing this.
I'm going to push and backpatch it if no objections.

Pushed and backpatched to 12, where jsonpath first appeared.

------
Regards,
Alexander Korotkov

On Thu, Jan 12, 2023 at 8:31 AM Alexander Korotkov <aekorotkov@gmail.com>
wrote:

Pushed and backpatched to 12, where jsonpath first appeared.

Thanks. I've created a todo to take a peek at the docs around this.

David J.