Re: [Tarantool-patches] [PATCH v2] net.box: add __serialize and __tostring methods for future objects

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Wed, Aug 25, 2021 at 12:39:27AM +0200, Vladislav Shpilevoy wrote:
> > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> > index 1783da607dcc..3ed464a93437 100644
> > --- a/src/box/lua/net_box.c
> > +++ b/src/box/lua/net_box.c
> > @@ -1432,6 +1432,29 @@ luaT_netbox_request_gc(struct lua_State *L)
> >  	return 0;
> >  }
> >  
> > +static int
> > +luaT_netbox_request_tostring(struct lua_State *L)
> > +{
> > +	lua_pushstring(L, netbox_request_typename);
> > +	return 1;
> > +}
> > +
> > +static int
> > +luaT_netbox_request_serialize(struct lua_State *L)
> > +{
> > +	struct netbox_request *request = luaT_check_netbox_request(L, 1);
> > +	/*
> > +	 * If there are user-defined fields attached to the future object,
> > +	 * return them, otherwise push the type name, like __tostring does.
> > +	 */
> > +	if (request->index_ref != LUA_NOREF) {
> > +		lua_rawgeti(L, LUA_REGISTRYINDEX, request->index_ref);
> > +	} else {
> > +		lua_pushstring(L, netbox_request_typename);
> > +	}
> > +	return 1;
> > +}
> 
> 1. It does not look good that __serialize might return both a table
> and a string depending on the object content. Perhaps it is worth to
> return an empty table when it has no members. Otherwise, say, if I
> store an optional value in the index and want to get it like this:
> 
> 	serialized_req.member
> 
> then I will get sometimes nil, sometimes an error would be thrown.
> 

Agree. Fixed the patch to return an empty table in case there's no user
data stored in the future object.

> > +
> >  static int
> >  luaT_netbox_request_index(struct lua_State *L)
> >  {
> > @@ -2107,6 +2130,8 @@ luaopen_net_box(struct lua_State *L)
> >  
> >  	static const struct luaL_Reg netbox_request_meta[] = {
> >  		{ "__gc",           luaT_netbox_request_gc },
> > +		{"__tostring",      luaT_netbox_request_tostring },
> > +		{"__serialize",     luaT_netbox_request_serialize },
> 
> 2. You have a whitespace after { in all the other lines. Could you
> please add them here too?

Sorry about that. Fixed.

Incremental diff:
--
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 3ed464a93437..7a72cb324df7 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -1443,14 +1443,10 @@ static int
 luaT_netbox_request_serialize(struct lua_State *L)
 {
 	struct netbox_request *request = luaT_check_netbox_request(L, 1);
-	/*
-	 * If there are user-defined fields attached to the future object,
-	 * return them, otherwise push the type name, like __tostring does.
-	 */
 	if (request->index_ref != LUA_NOREF) {
 		lua_rawgeti(L, LUA_REGISTRYINDEX, request->index_ref);
 	} else {
-		lua_pushstring(L, netbox_request_typename);
+		lua_newtable(L);
 	}
 	return 1;
 }
@@ -2130,8 +2126,8 @@ luaopen_net_box(struct lua_State *L)
 
 	static const struct luaL_Reg netbox_request_meta[] = {
 		{ "__gc",           luaT_netbox_request_gc },
-		{"__tostring",      luaT_netbox_request_tostring },
-		{"__serialize",     luaT_netbox_request_serialize },
+		{ "__tostring",     luaT_netbox_request_tostring },
+		{ "__serialize",    luaT_netbox_request_serialize },
 		{ "__index",        luaT_netbox_request_index },
 		{ "__newindex",     luaT_netbox_request_newindex },
 		{ "is_ready",       luaT_netbox_request_is_ready },
diff --git a/test/box/net.box_fiber-async_gh-3107.result b/test/box/net.box_fiber-async_gh-3107.result
index 450934cd6593..98c9af953efa 100644
--- a/test/box/net.box_fiber-async_gh-3107.result
+++ b/test/box/net.box_fiber-async_gh-3107.result
@@ -263,7 +263,7 @@ tostring(future)
 ...
 future
 ---
-- net.box.request
+- []
 ...
 future.abc = 123
 ---