Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

box.errors: add "__concat" metamethod #4489

Closed
olegrok opened this issue Sep 11, 2019 · 9 comments
Closed

box.errors: add "__concat" metamethod #4489

olegrok opened this issue Sep 11, 2019 · 9 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@olegrok
Copy link
Collaborator

olegrok commented Sep 11, 2019

Tarantool Enterprise For Mac 2.2.1-13-g8bfcb4422
type 'help' for interactive help
tarantool> fio = require('fio')
tarantool> fio.open('test', { 'O_CREAT', 'O_WRONLY', 'O_APPEND' }, tonumber('644', 8))
---
- null
- 'fio: Permission denied'
...

tarantool> r, e = fio.open('test', { 'O_CREAT', 'O_WRONLY', 'O_APPEND' }, tonumber('644', 8))
tarantool> e
---
- 'fio: Permission denied'
...

tarantool> type(e)
---
- cdata                    -- expected string
...

tarantool> e .. 'something'
---
- error: '[string "return e .. ''something''"]:1: attempt to concatenate ''struct error''
    and ''string'''
...

Follow-up: #4044

Please, check that all fio functions return error as string

@Totktonada
Copy link
Member

It is box.error object and you can, say, look into a lua backtrace with e.trace. Sooner or later we should use it for all errors that comes from tarantool I think. See also #4398.

@olegrok
Copy link
Collaborator Author

olegrok commented Sep 11, 2019

Could we redefine __concat for this object?

@olegrok
Copy link
Collaborator Author

olegrok commented Sep 11, 2019

@Totktonada

I propose following patch to close this issue

From 55b2fd6983d415f8924c10ecf1949db1f1ae6e9f Mon Sep 17 00:00:00 2001
From: Oleg Babin
Date: Wed, 11 Sep 2019 18:53:56 +0300
Subject: [PATCH] error: Add __concat method to error object

Usually functions return pair {nil, err} and user expects that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod.

Closes tarantool/tarantool#4489
---
 src/lua/error.lua | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/lua/error.lua b/src/lua/error.lua
index 28fc0377d..b44fab7b6 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -150,9 +150,17 @@ local function error_index(err, key)
     return error_methods[key]
 end
 
+local function error_concat(lhs, rhs)
+    if lhs == nil or rhs == nil then
+        error("attempt to concatenate struct error and nil")
+    end
+    return tostring(lhs) .. tostring(rhs)
+end
+
 local error_mt = {
     __index = error_index;
     __tostring = error_message;
+    __concat = error_concat;
 };
 
 ffi.metatype('struct error', error_mt);
-- 
2.22.0

That do you think about it?

@Totktonada
Copy link
Member

I would say that it should not allow more then a string: say, '' .. box.NULL gives an error, so box.error.new(box.error.CFG, 'foo', 'bar') .. box.NULL should too. At the first glance it looks that we should allow only an error as the first argument and a string, a number or an error as the second one.

I think it is harmless, so I'm okay in general.

@kyukhin kyukhin added the bug Something isn't working label Sep 26, 2019
@kyukhin kyukhin added this to the 2.4.1 milestone Sep 26, 2019
@olegrok
Copy link
Collaborator Author

olegrok commented Oct 22, 2019

@kyukhin kyukhin modified the milestones: 2.4.1, 2.3.1 Oct 24, 2019
@olegrok
Copy link
Collaborator Author

olegrok commented Nov 27, 2019

@kyukhin kyukhin modified the milestones: 2.3.1, 2.3.2 Dec 9, 2019
olegrok added a commit that referenced this issue Jan 10, 2020
Usually functions return pair {nil, err} and expected that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod

Closes #4489

The case of error "error_mt.__concat(): neither of args is an error"
is not covered by tests because of #4723
olegrok added a commit that referenced this issue Jan 10, 2020
Usually functions return pair {nil, err} and expected that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod.

Closes #4489

The case of error "error_mt.__concat(): neither of args is an error"
is not covered by tests because of #4723
@Totktonada
Copy link
Member

@kyukhin If it is considered as a bug, then it worth to set 1.10.6 milestone?

olegrok added a commit that referenced this issue Jan 15, 2020
Usually functions return pair `nil, err` and expected that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod.

The case of error "error_mt.__concat(): neither of args is an error"
is not covered by tests because of #4723

Closes #4489
@Totktonada Totktonada modified the milestones: 2.3.2, 1.10.6 Jan 16, 2020
Totktonada pushed a commit that referenced this issue Jan 16, 2020
Usually functions return pair `nil, err` and expected that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod.

The case of error "error_mt.__concat(): neither of args is an error"
is not covered by tests because of #4723

Closes #4489

(cherry picked from commit 935db17)
Totktonada pushed a commit that referenced this issue Jan 16, 2020
Usually functions return pair `nil, err` and expected that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod.

The case of error "error_mt.__concat(): neither of args is an error"
is not covered by tests because of #4723

Closes #4489

(cherry picked from commit 935db17)
Totktonada pushed a commit that referenced this issue Jan 16, 2020
Usually functions return pair `nil, err` and expected that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod.

The case of error "error_mt.__concat(): neither of args is an error"
is not covered by tests because of #4723

Closes #4489

(cherry picked from commit 935db17)
@Totktonada
Copy link
Member

@olegrok olegrok changed the title fio: "open" returns error as cdata box.errors: add "__concat" metamethod Jan 17, 2020
@olegrok
Copy link
Collaborator Author

olegrok commented Jan 17, 2020

I've changed issue title - to clarify that was done in fact

avtikhon pushed a commit to avtikhon/tarantool that referenced this issue Feb 3, 2020
Usually functions return pair `nil, err` and expected that err is string.
Let's make the behaviour of error object closer to string
and define __concat metamethod.

The case of error "error_mt.__concat(): neither of args is an error"
is not covered by tests because of tarantool#4723

Closes tarantool#4489
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants