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 6FC3A24236 for ; Fri, 18 Jan 2019 13:04:25 -0500 (EST) 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 nK9lE6UbRZW2 for ; Fri, 18 Jan 2019 13:04:25 -0500 (EST) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 4AF1126701 for ; Fri, 18 Jan 2019 13:04:19 -0500 (EST) Date: Fri, 18 Jan 2019 21:04:17 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v1 2/4] sql: disallow use of TYPEOF in Check Message-ID: <20190118180417.GB24439@chai> References: <467d962bb970e77481307b7e73892e35c1a6601f.1547128310.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <467d962bb970e77481307b7e73892e35c1a6601f.1547128310.git.kshcherbatov@tarantool.org> 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: korablev@tarantool.org, Kirill Shcherbatov * Kirill Shcherbatov [19/01/10 23:12]: > Due to the fact that we are going to perform CHECKs validations > on the server side, checks are performed on the fields with > affinity already applied that may differ with type of original > data. > After the introduction of static types, the need for type checks > based on the CHECKs disappeared. > > Need for #3691 > --- > src/box/sql/build.c | 23 ++++++++++ > test/sql-tap/check.test.lua | 89 ++----------------------------------- > 2 files changed, 26 insertions(+), 86 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 7e5bcc518..9c6b1b49a 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -756,12 +756,35 @@ primary_key_exit: > return; > } > > +static int > +sql_check_expr_test_callback(struct Walker *walker, struct Expr *expr) > +{ > + if (expr->op == TK_FUNCTION && strcmp(expr->u.zToken, "TYPEOF") == 0) { > + diag_set(ClientError, ER_SQL, "TYPEOF function is forbidden " > + "for usage in Check constraint"); > + walker->eCode = 1; > + return WRC_Abort; > + } > + return WRC_Continue; > +} This callback needs a comment. All functions need a comment, even static ones. A commonly accepted abbreviation for callbacks is _cb ending. A better option for function name would be ck_constraint_walk_expr_cb. Please reply to my questions in previous emails. > void > sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span) > { > struct Expr *expr = span->pExpr; > struct Table *table = parser->pNewTable; > if (table != NULL) { > + /* Test if Check expression valid. */ is valid. valid is a general term. What problems specifically are you looking for? What is forbidden in a check expression? please add a better comment. > + struct Walker w; > + memset(&w, 0, sizeof(w)); > + w.xExprCallback = sql_check_expr_test_callback; > + sqlite3WalkExpr(&w, expr); > + if (w.eCode != 0) { > + parser->rc = SQL_TARANTOOL_ERROR; > + parser->nErr++; > + goto release_expr; > + } > + I don't understand the premise for this code. Why do yo uneed a separate walker, aren't these expressions checked already someplace else in the code? -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov