Re: [Tarantool-patches] [PATCH 1/1] tuple: turn invalid bar update into nop

[Nikita Pettik <korablev@tarantool.org>]

On 04 Jul 19:20, Vladislav Shpilevoy wrote:

LGTM

> While working on this I found an inconsistency in which errors do
> we ignore. https://github.com/tarantool/tarantool/issues/5137

Great!
 
> diff --git a/src/box/xrow_update_bar.c b/src/box/xrow_update_bar.c
> index 0033f0044..1d20d6c74 100644
> --- a/src/box/xrow_update_bar.c
> +++ b/src/box/xrow_update_bar.c
> @@ -31,6 +31,22 @@
>  #include "xrow_update_field.h"
>  #include "tuple.h"
>  
> +/**
> + * 'Commit' bar creation only when it is fully initialized and
> + * valid. Because if all this is happening inside an upsert()
> + * operation, it won't stop the whole xrow upsert. This field will
> + * still be saved in the result tuple. But in case of an error
> + * this operation should be skipped. So this is kept 'nop' when
> + * error happens.
> + */
> +static inline int
> +xrow_update_bar_commit(struct xrow_update_field *field)

Mb _done/_finish are better names? Up to you. 

> +{
> +	assert(field->type == XUPDATE_NOP);
> +	field->type = XUPDATE_BAR;
> +	return 0;
> +}
> +
>  /**
>   * Locate a field to update by @a op's JSON path and initialize
>   * @a field as a bar update.
> diff --git a/test/box/update.test.lua b/test/box/update.test.lua
> index 1538cae9c..27cf55467 100644
> --- a/test/box/update.test.lua
> +++ b/test/box/update.test.lua
> @@ -343,20 +343,52 @@ t2:update({{'!', 'g[6][1]', 50}})
>  t4_array:update({{'!', '[4][1]', 100}})
>  t4_map:update({{'!', '[4].a', 100}})
>  -- Test errors.
> -t:update({{'!', 'a', 100}}) -- No such field.
> -t:update({{'!', 'f.a', 300}}) -- Key already exists.
> -t:update({{'!', 'f.c.f[0]', 3.5}}) -- No such index, too small.
> -t:update({{'!', 'f.c.f[100]', 100}}) -- No such index, too big.
> -t:update({{'!', 'g[4][100]', 700}}) -- Insert index into map.
> -t:update({{'!', 'g[1][1]', 300}})
> -t:update({{'!', 'f.g.a', 700}}) -- Insert key into array.
> -t:update({{'!', 'f.g[1].a', 700}})
> -t:update({{'!', 'f[*].k', 20}}) -- 'Any' is not considered valid JSON.

New tests are cool, but mb it is worth reducing diff by placing
new tests right after old ones? Don't insist tho.