From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3])
 (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 C202B4765E0
 for <tarantool-patches@dev.tarantool.org>;
 Mon, 21 Dec 2020 20:26:04 +0300 (MSK)
References: <20201221122754.765559-1-void@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <c1952320-c3b5-6870-9da5-20578b5d2436@tarantool.org>
Date: Mon, 21 Dec 2020 18:26:02 +0100
MIME-Version: 1.0
In-Reply-To: <20201221122754.765559-1-void@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v3] lua/key_def: fix
	compare_with_key() part count check
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Sergey Nikiforov <void@tarantool.org>, tarantool-patches@dev.tarantool.org
Cc: Alexander Turenko <alexander.turenko@tarantool.org>

Hi! Thanks for the patch!

It seems you didn't push the latest version on the branch.
The diff below is different from what I see in `git show`.

See 2 comments below.

> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
> index a781aeff9..5143ef3a4 100644
> --- a/src/box/lua/key_def.c
> +++ b/src/box/lua/key_def.c
> @@ -360,17 +360,14 @@ lbox_key_def_compare_with_key(struct lua_State *L)
>  	struct region *region = &fiber()->gc;
>  	size_t region_svp = region_used(region);
>  	size_t key_len;
> -	const char *key_end, *key = lbox_encode_tuple_on_gc(L, 3, &key_len);
> -	uint32_t part_count = mp_decode_array(&key);
> -	if (key_validate_parts(key_def, key, part_count, true,
> -			       &key_end) != 0) {
> +	const char *key = lbox_encode_tuple_on_gc(L, 3, &key_len);
> +	if (box_key_def_validate_key(key_def, key, NULL)) {
>  		region_truncate(region, region_svp);
>  		tuple_unref(tuple);
>  		return luaT_error(L);
>  	}
>  
> -	int rc = tuple_compare_with_key(tuple, HINT_NONE, key,
> -					part_count, HINT_NONE, key_def);
> +	int rc = box_tuple_compare_with_key(tuple, key, key_def);

1. Looks good. The old version with decoding the array only once also
was fine to me though. Up to you and Alexander T.

>  	region_truncate(region, region_svp);
>  	tuple_unref(tuple);
>  	lua_pushinteger(L, rc);
> diff --git a/test/box-tap/gh-5307-key_def-part-count-check.test.lua b/test/box-tap/gh-5307-key_def-part-count-check.test.lua
> new file mode 100755
> index 000000000..dfd50f799
> --- /dev/null
> +++ b/test/box-tap/gh-5307-key_def-part-count-check.test.lua
> @@ -0,0 +1,29 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local mytest = tap.test('key_def part count tests')
> +
> +mytest:plan(3)
> +
> +local key_def = require('key_def')
> +local kd = key_def.new({{fieldno = 1, type = 'unsigned'}})
> +local ok, res
> +
> +-- Should succeed
> +ok, res = pcall(kd.compare_with_key, kd, {1}, {1})
> +print("First: ", ok, res)

2. Try to avoid print() function. By using print() you produce
output not compatible with TAP, and complicate this issue:
https://github.com/tarantool/tarantool/issues/5000

You can use test:diag() method to print any messages.