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

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

[Vladimir Davydov via Tarantool-patches]

Example output (for more examples see the test output):

  tarantool> f = c:eval('return 123', {}, {is_async = true})
  ---
  ...

  tarantool> tostring(f)
  ---
  - 'net.box.request: 7'
  ...

  tarantool> f
  ---
  - method: eval
    on_push_ctx: []
    result: [123]
    on_push: 'function: builtin#91'
    sync: 7
  ...

Closes #6314
---
https://github.com/tarantool/tarantool/issues/6314
https://github.com/tarantool/tarantool/tree/vdavydov/netbox-future-serialize

 src/box/lua/net_box.c                         | 68 +++++++++++++
 test/box/net.box_fiber-async_gh-3107.result   | 95 ++++++++++++++++++-
 test/box/net.box_fiber-async_gh-3107.test.lua | 28 +++++-
 3 files changed, 183 insertions(+), 8 deletions(-)

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 229dec590cf9..19ceec9544b6 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -82,6 +82,30 @@ enum netbox_method {
 	netbox_method_MAX
 };
 
+static const char *netbox_method_str[] = {
+	[NETBOX_PING]      = "ping",
+	[NETBOX_CALL_16]   = "call_16",
+	[NETBOX_CALL_17]   = "call",
+	[NETBOX_EVAL]      = "eval",
+	[NETBOX_INSERT]    = "insert",
+	[NETBOX_REPLACE]   = "replace",
+	[NETBOX_DELETE]    = "delete",
+	[NETBOX_UPDATE]    = "update",
+	[NETBOX_UPSERT]    = "upsert",
+	[NETBOX_SELECT]    = "select",
+	[NETBOX_EXECUTE]   = "execute",
+	[NETBOX_PREPARE]   = "prepare",
+	[NETBOX_UNPREPARE] = "unprepare",
+	[NETBOX_GET]       = "get",
+	[NETBOX_MIN]       = "min",
+	[NETBOX_MAX]       = "max",
+	[NETBOX_COUNT]     = "count",
+	[NETBOX_BEGIN]     = "begin",
+	[NETBOX_COMMIT]    = "commit",
+	[NETBOX_ROLLBACK]  = "rollback",
+	[NETBOX_INJECT]    = "inject",
+};
+
 struct netbox_registry {
 	/** Next request id. */
 	uint64_t next_sync;
@@ -1420,6 +1444,48 @@ luaT_netbox_request_gc(struct lua_State *L)
 	return 0;
 }
 
+static int
+luaT_netbox_request_serialize(struct lua_State *L)
+{
+	struct netbox_request *request = luaT_check_netbox_request(L, 1);
+	lua_newtable(L);
+	luaL_pushuint64(L, request->sync);
+	lua_setfield(L, -2, "sync");
+	lua_pushstring(L, netbox_method_str[request->method]);
+	lua_setfield(L, -2, "method");
+	lua_rawgeti(L, LUA_REGISTRYINDEX, request->buffer_ref);
+	lua_setfield(L, -2, "buffer");
+	if (request->skip_header) {
+		lua_pushboolean(L, true);
+		lua_setfield(L, -2, "skip_header");
+	}
+	lua_rawgeti(L, LUA_REGISTRYINDEX, request->on_push_ref);
+	lua_setfield(L, -2, "on_push");
+	lua_rawgeti(L, LUA_REGISTRYINDEX, request->on_push_ctx_ref);
+	lua_setfield(L, -2, "on_push_ctx");
+	if (request->result_ref != LUA_NOREF) {
+		lua_rawgeti(L, LUA_REGISTRYINDEX, request->result_ref);
+		lua_setfield(L, -2, "result");
+	}
+	if (request->error != NULL) {
+		diag_set_error(diag_get(), request->error);
+		luaT_pusherror(L, request->error);
+		lua_setfield(L, -2, "error");
+	}
+	return 1;
+}
+
+static int
+luaT_netbox_request_tostring(struct lua_State *L)
+{
+	char buf[32];
+	struct netbox_request *request = luaT_check_netbox_request(L, 1);
+	snprintf(buf, sizeof(buf), "%s: %llu", netbox_request_typename,
+		 (unsigned long long)request->sync);
+	lua_pushstring(L, buf);
+	return 1;
+}
+
 /**
  * Returns true if the response was received for the given request.
  */
@@ -2052,6 +2118,8 @@ luaopen_net_box(struct lua_State *L)
 
 	static const struct luaL_Reg netbox_request_meta[] = {
 		{ "__gc",           luaT_netbox_request_gc },
+		{"__serialize",     luaT_netbox_request_serialize },
+		{"__tostring",      luaT_netbox_request_tostring },
 		{ "is_ready",       luaT_netbox_request_is_ready },
 		{ "result",         luaT_netbox_request_result },
 		{ "wait_result",    luaT_netbox_request_wait_result },
diff --git a/test/box/net.box_fiber-async_gh-3107.result b/test/box/net.box_fiber-async_gh-3107.result
index aaaca351a579..f2fbbde3d1bd 100644
--- a/test/box/net.box_fiber-async_gh-3107.result
+++ b/test/box/net.box_fiber-async_gh-3107.result
@@ -1,3 +1,6 @@
+test_run = require('test_run').new()
+---
+...
 fiber = require 'fiber'
 ---
 ...
@@ -10,10 +13,7 @@ net = require('net.box')
 cond = nil
 ---
 ...
-box.schema.func.create('long_function')
----
-...
-box.schema.user.grant('guest', 'execute', 'function', 'long_function')
+box.schema.user.grant('guest', 'execute', 'universe')
 ---
 ...
 function long_function(...) cond = fiber.cond() cond:wait() return ... end
@@ -104,7 +104,92 @@ err:find('Usage') ~= nil
 ---
 - true
 ...
-box.schema.func.drop('long_function')
+--
+-- __serialize and __tostring future methods
+--
+future = c:call('long_function', {1, 2, 3}, {is_async = true})
+---
+...
+tostring(future)
+---
+- 'net.box.request: 5'
+...
+future
+---
+- on_push_ctx: []
+  method: call
+  sync: 5
+  on_push: 'function: builtin#91'
+...
+finalize_long()
+---
+...
+future:wait_result()
+---
+- [1, 2, 3]
+...
+future
+---
+- method: call
+  on_push_ctx: []
+  result: [1, 2, 3]
+  on_push: 'function: builtin#91'
+  sync: 5
+...
+future = c:eval('assert(false)', {}, {is_async = true})
+---
+...
+tostring(future)
+---
+- 'net.box.request: 6'
+...
+future:wait_result()
+---
+- null
+- 'eval:1: assertion failed!'
+...
+future
+---
+- error: 'eval:1: assertion failed!'
+  method: eval
+  on_push_ctx: []
+  on_push: 'function: builtin#91'
+  sync: 6
+...
+future = c:eval('return 123', {}, {is_async = true, skip_header = true, \
+                                   buffer = require('buffer').ibuf()})
+---
+...
+tostring(future)
+---
+- 'net.box.request: 7'
+...
+future:wait_result()
+---
+- 6
+...
+test_run:cmd("push filter '0x[a-f0-9]+' to '<addr>'")
+---
+- true
+...
+future
+---
+- method: eval
+  result: 6
+  on_push_ctx: []
+  buffer:
+    ibuf:
+      rpos: 'cdata<char *>: <addr>'
+      wpos: 'cdata<char *>: <addr>'
+  on_push: 'function: builtin#91'
+  sync: 7
+  skip_header: true
+...
+test_run:cmd("clear filter")
+---
+- true
+...
+box.schema.user.revoke('guest', 'execute', 'universe')
 ---
 ...
 c:close()
diff --git a/test/box/net.box_fiber-async_gh-3107.test.lua b/test/box/net.box_fiber-async_gh-3107.test.lua
index d23f368cbce4..55f9afabad10 100644
--- a/test/box/net.box_fiber-async_gh-3107.test.lua
+++ b/test/box/net.box_fiber-async_gh-3107.test.lua
@@ -1,3 +1,5 @@
+test_run = require('test_run').new()
+
 fiber = require 'fiber'
 net = require('net.box')
 
@@ -5,8 +7,7 @@ net = require('net.box')
 -- gh-3107: fiber-async netbox.
 --
 cond = nil
-box.schema.func.create('long_function')
-box.schema.user.grant('guest', 'execute', 'function', 'long_function')
+box.schema.user.grant('guest', 'execute', 'universe')
 function long_function(...) cond = fiber.cond() cond:wait() return ... end
 function finalize_long() while not cond do fiber.sleep(0.01) end cond:signal() cond = nil end
 s = box.schema.create_space('test')
@@ -36,7 +37,28 @@ err:find('Usage') ~= nil
 _, err = pcall(future.wait_result, future, '100')
 err:find('Usage') ~= nil
 
-box.schema.func.drop('long_function')
+--
+-- __serialize and __tostring future methods
+--
+future = c:call('long_function', {1, 2, 3}, {is_async = true})
+tostring(future)
+future
+finalize_long()
+future:wait_result()
+future
+future = c:eval('assert(false)', {}, {is_async = true})
+tostring(future)
+future:wait_result()
+future
+future = c:eval('return 123', {}, {is_async = true, skip_header = true, \
+                                   buffer = require('buffer').ibuf()})
+tostring(future)
+future:wait_result()
+test_run:cmd("push filter '0x[a-f0-9]+' to '<addr>'")
+future
+test_run:cmd("clear filter")
+
+box.schema.user.revoke('guest', 'execute', 'universe')
 
 c:close()
 s:drop()
-- 
2.25.1

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

[Yaroslav Dynnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

Hi! Thank you for the patch. Find my comments below.

Best regards
Yaroslav Dynnikov


On Fri, 13 Aug 2021 at 15:58, Vladimir Davydov <vdavydov@tarantool.org>
wrote:

> Example output (for more examples see the test output):
>
>   tarantool> f = c:eval('return 123', {}, {is_async = true})
>   ---
>   ...
>
>   tarantool> tostring(f)
>   ---
>   - 'net.box.request: 7'
>   ...
>

I think sync has no sense here. It's useless without other info (peer,
function).
Also, it complicates the tests and makes them less reliable.
IMHO simple 'net.box.request' is enough.


>   tarantool> f
>   ---
>   - method: eval
>     on_push_ctx: []
>     result: [123]
>     on_push: 'function: builtin#91'
>     sync: 7
>   ...
>

This is also weird:

tarantool> f
---
- on_push_ctx: []
  method: call
  sync: 4
  on_push: 'function: builtin#91'
...

tarantool> f.sync
---
- null
...

tarantool> f.method
---
- null
...

Why displaying inaccessible fields? You leave a developer no choice:

tarantool> debug.getmetatable(f).__serialize(f).method
---
- call
...

Of course, I miss that fields, but please, there should be another way to
access them.

Also, speaking of
https://github.com/tarantool/tarantool/commit/b996502b403c1698c4b0886de636faa0a63cfb70
,
which of them are you going to merge first? Whichever it is, I guess the
second one is destined to be refactored afterward.

Closes #6314
> ---
> https://github.com/tarantool/tarantool/issues/6314
>
> https://github.com/tarantool/tarantool/tree/vdavydov/netbox-future-serialize
>
>

[-- Attachment #2: Type: text/html, Size: 3072 bytes --]

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

[Vladimir Davydov via Tarantool-patches]

On Fri, Aug 13, 2021 at 08:41:28PM +0300, Yaroslav Dynnikov wrote:
> On Fri, 13 Aug 2021 at 15:58, Vladimir Davydov <vdavydov@tarantool.org>
> wrote:
> 
> > Example output (for more examples see the test output):
> >
> >   tarantool> f = c:eval('return 123', {}, {is_async = true})
> >   ---
> >   ...
> >
> >   tarantool> tostring(f)
> >   ---
> >   - 'net.box.request: 7'
> >   ...
> >
> 
> I think sync has no sense here. It's useless without other info (peer,
> function).
> Also, it complicates the tests and makes them less reliable.
> IMHO simple 'net.box.request' is enough.

Agree.

> 
> 
> >   tarantool> f
> >   ---
> >   - method: eval
> >     on_push_ctx: []
> >     result: [123]
> >     on_push: 'function: builtin#91'
> >     sync: 7
> >   ...
> >
> 
> This is also weird:
> 
> tarantool> f
> ---
> - on_push_ctx: []
>   method: call
>   sync: 4
>   on_push: 'function: builtin#91'
> ...
> 
> tarantool> f.sync
> ---
> - null
> ...
> 
> tarantool> f.method
> ---
> - null
> ...
> 
> Why displaying inaccessible fields? You leave a developer no choice:
> 
> tarantool> debug.getmetatable(f).__serialize(f).method
> ---
> - call
> ...
> 
> Of course, I miss that fields, but please, there should be another way to
> access them.
> 
> Also, speaking of
> https://github.com/tarantool/tarantool/commit/b996502b403c1698c4b0886de636faa0a63cfb70
> ,
> which of them are you going to merge first? Whichever it is, I guess the
> second one is destined to be refactored afterward.

Reworked the patch. Now __serialize returns user-defined fields if there
are any. If there's no fields, it falls back on __tostring. Will send v2
in reply to this email. Please take a look.

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

[Vladimir Davydov via Tarantool-patches]

__tostring always returns the type name, which is 'net.box.request'.

The output of __serialize depends on whether there are user-defined
fields attached to the future. If yes, it returns the fields in a table,
otherwise it falls back on __tostring.

Closes #6314
---
https://github.com/tarantool/tarantool/issues/6314
https://github.com/tarantool/tarantool/tree/vdavydov/netbox-future-serialize

Changes in v2:
 - Remove 'sync' from the __tostring output.
 - Remove inaccessible fields from the __serialize output.
   Return user-defined fields instead if any.

 src/box/lua/net_box.c                         | 25 ++++++++++++++
 test/box/net.box_fiber-async_gh-3107.result   | 33 +++++++++++++++++++
 test/box/net.box_fiber-async_gh-3107.test.lua | 12 +++++++
 3 files changed, 70 insertions(+)

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;
+}
+
 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 },
 		{ "__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 ae1c2c308092..450934cd6593 100644
--- a/test/box/net.box_fiber-async_gh-3107.result
+++ b/test/box/net.box_fiber-async_gh-3107.result
@@ -251,6 +251,39 @@ gc.data3 == nil
 ---
 - true
 ...
+--
+-- __tostring and __serialize future methods
+--
+future = c:eval('return 123', {}, {is_async = true})
+---
+...
+tostring(future)
+---
+- net.box.request
+...
+future
+---
+- net.box.request
+...
+future.abc = 123
+---
+...
+future.xyz = 'abc'
+---
+...
+tostring(future)
+---
+- net.box.request
+...
+future
+---
+- xyz: abc
+  abc: 123
+...
+future:wait_result()
+---
+- [123]
+...
 box.schema.user.revoke('guest', 'execute', 'universe')
 ---
 ...
diff --git a/test/box/net.box_fiber-async_gh-3107.test.lua b/test/box/net.box_fiber-async_gh-3107.test.lua
index b6bf35ca2fa1..f93d962ae88e 100644
--- a/test/box/net.box_fiber-async_gh-3107.test.lua
+++ b/test/box/net.box_fiber-async_gh-3107.test.lua
@@ -89,6 +89,18 @@ _ = collectgarbage('collect')
 _ = collectgarbage('collect')
 gc.data3 == nil
 
+--
+-- __tostring and __serialize future methods
+--
+future = c:eval('return 123', {}, {is_async = true})
+tostring(future)
+future
+future.abc = 123
+future.xyz = 'abc'
+tostring(future)
+future
+future:wait_result()
+
 box.schema.user.revoke('guest', 'execute', 'universe')
 
 c:close()
-- 
2.25.1

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

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

See 2 comments below.

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

> +
>  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?

>  		{ "__index",        luaT_netbox_request_index },
>  		{ "__newindex",     luaT_netbox_request_newindex },
>  		{ "is_ready",       luaT_netbox_request_is_ready },

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

[Vladimir Davydov via Tarantool-patches]

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
 ---

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

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the fixes!

LGTM.

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

[Yaroslav Dynnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4394 bytes --]

Hi! Thanks for the patch.

While reviewing it, I've noticed that the autocompletion doesn't work
for a net.box.future object. I guess it's a topic for another issue,
isn't it? Besides that, LGTM.

Best regards
Yaroslav Dynnikov

On Wed, 25 Aug 2021 at 16:35, Vladimir Davydov <vdavydov@tarantool.org>
wrote:

> 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
>  ---
>

[-- Attachment #2: Type: text/html, Size: 5695 bytes --]

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

[Vladimir Davydov via Tarantool-patches]

On Thu, Aug 26, 2021 at 05:56:46PM +0300, Yaroslav Dynnikov wrote:
> While reviewing it, I've noticed that the autocompletion doesn't work
> for a net.box.future object. I guess it's a topic for another issue,
> isn't it? Besides that, LGTM.

I'm not sure, we can possibly make autocompletion work with __index,
but yes, it's a different issue.

Pushed to master.