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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

> 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.