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 5498E24D8F for ; Tue, 15 Jan 2019 11:10:13 -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 uEYhKoc3wgge for ; Tue, 15 Jan 2019 11:10:13 -0500 (EST) Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 DA70A1FBF4 for ; Tue, 15 Jan 2019 11:10:12 -0500 (EST) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v7 5/6] lua: parameter binding for new execute() Date: Tue, 15 Jan 2019 19:10:10 +0300 Message-Id: <544b9b2a0b1f1fa5b30323282d60ac46ca7db98c.1547565887.git.imeevma@gmail.com> 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 patch below. On 12/29/18 4:19 PM, Vladislav Shpilevoy wrote: > Thanks for the patch! See 10 comments below. > > On 28/12/2018 21:11, imeevma@tarantool.org wrote: >> 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. > > It is suspicious. I looked at lua_next source and it does > convert negative index to positive with ease. Also, it is used > already in cosole.c, space.cc, httpc. and other places. > > I tried it myself, and -2 value worked for me. Yes, -1 was > incorrect, my fault. > >> >>> 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. > > 1. Or just change MP_MAP to table, and {'name': value} to {name = value}, > and in other places too. But it is up to you. Your comment is ok as well. > Fixed. >> */ >> 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) { > > 2. Why so complex? Why not "! lua_isstring()" ? > Fixed. >> + 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; >> + } > > 3. How is it possible? Lua field reports that it is an unsigned > integer. If it is a way of checking for overflow, then please, > make it more straightforward. I looked into the source of > luaL_tofield and I saw that ival stores uint64_t just casting it > to int64_t. So here you should cast it back and check if it > is > INT64_MAX. > > (uint64_t) field.ival > INT64_MAX > > Also, you do not need braces {} for 'case MP_UINT'. > Fixed. >> bind->i64 = field.ival; >> bind->type = SQLITE_INTEGER; >> bind->bytes = sizeof(bind->i64); >> 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 >> @@ -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); > > 4. I remembered why I do not like luaL_field. When it meets a > non-scalar value like map, it traverses it without necessity. > Lets refactor it a bit. > > diff --git a/src/box/execute.c b/src/box/execute.c > index ade6e6c95..3a45fc524 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -281,33 +281,28 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) > 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; > - } > + if (lua_istable(L, -1)) { > /* > * Get key and value of the only table element to > * lua stack. > */ > lua_pushnil(L); > - lua_next(L, lua_gettop(L) - 1); > + lua_next(L, -2); > if (lua_type(L, -2) != LUA_TSTRING) { > diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\ > "parameter should be a string."); > return -1; > } > + /* Check that the table is one-row sized. */ > + lua_pushvalue(L, -2); > + if (lua_next(L, -4) != 0) { > + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ > + "named parameter should be a single-row "\ > + "table with one key-value pair"); > + return -1; > + } > + lua_pop(L, 1); > size_t name_len; > bind->name = lua_tolstring(L, -2, &name_len); > /* > @@ -323,11 +318,11 @@ 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; > - luaL_tofield(L, luaL_msgpack_default, -1, &field); > } else { > bind->name = NULL; > bind->name_len = 0; > } > + luaL_tofield(L, luaL_msgpack_default, -1, &field); > switch (field.type) { > case MP_UINT: { > if (field.ival < 0) { > > Here I removed common traversal and added an explicit > check that a table consists of one key-value pair. > > Are you ok with these changes? If you are - squash. But > I did not test it though. > Thanks, squashed. The only change: I removed lua_pop(L, 1) that was after lua_next(L, -4) as lua_next() pops in any case. >> + 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. >> + */ > > 5. Are you sure this comment is necessary? It just repeats the > error message below. > Removed. >> + 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; > > 6. How about this? > > diff --git a/src/box/execute.c b/src/box/execute.c > index ade6e6c95..c1b06b345 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -335,11 +335,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) > sql_bind_name(bind), "INTEGER"); > return -1; > } > - bind->i64 = field.ival; > - bind->type = SQLITE_INTEGER; > - bind->bytes = sizeof(bind->i64); > - break; > - } > + FALLTHROUGH; > case MP_INT: > bind->i64 = field.ival; > bind->type = SQLITE_INTEGER; > Squashed. >> + 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; > > 7. > > diff --git a/src/box/execute.c b/src/box/execute.c > index ade6e6c95..fb2e5ea29 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -362,10 +362,6 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) > 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; > Squashed. >> + 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)); > > 8. Why? What a point of copying struct luaL_field onto region? As I > see, MP_EXT is set for unknown cdata like just void *, and the same > for unknown userdata. When void * is met, we do not even know its > size, nor what a value it stores. When we meet userdata, we can learn > its size, but we do not know what a value is stored here and can not > decode it. > > Please, set and error when such thing happens. > Fixed. I used word "USERDATA" in error message to describe data that cannot be used for binding. >> + 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; >> +} > > 9. Also, I noticed one more problem with luaL_tofield. It can > deliberately throw during a number decoding, see at CHECK_NUMBER > macro. Moreover, it throws during table/map traversing in > lua_field_inspect_table. I once tried to remove these exceptions > but failed. > > Here on a throw region leaks. > > Also port_sql_dump_lua can throw during pushing data onto the stack > and the whole port_sql leaks. > > Both problems can be solved using lua_cpcall, but please, ask in > the server team chat if it is worth doing. As I remember, Kostja > says 'let it crash' in such cases, but if a caller will use > > pcall(box.execute) > > then it will not crash, but just leak. > After discussion, it was decided that exceptions generated by lua_error() should be removed. Any other exceptions should be left as is. >> 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])"); > > 10. How can it be NULL, if you have checked above that here a string > is stored? > Fixed. >> + >> + 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) >> Diff between versions: commit 10613d0febaca704155c57e73f3810fcc7cd2eaf Author: Mergen Imeev Date: Thu Jan 10 20:36:42 2019 +0300 Temporary: review fix diff --git a/src/box/execute.c b/src/box/execute.c index 8e76234..1e9ee32 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -347,33 +347,27 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) 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; - } + if (lua_istable(L, -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) { + lua_next(L, -2); + if (! lua_isstring(L, -2)) { diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\ "parameter should be a string."); return -1; } + /* Check that the table is one-row sized. */ + lua_pushvalue(L, -2); + if (lua_next(L, -4) != 0) { + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ + "named parameter should be a table with "\ + "one key - {name = value}"); + return -1; + } size_t name_len; bind->name = lua_tolstring(L, -2, &name_len); /* @@ -389,23 +383,20 @@ 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; - luaL_tofield(L, luaL_msgpack_default, -1, &field); } else { bind->name = NULL; bind->name_len = 0; } + if (luaL_tofield(L, luaL_msgpack_default, -1, &field) < 0) + return -1; switch (field.type) { - case MP_UINT: { - if (field.ival < 0) { + case MP_UINT: + if ((uint64_t) field.ival > INT64_MAX) { 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; - } + FALLTHROUGH; case MP_INT: bind->i64 = field.ival; bind->type = SQLITE_INTEGER; @@ -428,10 +419,6 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) 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; @@ -452,21 +439,9 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i) 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; + diag_set(ClientError, ER_SQL_BIND_TYPE, "USERDATA", + sql_bind_name(bind)); + return -1; case MP_ARRAY: diag_set(ClientError, ER_SQL_BIND_TYPE, "ARRAY", sql_bind_name(bind)); New version: commit 544b9b2a0b1f1fa5b30323282d60ac46ca7db98c 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 10b309a..1e9ee32 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, @@ -327,6 +328,171 @@ 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); + bind->pos = i + 1; + if (lua_istable(L, -1)) { + /* + * Get key and value of the only table element to + * lua stack. + */ + lua_pushnil(L); + lua_next(L, -2); + if (! lua_isstring(L, -2)) { + diag_set(ClientError, ER_ILLEGAL_PARAMS, "name of the "\ + "parameter should be a string."); + return -1; + } + /* Check that the table is one-row sized. */ + lua_pushvalue(L, -2); + if (lua_next(L, -4) != 0) { + diag_set(ClientError, ER_ILLEGAL_PARAMS, "SQL bind "\ + "named parameter should be a table with "\ + "one key - {name = value}"); + 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; + } else { + bind->name = NULL; + bind->name_len = 0; + } + if (luaL_tofield(L, luaL_msgpack_default, -1, &field) < 0) + return -1; + switch (field.type) { + case MP_UINT: + if ((uint64_t) field.ival > INT64_MAX) { + diag_set(ClientError, ER_SQL_BIND_VALUE, + sql_bind_name(bind), "INTEGER"); + return -1; + } + FALLTHROUGH; + 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: + 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: + diag_set(ClientError, ER_SQL_BIND_TYPE, "USERDATA", + sql_bind_name(bind)); + return -1; + 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 1ed9ab5..2b6cd4e 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 f18a797..2999b3e 100644 --- a/src/box/lua/sql.c +++ b/src/box/lua/sql.c @@ -120,10 +120,18 @@ lbox_execute(struct lua_State *L) struct port port; int top = lua_gettop(L); - if (top != 1 || ! lua_isstring(L, 1)) - return luaL_error(L, "Usage: box.execute(sqlstring)"); - + if (! (top == 1 || top == 2) || ! lua_isstring(L, 1)) + return luaL_error(L, "Usage: box.execute(sqlstring[, params])"); const char *sql = lua_tolstring(L, 1, &length); + + 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) return luaT_error(L); -- 2.7.4