Re: [PATCH v1 4/5] box: refactor ER_{FIELD_TYPE, ACTION_MISMATCH}

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Sun, Dec 23, 2018 at 03:40:39PM +0300, Kirill Shcherbatov wrote:
> Reworked ER_FIELD_TYPE and ER_ACTION_MISMATCH error to pass
> JSON path to field string instead of field number.
> This patch is required for further JSON patches, to give detailed
> information about field on error.
> 
> Needed for #1012
> ---
>  src/box/errcode.h                   |  4 +-
>  src/box/memtx_rtree.c               |  2 +-
>  src/box/tuple.h                     | 15 +++--
>  src/box/tuple_format.c              | 36 ++++++------
>  test/box/alter.result               |  6 +-
>  test/box/alter_limits.result        |  7 ++-
>  test/box/blackhole.result           |  6 +-
>  test/box/ddl.result                 |  8 ++-
>  test/box/hash.result                | 15 +++--
>  test/box/rtree_misc.result          | 18 ++++--
>  test/box/sequence.result            |  3 +-
>  test/box/tree_pk.result             |  9 ++-
>  test/engine/ddl.result              | 88 ++++++++++++++++++-----------
>  test/engine/indices_any_type.result | 12 ++--
>  test/engine/null.result             | 33 +++++++----
>  test/engine/tree.result             | 12 ++--
>  test/engine/tree_variants.result    | 12 ++--
>  test/engine/update.result           |  6 +-
>  test/engine/upsert.result           |  9 ++-
>  test/replication/misc.result        |  3 +-
>  test/vinyl/constraint.result        | 18 ++++--
>  test/vinyl/errinj.result            |  3 +-
>  test/vinyl/gh.result                |  3 +-
>  test/vinyl/savepoint.result         |  3 +-
>  24 files changed, 206 insertions(+), 125 deletions(-)
> 
> diff --git a/src/box/tuple.h b/src/box/tuple.h
> index 3361b6757..3c8b8825e 100644
> --- a/src/box/tuple.h
> +++ b/src/box/tuple.h
> @@ -784,7 +786,8 @@ tuple_field_u32(const struct tuple *tuple, uint32_t fieldno, uint32_t *out)
>  		return -1;
>  	*out = mp_decode_uint(&field);
>  	if (*out > UINT32_MAX) {
> -		diag_set(ClientError, ER_FIELD_TYPE, fieldno + TUPLE_INDEX_BASE,
> +		diag_set(ClientError, ER_FIELD_TYPE,
> +			 tt_sprintf("%u", fieldno + TUPLE_INDEX_BASE),

Please use int2str helper instead of tt_sprintf wherever applicable.
BTW I told you that during the previous review iteration.

>  			 field_type_strs[FIELD_TYPE_UNSIGNED]);
>  		return -1;
>  	}
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 04343fd53..e29f84dc5 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -61,9 +61,17 @@ tuple_field_delete(struct tuple_field *field)
>  	free(field);
>  }
>  

Comment is missing.

> +static const char *
> +tuple_field_path(const struct tuple_field *field)
> +{
> +	char *msg = tt_static_buf();
> +	json_token_path_snprint(msg, TT_STATIC_BUF_LEN, &field->token,
> +				TUPLE_INDEX_BASE);

I don't like that sometimes we report a field number as [NUM] and
sometimes simply as NUM (e.g. see tuple_field_u32). Let's always
use NUM for top-level tuple fields. This means that for now this
function should look as simple as

	assert(field->token.parent != NULL);
	assert(field->token.parent->parent == NULL)
	assert(field->token.type == JSON_TOKEN_NUM);
	return int2str(field->token.num + TUPLE_INDEX_BASE);

We should start using json_token_path_snprint() later, in the main patch
of the series introducing JSON indexes.

> +	return msg;
> +}
> +