[Tarantool-patches] [PATCH] box: frommap() bug fix

[Tarantool-patches] [PATCH] box: frommap() bug fix

[Chris Sosnin]

From: Chris Sosnin <chris125_@live.com>

This patch already has LGTM from Vlad.

- If an optional argument is provided for
  space_object:frommap() (which is {table = true|false}),
  type match for first arguments is omitted, which is
  incorrect. We should return the result only after making
  sure it is possible to build a tuple.

- If there is a type mismatch, however, frommap() does not
  return nil, err as it is mentioned in the description, so we
  change it to be this way.

Closes #4262
---
branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4262-frommap-error-handle
issue:https://github.com/tarantool/tarantool/issues/4262

 src/box/lua/space.cc    | 12 ++++++++----
 test/box/tuple.result   |  8 +++++++-
 test/box/tuple.test.lua |  1 +
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 01b58afab..d0e44dd41 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -580,14 +580,18 @@ lbox_space_frommap(struct lua_State *L)
 		}
 		lua_rawseti(L, -3, fieldno+1);
 	}
-	if (table)
-		return 1;
 
 	lua_replace(L, 1);
 	lua_settop(L, 1);
 	tuple = luaT_tuple_new(L, -1, space->format);
-	if (tuple == NULL)
-		return luaT_error(L);
+	if (tuple == NULL) {
+		struct error *e = diag_last_error(&fiber()->diag);
+		lua_pushnil(L);
+		lua_pushstring(L, e->errmsg);
+		return 2;
+	}
+	if (table)
+		return 1;
 	luaT_pushtuple(L, tuple);
 	return 1;
 usage_error:
diff --git a/test/box/tuple.result b/test/box/tuple.result
index 9140211b7..78f919deb 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -1121,7 +1121,13 @@ s:frommap()
 ...
 s:frommap({})
 ---
-- error: Tuple field 1 required by space format is missing
+- null
+- Tuple field 1 required by space format is missing
+...
+s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
+---
+- null
+- 'Tuple field 4 type does not match one required by operation: expected unsigned'
 ...
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
 ---
diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
index 3ac2902fe..baf2f22d5 100644
--- a/test/box/tuple.test.lua
+++ b/test/box/tuple.test.lua
@@ -373,6 +373,7 @@ s:frommap({ddd = 1, aaa = 2, bbb = 3})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
 s:frommap()
 s:frommap({})
+s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH] box: frommap() bug fix

[Kirill Yukhin]

Hello,

On 14 янв 13:49, Chris Sosnin wrote:
> (backport version for 1.10)
> This patch is almost the same as the one I've sent
> for 2.3, the only difference is that here it is needed
> to change tuple interface to make it possible to borrow
> the error message after the luaT_tuple_new call.
> 
> - If an optional argument is provided for
>   space_object:frommap() (which is {table = true|false}),
>   type match for first arguments is omitted, which is
>   incorrect. We should return the result only after making
>   sure it is possible to build a tuple.
> 
> - If there is a type mismatch, however, frommap() does not
>   return nil, err as it is mentioned in the description, so we
>   change it to be this way.
> 
> Closes #4262
> ---
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4262-frommap-fix-1-10
> issue: https://github.com/tarantool/tarantool/issues/4262

I've checked your patch into 1.10, 2.2, 2.3 and master.

--
Regards, Kirill Yukhin

[Tarantool-patches] [PATCH] box: frommap() bug fix

[Chris Sosnin]

(backport version for 1.10)
This patch is almost the same as the one I've sent
for 2.3, the only difference is that here it is needed
to change tuple interface to make it possible to borrow
the error message after the luaT_tuple_new call.

- If an optional argument is provided for
  space_object:frommap() (which is {table = true|false}),
  type match for first arguments is omitted, which is
  incorrect. We should return the result only after making
  sure it is possible to build a tuple.

- If there is a type mismatch, however, frommap() does not
  return nil, err as it is mentioned in the description, so we
  change it to be this way.

Closes #4262
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4262-frommap-fix-1-10
issue: https://github.com/tarantool/tarantool/issues/4262

 src/box/lua/space.cc    | 14 +++++++++++---
 src/box/lua/tuple.c     | 13 ++++++++-----
 src/box/lua/tuple.h     |  2 +-
 test/box/tuple.result   | 10 ++++++++--
 test/box/tuple.test.lua |  1 +
 5 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 9162de21d..94e895148 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -461,6 +461,7 @@ lbox_space_frommap(struct lua_State *L)
 	struct tuple_dictionary *dict = NULL;
 	uint32_t id = 0;
 	struct space *space = NULL;
+	struct tuple *tuple = NULL;
 	int argc = lua_gettop(L);
 	bool table = false;
 	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
@@ -503,12 +504,19 @@ lbox_space_frommap(struct lua_State *L)
 		}
 		lua_rawseti(L, -3, fieldno+1);
 	}
-	if (table)
-		return 1;
 
 	lua_replace(L, 1);
 	lua_settop(L, 1);
-	return luaT_tuple_new(L, space->format);
+	tuple = luaT_tuple_new(L, space->format);
+	if (tuple == NULL) {
+		struct error *e = diag_last_error(diag_get());
+		lua_pushnil(L);
+		lua_pushstring(L, e->errmsg);
+		return 2;
+	}
+	if (!table)
+		luaT_pushtuple(L, tuple);
+	return 1;
 usage_error:
 	return luaL_error(L, "Usage: space:frommap(map, opts)");
 }
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 44ed38382..cff0beca5 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -97,7 +97,7 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-int
+struct tuple *
 luaT_tuple_new(struct lua_State *L, struct tuple_format *format)
 {
 	int argc = lua_gettop(L);
@@ -127,17 +127,20 @@ luaT_tuple_new(struct lua_State *L, struct tuple_format *format)
 	struct tuple *tuple = box_tuple_new(format, buf->buf,
 					   buf->buf + ibuf_used(buf));
 	if (tuple == NULL)
-		return luaT_error(L);
+		return NULL;
 	/* box_tuple_new() doesn't leak on exception, see public API doc */
-	luaT_pushtuple(L, tuple);
 	ibuf_reinit(tarantool_lua_ibuf);
-	return 1;
+	return tuple;
 }
 
 static int
 lbox_tuple_new(lua_State *L)
 {
-	return luaT_tuple_new(L, box_tuple_format_default());
+	struct tuple *tuple = luaT_tuple_new(L, box_tuple_format_default());
+	if (tuple == NULL)
+		return luaT_error(L);
+	luaT_pushtuple(L, tuple);
+	return 1;
 }
 
 static int
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index ba7f01f4b..48b630969 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -67,7 +67,7 @@ luaT_istuple(struct lua_State *L, int idx);
 
 /** \endcond public */
 
-int
+struct tuple *
 luaT_tuple_new(struct lua_State *L, struct tuple_format *format);
 
 static inline int
diff --git a/test/box/tuple.result b/test/box/tuple.result
index bc59dcf05..acc5ffa28 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -1121,8 +1121,14 @@ s:frommap()
 ...
 s:frommap({})
 ---
-- error: Tuple field count 0 is less than required by space format or defined indexes
-    (expected at least 1)
+- null
+- Tuple field count 0 is less than required by space format or defined indexes (expected
+  at least 1)
+...
+s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
+---
+- null
+- 'Tuple field 4 type does not match one required by operation: expected unsigned'
 ...
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
 ---
diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
index 8dd15a0a1..3a77c43ed 100644
--- a/test/box/tuple.test.lua
+++ b/test/box/tuple.test.lua
@@ -373,6 +373,7 @@ s:frommap({ddd = 1, aaa = 2, bbb = 3})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
 s:frommap()
 s:frommap({})
+s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH] box: frommap() bug fix

[Vladislav Shpilevoy]

Thanks for the patch!

See 4 comments below.

On 24/11/2019 22:18, Chris Sosnin wrote:
> - If an optional argument is provided for
>   space_object:frommap() (which is {table = true|false}),
>   type match for first arguments is omitted, which is
>   incorrect. We should return the result only after making
>   sure it is possible to build a tuple.

1. This is a good catch. Please, provide a test. To ensure,
that it won't break again in the future.

I think you fit it in an existing test file, where other
frommap() cases are checked. It is not worth creating a new
test file just for this tiny ticket.

> 
> - If there is a type mismatch, however, frommap() does not
>   return nil, err as it is mentioned in the description, so we
>   change it to be this way.
> 
> Closes: #4262
> ---

2. All the same as for #4515 email.

>  src/box/lua/space.cc  | 11 +++++++----
>  test/box/tuple.result |  3 ++-
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index f6e96f0c0..a4ffa8240 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -580,14 +580,17 @@ lbox_space_frommap(struct lua_State *L)
>  		}
>  		lua_rawseti(L, -3, fieldno+1);
>  	}
> -	if (table)
> -		return 1;
> 
>  	lua_replace(L, 1);
>  	lua_settop(L, 1);
>  	tuple = luaT_tuple_new(L, -1, space->format);
> -	if (tuple == NULL)
> -		return luaT_error(L);
> +	if (tuple == NULL) {
> +		struct error *e = diag_last_error(&fiber()->diag);
> +		lua_pushnil(L);
> +		lua_pushstring(L, e->errmsg);
> +		return 2;
> +	} else if (table)
> +		return 1;

3. Nit: I think you may omit 'else' keyword here. Anyway
the previous condition makes a return.

4. When any if's or else's body has {}, all the others
in the same chain should have it as well. So 'return 1;'
should also be in {}.

>  	luaT_pushtuple(L, tuple);
>  	return 1;
>  usage_error:

[Tarantool-patches] [PATCH] box: frommap() bug fix

[Chris Sosnin]

- If an optional argument is provided for
  space_object:frommap() (which is {table = true|false}),
  type match for first arguments is omitted, which is
  incorrect. We should return the result only after making
  sure it is possible to build a tuple.

- If there is a type mismatch, however, frommap() does not
  return nil, err as it is mentioned in the description, so we
  change it to be this way.

Closes: #4262
---
 src/box/lua/space.cc  | 11 +++++++----
 test/box/tuple.result |  3 ++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index f6e96f0c0..a4ffa8240 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -580,14 +580,17 @@ lbox_space_frommap(struct lua_State *L)
 		}
 		lua_rawseti(L, -3, fieldno+1);
 	}
-	if (table)
-		return 1;

 	lua_replace(L, 1);
 	lua_settop(L, 1);
 	tuple = luaT_tuple_new(L, -1, space->format);
-	if (tuple == NULL)
-		return luaT_error(L);
+	if (tuple == NULL) {
+		struct error *e = diag_last_error(&fiber()->diag);
+		lua_pushnil(L);
+		lua_pushstring(L, e->errmsg);
+		return 2;
+	} else if (table)
+		return 1;
 	luaT_pushtuple(L, tuple);
 	return 1;
 usage_error:
diff --git a/test/box/tuple.result b/test/box/tuple.result
index 9140211b7..bbb60d5e7 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -1121,7 +1121,8 @@ s:frommap()
 ...
 s:frommap({})
 ---
-- error: Tuple field 1 required by space format is missing
+- null
+- Tuple field 1 required by space format is missing
 ...
 s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
 ---
--
2.24.0