[tarantool-patches] Re: [PATCH v1 2/2] sql: check returned value type for UDF

[Nikita Pettik]

On 16 Sep 16:23, Kirill Shcherbatov wrote:
> box.schema.func create endpoint defines a returned
> value type 'returns', but it used to be ignored.
> This patch implements required type checks for user
> defined functions.

All user-defined functions feature type of returned value (if it is not
specified during function creation, it is assumed to be ANY), but currently
it is ignored. This patch introduces check which verifies that returned
value is of specified in function's definition type. There's no attempt
at implicit conversion to specified (target) type - returned value must
be literally of the specified type.


> Closes #4387
> ---
>  src/box/errcode.h               |  1 +
>  src/box/field_def.h             | 17 +++++++-
>  src/box/sql/vdbe.c              | 12 ++++++
>  test/sql-tap/func.test.lua      | 74 ++++++++++++++++++++++++++++++++-
>  test/sql/func-recreate.result   |  8 ++--
>  test/sql/func-recreate.test.lua |  6 +--
>  6 files changed, 108 insertions(+), 10 deletions(-)
> 
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index dcb90f678..e71737ba1 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -256,6 +256,7 @@ struct errcode_record {
>  	/*201 */_(ER_NO_SUCH_FIELD_NAME,	"Field '%s' was not found in the tuple") \
>  	/*202 */_(ER_FUNC_WRONG_ARG_COUNT,	"Wrong number of arguments is passed to %s(): expected %s, got %d") \
>  	/*203 */_(ER_BOOTSTRAP_READONLY,	"Trying to bootstrap a local read-only instance as master") \
> +	/*204 */_(ER_FUNC_INVALID_RETURN,	"Function '%s' returned invalid value: expected %s got %s") \

I'd say: Function %s returned value of type %s but expected %s OR
Function %s returned value of wrong/invalid type: expected %s got %s
  
>  /*
>   * !IMPORTANT! Please follow instructions at start of the file
> diff --git a/src/box/field_def.h b/src/box/field_def.h
> index fdedc9622..49c2998f5 100644
> --- a/src/box/field_def.h
> +++ b/src/box/field_def.h
> @@ -144,6 +144,19 @@ struct field_def {
>  	struct Expr *default_value_expr;
>  };
>  
> +/**
> + * Checks if mp_type (except MP_EXT) (MsgPack) is compatible
> + * with given field type.
> + */
> +static inline bool
> +field_mp_plain_type_is_compatible(enum field_type type, enum mp_type mp_type,
> +				  bool is_nullable)
> +{
> +	assert(mp_type != MP_EXT);
> +	uint32_t mask = field_mp_type[type] | (is_nullable * (1U << MP_NIL));
> +	return (mask & (1U << mp_type)) != 0;
> +}
> +
>  /** Checks if mp_type (MsgPack) is compatible with field type. */
>  static inline bool
>  field_mp_type_is_compatible(enum field_type type, const char *data,
> @@ -154,8 +167,8 @@ field_mp_type_is_compatible(enum field_type type, const char *data,
>  	assert((size_t)mp_type < CHAR_BIT * sizeof(*field_mp_type));
>  	uint32_t mask;
>  	if (mp_type != MP_EXT) {
> -		mask = field_mp_type[type] | (is_nullable * (1U << MP_NIL));
> -		return (mask & (1U << mp_type)) != 0;
> +		return field_mp_plain_type_is_compatible(type, mp_type,
> +							 is_nullable);
>  	} else {
>  		int8_t ext_type;
>  		mp_decode_extl(&data, &ext_type);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 9a9648c1a..20c1d2291 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -348,6 +348,7 @@ mem_apply_type(struct Mem *record, enum field_type type)
>  			return -1;
>  		return 0;
>  	case FIELD_TYPE_SCALAR:
> +	case FIELD_TYPE_ANY:

This fix is already on master branch.

>  		return 0;
>  	default:
>  		return -1;
> @@ -1810,6 +1811,11 @@ case OP_Function: {
>  		diag_set(ClientError, ER_NO_SUCH_FUNCTION, pOp->p4.z);
>  		goto abort_due_to_error;
>  	}
> +	/*
> +	 * Function call may yield and func pointer may be
> +	 * invalid above.

-> so pointer to the function may turn out to be invalid
after call.

Did you add test case covering this situation? If not, please
introduce it.

> +	 */
> +	enum field_type returns = func->def->returns;
>  	int argc = pOp->p5;
>  	struct Mem *argv = &aMem[pOp->p2];
>  	struct port args, ret;
> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
> index a96fc611e..9d3a16c96 100755
> --- a/test/sql-tap/func.test.lua
> +++ b/test/sql-tap/func.test.lua
> @@ -1,6 +1,6 @@
>  #!/usr/bin/env tarantool
>  test = require("sqltester")
> -test:plan(14606)
> +test:plan(14642)
>  
>  --!./tcltestrunner.lua
>  -- 2001 September 15
> @@ -2909,4 +2909,76 @@ test:do_execsql_test(
>      "SELECT POSITION(CHAR(00), CHAR(65,66));",
>      {0})
>  
> +box.schema.func.create('TOSTRING', {language = 'Lua', is_deterministic = true,
> +                                    body = 'function(a) return a end',
> +                                    param_list = {'any'}, returns = 'string',
> +                                    exports = {'LUA', 'SQL'}})
> +test:do_execsql_test("func-77", "SELECT TOSTRING('1');", {'1'})
> +test:do_execsql_test("func-78", "SELECT TOSTRING('a');", {'a'})
> +test:do_catchsql_test("func-79", "SELECT TOSTRING(1);", {1, "Function 'TOSTRING' returned invalid value: expected string got unsigned"})
> +test:do_catchsql_test("func-80", "SELECT TOSTRING(-1);", {1, "Function 'TOSTRING' returned invalid value: expected string got integer"})
> +test:do_catchsql_test("func-81", "SELECT TOSTRING(-1.1);", {1, "Function 'TOSTRING' returned invalid value: expected string got double"})
> +test:do_catchsql_test("func-83", "SELECT TOSTRING(TRUE);", {1, "Function 'TOSTRING' returned invalid value: expected string got boolean"})
> +box.func.TOSTRING:drop()

What about returning NULL value and cdata?