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 1564824719 for ; Thu, 17 May 2018 06:40:00 -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 T0k86q1i3jw1 for ; Thu, 17 May 2018 06:39:59 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 05D3E20E76 for ; Thu, 17 May 2018 06:39:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: remove unnecessary templates for bindings References: From: Kirill Shcherbatov Message-ID: <83d1862c-47af-118e-11cd-7c8ce786f468@tarantool.org> Date: Thu, 17 May 2018 13:39:49 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: tarantool-patches@freelists.org Cc: "v.shpilevoy@tarantool.org" On 16.05.2018 21:28, Vladislav Shpilevoy wrote: > 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. Branch: http://github.com/tarantool/tarantool/tree/gh-2948-unnecessary-bindings Issue: https://github.com/tarantool/tarantool/issues/2948 > 2. I still can grep ?nnn in sqliteLimit.h. --- a/src/box/sql/sqliteLimit.h +++ b/src/box/sql/sqliteLimit.h @@ -39,7 +39,7 @@ - * The maximum value of a ?nnn wildcard that the parser will accept. + * The maximum value of a $nnn wildcard that the parser will accept. > 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. iff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua index 90cd15f..75f5c3c 100755 --- a/test/sql-tap/e_expr.test.lua +++ b/test/sql-tap/e_expr.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(14749) +test:plan(14747) --!./tcltestrunner.lua -- 2010 July 16 @@ -156,11 +156,11 @@ for _, op1 in ipairs(oplist) do local B = val[2] local C = val[3] local testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn) - -- If $op2 groups more tightly than $op1, then the result - -- of executing $sql1 whould be the same as executing $sql3. - -- If $op1 groups more tightly, or if $op1 and $op2 have - -- the same precedence, then executing $sql1 should return - -- the same value as $sql2. + -- If ?op2 groups more tightly than ?op1, then the result + -- of executing ?sql1 whould be the same as executing ?sql3. + -- If ?op1 groups more tightly, or if ?op1 and ?op2 have + -- the same precedence, then executing ?sql1 should return + -- the same value as ?sql2. -- local sql1 = string.format("SELECT %s %s %s %s %s", A, op1, B, op2, C) local sql2 = string.format("SELECT (%s %s %s) %s %s", A, op1, B, op2, C) @@ -1214,55 +1214,13 @@ if (0>0) then SELECT @เอศขูเอล ]], "1 @เอศขูเอล", -1) parameter_test("e_expr-11.3.6", "SELECT @€", "1 @€", -1) - -- EVIDENCE-OF: R-62610-51329 A dollar-sign followed by an identifier - -- name also holds a spot for a named parameter with the name $AAAA. - -- - -- EVIDENCE-OF: R-55025-21042 The identifier name in this case can - -- include one or more occurrences of "::" and a suffix enclosed in - -- "(...)" containing any text at all. - -- - -- Note: Looks like an identifier cannot consist entirely of "::" - -- characters or just a suffix. Also, the other named variable characters - -- (: and @) work the same way internally. Why not just document it that way? - -- - parameter_test("e_expr-11.4.1", "SELECT $AAAA", "1 $AAAA", -1) - parameter_test("e_expr-11.4.2", "SELECT $123", "1 $123", -1) - parameter_test("e_expr-11.4.3", "SELECT $__", "1 $__", -1) - parameter_test("e_expr-11.4.4", "SELECT $_$_", "1 $_$_", -1) - parameter_test("e_expr-11.4.5", [[ - SELECT $เอศขูเอล - ]], "1 $เอศขูเอล", -1) - parameter_test("e_expr-11.4.6", "SELECT $€", "1 $€", -1) - parameter_test("e_expr-11.5.1", "SELECT $::::a(++--++)", "1 $::::a(++--++)", -1) - parameter_test("e_expr-11.5.2", "SELECT $::a()", "1 $::a()", -1) - parameter_test("e_expr-11.5.3", "SELECT $::1(::#$)", "1 $::1(::#$)", -1) - -- EVIDENCE-OF: R-11370-04520 Named parameters are also numbered. The - -- number assigned is one greater than the largest parameter number - -- already assigned. - -- - -- EVIDENCE-OF: R-42620-22184 If this means the parameter would be - -- assigned a number greater than SQLITE_MAX_VARIABLE_NUMBER, it is an - -- error. - -- - parameter_test("e_expr-11.6.1", "SELECT ?, @abc", "1 {} 2 @abc", "-1 -2") - parameter_test("e_expr-11.6.3", "SELECT $a, ?8, ?, $b, ?2, $c", [[ - 1 $a 8 ?8 9 {} 10 $b 2 ?2 11 $c - ]], "-1 -8 -9 -10 -2 -11") - for _ in X(0, "X!foreach", [=[["tn sql",[["list","1",["SELECT ?",["mvn"],", $::a"],"2",["SELECT ?",["mvn"],", ?4, @a1"],"3",["SELECT ?",[["expr",[["mvn"],"-2"]]],", :bag, @123, $x"]]]]]=]) do - test:do_catchsql_test( - "e_expr-11.7."..tn, - sql, { - 1, "too many SQL variables" - }) - - end -- EVIDENCE-OF: R-14068-49671 Parameters that are not assigned values -- using sqlite3_bind() are treated as NULL. -- test:do_test( "e_expr-11.7.1", function() - stmt = sqlite3_prepare_v2("db", " SELECT ?, :a, @b, $d ", -1) + stmt = sqlite3_prepare_v2("db", " SELECT ?, :a, @b, ?d ", -1) sqlite3_step(stmt) return { sqlite3_column_type(stmt, 0), sqlite3_column_type(stmt, 1), sqlite3_column_type(stmt, 2), sqlite3_column_type(stmt, 3) } end, { @@ -1506,7 +1464,6 @@ local test_cases12 ={ {49, "CAST ( EXPR AS integer )"}, {50, "CAST ( EXPR AS 'abcd' )"}, - {51, "CAST ( EXPR AS 'ab$ $cd' )"}, {52, "EXPR COLLATE \"unicode_ci\""}, {53, "EXPR COLLATE binary"}, @@ -1634,20 +1591,6 @@ for _, val in ipairs(test_cases12) do end end --- # -- syntax diagram raise-function --- # --- foreach {tn raiseexpr} { --- 1 "RAISE(IGNORE)" --- 2 "RAISE(ROLLBACK, 'error message')" --- 3 "RAISE(ABORT, 'error message')" --- 4 "RAISE(FAIL, 'error message')" --- } { --- do_execsql_test e_expr-12.4.$tn " --- CREATE TRIGGER dbname.tr$tn BEFORE DELETE ON tblname BEGIN --- SELECT $raiseexpr ; --- END; --- " {} --- } --------------------------------------------------------------------------- -- Test the statements related to the BETWEEN operator. -- @@ -1067,20 +1067,20 @@ sqlite3ExprFunction(Parse * pParse, ExprList * pList, Token * pToken) } /* - * Assign a variable number to an expression that encodes a wildcard - * in the original SQL statement. + * Assign a variable number to an expression that encodes a + * wildcard in the original SQL statement. * - * Wildcards consisting of a single "?" are assigned the next sequential - * variable number. + * Wildcards consisting of a single "?" are assigned the next + * sequential variable number. * - * Wildcards of the form "$nnn" are assigned the number "nnn". We make - * 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 "$nnn" are assigned the number "nnn". + * We make 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", are assigned the same number - * as the previous instance of the same wildcard. Or if this is the first - * instance of the wildcard, the next sequential variable number is - * assigned. + * Wildcards of the form ":aaa", "@aaa", are assigned the same + * number as the previous instance of the same wildcard. Or if + * this is the first instance of the wildcard, the next sequential variable + * number is assigned. */ > 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. +++ b/src/box/sql/parse.y @@ -897,7 +897,11 @@ expr(A) ::= VARIABLE(X). { if( !(X.z[0]=='#' && sqlite3Isdigit(X.z[1])) ){ u32 n = X.n; spanExpr(&A, pParse, TK_VARIABLE, X); - sqlite3ExprAssignVarNumber(pParse, A.pExpr, n); + if (A.pExpr->u.zToken[0] == '?' && n > 1) { + sqlite3ErrorMsg(pParse, "Unsupported variable format"); + } else { + sqlite3ExprAssignVarNumber(pParse, A.pExpr, n); + } }else{ +++ b/src/box/sql/expr.c @@ -1103,10 +1103,8 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n) x = (ynVar) (++pParse->nVar); } else { int doAdd = 0; - if (z[0] == '?') { - sqlite3ErrorMsg(pParse, "Unsupported variable format"); - return; - } else if (z[0] == '$') { + assert(z[0] != '?'); + if (z[0] == '$') { > 6. Error message in this 'if' body still outputs '?NNN' @@ -1120,7 +1118,7 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n) testcase(i == SQL_BIND_PARAMETER_MAX); if (bOk == 0 || i < 1 || i > SQL_BIND_PARAMETER_MAX) { sqlite3ErrorMsg(pParse, - "variable number must be between ?1 and ?%d", + "variable number must be between $1 and $%d", SQL_BIND_PARAMETER_MAX); > 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. @@ -1141,6 +1139,13 @@ sqlite3ExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n) x = (ynVar) (++pParse->nVar); doAdd = 1; } + if (n > 1 && (!sqlite3Isalpha(z[1]) || + sqlite3CheckIdentifierName(pParse, &z[1]) != + SQLITE_OK)) { + sqlite3ErrorMsg(pParse, + "name '%s' is invalid identifier", z); + return; + } } --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3960,7 +3960,7 @@ Expr *sqlite3ExprAddCollateToken(Parse * pParse, Expr *, const Token *, int); Expr *sqlite3ExprAddCollateString(Parse *, Expr *, const char *); Expr *sqlite3ExprSkipCollate(Expr *); int sqlite3CheckCollSeq(Parse *, struct coll *); -int sqlite3CheckIdentifierName(Parse *, char *); +int sqlite3CheckIdentifierName(Parse *, const char *); diff --git a/test/sql/iproto.result b/test/sql/iproto.result index 4305eb9..7be4470 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -447,6 +447,10 @@ cn:execute('select $2, $1, $3', parameters) rows: - [22, 11, 33] ... +cn:execute('select * from test where id = :1', {1}) +--- +- error: 'Failed to execute SQL statement: name '':1'' is invalid identifier' +... -- gh-2602 obuf_alloc breaks the tuple in different slabs _ = space:replace{1, 1, string.rep('a', 4 * 1024 * 1024)} --- diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index cbeaddd..c7e9695 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -170,6 +170,7 @@ parameters[1] = 11 parameters[2] = 22 parameters[3] = 33 cn:execute('select $2, $1, $3', parameters) +cn:execute('select * from test where id = :1', {1}) -- gh-2602 obuf_alloc breaks the tuple in different slabs _ = space:replace{1, 1, string.rep('a', 4 * 1024 * 1024)}