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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 12 comments below.

On 28/01/2020 13:50, 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
> ---
>  src/box/lua/session.c                         | 106 ++++++++++++++++++
>  src/box/sql.c                                 |   5 -
>  ...1-access-settings-from-any-frontend.result |  57 ++++++++++
>  ...access-settings-from-any-frontend.test.lua |  17 +++
>  4 files changed, 180 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/lua/session.c b/src/box/lua/session.c
> index c6a600f6f..3ba1253c7 100644
> --- a/src/box/lua/session.c
> +++ b/src/box/lua/session.c
> @@ -42,6 +42,9 @@
>  #include "box/user.h"
>  #include "box/schema.h"
>  #include "box/port.h"
> +#include "box/session_settings.h"
> +
> +extern void sql_session_settings_init();

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.

>  static const char *sessionlib_name = "box.session";
>  
> @@ -411,6 +414,108 @@ 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); /* [setting_name : value] */

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.

> +	mp_decode_str(&mp_pair, &len);
> +	uint32_t 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 = (char *)region_alloc(&fiber()->gc,
> +							      size);

3. Just use static_alloc. The same below.

> +			mp_encode_bool(mp_value, value);
> +			if (setting->set(sid, mp_value) != 0)
> +				luaT_error(L);

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().

> +			lua_pushstring(L, setting->name);
> +			lua_pushboolean(L, value);
> +			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 *)region_alloc(&fiber()->gc,
> +							      size);
> +			mp_encode_str(mp_value, str, len);
> +			if (setting->set(sid, mp_value) != 0)
> +				luaT_error(L);
> +			lua_pushstring(L, setting->name);
> +			lua_pushstring(L, str);
> +			break;
> +		}
> +		default:
> +			goto error;
> +	}
> +	return 2;
> +error:
> +	luaL_error(L, "box.session.settings.set(): bad arguments");
> +	return -1;
> +}
> +
> +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);
> +	lua_newtable(L); /* setting metatable */

5. The same as above about code + comment in one line. Also,
please, use capital letter to start a sentence, and put a dot
in the end.

> +	lua_pushstring(L, "__serialize");
> +	lua_pushcfunction(L, session_setting_serialize);
> +	lua_settable(L, -3);
> +	lua_setmetatable(L, -2);

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().

> +	lua_pushcfunction(L, session_setting_set);
> +	lua_setfield(L, -2, "set");
> +}
> +
> +void
> +session_settings_init(struct lua_State *L)
> +{
> +	/* Init settings that are avaliable right after session creation. */

7. avaliable -> available.

8. Please, keep comments in 66 line width limit.

> +	sql_session_settings_init();
> +	static const struct luaL_Reg settingslib[] = {
> +		{NULL, NULL}
> +	};
> +	luaL_register_module(L, "box.session.settings", settingslib);
> +	int id = sql_session_setting_BEGIN;

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.

> +	for (; id <= sql_session_setting_END; ++id) {
> +		/* Store settings as name : id */

10. Not sure this comment is still correct. You use
name : table, rather than name : id. But honestly here
this is obvious anyway, so I would drop that comment
at all.

> +		struct session_setting *setting = &session_settings[id];
> +		lua_pushstring(L, setting->name);
> +		session_setting_create(L, id);
> +		lua_settable(L, -3);
> +	}
> +	lua_pop(L, 1);
> +}
> +
>  void
>  session_storage_cleanup(int sid)
>  {
> @@ -448,6 +553,7 @@ exit:
>  void
>  box_lua_session_init(struct lua_State *L)
>  {
> +	session_settings_init(L);

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:

    static const struct luaL_Reg settingslib[] = {
    	{NULL, NULL}
    };

Because after

    luaL_register_module(L, sessionlib_name, sessionlib)

you have box.session on the stack and can add whatever you
want there, including settings.

>  	static const struct luaL_Reg session_internal_lib[] = {
>  		{"create", lbox_session_create},
>  		{"run_on_connect",    lbox_session_run_on_connect},
> diff --git a/test/box/gh-4511-access-settings-from-any-frontend.test.lua b/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> index 40a58ad04..53f03450d 100644
> --- a/test/box/gh-4511-access-settings-from-any-frontend.test.lua
> +++ b/test/box/gh-4511-access-settings-from-any-frontend.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.

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.

> +settings = box.session.settings
> +assert(settings ~= nil)
> +
> +s:update('sql_default_engine', {{'=', 2, 'vinyl'}})
> +settings.sql_default_engine
> +settings.sql_default_engine:set('memtx')
> +assert(s:get('sql_default_engine').value == 'memtx')
> +settings.sql_defer_foreign_keys:set(true)
> +assert(s:get('sql_defer_foreign_keys').value == true)
> +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')
>