From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 28EF024A19 for ; Wed, 16 May 2018 14:28:40 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8LDtySdW64Bu for ; Wed, 16 May 2018 14:28:40 -0400 (EDT) Received: from smtp18.mail.ru (smtp18.mail.ru [94.100.176.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D889723EE3 for ; Wed, 16 May 2018 14:28:39 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings References: From: Vladislav Shpilevoy Message-ID: Date: Wed, 16 May 2018 21:28:37 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Shcherbatov , tarantool-patches@freelists.org 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.