[tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings

[Vladislav Shpilevoy]

Hello. Thanks for the patch! See my 7 comments below.

On 16/05/2018 20:14, Kirill Shcherbatov wrote:
> Removed ?N binding, changed $V to $N semantics to match
> other vendors standarts.
> 
> Closes #2948
> ---

1. Put the branch and issue links here please.

>   src/box/sql/expr.c           | 13 +++++++-----
>   test/sql-tap/e_expr.test.lua | 50 +++-----------------------------------------
>   test/sql/iproto.result       | 33 +++++++++++++++++++++++++----
>   test/sql/iproto.test.lua     | 14 +++++++++++--
>   4 files changed, 52 insertions(+), 58 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 1b51823..cd2f549 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1073,11 +1073,11 @@ sqlite3ExprFunction(Parse * pParse, ExprList * pList, Token * pToken)
>    * Wildcards consisting of a single "?" are assigned the next sequential
>    * variable number.
>    *
> - * Wildcards of the form "?nnn" are assigned the number "nnn".  We make
> + * Wildcards of the form "$nnn" are assigned the number "nnn".  We make

2. I still can grep ?nnn in sqliteLimit.h.

>    * sure "nnn" is not too big to avoid a denial of service attack when
>    * the SQL statement comes from an external source.
>    *
> - * Wildcards of the form ":aaa", "@aaa", or "$aaa" are assigned the same number
> + * Wildcards of the form ":aaa", "@aaa", are assigned the same number

3. I still see tests on $aaa here: sql-tap/e_expr.test.lua.

4. When you fix comments, please, align them by 66 symbols.

> @@ -1104,7 +1104,10 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>   	} else {
>   		int doAdd = 0;
>   		if (z[0] == '?') {
> -			/* Wildcard of the form "?nnn".  Convert "nnn" to an integer and
> +			sqlite3ErrorMsg(pParse, "Unsupported variable format");

5. This function always must take valid variable, it is guaranteed by a parser. Please,
do this check in parse.y. ?nnn - is syntax error.

> +			return;
> +		} else if (z[0] == '$') {

6. Error message in this 'if' body still outputs '?NNN'

7. I found, that :NNN works too, including SQLite. Please, remove it too.
It works because SQLite interprets any symbols except '$' and '?' as prefix for
name or number parameter.

Example:
tarantool> cn:execute('select * from test where id = :1', {1})
---
- metadata: [{'name': ID}, {'name': 'A'}, {'name': 'B'}]
   rows:
   - [1, 2, '3']
...

We must forbid it.