[tarantool-patches] Re: [PATCH 2/2] sql: allow <COLLATE> only for string-like args

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes! See 8 comments below.

>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index ba7eea59d..29e3386fa 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -4215,6 +4215,22 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>>> 		}
>>> 	case TK_SPAN:
>>> 	case TK_COLLATE:{
>>> +			enum field_type type;
>>> +			struct Expr *left = pExpr->pLeft;
>>> +			if (left->op == TK_COLUMN) {
>>> +				int col_num = left->iColumn;
>>> +				type = left->space_def->fields[col_num].type;
>>> +			} else
>>> +				type = left->type;
>>> +			if (left->op != TK_CONCAT &&
>>
>> Why do you check for TK_CONCAT? Its type should be FIELD_TYPE_STRING.
> Concatenations are rejected without this check.

1. It is not a good answer, when you can't explain, why you need
a line of code, but a program does not work without it. Please, investigate
and explain, why do you check for both 'type == STRING' and 'op == CONCAT'.
If an op is CONCAT, then type is already STRING, so the additional check
for 'op' should not be necessary.

> 
> commit 5feaa9330f2c642fc16b479383b1c5cac96c28cc
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Mon May 6 14:30:21 2019 +0300
> 
>     sql: allow <COLLATE> only for string-like args
>     
>     Before this patch, user could use COLLATE with non-string-like
>     literals, columns or subquery results. Disallow such usage.
>     
>     Closes #3804
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index ba7eea59d..e558cbb18 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -197,6 +197,40 @@ sqlExprSkipCollate(Expr * pExpr)
>  	return pExpr;
>  }
>  
> +/*
> + * Check that left node of @expr with the collation in the root

2. '@expr' is an invalid Doxygen command. If you want to reference
a parameter, use <@a parameter_name>. See Doxygen documentation.

> + * can be used with <COLLATE>. If it is not, leave an error
> + * message in pParse.

3. There is no argument 'pParse'.

> + *
> + * @param pParse Parser context.
> + * @param expr Expression for checking.
> + *
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +static int
> +check_collate_arg(Parse *parse, Expr *expr)

4. We use 'struct' prefix in all new SQL code.

> +{
> +	struct Expr *left = expr->pLeft;
> +	while (left->op == TK_COLLATE)
> +		left = left->pLeft;
> +	enum field_type type;
> +	if (left->op == TK_COLUMN) {
> +		int col_num = left->iColumn;
> +		type = left->space_def->fields[col_num].type;
> +	} else
> +		type = left->type;

5. We always either use '{}' for both 'if-else' parts,
or do not use it at all.

> +	if (left->op != TK_CONCAT && type != FIELD_TYPE_STRING &&
> +	    type != FIELD_TYPE_SCALAR) {
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +			 "COLLATE can't be used with "
> +			 "non-string arguments");
> +		parse->is_aborted = true;
> +		return -1;
> +	}
> +	return 0;
> @@ -3935,6 +3969,9 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>  			testcase(op == TK_BITNOT);
>  			assert(TK_NOT == OP_Not);
>  			testcase(op == TK_NOT);
> +			if (pExpr->pLeft->op == TK_COLLATE &&
> +			    check_collate_arg(pParse, pExpr->pLeft) != 0)
> +				break;
>  			r1 = sqlExprCodeTemp(pParse, pExpr->pLeft,
>  						 &regFree1);

6. Why do you check for COLLATE before CodeTemp()? This function
does exactly the same check.

>  			testcase(regFree1 == 0);
> diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
> index c4724e636..566f2e723 100755
> --- a/test/sql-tap/e_select1.test.lua
> +++ b/test/sql-tap/e_select1.test.lua
> @@ -1938,7 +1938,7 @@ test:do_execsql_test(
>  test:do_execsql_test(
>      "e_select-8.9.2",
>      [[
> -        SELECT x COLLATE "binary" FROM d4 ORDER BY 1 COLLATE "unicode_ci"
> +        SELECT x COLLATE "unicode_ci" FROM d4 ORDER BY 1

7. Why? The old test should pass as well. '1' here is not a value, but an
index of a selected column.

>      ]], {
>          -- <e_select-8.9.2>
>          "abc", "DEF", "ghi", "JKL"
> diff --git a/test/sql-tap/selectE.test.lua b/test/sql-tap/selectE.test.lua
> index 32c3d6a06..16075d38e 100755
> --- a/test/sql-tap/selectE.test.lua
> +++ b/test/sql-tap/selectE.test.lua
> @@ -57,8 +57,7 @@ test:do_test(
>              CREATE TABLE t3(a TEXT primary key);
>              INSERT INTO t3 VALUES('def'),('jkl');
>  
> -            SELECT a FROM t1 EXCEPT SELECT a FROM t2
> -             ORDER BY a COLLATE "unicode_ci";
> +            SELECT a FROM t1 EXCEPT SELECT a FROM t2 ORDER BY a COLLATE "unicode_ci";
>          ]]
>      end, {
>          -- <selectE-1.0>
> @@ -70,8 +69,7 @@ test:do_test(
>      "selectE-1.1",
>      function()
>          return test:execsql [[
> -            SELECT a FROM t2 EXCEPT SELECT a FROM t3
> -             ORDER BY a COLLATE "unicode_ci";
> +            SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a COLLATE "unicode_ci";
>          ]]
>      end, {
>          -- <selectE-1.1>
> @@ -83,8 +81,7 @@ test:do_test(
>      "selectE-1.2",
>      function()
>          return test:execsql [[
> -            SELECT a FROM t2 EXCEPT SELECT a FROM t3
> -             ORDER BY a COLLATE "binary";
> +            SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a COLLATE "binary";
>          ]]
>      end, {
>          -- <selectE-1.2>
> @@ -96,8 +93,7 @@ test:do_test(
>      "selectE-1.3",
>      function()
>          return test:execsql [[
> -            SELECT a FROM t2 EXCEPT SELECT a FROM t3
> -             ORDER BY a;
> +            SELECT a FROM t2 EXCEPT SELECT a FROM t3 ORDER BY a;
>          ]]
>      end, {
>          -- <selectE-1.3>
> @@ -113,8 +109,7 @@ test:do_test(
>              DELETE FROM t3;
>              INSERT INTO t2 VALUES('ABC'),('def'),('GHI'),('jkl');
>              INSERT INTO t3 SELECT lower(a) FROM t2;
> -            SELECT a COLLATE "unicode_ci" FROM t2 EXCEPT SELECT a FROM t3
> -             ORDER BY 1
> +            SELECT a COLLATE "unicode_ci" FROM t2 EXCEPT SELECT a FROM t3 ORDER BY 1
>          ]]
>      end, {

8. Why do you need changes in this file? You just made multiline requests
one line.