Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

Shouldn't these be the same?

tarantool> box.execute('SELECT $name', {})
---
- null
- Index of binding slots must start from 1
...

tarantool> box.execute('SELECT @name', {})
---
- metadata:
  - name: COLUMN_1
    type: boolean
  rows:
  - [null]
...

See 4 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index eb169aeb8..74a98c550 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>  	}
>  }
>  
> +struct Expr *
> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)

1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
To be consistent with our naming policy for constructors allocating memory
and for consistency with with sql_expr_new_column(), sql_expr_new(),
sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().


2. spec and id can be made const.

> +{
> +	assert(spec != 0 && id != NULL && spec->n == 1);
> +	if (id->z - spec->z != 1) {
> +		diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, parse->line_count,
> +			 spec->z - parse->zTail + 1, spec->n, spec->z);
> +		parse->is_aborted = true;
> +		return NULL;
> +	}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 92fb37dd4..06e6244e3 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1087,31 +1087,29 @@ term(A) ::= INTEGER(X). {
>    A.zEnd = X.z + X.n;
>    if( A.pExpr ) A.pExpr->flags |= EP_Leaf;
>  }
> -expr(A) ::= VARIABLE(X).     {
> -  Token t = X;
> +expr(A) ::= VARNUM(X). {
>    if (pParse->parse_only) {
> -    spanSet(&A, &t, &t);
>      diag_set(ClientError, ER_SQL_PARSER_GENERIC_WITH_POS, pParse->line_count,
>               pParse->line_pos, "bindings are not allowed in DDL");
>      pParse->is_aborted = true;

3. Hm. It looks not so good that this complicated error is now in
2 places and is exactly the same. Maybe we could have a common
function both for positional and named arguments? Or 2 functions,
but they would use one more generic function inside?

For instance, one function could be

	sql_expr_new_var(struct Parse *parse, const struct Token *spec,
			 const struct Token *id);

For positionals it would be called as

	sql_expr_new_var(pParse, &X, NULL);

For named:

	sql_expr_new_var(pParse, &X, &Y); (like now)

Then we could also drop TK_VARIABLE from spanExpr() completely.

>      A.pExpr = NULL;
> -  } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
> -    u32 n = X.n;
> +    A.zEnd = &X.z[X.n];
> +  } else {
>      spanExpr(&A, pParse, TK_VARIABLE, X);
> -    if (A.pExpr->u.zToken[0] == '?' && n > 1) {
> -      diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
> -      pParse->is_aborted = true;
> -    } else {
> -      sqlExprAssignVarNumber(pParse, A.pExpr, n);
> -    }
> -  }else{
> -    assert( t.n>=2 );
> -    spanSet(&A, &t, &t);
> -    diag_set(ClientError, ER_SQL_SYNTAX_NEAR_TOKEN, pParse->line_count, t.n, t.z);
> -    pParse->is_aborted = true;
> -    A.pExpr = NULL;
> +    assert(A.pExpr->u.zToken[0] == '?' && X.n == 1);
> +    sqlExprAssignVarNumber(pParse, A.pExpr, X.n);
>    }
>  }
> +expr(A) ::= VARIABLE(X) id(Y).     {
> +  A.pExpr = expr_variable(pParse, &X, &Y);
> +  A.zStart = X.z;
> +  A.zEnd = &Y.z[Y.n];
> +}
> +expr(A) ::= VARIABLE(X) INTEGER(Y).     {
> +  A.pExpr = expr_variable(pParse, &X, &Y);
> +  A.zStart = X.z;
> +  A.zEnd = &Y.z[Y.n];
> +}

4. Lets drop the extra spaces before { symbols.