[tarantool-patches] [PATCH v6 4/5] lua: parameter binding for new execute()
imeevma at tarantool.org
imeevma at tarantool.org
Fri Dec 28 21:11:18 MSK 2018
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 <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.
*/
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 <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
@@ -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
More information about the Tarantool-patches
mailing list