Question: auto_delete: undefined mapped to true

[pafmaf]

Hi,

(@mkuratczyk in ref to the chat in Discord)

I stumbled upon this diff , and wondered why it would map undefined to true - and not false.

Intuitively, I would assume that undefined would "evaluate" to false, and also by default the auto_delete attribute should be false, no?

To my understanding, Rabbit may decide to skip the default info (for whatever reason), and return auto_delete: undefined instead of auto_delete: false. But interpreting it as true makes no sense to me here.

And then: why only for auto_delete and not, for e.g. durable as well?

Thanks upfront!

---

I'll try to elaborate a bit / dig a little deeper. aka going down the rabbit-hole..

I can load definitions in the management UI from a JSON file:

{
    "vhosts": [
        {
            "name": "/"
        }
    ],
    "topic_permissions": [],
    "parameters": [],
    "global_parameters": [],
    "policies": [],
    "queues": [
        {
            "name": "yellow.queue",
            "vhost": "/",
            "durable": true,
            "auto_delete": false,
            "arguments": {
                "x-queue-type": "classic"
            }
        }
    ],
    "exchanges": [
        {
            "name": "zebra.uno",
            "vhost": "/",
            "type": "fanout",
            "durable": true,
            "auto_delete": true,
            "internal": false,
            "arguments": {}
        },
        {
            "name": "zebra.dos",
            "vhost": "/",
            "type": "fanout",
            "durable": true,
            "auto_delete": false,
            "internal": false,
            "arguments": {}
        },
        {
            "name": "zebra.tres",
            "vhost": "/",
            "type": "fanout",
            "durable": false,
            "wrong_auto_delete": "basically undefined",
            "internal": false,
            "arguments": {}
        },
        {
            "name": "zebra.quatro",
            "vhost": "/",
            "type": "fanout",
            "durable": true,
            "wrong_auto_delete": "basically undefined",
            "internal": false,
            "arguments": {}
        },
    ],
    "bindings": []
}

Note: you can't omit durable, because the import would fail:

Also, you can't omit auto_delete in queues, because the import would fail:

GET http://localhost:15672/api/exchanges returns:

        {
            "arguments": {},
            "auto_delete": false,
            "durable": true,
            "internal": false,
            "name": "zebra.dos",
            "type": "fanout",
            "user_who_performed_action": "admin",
            "vhost": "/"
        },
      {
            "arguments": {},
            "auto_delete": "undefined",
            "durable": true,
            "internal": false,
            "name": "zebra.quatro",
            "type": "fanout",
            "user_who_performed_action": "admin",
            "vhost": "/"
        },
        {
            "arguments": {},
            "auto_delete": "undefined",
            "durable": false,
            "internal": false,
            "name": "zebra.tres",
            "type": "fanout",
            "user_who_performed_action": "admin",
            "vhost": "/"
        },
        {
            "arguments": {},
            "auto_delete": true,
            "durable": true,
            "internal": false,
            "name": "zebra.uno",
            "type": "fanout",
            "user_who_performed_action": "admin",
            "vhost": "/"
       },

And this is what it looks like in the UI:

So I can directly store auto_delete: undefined in the exchange configuration and get the same value back.

The management UI is displaying AD for "auto_delete": "undefined", - so the referenced diff is "correct" or at least it's consistent with the UI. (Which probably checks something like if (auto_delete), and it's true since it's a string "undefined".)

UPDATE: Yep:

function args_to_features(obj) {
    var res = {};
   // ...
    if (obj.auto_delete) {
        res['auto-delete'] = true;
    }

and.. if ("hello") { console.log("yep") }

---

I had a look at the Rabbit code (I don't know Erlang):

-spec declare
        (name(), type(), boolean(), boolean(), boolean(),
         rabbit_framing:amqp_table(), rabbit_types:username())
        -> rabbit_types:exchange().

declare(XName, Type, Durable, AutoDelete, Internal, Args, Username) ->

But here I'm thinking that declaring an exchange with anything undefined should not be possible.

add_queue in rabbit_definitions.erl tries to set defaults, here maps:get(auto_delete, Queue, false).

add_queue_int(Queue, Name = #resource{virtual_host = VHostName}, ActingUser) ->
    case rabbit_amqqueue:exists(Name) of
        true ->
            ok;
        false ->
            AutoDelete = maps:get(auto_delete, Queue, false),
            DurableDeclare = maps:get(durable, Queue, true),
            ExclusiveDeclare = maps:get(exclusive, Queue, false),

While add_exchange does not, but defaults to undefined. (add_queue looked the same once, until someone refactored)

add_exchange_int(Exchange, Name, ActingUser) ->
    case rabbit_exchange:exists(Name) of
        true ->
            ok;
        false ->
            Internal = case maps:get(internal, Exchange, undefined) of
                           undefined -> false; %% =< 2.2.0
                           I         -> I
                       end,
            rabbit_exchange:declare(Name,
                                    rabbit_exchange:check_type(maps:get(type, Exchange, undefined)),
                                    maps:get(durable,                         Exchange, undefined),
                                    maps:get(auto_delete,                     Exchange, undefined),

So my feeling is that there is a step missing for exchanges, some kind of validation:

% amqqueue.erl
new(#resource{kind = queue} = Name,
    Pid,
    Durable,
    AutoDelete,
    Owner,
    Args,
    VHost,
    Options,
    Type)
% checking whether AutoDelete is bool!
  when (is_pid(Pid) orelse is_tuple(Pid) orelse Pid =:= none) andalso
       is_boolean(Durable) andalso
       is_boolean(AutoDelete) andalso
       (is_pid(Owner) orelse Owner =:= none) andalso
       is_list(Args) andalso
       (is_binary(VHost) orelse VHost =:= undefined) andalso
       is_map(Options) andalso
       is_atom(Type) ->

Exchanges are read from JSON and stored directly to DB, while Queues are validated upfront.

UPDATE: seems like new is not really explicit "validation", but it's just a nice coincidence that it works as a kind of verification here. AFAICS there is no real validation happening in the code. I also could not find any tests checking for valid entities being stored to the DB.

So the real question is: why does Rabbit ever return exchange.auto_delete: undefined it's wrong in my opinion. But please correct me if I'm wrong. I have never looked at this before.

---

The more I look at it, the more I think that the auto_delete: undefined == true is wrong. 🤷 I just don't know what impact the change to false would have. It may break some lib users somewhere.

-define(amqqueue_is_auto_delete(Q),
        (?is_amqqueue_v2(Q) andalso
         ?amqqueue_v2_field_auto_delete(Q) =:= true)). % undefined -> false

maybe_auto_delete_in_khepri(XName, OnlyDurable) ->
    case khepri_tx:get(khepri_exchange_path(XName)) of
        {ok, #exchange{auto_delete = false} = X} ->
            {not_deleted, X};
        {ok, #exchange{auto_delete = true} = X} ->
            case conditional_delete_in_khepri(X, OnlyDurable) of
                {error, in_use}             -> {not_deleted, X};
                {deleted, X, [], Deletions} -> {deleted, X, Deletions}
            end;
        {error, _} ->
            {not_deleted, undefined}
        % `undefined` -> not deleted, basically false
    end.