Re: [Tarantool-patches] [PATCH 2/6] error: Add the custom error type

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

Thanks for the patch!

See 6 comments below.

On 24/03/2020 13:46, Leonid Vasiliev wrote:
> A possibility to create an error with a custom subtype was added.
> 
> In accordance with https://github.com/tarantool/tarantool/issues/4398
> a custom error type has been added to the box.error.
> Now, it's possible to create an error with a custom subtype (string value)
> for use it in applications.
> 
> @TarantoolBot document
> Title: error.custom_type
> A custom error type has been added to the box.error.
> Now, it's possible to create an error with a custom subtype (string value)
> for use it in applications.
> 
> Example:
> 
> err_custom = box.error.new(box.error.CUSTOM_ERROR, "My Custom Type", "Reason")
> Now:
> err_custom.type == "CustomError"
> err_custom.custom_type == "My Custom Type"

1. Regarding types we discussed that verbally and then I
wrote it in the ticket, that better return custom type in
err.type. And return the original type in a new field. To
simplify cascade error checking, when one type is compared
to many possible types to make appropriate actions. Just for
record.

> err_custom.message == "User custom error: Reason"

2. The whole point of having custom errors - be fully customizable.
I don't think it is a good idea to seal this prefix into every
user defined error. After all, usually errors are created not for
end users, but for systems built on top of tarantool. Hence it
looks odd to even mention 'User'.

3. In the doc request you didn't say a word about custom error
type length limit.

> diff --git a/src/box/error.cc b/src/box/error.cc
> index 47dce33..25e7eff 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -253,3 +274,38 @@ BuildAccessDeniedError(const char *file, unsigned int line,
>  		return e;
>  	}
>  }
> +
> +static struct method_info customerror_methods[] = {
> +	make_method(&type_CustomError, "custom_type", &CustomError::custom_type),
> +	METHODS_SENTINEL
> +};
> +
> +const struct type_info type_CustomError =
> +	make_type("CustomError", &type_ClientError,
> +		  customerror_methods);
> +
> +CustomError::CustomError(const char *file, unsigned int line,
> +			 const char *custom_type)
> +	:ClientError(&type_CustomError, file, line, ER_CUSTOM_ERROR)
> +{
> +	error_format_msg(this, tnt_errcode_desc(m_errcode),
> +			 custom_type ?: "");
> +
> +	if (custom_type) {
> +		strncpy(m_custom_type, custom_type, 63);
> +		m_custom_type[63] = '\0';

4. Please, don't do that. Never use constants the same constant
in several places, and depending on other constants. Use
sizeof(m_custom_type) - 1.

> +	} else {
> +		m_custom_type[0] = '\0';
> +	}
> +}
> diff --git a/src/box/error.h b/src/box/error.h
> index b8c7cf7..3e0beb8 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -70,6 +73,14 @@ const char *
>  box_error_type(const box_error_t *error);
>  
>  /**
> + * Return the error custom type,
> + * \param error
> + * \return pointer to custom error type. On error, return NULL
> + */
> +const char *
> +box_custom_error_type(const box_error_t *e);
> +

5. Please, move this out of '\cond public'. I don't think
we are ready to release custom errors as part of the public
C API. The same for box_custom_error_set().

> +/**
>   * Return IPROTO error code
>   * \param error error
>   * \return enum box_error_code
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 765ce73..26abec8 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -148,11 +152,16 @@ local function error_serialize(err)
>      return error_message(err)
>  end
>  
> +local function error_is_custom(err)
> +    return ffi.string(err._type.name) == 'CustomError'
> +end
> +
>  local error_methods = {
> -    ["unpack"] = error_unpack;
> -    ["raise"] = error_raise;
> -    ["match"] = error_match; -- Tarantool 1.6 backward compatibility
> -    ["__serialize"] = error_serialize;
> +    ["unpack"] = error_unpack,
> +    ["raise"] = error_raise,
> +    ["match"] = error_match, -- Tarantool 1.6 backward compatibility
> +    ["is_custom"] = error_is_custom,
> +    ["__serialize"] = error_serialize

6. The same as in the previous patch - try not to change
existing code when possible. Here it is possible. And below too.

>  }
>  
>  local function error_index(err, key)
> @@ -181,11 +190,11 @@ local function error_concat(lhs, rhs)
>  end
>  
>  local error_mt = {
> -    __index = error_index;
> -    __tostring = error_message;
> -    __concat = error_concat;
> +    __index = error_index,
> +    __tostring = error_message,
> +    __concat = error_concat
>  };
>  
> -ffi.metatype('struct error', error_mt);
> +ffi.metatype('struct error', error_mt)
>  
>  return error