Also enable node decom using string "true"

[jaydoane]

Overview

To decom a node, one normally inserts {"decom":true} into that node's
document via the admin /nodes endpoint. However, it's easy to
accidentally insert {"decom":"true"} instead, in which case the
current mem3 allowed_nodes logic will include that node as allowed,
which is probably not what was intended, and ultimately result in "500
badard" errors sent to the client.

This changes the allowed_nodes algorithm to also accept {"decom":"true"}
to decom a node.

Thanks to Nick Vatamaniuc for discovering this shortcoming.

Testing recommendations

I've included a new unit test for this code, but I wonder if it's more trouble than it's worth.

GitHub issue number

Related Pull Requests

Checklist

• Code is written and works correctly;
• Changes are covered by tests;
• Documentation reflects the changes;

[eiri]

Hmm, jiffy actually supporst JSON booleans

(node1@192.168.50.50)1> jiffy:decode(<<"{\"a\":true}">>).
{[{<<"a">>,true}]}
(node1@192.168.50.50)2> jiffy:decode(<<"{\"a\":\"true\"}">>).
{[{<<"a">>,<<"true">>}]}

So this is more about invalid doc field allowed on admin /nodes endpoint rather than mem3 bug.

I'd suggest either add validation on adding that "decom" field or make mem3 accept both boolean and string, after all you are trading one bug for another here.

[nickva]

Agree with Eric. Since it's a user settable value maybe accept both values as truth?

Ideally a VDU or other validation to prevent a non-boolean value to be set would be better.

[jaydoane]

@nickva @eiri thanks for the reviews!

I've updated this PR to allow "decom" to be either true or "true" to work.

[nickva]

Minor nit: wonder if andalso might work better in general. I usually see that used in conditionals and guards more than and

[eiri]

Right. A bit of the details here: and is a boolean operator with both parts always evaluated and strictly speaking it's not intended for control, while andalso is syntax sugar control's short-circuit which compiler expands to nested case clause, so it's natural it used more in conditionals. Essentially it is the same as writing Decom /= true, Decom /= <<"true">> just in more user-friendly manner.

Basically I agree with @nickva, it's a minor nit, but andalso is more fitting here.

[nickva]

@jaydoane looks good!

+1 (but see comment about andalso)

[jaydoane]

Updated to use andalso. If acceptable, can a committer please merge as well? Thanks.