[tarantool-patches] Re: [PATCH v2 4/4] app: allow to raise an error on too nested tables

[Alexander Turenko]

> app: allow to raise an error on too nested tables

The commit header and the description looks like we have 'crop'
behaviour as default, however it is not more so.

I agree that we should change the default behaviour and raise an error
by default. This however should be clearly stated in the docbot request.

See more comments below.

WBR, Alexander Turenko.

>     msgpack.cfg({encode_crop_too_deep = false})

We have encode_invalid_as_nil with the similar meanings, so, maybe,
encode_too_deep_as_nil?

Or maybe rephrase 'too deep' as one word: foots, dregs, underside? Those
variants don't look good, but maybe you know some word that would fit
better.

> +			if (! cfg->encode_crop_too_deep)
> +				return luaL_error(L, "Too high nest level");

We can easily give more information: say, a current max depth level; so
I think it worth to add to the error message. (The same for msgpackffi
and json.)

> +    -- gh-4434 (yes, the same issue): let users choose whether
> +    -- they want to raise an error on tables with too high nest
> +    -- level.
> +    --
> +    s.cfg({encode_crop_too_deep = false})
> +
> +    local t = nil
> +    for i = 1, max_depth + 1 do t = {t} end
> +    local ok, err = pcall(s.encode, t)
> +    test:ok(not ok, "too deep encode depth")
> +
> +    s.cfg({encode_max_depth = max_depth + 1})
> +    ok, err = pcall(s.encode, t)
> +    test:ok(ok, "no throw in a corner case")
> +
> +    s.cfg({encode_crop_too_deep = true, encode_max_depth = max_depth})

I think we should save a default value of the option and set it here as
we do with encode_max_depth. (The same for msgpackffi.test.lua.)

BTW, the default is 'false' and we should set the default value here
(now we set 'true'). Following test cases (e.g. gh-2888 test case in
app-tap/json.test.lua) should set and restore the option locally.

Maybe it also worth add a test case that will verify that the default
behaviour is to raise an error?