From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp14.mail.ru (smtp14.mail.ru [94.100.181.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 615714696C3 for ; Fri, 17 Apr 2020 03:49:25 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Fri, 17 Apr 2020 02:49:21 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH V4 1/6] error: add custom error type List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! See 2 comments below. On 16/04/2020 19:38, Leonid Vasiliev wrote: > Co-authored-by: Vladislav Shpilevoy > > Part of #4398 > > @TarantoolBot document > Title: Custom error types for Lua errors > > Errors can be created in 2 ways: `box.error.new()` and `box.error()`. > > Both used to take either `code, reason, ` or > `{code = code, reason = reason, ...}` arguments. > > Now in the first option instead of code a user can specify a > string as its own error type. In the second option a user can > specify both code and type. For example: > > ```Lua > box.error('MyErrorType', 'Message') > box.error({type = 'MyErrorType', code = 1024, reason = 'Message'}) > ``` > Or no-throw version: > ```Lua > box.error.new('MyErrorType', 'Message') > box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'}) > ``` > When a custom type is specified, it is shown in `err.type` > attribute. When it is not specified, `err.type` shows one of > built-in errors such as 'ClientError', 'OurOfMemory', etc. > > Name length limit on the custom type is 63 bytes. All is longer > is truncated. 1. I changed 'All is longer' -> 'All what is longer'. Force pushed. > diff --git a/extra/exports b/extra/exports > index a9add2c..9467398 100644 > --- a/extra/exports > +++ b/extra/exports > @@ -235,6 +235,7 @@ box_index_min > box_index_max > box_index_count > box_error_type > +box_error_custom_type > box_error_code > box_error_message > box_error_last > diff --git a/src/box/errcode.h b/src/box/errcode.h > index d1e4d02..e37b7bd 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -266,6 +266,7 @@ struct errcode_record { > /*211 */_(ER_WRONG_QUERY_ID, "Prepared statement with id %u does not exist") \ > /*212 */_(ER_SEQUENCE_NOT_STARTED, "Sequence '%s' is not started") \ > /*213 */_(ER_NO_SUCH_SESSION_SETTING, "Session setting %s doesn't exist") \ > + /*214 */_(ER_CUSTOM_ERROR, "%s") \ 2. We discussed that with Alexander - what to do with error codes. Because there are 3 options: - CustomErrors don't have a code. Only type name. Code will be added later as a part of payload. In that case the syntax: box.error.new({code = 123, type = 'MyType'}) becomes illegal. 'error.code' returns nil in Lua for CustomError objects. And CustomError should not be a descendant of ClientError in our code. - CustomError has a code, it is always 214. Syntax with custom error code is banned too. But 'error.code' is 214 always. Which is weird. 214 is a very strange number, and it creates an unnecessary link with box.error.* codes, which are valid only for ClientErrors. - CustomError has a code and a user can choose it. By default it is 0. Now the patch is neither of this. You allow to specify code in box.error.new() and box.error(), but it is silently ignored and replaced with 214. >From what I understand, Alexander wants one of the versions above. Certainly not the one where we ignore code argument in box.error.new() and box.error() functions. I am for the last option. So when you write box.error.new({code = 123, type = 'MyType'}) You can take 'error.code' and it is 123. That is the simplest solution in terms of implementation, and the simplest in terms of the API. Because you have single format for all errors: {type = ..., code = ..., reason = ...} Where type and code are optional. If we go for any other option, we will need to split it in 2 versions: either {type = ..., reason = ...} or {code = ..., reason = ...} One can argue, that error codes can intersect, but the thing is that ClientError and CustomError are different types for a user. box.error.* codes are valid only and only for ClientError objects. Moreover, even now I can specify not existing codes when create a ClientError object. So intersection argument does not work - codes can intersect even now. And it seems to be not a problem. Just consider this example: tarantool> args = {code = 214, reason = 'Message'} --- ... tarantool> err = box.error.new(args) --- ... tarantool> err:unpack() --- - code: 214 base_type: ClientError type: ClientError message: Message trace: - file: '[string "err = box.error.new(args)"]' line: 1 ... I created a ClientError with code 214. Not CustomError. ClientError. It means, that code is never going to be enough to detect error type. You will never be able to compare code with 214, and if they are equal, say that it is CustomError. The example above proves it is not true. You always need to look at error type first. Therefore it makes no sense to ban codes for CustomErrors. It won't protect from anything.