Re: [Tarantool-patches] [PATCH V4 1/6] error: add custom error type

[lvasiliev <lvasiliev@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<v.shpilevoy@tarantool.org>
>>
>> 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, <reason string args>` 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.