[Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key()

[Vladislav Shpilevoy]

Thanks for the patch!

On 23.09.2020 03:14, Alexander Turenko wrote:
> XXX: Add a module API test.
> 
> Part of #5273
> ---
>  src/box/key_def_api.c | 15 +++++++++++++++
>  src/box/key_def_api.h | 15 +++++++++++++++
>  src/exports.h         |  1 +
>  3 files changed, 31 insertions(+)
> 
> diff --git a/src/box/key_def_api.c b/src/box/key_def_api.c
> index 1257e9060..2555b9fdd 100644
> --- a/src/box/key_def_api.c
> +++ b/src/box/key_def_api.c
> @@ -305,4 +305,19 @@ box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
>  	return tuple_extract_key(tuple, key_def, multikey_idx, key_size_ptr);
>  }
>  
> +int
> +box_key_def_validate_key(const box_key_def_t *key_def, const char *key,
> +			 bool allow_nullable)

I don't like exposing such internal parameters as 'allow_nullable'. It
took me some time to find why is it even needed. Maybe better export 2
separate functions?

    box_key_def_validate_exact_key
    box_key_def_validate_range_key

Or

    box_key_def_validate_unique_key
    box_key_def_validate_key

Or

    box_key_def_validate_unique_key
    box_key_def_validate_non_unique_key

Or

    box_key_def_validate_primary_key
    box_key_def_validate_secondary_key

Also did you think about making this function not setting any error?
What if a module code does not want to spoil diag, and just wants
to check the key? So as to set an own error later. Maybe would be
more flexible to expose a simple 'is_valid' instead of 'validate'?

Or do we do that to get a more precise error message and this is
more important than not setting a diag when not needed?

> +{
> +	uint32_t part_count = mp_decode_array(&key);
> +	if (part_count > key_def->part_count) {
> +		diag_set(ClientError, ER_KEY_PART_COUNT, key_def->part_count,
> +			 part_count);
> +		return -1;
> +	}
> +	const char *key_end;
> +	return key_validate_parts(key_def, key, part_count, allow_nullable,
> +				  &key_end);
> +}