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 3E44F22485 for ; Fri, 28 Dec 2018 13:11:21 -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 dk3QkWr5xxIR for ; Fri, 28 Dec 2018 13:11:21 -0500 (EST) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 D0D4A23C79 for ; Fri, 28 Dec 2018 13:11:20 -0500 (EST) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v6 4/5] lua: parameter binding for new execute() Date: Fri, 28 Dec 2018 21:11:18 +0300 Message-Id: In-Reply-To: References: 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, v.shpilevoy@tarantool.org Hi! Thank you for review! My answers, diff between versions and new version below. On 12/25/18 5:57 PM, Vladislav Shpilevoy wrote: > Thanks for the patch! See 9 comments below. > 1. It is not msgpack. Fixed. > 2. In Lua syntax {'name' : value} is incorrect. Fixed. > 3. Just -1. Stack indexes can be negative. lua_next() throws an assert when I change arg value to -1: tarantool: lj_api.c:892: lua_next: Assertion `(((t)->it) == (~11u))' failed. > 4. Please, do not pop the value stored in a luaL_field. Pop 2 > values at one time below, on line 294. Fixed. Actually, I found out that my popping policy was quite a mess in previous version. Now everything will be popped at the end of the function. > 5. As I know, luaL_checklstring never returns NULL. It throws. Please, > check for string type explicitly and throw normal usage error. Fixed. > 6. As I said above, you've already popped it. Fixed. > 7. Please, check for overflow. Fixed. > 8. To be honest, I feel utterly uncomfortable with luaL_field. IMO, > this vast structure is too overloaded. Can you please avoid its usage? > Almost nothing will change as I see. For switch on types you can use > lua_type(). After some discussion it was decided to left it as it is now. The main reason for this is that if we remove luaL_field() than this function become downgraded version of luaL_field(). > 9. Not msgpack. Please, do not blindly copy-paste code. Check out the > commit more accurately. I will not point out other places. Fixed. Moved this check to sql.c. Diff between versions: commit 93e4ea0ffcc195c4a904dab99c5cc516db33fa44 Author: Mergen Imeev Date: Thu Dec 27 17:00:51 2018 +0300 Temporary: review fixes diff --git a/src/box/execute.c b/src/box/execute.c index 457403b..ade6e6c 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -278,43 +278,43 @@ static inline int lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) { struct luaL_field field; + struct region *region = &fiber()->gc; char *buf; lua_rawgeti(L, idx, i + 1); luaL_tofield(L, luaL_msgpack_default, -1, &field); bind->pos = i + 1; if (field.type == MP_MAP) { /* - * A named parameter is an MP_MAP with - * one key - {'name': value}. - * Report parse error otherwise. + * A named parameter is a single-row table that + * contains one value associated with a key. The + * key must be a string. This key is the name of + * the parameter, and the value associated with + * the key is the value of the parameter. */ if (field.size != 1) { diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ - "parameter should be {'name': value}"); + "named parameter should be a single-row "\ + "table with one key-value pair"); return -1; } /* - * Get key and value of the only map element to + * Get key and value of the only table element to * lua stack. */ lua_pushnil(L); lua_next(L, lua_gettop(L) - 1); - /* At first we should deal with the value. */ - luaL_tofield(L, luaL_msgpack_default, -1, &field); - lua_pop(L, 1); - /* Now key is on the top of Lua stack. */ - size_t name_len = 0; - bind->name = luaL_checklstring(L, -1, &name_len); - if (bind->name == NULL) { - diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ - "parameter should be {'name': value}"); + if (lua_type(L, -2) != LUA_TSTRING) { + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\ + "parameter should be a string."); return -1; } + size_t name_len; + bind->name = lua_tolstring(L, -2, &name_len); /* * Name should be saved in allocated memory as it * will be poped from Lua stack. */ - buf = region_alloc(&fiber()->gc, name_len + 1); + buf = region_alloc(region, name_len + 1); if (buf == NULL) { diag_set(OutOfMemory, name_len + 1, "region_alloc", "buf"); @@ -323,13 +323,18 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) memcpy(buf, bind->name, name_len + 1); bind->name = buf; bind->name_len = name_len; - lua_pop(L, 1); + luaL_tofield(L, luaL_msgpack_default, -1, &field); } else { bind->name = NULL; bind->name_len = 0; } switch (field.type) { case MP_UINT: { + if (field.ival < 0) { + diag_set(ClientError, ER_SQL_BIND_VALUE, + sql_bind_name(bind), "INTEGER"); + return -1; + } bind->i64 = field.ival; bind->type = SQLITE_INTEGER; bind->bytes = sizeof(bind->i64); @@ -345,7 +350,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) * Data should be saved in allocated memory as it * will be poped from Lua stack. */ - buf = region_alloc(&fiber()->gc, field.sval.len + 1); + buf = region_alloc(region, field.sval.len + 1); if (buf == NULL) { diag_set(OutOfMemory, field.sval.len + 1, "region_alloc", "buf"); @@ -385,7 +390,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) * Data should be saved in allocated memory as it * will be poped from Lua stack. */ - buf = region_alloc(&fiber()->gc, sizeof(field)); + buf = region_alloc(region, sizeof(field)); if (buf == NULL) { diag_set(OutOfMemory, sizeof(field), "region_alloc", "buf"); @@ -407,7 +412,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) default: unreachable(); } - lua_pop(L, 1); + lua_pop(L, lua_gettop(L) - idx); return 0; } @@ -416,10 +421,6 @@ lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind, int idx) { assert(out_bind != NULL); - if (! lua_istable(L, idx)) { - diag_set(ClientError, ER_INVALID_MSGPACK, "SQL parameter list"); - return -1; - } uint32_t bind_count = lua_objlen(L, idx); if (bind_count == 0) return 0; diff --git a/src/box/execute.h b/src/box/execute.h index 0625e2a..cb149a0 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -69,10 +69,16 @@ int sql_bind_list_decode(const char *data, struct sql_bind **out_bind); /** - * Parse Lua table of SQL parameters and store a result - * into the @request->bind, bind_count. + * Parse Lua table of SQL parameters. * - * @param L Lua stack to get data from. + * @param L Lua stack contains table with parameters. Each + * parameter either must have scalar type, or must be a + * single-row table with the following format: + * table[name] = value. Name - string name of the named + * parameter, value - scalar value of the parameter. + * Named and positioned parameters can be mixed. + * For more details + * @sa https://www.sqlite.org/lang_expr.html#varparam. * @param[out] out_bind Pointer to save decoded parameters. * @param idx Position of table with parameters on Lua stack. * diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c index a27e0c5..20ba43b 100644 --- a/src/box/lua/sql.c +++ b/src/box/lua/sql.c @@ -118,17 +118,22 @@ lbox_execute(struct lua_State *L) int bind_count = 0; size_t length; struct port port; + int top = lua_gettop(L); - if (lua_type(L, 1) != LUA_TSTRING) - return luaL_error(L, "Usage: box.execute(sqlstring)"); + if ((top != 1 && top != 2) || lua_type(L, 1) != LUA_TSTRING) + return luaL_error(L, "Usage: box.execute(sqlstring[, params])"); const char *sql = lua_tolstring(L, 1, &length); if (sql == NULL) - return luaL_error(L, "Usage: box.execute(sqlstring)"); - - if (lua_gettop(L) == 2 && - (bind_count = lua_sql_bind_list_decode(L, &bind, 2)) < 0) - return luaT_error(L); + return luaL_error(L, "Usage: box.execute(sqlstring[, params])"); + + if (top == 2) { + if (! lua_istable(L, 2)) + return luaL_error(L, "Second argument must be a table"); + bind_count = lua_sql_bind_list_decode(L, &bind, 2); + if (bind_count < 0) + return luaT_error(L); + } if (sql_prepare_and_execute(sql, length, bind, bind_count, &port, &fiber()->gc) != 0) New version: commit a1fe2aa7edc82097b2fcf63a41fb7fa2145b9d3c Author: Mergen Imeev Date: Fri Dec 21 17:54:16 2018 +0300 lua: parameter binding for new execute() This patch defines parameters binding for SQL statements executed through box. Part of #3505 Closes #3401 diff --git a/src/box/execute.c b/src/box/execute.c index 4606239..ade6e6c 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -44,6 +44,7 @@ #include "tuple.h" #include "sql/vdbe.h" #include "lua/utils.h" +#include "lua/msgpack.h" const char *sql_type_strs[] = { NULL, @@ -261,6 +262,196 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind) return bind_count; } + +/** + * Decode a single bind column from Lua stack. + * + * @param L Lua stack. + * @param[out] bind Bind to decode to. + * @param idx Position of table with bind columns on Lua stack. + * @param i Ordinal bind number. + * + * @retval 0 Success. + * @retval -1 Memory or client error. + */ +static inline int +lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) +{ + struct luaL_field field; + struct region *region = &fiber()->gc; + char *buf; + lua_rawgeti(L, idx, i + 1); + luaL_tofield(L, luaL_msgpack_default, -1, &field); + bind->pos = i + 1; + if (field.type == MP_MAP) { + /* + * A named parameter is a single-row table that + * contains one value associated with a key. The + * key must be a string. This key is the name of + * the parameter, and the value associated with + * the key is the value of the parameter. + */ + if (field.size != 1) { + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ + "named parameter should be a single-row "\ + "table with one key-value pair"); + return -1; + } + /* + * Get key and value of the only table element to + * lua stack. + */ + lua_pushnil(L); + lua_next(L, lua_gettop(L) - 1); + if (lua_type(L, -2) != LUA_TSTRING) { + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\ + "parameter should be a string."); + return -1; + } + size_t name_len; + bind->name = lua_tolstring(L, -2, &name_len); + /* + * Name should be saved in allocated memory as it + * will be poped from Lua stack. + */ + buf = region_alloc(region, name_len + 1); + if (buf == NULL) { + diag_set(OutOfMemory, name_len + 1, "region_alloc", + "buf"); + return -1; + } + memcpy(buf, bind->name, name_len + 1); + bind->name = buf; + bind->name_len = name_len; + luaL_tofield(L, luaL_msgpack_default, -1, &field); + } else { + bind->name = NULL; + bind->name_len = 0; + } + switch (field.type) { + case MP_UINT: { + if (field.ival < 0) { + diag_set(ClientError, ER_SQL_BIND_VALUE, + sql_bind_name(bind), "INTEGER"); + return -1; + } + bind->i64 = field.ival; + bind->type = SQLITE_INTEGER; + bind->bytes = sizeof(bind->i64); + break; + } + case MP_INT: + bind->i64 = field.ival; + bind->type = SQLITE_INTEGER; + bind->bytes = sizeof(bind->i64); + break; + case MP_STR: + /* + * Data should be saved in allocated memory as it + * will be poped from Lua stack. + */ + buf = region_alloc(region, field.sval.len + 1); + if (buf == NULL) { + diag_set(OutOfMemory, field.sval.len + 1, + "region_alloc", "buf"); + return -1; + } + memcpy(buf, field.sval.data, field.sval.len + 1); + bind->s = buf; + bind->type = SQLITE_TEXT; + bind->bytes = field.sval.len; + break; + case MP_DOUBLE: + bind->d = field.dval; + bind->type = SQLITE_FLOAT; + bind->bytes = sizeof(bind->d); + break; + case MP_FLOAT: + bind->d = field.dval; + bind->type = SQLITE_FLOAT; + bind->bytes = sizeof(bind->d); + break; + case MP_NIL: + bind->type = SQLITE_NULL; + bind->bytes = 1; + break; + case MP_BOOL: + /* SQLite doesn't support boolean. Use int instead. */ + bind->i64 = field.bval ? 1 : 0; + bind->type = SQLITE_INTEGER; + bind->bytes = sizeof(bind->i64); + break; + case MP_BIN: + bind->s = mp_decode_bin(&field.sval.data, &bind->bytes); + bind->type = SQLITE_BLOB; + break; + case MP_EXT: + /* + * Data should be saved in allocated memory as it + * will be poped from Lua stack. + */ + buf = region_alloc(region, sizeof(field)); + if (buf == NULL) { + diag_set(OutOfMemory, sizeof(field), "region_alloc", + "buf"); + return -1; + } + memcpy(buf, &field, sizeof(field)); + bind->s = buf; + bind->bytes = sizeof(field); + bind->type = SQLITE_BLOB; + break; + case MP_ARRAY: + diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY", + sql_bind_name(bind)); + return -1; + case MP_MAP: + diag_set(ClientError, ER_SQL_BIND_TYPE, "MAP", + sql_bind_name(bind)); + return -1; + default: + unreachable(); + } + lua_pop(L, lua_gettop(L) - idx); + return 0; +} + +int +lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind, + int idx) +{ + assert(out_bind != NULL); + uint32_t bind_count = lua_objlen(L, idx); + if (bind_count == 0) + return 0; + if (bind_count > SQL_BIND_PARAMETER_MAX) { + diag_set(ClientError, ER_SQL_BIND_PARAMETER_MAX, + (int) bind_count); + return -1; + } + struct region *region = &fiber()->gc; + uint32_t used = region_used(region); + size_t size = sizeof(struct sql_bind) * bind_count; + /* + * Memory allocated here will be freed in + * sqlite3_finalize() or in txn_commit()/txn_rollback() if + * there is an active transaction. + */ + struct sql_bind *bind = (struct sql_bind *) region_alloc(region, size); + if (bind == NULL) { + diag_set(OutOfMemory, size, "region_alloc", "bind"); + return -1; + } + for (uint32_t i = 0; i < bind_count; ++i) { + if (lua_sql_bind_decode(L, &bind[i], idx, i) != 0) { + region_truncate(region, used); + return -1; + } + } + *out_bind = bind; + return bind_count; +} + /** * Serialize a single column of a result set row. * @param stmt Prepared and started statement. At least one diff --git a/src/box/execute.h b/src/box/execute.h index c90789f..cb149a0 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -69,6 +69,27 @@ int sql_bind_list_decode(const char *data, struct sql_bind **out_bind); /** + * Parse Lua table of SQL parameters. + * + * @param L Lua stack contains table with parameters. Each + * parameter either must have scalar type, or must be a + * single-row table with the following format: + * table[name] = value. Name - string name of the named + * parameter, value - scalar value of the parameter. + * Named and positioned parameters can be mixed. + * For more details + * @sa https://www.sqlite.org/lang_expr.html#varparam. + * @param[out] out_bind Pointer to save decoded parameters. + * @param idx Position of table with parameters on Lua stack. + * + * @retval >= 0 Number of decoded parameters. + * @retval -1 Client or memory error. + */ +int +lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind, + int idx); + +/** * Prepare and execute an SQL statement. * @param sql SQL statement. * @param len Length of @a sql. diff --git a/src/box/lua/sql.c b/src/box/lua/sql.c index a55566e..20ba43b 100644 --- a/src/box/lua/sql.c +++ b/src/box/lua/sql.c @@ -118,13 +118,22 @@ lbox_execute(struct lua_State *L) int bind_count = 0; size_t length; struct port port; + int top = lua_gettop(L); - if (lua_type(L, 1) != LUA_TSTRING) - return luaL_error(L, "Usage: box.execute(sqlstring)"); + if ((top != 1 && top != 2) || lua_type(L, 1) != LUA_TSTRING) + return luaL_error(L, "Usage: box.execute(sqlstring[, params])"); const char *sql = lua_tolstring(L, 1, &length); if (sql == NULL) - return luaL_error(L, "Usage: box.execute(sqlstring)"); + return luaL_error(L, "Usage: box.execute(sqlstring[, params])"); + + if (top == 2) { + if (! lua_istable(L, 2)) + return luaL_error(L, "Second argument must be a table"); + bind_count = lua_sql_bind_list_decode(L, &bind, 2); + if (bind_count < 0) + return luaT_error(L); + } if (sql_prepare_and_execute(sql, length, bind, bind_count, &port, &fiber()->gc) != 0) -- 2.7.4