From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 910B54696C3 for ; Sat, 18 Apr 2020 20:14:07 +0300 (MSK) References: From: lvasiliev Message-ID: <71694537-8ac1-471e-2034-224e7434b600@tarantool.org> Date: Sat, 18 Apr 2020 20:14:06 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. On 17.04.2020 3:49, Vladislav Shpilevoy wrote: > 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. > Ok. >> 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. > Updated, now the CustomError code is independent of the ClientError code.