From 215360b921334c1810f35ad98a33eebadd874273 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Thu, 25 Jul 2024 22:13:12 +0900 Subject: [PATCH 3/5] SQL/JSON: Fix handling of errors coercing JsonBehavior expressions Instead of returning a NULL when the JsonBehavior value cannot be coerced to the RETURNING type, throw the error message informing the user that it is the JsonBehavior expression that caused the error with the actual error message show in the DETAIL. To ensure that the errors of executing a JsonBehavior expression that is coerced in the parser are caught instead of being thrown directly, pass ErrorSaveContext to ExecInitExprRec() when initializing it. Discussion: https://postgr.es/m/CACJufxEo4sUjKCYtda0_qt9tazqqKPmF1cqhW9KBOUeJFqQd2g@mail.gmail.com --- src/backend/executor/execExpr.c | 47 ++++++++++- src/backend/executor/execExprInterp.c | 81 +++++++++++++++++-- .../regress/expected/sqljson_jsontable.out | 40 ++++----- .../regress/expected/sqljson_queryfuncs.out | 34 +++----- 4 files changed, 146 insertions(+), 56 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index db1ec11390..23d907451d 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -4402,6 +4402,8 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, if (jsexpr->on_error && jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR) { + ErrorSaveContext *saved_escontext; + jsestate->jump_error = state->steps_len; /* JUMP to end if false, that is, skip the ON ERROR expression. */ @@ -4412,9 +4414,15 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, scratch->d.jump.jumpdone = -1; /* set below */ ExprEvalPushStep(state, scratch); - /* Steps to evaluate the ON ERROR expression */ + /* + * Steps to evaluate the ON ERROR expression; handle errors softly to + * rethrow them in COERCION_FINISH step that will be added later. + */ + saved_escontext = state->escontext; + state->escontext = escontext; ExecInitExprRec((Expr *) jsexpr->on_error->expr, state, resv, resnull); + state->escontext = saved_escontext; /* Step to coerce the ON ERROR expression if needed */ if (jsexpr->on_error->coerce) @@ -4422,6 +4430,19 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, jsexpr->omit_quotes, false, resv, resnull); + /* + * Add a COERCION_FINISH step to check for errors that may occur + * when coercing and rethrow them. + */ + if (jsexpr->on_error->coerce || + IsA(jsexpr->on_error->expr, CoerceViaIO) || + IsA(jsexpr->on_error->expr, CoerceToDomain)) + { + scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH; + scratch->d.jsonexpr.jsestate = jsestate; + ExprEvalPushStep(state, scratch); + } + /* JUMP to end to skip the ON EMPTY steps added below. */ jumps_to_end = lappend_int(jumps_to_end, state->steps_len); scratch->opcode = EEOP_JUMP; @@ -4436,6 +4457,8 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, if (jsexpr->on_empty != NULL && jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR) { + ErrorSaveContext *saved_escontext; + jsestate->jump_empty = state->steps_len; /* JUMP to end if false, that is, skip the ON EMPTY expression. */ @@ -4446,15 +4469,35 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, scratch->d.jump.jumpdone = -1; /* set below */ ExprEvalPushStep(state, scratch); - /* Steps to evaluate the ON EMPTY expression */ + /* + * Steps to evaluate the ON EMPTY expression; handle errors softly to + * rethrow them in COERCION_FINISH step that will be added later. + */ + saved_escontext = state->escontext; + state->escontext = escontext; ExecInitExprRec((Expr *) jsexpr->on_empty->expr, state, resv, resnull); + state->escontext = saved_escontext; /* Step to coerce the ON EMPTY expression if needed */ if (jsexpr->on_empty->coerce) ExecInitJsonCoercion(state, jsexpr->returning, escontext, jsexpr->omit_quotes, false, resv, resnull); + + /* + * Add a COERCION_FINISH step to check for errors that may occur + * when coercing and rethrow them. + */ + if (jsexpr->on_empty->coerce || + IsA(jsexpr->on_empty->expr, CoerceViaIO) || + IsA(jsexpr->on_empty->expr, CoerceToDomain)) + { + + scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH; + scratch->d.jsonexpr.jsestate = jsestate; + ExprEvalPushStep(state, scratch); + } } foreach(lc, jumps_to_end) diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index d463e831ed..ce102d7665 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -4284,13 +4284,12 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, memset(&jsestate->error, 0, sizeof(NullableDatum)); memset(&jsestate->empty, 0, sizeof(NullableDatum)); - /* - * Also reset ErrorSaveContext contents for the next row. Since we don't - * set details_wanted, we don't need to also reset error_data, which would - * be NULL anyway. - */ - Assert(!jsestate->escontext.details_wanted && - jsestate->escontext.error_data == NULL); + /* Also reset ErrorSaveContext contents for the next row. */ + if (jsestate->escontext.details_wanted) + { + jsestate->escontext.error_data = NULL; + jsestate->escontext.details_wanted = false; + } jsestate->escontext.error_occurred = false; switch (jsexpr->op) @@ -4394,6 +4393,15 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, error = true; } + /* + * When setting up the ErrorSaveContext (if needed) for capturing the + * errors that occur when coercing the JsonBehavior expression, set + * details_wanted to be able to show the actual error message as the + * DETAIL of the error message that tells that it is the + * JsonBehavior expression that caused the error; see + * ExecEvalJsonCoercionFinish(). + */ + /* Handle ON EMPTY. */ if (empty) { @@ -4404,6 +4412,9 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, if (jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR) { jsestate->empty.value = BoolGetDatum(true); + /* Set up to catch coercion errors of the ON EMPTY value. */ + jsestate->escontext.error_occurred = false; + jsestate->escontext.details_wanted = true; Assert(jsestate->jump_empty >= 0); return jsestate->jump_empty; } @@ -4411,6 +4422,9 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, else if (jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR) { jsestate->error.value = BoolGetDatum(true); + /* Set up to catch coercion errors of the ON ERROR value. */ + jsestate->escontext.error_occurred = false; + jsestate->escontext.details_wanted = true; Assert(!throw_error && jsestate->jump_error >= 0); return jsestate->jump_error; } @@ -4436,6 +4450,9 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, *op->resvalue = (Datum) 0; *op->resnull = true; jsestate->error.value = BoolGetDatum(true); + /* Set up to catch coercion errors of the ON ERROR value. */ + jsestate->escontext.error_occurred = false; + jsestate->escontext.details_wanted = true; return jsestate->jump_error; } @@ -4574,9 +4591,33 @@ ExecEvalJsonCoercion(ExprState *state, ExprEvalStep *op, (Node *) escontext); } +static char * +GetJsonBehaviorValueString(JsonBehavior *behavior) +{ + /* + * The order of array elements must correspond to the order of + * JsonBehaviorType members. + */ + const char *behavior_names[] = + { + "NULL", + "ERROR", + "EMPTY", + "TRUE", + "FALSE", + "UNKNOWN", + "EMPTY ARRAY", + "EMPTY OBJECT", + "DEFAULT" + }; + + return pstrdup(behavior_names[behavior->btype]); +} + /* * Checks if an error occurred in ExecEvalJsonCoercion(). If so, this sets - * JsonExprState.error to trigger the ON ERROR handling steps. + * JsonExprState.error to trigger the ON ERROR handling steps, unless the + * error is thrown when coercing a JsonBehavior value. */ void ExecEvalJsonCoercionFinish(ExprState *state, ExprEvalStep *op) @@ -4585,9 +4626,33 @@ ExecEvalJsonCoercionFinish(ExprState *state, ExprEvalStep *op) if (SOFT_ERROR_OCCURRED(&jsestate->escontext)) { + /* + * jsestate->error or jsetate->empty being set means that the error + * occurred when coercing the JsonBehavior value. Throw the error in + * that case with the actual coercion error message shown in the + * DETAIL part. + */ + if (DatumGetBool(jsestate->error.value)) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("could not coerce ON ERROR expression (%s) to the RETURNING type", + GetJsonBehaviorValueString(jsestate->jsexpr->on_error)), + errdetail("%s", jsestate->escontext.error_data->message))); + else if (DatumGetBool(jsestate->empty.value)) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("could not coerce ON EMPTY expression (%s) to the RETURNING type", + GetJsonBehaviorValueString(jsestate->jsexpr->on_empty)), + errdetail("%s", jsestate->escontext.error_data->message))); + *op->resvalue = (Datum) 0; *op->resnull = true; + jsestate->error.value = BoolGetDatum(true); + + /* Set up to catch coercion errors of the ON ERROR value. */ + jsestate->escontext.error_occurred = false; + jsestate->escontext.details_wanted = true; } } diff --git a/src/test/regress/expected/sqljson_jsontable.out b/src/test/regress/expected/sqljson_jsontable.out index e634b1e3b5..2b1c2d20ac 100644 --- a/src/test/regress/expected/sqljson_jsontable.out +++ b/src/test/regress/expected/sqljson_jsontable.out @@ -227,7 +227,8 @@ SELECT * FROM JSON_TABLE(jsonb '{"d1": "H"}', '$' SELECT * FROM JSON_TABLE(jsonb '{"d1": "H"}', '$' COLUMNS (js1 jsonb_test_domain PATH '$.a2' DEFAULT 'foo'::jsonb_test_domain ON EMPTY)); -ERROR: value for domain jsonb_test_domain violates check constraint "jsonb_test_domain_check" +ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING type +DETAIL: value for domain jsonb_test_domain violates check constraint "jsonb_test_domain_check" SELECT * FROM JSON_TABLE(jsonb '{"d1": "H"}', '$' COLUMNS (js1 jsonb_test_domain PATH '$.a2' DEFAULT 'foo1'::jsonb_test_domain ON EMPTY)); js1 @@ -561,30 +562,18 @@ SELECT * FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a int4 EXISTS PATH '$' ERROR (1 row) SELECT * FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a int2 EXISTS PATH '$.a')); - a ---- - -(1 row) - +ERROR: could not coerce ON ERROR expression (FALSE) to the RETURNING type +DETAIL: invalid input syntax for type smallint: "false" SELECT * FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a int8 EXISTS PATH '$.a')); - a ---- - -(1 row) - +ERROR: could not coerce ON ERROR expression (FALSE) to the RETURNING type +DETAIL: invalid input syntax for type bigint: "false" SELECT * FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a float4 EXISTS PATH '$.a')); - a ---- - -(1 row) - +ERROR: could not coerce ON ERROR expression (FALSE) to the RETURNING type +DETAIL: invalid input syntax for type real: "false" -- Default FALSE (ON ERROR) doesn't fit char(3) SELECT * FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a char(3) EXISTS PATH '$.a')); - a ---- - -(1 row) - +ERROR: could not coerce ON ERROR expression (FALSE) to the RETURNING type +DETAIL: value too long for type character(3) SELECT * FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a char(3) EXISTS PATH '$.a' ERROR ON ERROR)); ERROR: value too long for type character(3) SELECT * FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a char(5) EXISTS PATH '$.a' ERROR ON ERROR)); @@ -616,11 +605,13 @@ SELECT a, a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4 EXISTS PATH (1 row) SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS PATH '$.b')); -ERROR: value for domain dint4_0 violates check constraint "dint4_0_check" +ERROR: could not coerce ON ERROR expression (FALSE) to the RETURNING type +DETAIL: value for domain dint4_0 violates check constraint "dint4_0_check" SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS PATH '$.b' ERROR ON ERROR)); ERROR: value for domain dint4_0 violates check constraint "dint4_0_check" SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS PATH '$.b' FALSE ON ERROR)); -ERROR: value for domain dint4_0 violates check constraint "dint4_0_check" +ERROR: could not coerce ON ERROR expression (FALSE) to the RETURNING type +DETAIL: value for domain dint4_0 violates check constraint "dint4_0_check" SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS PATH '$.b' TRUE ON ERROR)); a | a ---+--- @@ -636,7 +627,8 @@ SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_1 EXISTS SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_1 EXISTS PATH '$.a' ERROR ON ERROR)); ERROR: value for domain dint4_1 violates check constraint "dint4_1_check" SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_1 EXISTS PATH '$.a' TRUE ON ERROR)); -ERROR: value for domain dint4_1 violates check constraint "dint4_1_check" +ERROR: could not coerce ON ERROR expression (TRUE) to the RETURNING type +DETAIL: value for domain dint4_1 violates check constraint "dint4_1_check" SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_1 EXISTS PATH '$.a' FALSE ON ERROR)); a | a ---+--- diff --git a/src/test/regress/expected/sqljson_queryfuncs.out b/src/test/regress/expected/sqljson_queryfuncs.out index e7b64a9882..3080f5ed3e 100644 --- a/src/test/regress/expected/sqljson_queryfuncs.out +++ b/src/test/regress/expected/sqljson_queryfuncs.out @@ -313,11 +313,8 @@ SELECT JSON_VALUE(jsonb '"2017-02-20"', '$' RETURNING date) + 9; -- Test NULL checks execution in domain types CREATE DOMAIN sqljsonb_int_not_null AS int NOT NULL; SELECT JSON_VALUE(jsonb 'null', '$' RETURNING sqljsonb_int_not_null); - json_value ------------- - -(1 row) - +ERROR: could not coerce ON ERROR expression (NULL) to the RETURNING type +DETAIL: domain sqljsonb_int_not_null does not allow null values SELECT JSON_VALUE(jsonb 'null', '$' RETURNING sqljsonb_int_not_null ERROR ON ERROR); ERROR: domain sqljsonb_int_not_null does not allow null values SELECT JSON_VALUE(jsonb 'null', '$' RETURNING sqljsonb_int_not_null DEFAULT 2 ON EMPTY ERROR ON ERROR); @@ -881,17 +878,11 @@ SELECT JSON_QUERY(jsonb '[1,2]', '$[*]' RETURNING jsonb EMPTY OBJECT ON ERROR); (1 row) SELECT JSON_QUERY(jsonb '[3,4]', '$[*]' RETURNING bigint[] EMPTY OBJECT ON ERROR); - json_query ------------- - -(1 row) - +ERROR: could not coerce ON ERROR expression (EMPTY OBJECT) to the RETURNING type +DETAIL: expected JSON array SELECT JSON_QUERY(jsonb '"[3,4]"', '$[*]' RETURNING bigint[] EMPTY OBJECT ON ERROR); - json_query ------------- - -(1 row) - +ERROR: could not coerce ON ERROR expression (EMPTY OBJECT) to the RETURNING type +DETAIL: expected JSON array -- Coercion fails with quotes on SELECT JSON_QUERY(jsonb '"123.1"', '$' RETURNING int2 error on error); ERROR: invalid input syntax for type smallint: ""123.1"" @@ -1047,11 +1038,8 @@ SELECT JSON_QUERY(jsonb '{"a": 1}', '$.a' RETURNING sqljsonb_int_not_null); (1 row) SELECT JSON_QUERY(jsonb '{"a": 1}', '$.b' RETURNING sqljsonb_int_not_null); - json_query ------------- - -(1 row) - +ERROR: could not coerce ON EMPTY expression (NULL) to the RETURNING type +DETAIL: domain sqljsonb_int_not_null does not allow null values SELECT JSON_QUERY(jsonb '{"a": 1}', '$.b' RETURNING sqljsonb_int_not_null ERROR ON EMPTY ERROR ON ERROR); ERROR: no SQL/JSON item found for specified path -- Test timestamptz passing and output @@ -1244,7 +1232,8 @@ DROP TABLE test_jsonb_mutability; DROP FUNCTION ret_setint; CREATE DOMAIN queryfuncs_test_domain AS text CHECK (value <> 'foo'); SELECT JSON_VALUE(jsonb '{"d1": "H"}', '$.a2' RETURNING queryfuncs_test_domain DEFAULT 'foo'::queryfuncs_test_domain ON EMPTY); -ERROR: value for domain queryfuncs_test_domain violates check constraint "queryfuncs_test_domain_check" +ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING type +DETAIL: value for domain queryfuncs_test_domain violates check constraint "queryfuncs_test_domain_check" SELECT JSON_VALUE(jsonb '{"d1": "H"}', '$.a2' RETURNING queryfuncs_test_domain DEFAULT 'foo1'::queryfuncs_test_domain ON EMPTY); json_value ------------ @@ -1430,7 +1419,8 @@ SELECT JSON_VALUE(jsonb '1234', '$' RETURNING queryfuncs_d_varbit3 DEFAULT '010 (1 row) SELECT JSON_VALUE(jsonb '1234', '$' RETURNING queryfuncs_d_varbit3 DEFAULT '01' ON ERROR); -ERROR: value for domain queryfuncs_d_varbit3 violates check constraint "queryfuncs_d_varbit3_check" +ERROR: could not coerce ON ERROR expression (DEFAULT) to the RETURNING type +DETAIL: value for domain queryfuncs_d_varbit3 violates check constraint "queryfuncs_d_varbit3_check" SELECT JSON_VALUE(jsonb '"111"', '$' RETURNING bit(2) ERROR ON ERROR); ERROR: bit string length 3 does not match type bit(2) SELECT JSON_VALUE(jsonb '1234', '$' RETURNING bit(3) DEFAULT 1 ON ERROR); -- 2.43.0