[Tarantool-patches] [PATCH 3/4] box: provide a user friendly frontend for accessing session settings
Chris Sosnin
k.sosnin at tarantool.org
Tue Feb 4 22:31:25 MSK 2020
Thank you for the review and your comments!
My answers below:
> 1. Please, don't mix SQL and Lua. At least here. Call it in
> session.cc in session_init(). This file should not initialize
> any session backends.
Done.
> 2. Avoid writing comments in the same line as the code, unless
> this is a special case like a structure inlined initialization.
> This has simple reasons: lines are shorter and therefore easier
> to read; diff is smaller when only the code or only the comment
> are changed.
Actually, I decided that comments are not essential here, so I
dropped them. Starting from here, everything about comments is
skipped.
> 3. Just use static_alloc. The same below.
+char *mp_value = static_alloc(size);
> 4. According to the new agreement, all new Lua functions should
> return nil, err when an error happens. Except OOM and invalid
> usage of API. So if set() returns != 0, then please, use
> luaT_push_nil_and_error().
+return luaT_push_nil_and_error(L);
> 6. You create lots of duplicate metatables here. I would
> try to create it only once and use that one object for
> all lua_setmetatable().
I did it the following way:
+ static const struct luaL_Reg session_setting_meta[] = {
+ {"__serialize", session_setting_serialize},
+ {"set", session_setting_set},
+ {NULL, NULL}
+ };
+ luaL_register_type(L, "session_setting", session_setting_meta);
...
+ luaL_getmetatable(L, "session_setting");
+ lua_setmetatable(L, -2);
> 9. If you start from SQL, you will need to change this code
> in case something will be added prior to SQL settings. Lets
> start from 0 and iterate to SESSION_SETTING_COUNT.
Well, I did it because SQL settings are available right after session
creation, if we add some settings that are not, then trying to access
them would lead to crash. However, currently it won't affect anything,
so ok.
+ for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+ lua_pushstring(L, session_setting_strs[id]);
+ session_setting_create(L, id);
+ lua_settable(L, -3);
+ }
> 11. I propose you to move it to the end of this function. You
> initialize box.session.settings before box.session is
> created. If you move it to the end, you will be able to drop
> this dummy module registration, which you are doing above:
Done:
luaL_register_module(L, sessionlib_name, sessionlib);
+ session_settings_init(L);
lua_pop(L, 1);
+void
+session_settings_init(struct lua_State *L)
+{
+ lua_pushstring(L, "settings");
+ lua_newtable(L);
+ for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+ lua_pushstring(L, session_setting_strs[id]);
+ session_setting_create(L, id);
+ lua_settable(L, -3);
+ }
+ lua_settable(L, -3);
+}
> 12. The file has name 'gh-4511-*', so you can't add issue 4711 tests
> here. Either add to a new file, or rename the test file to
> box/session_settings.
I renamed it.
See new version of the patch below:
================================================================================
- space_object:update() is hard to use for configuring session settings,
so we provide box.session.setting table, which can be used in a much more
native way.
- Prior to this patch sql settings were not accessible before box.cfg()
call, even though these flags can be set right after session creation.
Part of #4711
---
issue:https://github.com/tarantool/tarantool/issues/4711
branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-search-settings
src/box/lua/session.c | 95 +++++++++++++++++++
src/box/session.cc | 1 +
src/box/session.h | 2 +
src/box/sql.c | 5 -
...rontend.result => session_settings.result} | 59 ++++++++++++
...end.test.lua => session_settings.test.lua} | 17 ++++
6 files changed, 174 insertions(+), 5 deletions(-)
rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (87%)
rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (87%)
diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index c6a600f6f..b324cdab0 100644
--- a/src/box/lua/session.c
+++ b/src/box/lua/session.c
@@ -42,6 +42,8 @@
#include "box/user.h"
#include "box/schema.h"
#include "box/port.h"
+#include "box/session_settings.h"
+#include "tt_static.h"
static const char *sessionlib_name = "box.session";
@@ -411,6 +413,91 @@ lbox_session_on_access_denied(struct lua_State *L)
lbox_push_on_access_denied_event, NULL);
}
+static int
+session_setting_serialize(lua_State *L)
+{
+ lua_getfield(L, -1, "_id");
+ int sid = lua_tointeger(L, -1);
+ const char *mp_pair, *mp_pair_end;
+ session_settings[sid].get(sid, &mp_pair, &mp_pair_end);
+ uint32_t len;
+ mp_decode_array(&mp_pair);
+ mp_decode_str(&mp_pair, &len);
+ enum field_type field_type = session_settings[sid].metadata.field_type;
+ if (field_type == FIELD_TYPE_BOOLEAN) {
+ bool value = mp_decode_bool(&mp_pair);
+ lua_pushboolean(L, value);
+ } else {
+ const char *str = mp_decode_str(&mp_pair, &len);
+ lua_pushlstring(L, str, len);
+ }
+ return 1;
+}
+
+static int
+session_setting_set(lua_State *L)
+{
+ if (lua_gettop(L) != 2)
+ goto error;
+ int arg_type = lua_type(L, -1);
+ lua_getfield(L, -2, "_id");
+ int sid = lua_tointeger(L, -1);
+ struct session_setting *setting = &session_settings[sid];
+ lua_pop(L, 1);
+ switch (arg_type) {
+ case LUA_TBOOLEAN: {
+ bool value = lua_toboolean(L, -1);
+ size_t size = mp_sizeof_bool(value);
+ char *mp_value = static_alloc(size);
+ mp_encode_bool(mp_value, value);
+ if (setting->set(sid, mp_value) != 0)
+ return luaT_push_nil_and_error(L);
+ break;
+ }
+ case LUA_TSTRING: {
+ const char *str = lua_tostring(L, -1);
+ size_t len = strlen(str);
+ uint32_t size = mp_sizeof_str(len);
+ char *mp_value = static_alloc(size);
+ mp_encode_str(mp_value, str, len);
+ if (setting->set(sid, mp_value) != 0)
+ return luaT_push_nil_and_error(L);
+ break;
+ }
+ default:
+ goto error;
+ }
+ lua_pushstring(L, session_setting_strs[sid]);
+ lua_pushvalue(L, -2);
+ return 2;
+error:
+ return luaL_error(L, "Usage: setting:set(value)");
+}
+
+static void
+session_setting_create(struct lua_State *L, int id)
+{
+ lua_newtable(L);
+ lua_pushstring(L, "_id");
+ lua_pushinteger(L, id);
+ lua_settable(L, -3);
+ luaL_getmetatable(L, "session_setting");
+ lua_setmetatable(L, -2);
+}
+
+void
+session_settings_init(struct lua_State *L)
+{
+ lua_pushstring(L, "settings");
+ lua_newtable(L);
+ for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+ lua_pushstring(L, session_setting_strs[id]);
+ session_setting_create(L, id);
+ lua_settable(L, -3);
+ }
+ lua_settable(L, -3);
+}
+
void
session_storage_cleanup(int sid)
{
@@ -458,6 +545,13 @@ box_lua_session_init(struct lua_State *L)
luaL_register(L, "box.internal.session", session_internal_lib);
lua_pop(L, 1);
+ static const struct luaL_Reg session_setting_meta[] = {
+ {"__serialize", session_setting_serialize},
+ {"set", session_setting_set},
+ {NULL, NULL}
+ };
+ luaL_register_type(L, "session_setting", session_setting_meta);
+
static const struct luaL_Reg sessionlib[] = {
{"id", lbox_session_id},
{"type", lbox_session_type},
@@ -478,5 +572,6 @@ box_lua_session_init(struct lua_State *L)
{NULL, NULL}
};
luaL_register_module(L, sessionlib_name, sessionlib);
+ session_settings_init(L);
lua_pop(L, 1);
}
diff --git a/src/box/session.cc b/src/box/session.cc
index 881318252..b557eed62 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -283,6 +283,7 @@ session_init()
panic("out of memory");
mempool_create(&session_pool, &cord()->slabc, sizeof(struct session));
credentials_create(&admin_credentials, admin_user);
+ sql_session_settings_init();
}
void
diff --git a/src/box/session.h b/src/box/session.h
index 6dfc7cba5..1c47b8986 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -41,6 +41,8 @@
extern "C" {
#endif /* defined(__cplusplus) */
+extern void sql_session_settings_init();
+
struct port;
struct session_vtab;
diff --git a/src/box/sql.c b/src/box/sql.c
index 1256df856..ba98ce5df 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -64,14 +64,9 @@ static const uint32_t default_sql_flags = SQL_EnableTrigger
| SQL_AutoIndex
| SQL_RecTriggers;
-extern void
-sql_session_settings_init();
-
void
sql_init()
{
- sql_session_settings_init();
-
default_flags |= default_sql_flags;
current_session()->sql_flags |= default_sql_flags;
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/session_settings.result
similarity index 87%
rename from test/box/gh-4511-access-settings-from-any-frontend.result
rename to test/box/session_settings.result
index f072bafae..403a4506c 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/session_settings.result
@@ -298,3 +298,62 @@ s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
| ---
| - error: Session setting sql_defer_foreign_keys expected a value of type boolean
| ...
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+ | ---
+ | ...
+assert(settings ~= nil)
+ | ---
+ | - true
+ | ...
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+ | ---
+ | - ['sql_default_engine', 'vinyl']
+ | ...
+settings.sql_default_engine
+ | ---
+ | - vinyl
+ | ...
+settings.sql_default_engine:set('memtx')
+ | ---
+ | - sql_default_engine
+ | - memtx
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+settings.sql_defer_foreign_keys:set(true)
+ | ---
+ | - sql_defer_foreign_keys
+ | - true
+ | ...
+s:get('sql_defer_foreign_keys').value
+ | ---
+ | - true
+ | ...
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+ | ---
+ | - ['sql_defer_foreign_keys', false]
+ | ...
+settings.sql_defer_foreign_keys
+ | ---
+ | - false
+ | ...
+
+settings.sql_default_engine:set(true)
+ | ---
+ | - null
+ | - Session setting sql_default_engine expected a value of type string
+ | ...
+settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
+ | ---
+ | - error: 'Usage: setting:set(value)'
+ | ...
+settings.sql_parser_debug:set('string')
+ | ---
+ | - null
+ | - Session setting sql_parser_debug expected a value of type boolean
+ | ...
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/session_settings.test.lua
similarity index 87%
rename from test/box/gh-4511-access-settings-from-any-frontend.test.lua
rename to test/box/session_settings.test.lua
index 40a58ad04..99559100d 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
+++ b/test/box/session_settings.test.lua
@@ -118,3 +118,20 @@ s:update('sql_defer_foreign_keys', {{'=', 'some text', true}})
s:update('sql_defer_foreign_keys', {{'=', 'value', 1}})
s:update('sql_defer_foreign_keys', {{'=', 'value', {1}}})
s:update('sql_defer_foreign_keys', {{'=', 'value', '1'}})
+
+-- gh-4711: Provide a user-friendly frontend for accessing session settings.
+settings = box.session.settings
+assert(settings ~= nil)
+
+s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
+settings.sql_default_engine
+settings.sql_default_engine:set('memtx')
+s:get('sql_default_engine').value
+settings.sql_defer_foreign_keys:set(true)
+s:get('sql_defer_foreign_keys').value
+s:update('sql_defer_foreign_keys', {{'=', 2, false}})
+settings.sql_defer_foreign_keys
+
+settings.sql_default_engine:set(true)
+settings.sql_defer_foreign_keys:set(false, 1, 2, 3)
+settings.sql_parser_debug:set('string')
--
2.21.1 (Apple Git-122.3)
More information about the Tarantool-patches
mailing list