[Tarantool-patches] [PATCH V4 1/6] error: add custom error type
lvasiliev
lvasiliev at tarantool.org
Sat Apr 18 20:14:06 MSK 2020
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 at 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.
More information about the Tarantool-patches
mailing list