On Fri, Jun 21, 2024 at 8:18 PM Amit Langote <amitlangot...@gmail.com> wrote:
>
> >> JSON_VALUE on error, on empty semantics should be the same as json_query.
> >> like:
> >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> >> ON EMPTY ]
> >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> >> ON ERROR ])
> >>
> >> examples:
> >> select JSON_value(jsonb '[]' , '$' empty array on error);
> >> select JSON_value(jsonb '[]' , '$' empty object on error);
> >
> > Again the documented behavior seems to make sense though and the ability to
> > specify empty in the value function seems like a bug. If you really want
> > an empty array or object you do have access to default. The reason
> > json_query provides for an empty array/object is that it is already
> > expecting to produce an array (object seems a little odd).
> >
> > I agree our docs and code do not match which needs to be fixed, ideally in
> > the direction of the standard which I'm guessing our documentation is based
> > off of. But let's not go off of my guess.
>
> Oops, that is indeed not great and, yes, the problem is code not
> matching the documentation, the latter of which is "correct".
>
> Basically, the grammar allows specifying any of the all possible ON
> ERROR/EMPTY behavior values irrespective of the function, so parse
> analysis should be catching and flagging an attempt to specify
> incompatible value for a given function, which it does not.
>
> I've attached a patch to fix that, which I'd like to push before
> anything else we've been discussing.
>
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON ERROR behavior"),
+ errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
<value> is allowed in ON ERROR for JSON_QUERY()."),
+ parser_errposition(pstate, func->on_error->location));
`EMPTY [ ARRAY | OBJECT }` seems not correct,
maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
(apply to other places)
`DEFAULT <value>`
`DEFAULT <expression>` or just `DEFAULT expression` would be more correct?
(apply to other places)
I think we should make json_query, json_value on empty, on error
behave the same way.
otherwise, it will have consistency issues for scalar jsonb.
for example, we should expect the following two queries to return the
same result?
SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);
Also the json_table function will call json_value or json_query,
make these two functions on error, on empty behavior the same can
reduce unintended complexity.
So based on your
patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
and the above points, I have made some changes, attached.
it will make json_value, json_query not allow {true | false | unknown
} on error, {true | false | unknown } on empty.
json_table error message deal the same way as
b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af
BTW,
i found one JSON_TABLE document deficiency
[ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
expression } ON EMPTY ]
[ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
expression } ON ERROR ]
it should be
[ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
expression } ON EMPTY ]
[ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
expression } ON ERROR ]
From 0a8b756a56f3750ad07f09d7933c9cae0218ec93 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Sat, 22 Jun 2024 17:28:53 +0800
Subject: [PATCH v2 2/2] Disallow incompatible values in ON ERROR/EMPTY
make json_exists only allow
{ TRUE | FALSE | UNKNOWN | ERROR } ON ERROR
make json_query, json_value only allow
{ ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression } ON EMPTY
{ ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression } ON ERROR
make json_value, json_query on empty, on error behave the same way,
because for scalar jsonb, json_value, json_query behave the same.
so sometimes they may need to return the same value.
also json_table function may call json_value or json_query,
make these two function on error, on empty behavior the same can reduce unintended complex.
---
src/backend/parser/parse_expr.c | 143 ++++++++++--------
.../regress/expected/sqljson_jsontable.out | 30 ++++
.../regress/expected/sqljson_queryfuncs.out | 11 +-
src/test/regress/sql/sqljson_jsontable.sql | 7 +
src/test/regress/sql/sqljson_queryfuncs.sql | 2 +-
5 files changed, 126 insertions(+), 67 deletions(-)
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 3d79d171..f3ffa88f 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -4299,74 +4299,95 @@ transformJsonFuncExpr(ParseState *pstate, JsonFuncExpr *func)
parser_errposition(pstate, format->location));
}
- /* OMIT QUOTES is meaningless when strings are wrapped. */
+ /* check json_query, json_value, on error, on empty behavior */
+ if (func->op == JSON_QUERY_OP || func->op == JSON_VALUE_OP)
+ {
+ if (func->on_empty != NULL &&
+ (func->on_empty->btype == JSON_BEHAVIOR_TRUE ||
+ func->on_empty->btype == JSON_BEHAVIOR_FALSE ||
+ func->on_empty->btype == JSON_BEHAVIOR_UNKNOWN))
+ {
+ if (func->column_name)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON EMPTY behavior"),
+ errdetail("Only ERROR, NULL, EMPTY, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT expression is allowed in ON EMPTY for column \"%s\".",
+ func->column_name),
+ parser_errposition(pstate, func->on_empty->location));
+ else
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON EMPTY behavior"),
+ errdetail("Only ERROR, NULL, EMPTY, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT expression is allowed in ON EMPTY for %s().",
+ func_name),
+ parser_errposition(pstate, func->on_empty->location));
+ }
+
+ if (func->on_error != NULL &&
+ (func->on_error->btype == JSON_BEHAVIOR_TRUE ||
+ func->on_error->btype == JSON_BEHAVIOR_FALSE ||
+ func->on_error->btype == JSON_BEHAVIOR_UNKNOWN))
+ {
+ if (func->column_name)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON ERROR behavior"),
+ errdetail("Only ERROR, NULL, EMPTY, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT expression is allowed in ON ERROR for column \"%s\".",
+ func->column_name),
+ parser_errposition(pstate, func->on_error->location));
+ else
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON ERROR behavior"),
+ errdetail("Only ERROR, NULL, EMPTY, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT expression is allowed in ON ERROR for %s().",
+ func_name),
+ parser_errposition(pstate, func->on_error->location));
+ }
+ }
+
+ /* Check that ON ERROR/EMPTY behavior values are valid for json_exists. */
+ if (func->op == JSON_EXISTS_OP)
+ {
+ if (func->column_name && func->on_empty != NULL)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON EMPTY behavior"),
+ errdetail("ON EMPTY is not allowed for column \"%s\".",
+ func->column_name),
+ parser_errposition(pstate, func->on_empty->location));
+
+ if (func->on_error != NULL &&
+ func->on_error->btype != JSON_BEHAVIOR_ERROR &&
+ func->on_error->btype != JSON_BEHAVIOR_TRUE &&
+ func->on_error->btype != JSON_BEHAVIOR_FALSE &&
+ func->on_error->btype != JSON_BEHAVIOR_UNKNOWN)
+ {
+ if (!func->column_name)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON ERROR behavior"),
+ errdetail("Only ERROR, TRUE, FALSE, or UNKNOWN is allowed in ON ERROR for JSON_EXISTS()."),
+ parser_errposition(pstate, func->on_error->location));
+ else
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid ON ERROR behavior"),
+ errdetail("Only ERROR, TRUE, FALSE, or UNKNOWN is allowed in ON ERROR for column \"%s\".",
+ func->column_name),
+ parser_errposition(pstate, func->on_error->location));
+ }
+ }
+
if (func->op == JSON_QUERY_OP)
{
+ /* OMIT QUOTES is meaningless when strings are wrapped. */
if (func->quotes == JS_QUOTES_OMIT &&
- (func->wrapper == JSW_CONDITIONAL ||
- func->wrapper == JSW_UNCONDITIONAL))
+ (func->wrapper == JSW_CONDITIONAL ||
+ func->wrapper == JSW_UNCONDITIONAL))
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("SQL/JSON QUOTES behavior must not be specified when WITH WRAPPER is used"),
parser_errposition(pstate, func->location));
- if (func->on_empty != NULL &&
- func->on_empty->btype != JSON_BEHAVIOR_ERROR &&
- func->on_empty->btype != JSON_BEHAVIOR_NULL &&
- func->on_empty->btype != JSON_BEHAVIOR_EMPTY &&
- func->on_empty->btype != JSON_BEHAVIOR_EMPTY_ARRAY &&
- func->on_empty->btype != JSON_BEHAVIOR_EMPTY_OBJECT &&
- func->on_empty->btype != JSON_BEHAVIOR_DEFAULT)
- ereport(ERROR,
- errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid ON EMPTY behavior"),
- errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT <value> is allowed in ON EMPTY for JSON_QUERY()."),
- parser_errposition(pstate, func->on_empty->location));
- if (func->on_error != NULL &&
- func->on_error->btype != JSON_BEHAVIOR_ERROR &&
- func->on_error->btype != JSON_BEHAVIOR_NULL &&
- func->on_error->btype != JSON_BEHAVIOR_EMPTY &&
- func->on_error->btype != JSON_BEHAVIOR_EMPTY_ARRAY &&
- func->on_error->btype != JSON_BEHAVIOR_EMPTY_OBJECT &&
- func->on_error->btype != JSON_BEHAVIOR_DEFAULT)
- ereport(ERROR,
- errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid ON ERROR behavior"),
- errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT <value> is allowed in ON ERROR for JSON_QUERY()."),
- parser_errposition(pstate, func->on_error->location));
- }
-
- /* Check that ON ERROR/EMPTY behavior values are valid for the function. */
- if (func->op == JSON_EXISTS_OP &&
- func->on_error != NULL &&
- func->on_error->btype != JSON_BEHAVIOR_ERROR &&
- func->on_error->btype != JSON_BEHAVIOR_TRUE &&
- func->on_error->btype != JSON_BEHAVIOR_FALSE &&
- func->on_error->btype != JSON_BEHAVIOR_UNKNOWN)
- ereport(ERROR,
- errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid ON ERROR behavior"),
- errdetail("Only ERROR, TRUE, FALSE, or UNKNOWN is allowed in ON ERROR for JSON_EXISTS()."),
- parser_errposition(pstate, func->on_error->location));
- if (func->op == JSON_VALUE_OP)
- {
- if (func->on_empty != NULL &&
- func->on_empty->btype != JSON_BEHAVIOR_ERROR &&
- func->on_empty->btype != JSON_BEHAVIOR_NULL &&
- func->on_empty->btype != JSON_BEHAVIOR_DEFAULT)
- ereport(ERROR,
- errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid ON ERROR behavior"),
- errdetail("Only ERROR, NULL, or DEFAULT <value> is allowed in ON EMPTY for JSON_VALUE()."),
- parser_errposition(pstate, func->on_empty->location));
- if (func->on_error != NULL &&
- func->on_error->btype != JSON_BEHAVIOR_ERROR &&
- func->on_error->btype != JSON_BEHAVIOR_NULL &&
- func->on_error->btype != JSON_BEHAVIOR_DEFAULT)
- ereport(ERROR,
- errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid ON EMPTY behavior"),
- errdetail("Only ERROR, NULL, or DEFAULT <value> is allowed in ON ERROR for JSON_VALUE()."),
- parser_errposition(pstate, func->on_error->location));
}
jsexpr = makeNode(JsonExpr);
diff --git a/src/test/regress/expected/sqljson_jsontable.out b/src/test/regress/expected/sqljson_jsontable.out
index eace29ef..8d372764 100644
--- a/src/test/regress/expected/sqljson_jsontable.out
+++ b/src/test/regress/expected/sqljson_jsontable.out
@@ -1073,3 +1073,33 @@ ERROR: invalid ON ERROR behavior
LINE 1: ... * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int) NULL ON ER...
^
DETAIL: Only EMPTY or ERROR is allowed in the top-level ON ERROR clause.
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int true on empty));
+ERROR: invalid ON EMPTY behavior
+LINE 1: ...T * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int true on em...
+ ^
+DETAIL: Only ERROR, NULL, EMPTY, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT expression is allowed in ON EMPTY for column "a".
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int omit quotes true on error));
+ERROR: invalid ON ERROR behavior
+LINE 1: ...N_TABLE(jsonb '1', '$' COLUMNS (a int omit quotes true on er...
+ ^
+DETAIL: Only ERROR, NULL, EMPTY, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT expression is allowed in ON ERROR for column "a".
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int keep quotes empty on error));
+ERROR: cannot cast jsonb array to type integer
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int keep quotes empty object on error));
+ERROR: cannot cast jsonb object to type integer
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a jsonb keep quotes empty object on empty)); --ok
+ a
+----
+ {}
+(1 row)
+
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int exists empty object on error));
+ERROR: invalid ON ERROR behavior
+LINE 1: ...M JSON_TABLE(jsonb '1', '$' COLUMNS (a int exists empty obje...
+ ^
+DETAIL: Only ERROR, TRUE, FALSE, or UNKNOWN is allowed in ON ERROR for column "a".
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int exists true on empty));
+ERROR: invalid ON EMPTY behavior
+LINE 1: ...M JSON_TABLE(jsonb '1', '$' COLUMNS (a int exists true on em...
+ ^
+DETAIL: ON EMPTY is not allowed for column "a".
diff --git a/src/test/regress/expected/sqljson_queryfuncs.out b/src/test/regress/expected/sqljson_queryfuncs.out
index 0c4b62b0..37bb5ad7 100644
--- a/src/test/regress/expected/sqljson_queryfuncs.out
+++ b/src/test/regress/expected/sqljson_queryfuncs.out
@@ -1360,12 +1360,13 @@ LINE 1: SELECT JSON_EXISTS(jsonb '1', '$' DEFAULT 1 ON ERROR);
^
DETAIL: Only ERROR, TRUE, FALSE, or UNKNOWN is allowed in ON ERROR for JSON_EXISTS().
SELECT JSON_VALUE(jsonb '1', '$' EMPTY ON ERROR);
-ERROR: invalid ON EMPTY behavior
-LINE 1: SELECT JSON_VALUE(jsonb '1', '$' EMPTY ON ERROR);
- ^
-DETAIL: Only ERROR, NULL, or DEFAULT <value> is allowed in ON ERROR for JSON_VALUE().
+ json_value
+------------
+ 1
+(1 row)
+
SELECT JSON_QUERY(jsonb '1', '$' TRUE ON ERROR);
ERROR: invalid ON ERROR behavior
LINE 1: SELECT JSON_QUERY(jsonb '1', '$' TRUE ON ERROR);
^
-DETAIL: Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT <value> is allowed in ON ERROR for JSON_QUERY().
+DETAIL: Only ERROR, NULL, EMPTY, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT expression is allowed in ON ERROR for JSON_QUERY().
diff --git a/src/test/regress/sql/sqljson_jsontable.sql b/src/test/regress/sql/sqljson_jsontable.sql
index 1f81464c..3bb49ba9 100644
--- a/src/test/regress/sql/sqljson_jsontable.sql
+++ b/src/test/regress/sql/sqljson_jsontable.sql
@@ -521,3 +521,10 @@ DROP TABLE s;
-- Test ON ERROR / EMPTY value validity for the function
SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int) NULL ON ERROR); -- fail
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int true on empty));
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int omit quotes true on error));
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int keep quotes empty on error));
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int keep quotes empty object on error));
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a jsonb keep quotes empty object on empty)); --ok
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int exists empty object on error));
+SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int exists true on empty));
diff --git a/src/test/regress/sql/sqljson_queryfuncs.sql b/src/test/regress/sql/sqljson_queryfuncs.sql
index 4586fdb8..0f7b6ec7 100644
--- a/src/test/regress/sql/sqljson_queryfuncs.sql
+++ b/src/test/regress/sql/sqljson_queryfuncs.sql
@@ -462,5 +462,5 @@ SELECT JSON_QUERY(jsonb 'null', '$xyz' PASSING 1 AS xyz);
-- Test ON ERROR / EMPTY value validity for the function; all fail.
SELECT JSON_EXISTS(jsonb '1', '$' DEFAULT 1 ON ERROR);
-SELECT JSON_VALUE(jsonb '1', '$' EMPTY ON ERROR);
+SELECT JSON_VALUE(jsonb '1', '$' UNKNOWN ON ERROR);
SELECT JSON_QUERY(jsonb '1', '$' TRUE ON ERROR);
--
2.34.1
