[Tarantool-patches] [PATCH] box: provide a user friendly frontend for accessing session settings

[Tarantool-patches] [PATCH] box: provide a user friendly frontend for accessing session settings

[Chris Sosnin]

- 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
---
This patch provides the following syntax as was suggested by Nikita:
box.session.settings.<name> = <value>. Doc request in sql-patch is
updated too.

Note, that I had to break the null - error message rule as long as
__newindex doesn't return anything, the only way to tell the user
about the error is to raise it.

I moved it to the new branch:
branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
issuse:https://github.com/tarantool/tarantool/issues/4711

 src/box/lua/session.c                         | 113 ++++++++++++++++++
 src/box/session.cc                            |   1 +
 src/box/session.h                             |   2 +
 src/box/session_settings.c                    |  16 ++-
 src/box/session_settings.h                    |   3 +
 src/box/sql.c                                 |   5 -
 ...rontend.result => session_settings.result} |  61 ++++++++++
 ...end.test.lua => session_settings.test.lua} |  20 ++++
 8 files changed, 213 insertions(+), 8 deletions(-)
 rename test/box/{gh-4511-access-settings-from-any-frontend.result => session_settings.result} (86%)
 rename test/box/{gh-4511-access-settings-from-any-frontend.test.lua => session_settings.test.lua} (86%)

diff --git a/src/box/lua/session.c b/src/box/lua/session.c
index c6a600f6f..66d59c8c7 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,116 @@ lbox_session_on_access_denied(struct lua_State *L)
 				  lbox_push_on_access_denied_event, NULL);
 }
 
+static int
+lbox_session_setting_get_by_id(struct lua_State *L)
+{
+	assert(lua_type(L, -1) == LUA_TNUMBER);
+	int sid = lua_tointeger(L, -1);
+	lua_pop(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].field_type;
+	if (field_type == FIELD_TYPE_BOOLEAN) {
+		bool value = mp_decode_bool(&mp_pair);
+		lua_pushboolean(L, value);
+	} else {
+		assert(field_type == FIELD_TYPE_STRING);
+		const char *str = mp_decode_str(&mp_pair, &len);
+		lua_pushlstring(L, str, len);
+	}
+	return 1;
+}
+
+static int
+lbox_session_setting_get(struct lua_State *L) {
+	assert(lua_gettop(L) == 2);
+	const char *setting_name = lua_tostring(L, -1);
+	int sid = session_setting_find(setting_name);
+	if (sid < 0) {
+		diag_set(ClientError, ER_PROC_LUA, tt_sprintf("Session "\
+			 "setting %s doesn't exist", setting_name));
+		return luaT_error(L);
+	}
+	lua_pushinteger(L, sid);
+	return lbox_session_setting_get_by_id(L);
+}
+
+static int
+lbox_session_setting_set(struct lua_State *L)
+{
+	assert(lua_gettop(L) == 3);
+	int arg_type = lua_type(L, -1);
+	const char *setting_name = lua_tostring(L, -2);
+	int sid = session_setting_find(setting_name);
+	if (sid < 0) {
+		diag_set(ClientError, ER_PROC_LUA, tt_sprintf("Session "\
+			 "setting %s doesn't exist", setting_name));
+		return luaT_error(L);
+	}
+	struct session_setting *setting = &session_settings[sid];
+	switch (arg_type) {
+	case LUA_TBOOLEAN: {
+		bool value = lua_toboolean(L, -1);
+		size_t size = mp_sizeof_bool(value);
+		char *mp_value = (char *) static_alloc(size);
+		mp_encode_bool(mp_value, value);
+		if (setting->set(sid, mp_value) != 0)
+			return luaT_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 = (char *) static_alloc(size);
+		if (mp_value == NULL) {
+			diag_set(OutOfMemory, size, "static_alloc",
+				 "mp_value");
+			return luaT_error(L);
+		}
+		mp_encode_str(mp_value, str, len);
+		if (setting->set(sid, mp_value) != 0)
+			return luaT_error(L);
+		break;
+	}
+	default:
+		diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+			 session_setting_strs[sid],
+			 field_type_strs[setting->field_type]);
+		return luaT_error(L);
+	}
+	return 0;
+}
+
+static int
+lbox_session_settings_serialize(struct lua_State *L) {
+	lua_newtable(L);
+	for (int id = 0; id < SESSION_SETTING_COUNT; ++id) {
+		lua_pushinteger(L, id);
+		lbox_session_setting_get_by_id(L);
+		lua_setfield(L, -2, session_setting_strs[id]);
+	}
+	return 1;
+}
+
+static void
+lbox_session_settings_init(struct lua_State *L)
+{
+	lua_newtable(L);
+	lua_createtable(L, 0, 3);
+	lua_pushcfunction(L, lbox_session_settings_serialize);
+	lua_setfield(L, -2, "__serialize");
+	lua_pushcfunction(L, lbox_session_setting_get);
+	lua_setfield(L, -2, "__index");
+	lua_pushcfunction(L, lbox_session_setting_set);
+	lua_setfield(L, -2, "__newindex");
+	lua_setmetatable(L, -2);
+	lua_setfield(L, -2, "settings");
+}
+
 void
 session_storage_cleanup(int sid)
 {
@@ -478,5 +590,6 @@ box_lua_session_init(struct lua_State *L)
 		{NULL, NULL}
 	};
 	luaL_register_module(L, sessionlib_name, sessionlib);
+	lbox_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/session_settings.c b/src/box/session_settings.c
index 5e4a50427..15ab65a97 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -324,8 +324,8 @@ session_settings_index_get(struct index *base, const char *key,
 	uint32_t len;
 	key = mp_decode_str(&key, &len);
 	key = tt_cstr(key, len);
-	int sid = 0;
-	if (session_settings_set_forward(&sid, key, true, true) != 0) {
+	int sid = session_setting_find(key);
+	if (sid < 0) {
 		*result = NULL;
 		return 0;
 	}
@@ -426,7 +426,8 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
 	}
 	key = mp_decode_str(&key, &key_len);
 	key = tt_cstr(key, key_len);
-	if (session_settings_set_forward(&sid, key, true, true) != 0) {
+	sid = session_setting_find(key);
+	if (sid < 0) {
 		*result = NULL;
 		return 0;
 	}
@@ -520,3 +521,12 @@ const struct space_vtab session_settings_space_vtab = {
 	/* .prepare_alter = */ generic_space_prepare_alter,
 	/* .invalidate = */ generic_space_invalidate,
 };
+
+int
+session_setting_find(const char *name) {
+	int sid;
+	if (session_settings_set_forward(&sid, name, true, true) == 0)
+		return sid;
+	else
+		return -1;
+}
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index de24e3c6f..e2adc5289 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -84,3 +84,6 @@ struct session_setting {
 
 extern struct session_setting session_settings[SESSION_SETTING_COUNT];
 extern const char *session_setting_strs[SESSION_SETTING_COUNT];
+
+int
+session_setting_find(const char *name);
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 86%
rename from test/box/gh-4511-access-settings-from-any-frontend.result
rename to test/box/session_settings.result
index bae77192e..b32a0becb 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/session_settings.result
@@ -298,3 +298,64 @@ 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 = 'memtx'
+ | ---
+ | ...
+s:get('sql_default_engine').value
+ | ---
+ | - memtx
+ | ...
+settings.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 = true
+ | ---
+ | - error: Session setting sql_default_engine expected a value of type string
+ | ...
+settings.sql_defer_foreign_keys = 'false'
+ | ---
+ | - error: Session setting sql_defer_foreign_keys expected a value of type boolean
+ | ...
+settings.sql_parser_debug = 'string'
+ | ---
+ | - error: Session setting sql_parser_debug expected a value of type boolean
+ | ...
+
+str = string.rep('a', 20 * 1024)
+ | ---
+ | ...
+box.session.settings.sql_default_engine = str
+ | ---
+ | - error: Failed to allocate 20483 bytes in static_alloc for mp_value
+ | ...
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/session_settings.test.lua
similarity index 86%
rename from test/box/gh-4511-access-settings-from-any-frontend.test.lua
rename to test/box/session_settings.test.lua
index b243be15e..440bef7ce 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
+++ b/test/box/session_settings.test.lua
@@ -118,3 +118,23 @@ 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 = 'memtx'
+s:get('sql_default_engine').value
+settings.sql_defer_foreign_keys = 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 = true
+settings.sql_defer_foreign_keys = 'false'
+settings.sql_parser_debug = 'string'
+
+str = string.rep('a', 20 * 1024)
+box.session.settings.sql_default_engine = str
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH] box: provide a user friendly frontend for accessing session settings

[Vladislav Shpilevoy]

Hi!

Is this patch in scope of the other patches of the same thread?
If it is, it should be sent together with them. Otherwise I don't
see what is going to be pushed if I will give ok to the patch.

I see that at least sql patch is changed, but it not resent.

See 3 comments below.

On 17/03/2020 18:21, Chris Sosnin wrote:
> - 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
> ---
> This patch provides the following syntax as was suggested by Nikita:
> box.session.settings.<name> = <value>. Doc request in sql-patch is

1. In the commit message you said 'box.session.setting'. Not 'settings'.
What option did you want to use?

> updated too.
> 
> Note, that I had to break the null - error message rule as long as
> __newindex doesn't return anything, the only way to tell the user
> about the error is to raise it.
> 
> I moved it to the new branch:
> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
> issuse:https://github.com/tarantool/tarantool/issues/4711
> 
> diff --git a/src/box/lua/session.c b/src/box/lua/session.c
> index c6a600f6f..66d59c8c7 100644
> --- a/src/box/lua/session.c
> +++ b/src/box/lua/session.c
> @@ -411,6 +413,116 @@ lbox_session_on_access_denied(struct lua_State *L)
>  				  lbox_push_on_access_denied_event, NULL);
>  }
>  
> +static int
> +lbox_session_setting_get_by_id(struct lua_State *L)

2. This function is never called from Lua. You don't need
to pass ID via Lua stack. Add it as a C argument.

> +{
> +	assert(lua_type(L, -1) == LUA_TNUMBER);
> +	int sid = lua_tointeger(L, -1);
> +	lua_pop(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].field_type;
> +	if (field_type == FIELD_TYPE_BOOLEAN) {
> +		bool value = mp_decode_bool(&mp_pair);
> +		lua_pushboolean(L, value);
> +	} else {
> +		assert(field_type == FIELD_TYPE_STRING);
> +		const char *str = mp_decode_str(&mp_pair, &len);
> +		lua_pushlstring(L, str, len);
> +	}
> +	return 1;
> +}
> +
> +static int
> +lbox_session_setting_get(struct lua_State *L) {

3. Please, use a separate line for {. The same for serialize.

Re: [Tarantool-patches] [PATCH] box: provide a user friendly frontend for accessing session settings

[Chris Sosnin]

Hi! Thank you for the review!

> On 22 Mar 2020, at 22:31, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi!
> 
> Is this patch in scope of the other patches of the same thread?
> If it is, it should be sent together with them. Otherwise I don't
> see what is going to be pushed if I will give ok to the patch.

I’m sorry, I’ve sent these 2 separately for 2 different mail threads.
Will send them together next time.

> 
> I see that at least sql patch is changed, but it not resent.
> 
> See 3 comments below.
> 
> On 17/03/2020 18:21, Chris Sosnin wrote:
>> - 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
>> ---
>> This patch provides the following syntax as was suggested by Nikita:
>> box.session.settings.<name> = <value>. Doc request in sql-patch is
> 
> 1. In the commit message you said 'box.session.setting'. Not 'settings'.
> What option did you want to use?

The last one. Commit message fix done.

> 
>> updated too.
>> 
>> Note, that I had to break the null - error message rule as long as
>> __newindex doesn't return anything, the only way to tell the user
>> about the error is to raise it.
>> 
>> I moved it to the new branch:
>> branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4712-session-settings-v2
>> issuse:https://github.com/tarantool/tarantool/issues/4711
>> 
>> diff --git a/src/box/lua/session.c b/src/box/lua/session.c
>> index c6a600f6f..66d59c8c7 100644
>> --- a/src/box/lua/session.c
>> +++ b/src/box/lua/session.c
>> @@ -411,6 +413,116 @@ lbox_session_on_access_denied(struct lua_State *L)
>> 				  lbox_push_on_access_denied_event, NULL);
>> }
>> 
>> +static int
>> +lbox_session_setting_get_by_id(struct lua_State *L)
> 
> 2. This function is never called from Lua. You don't need
> to pass ID via Lua stack. Add it as a C argument.

I agree, fixed.

+static int
+lbox_session_setting_get_by_id(struct lua_State *L, int sid)
+{
+       assert(sid >= 0 && sid < SESSION_SETTING_COUNT);

But now I wonder if it should follow lbox_ naming style, this function
is not exposed to users, should I rename it?

> 
>> +{
>> +	assert(lua_type(L, -1) == LUA_TNUMBER);
>> +	int sid = lua_tointeger(L, -1);
>> +	lua_pop(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].field_type;
>> +	if (field_type == FIELD_TYPE_BOOLEAN) {
>> +		bool value = mp_decode_bool(&mp_pair);
>> +		lua_pushboolean(L, value);
>> +	} else {
>> +		assert(field_type == FIELD_TYPE_STRING);
>> +		const char *str = mp_decode_str(&mp_pair, &len);
>> +		lua_pushlstring(L, str, len);
>> +	}
>> +	return 1;
>> +}
>> +
>> +static int
>> +lbox_session_setting_get(struct lua_State *L) {
> 
> 3. Please, use a separate line for {. The same for serialize.

Done.

Re: [Tarantool-patches] [PATCH] box: provide a user friendly frontend for accessing session settings

[Vladislav Shpilevoy]

Hi!

The patch looks ok, but since other commits are also changed,
it is necessary to send the whole new thread so as we would be
able to review it properly.