From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9DEAC44643A for ; Sat, 26 Sep 2020 01:59:02 +0300 (MSK) References: <501646b13840c7f8166e47c7ce75385b76dc00b0.1600817803.git.alexander.turenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9ed17785-7743-827f-2fb3-d38627311699@tarantool.org> Date: Sat, 26 Sep 2020 00:59:00 +0200 MIME-Version: 1.0 In-Reply-To: <501646b13840c7f8166e47c7ce75385b76dc00b0.1600817803.git.alexander.turenko@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org 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); > +}