[tarantool-patches] [PATCH v7 5/6] lua: parameter binding for new execute()
imeevma at tarantool.org
imeevma at tarantool.org
Tue Jan 15 19:10:10 MSK 2019
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 at 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 <imeevma at gmail.com>
>> 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 <imeevma at gmail.com>
>> 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 <imeevma at gmail.com>
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 <imeevma at gmail.com>
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
More information about the Tarantool-patches
mailing list