[tarantool-patches] Re: sql: assertion fault on VALUES (#3888)

["n.pettik" <korablev@tarantool.org>]

> On 22 Jan 2019, at 16:42, Stanislav Zudin <szudin@tarantool.org> wrote:
> 
> Branch: https://github.com/tarantool/tarantool/tree/2.1
> Issue: https://github.com/tarantool/tarantool/issues/3888

You should add link to branch where your patch is located,
so that reviewer can pull and test it.

For instance,
https://github.com/tarantool/tarantool/tree/np/gh-3934-IN-operator-fix

Links to branch and issue should be placed after --- delimiter:

Closes #3888
---

> From ff49a389a68488b13a0e3093fce9f51ac4c186bf Mon Sep 17 00:00:00 2001
> From: Stanislav Zudin <szudin@tarantool.org>
> Date: Tue, 22 Jan 2019 15:41:26 +0300
> Subject: [PATCH] sql: if a keyword 'blob' is used as an ID treat it as error

These headers shouldn’t appear if you send patch via git send-email.

> 
> Closes #3888
> ---
>  src/box/sql/expr.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index b67b22c23..3eea7f4a3 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3751,10 +3751,24 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>          }
>  #ifndef SQLITE_OMIT_BLOB_LITERAL
>      case TK_BLOB:{
> -            int n;
> -            const char *z;
> +            int n = sqlite3Strlen30(pExpr->u.zToken);
> +            const char *z = pExpr->u.zToken;
>              char *zBlob;
> +
>              assert(!ExprHasProperty(pExpr, EP_IntValue));
> +
> +            if ((n < 3) ||    /* 'X' + opening ' + closing ' == 3 */
> +                (z[0] != 'x' && z[0] != 'X') ||
> +                (z[1] != '\'') || (z[n - 1] != '\'')) {
> +                /* It's definitely not a blob, report an error. */
> +                sqlite3ErrorMsg(pParse,
> +                            "Unexpected keyword '%s' within request",
> +                            z);

If we get so far, then we have already passed through Lemon
parser and about to generate byte code. Hence it is too late to
raise an error about unexpected keywords.

Look at parse.y:

    30	// This code runs whenever there is a syntax error
    31	//
    32	%syntax_error {
    33	  UNUSED_PARAMETER(yymajor);  /* Silence some compiler warnings */
    34	  assert( TOKEN.z[0] );  /* The tokenizer always gives us a token */
    35	  if (yypParser->is_fallback_failed && TOKEN.isReserved) {
    36	    sqlite3ErrorMsg(pParse, "keyword \"%T\" is reserved", &TOKEN);
    37	  } else {
    38	    sqlite3ErrorMsg(pParse, "near \"%T\": syntax error", &TOKEN);
    39	  }
    40	}

I guess we should terminate here.

> +                sqlite3MayAbort(pParse);
> +                return 0;
> +            }
> +
> +            /* ---------------------------------------------- */

We don’t use such comments as delimiters or whatever.

What about other cases which I mentioned?
For instance, SELECT FLOAT returns 0, but
must raise an error.

Yep, it may seem as a slightly different problem.
You could extend you patch and fix it right now.
Otherwise, file an issue(s), please.

>              assert(pExpr->u.zToken[0] == 'x'
>                     || pExpr->u.zToken[0] == 'X');
>              assert(pExpr->u.zToken[1] == '\'’);

Also, note that every patch should come with test verifying
that the problem has been resolved (whenever it is possible).