Malformed definition field in runtime parameters when exporting definitions

[LoisSotoLopez]

Describe the bug

When exporting definitions (rabbitmqctl export_definitions ./definitions.json) when e.g. an operator policy has been defined, the resulting parameters.value.definition field is malformed. This, as seen below, results on definition import failures.

Reproduction steps

1. sbin/rabbitmqctl set_operator_policy transient-queue-ttl-1 "^amq\." '{"expires":1800000}' --priority 1 --apply-to queues
2. sbin/rabbitmqctl export_definitions ./definitions.json
3. sbin/rabbitmqctl import_definitions ./definitions.json

❯ sbin/rabbitmqctl set_operator_policy transient-queue-ttl-1 "^amq\." '{"expires":1800000}' --priority 1 --apply-to queues
Setting operator policy override "transient-queue-ttl-1" for pattern "^amq\." to "{"expires":1800000}" with priority "1" for vhost "/" ...

❯ sbin/rabbitmqctl export_definitions ./definitions.json
Exporting definitions in JSON to a file at "./definitions.json" ...

❯ sbin/rabbitmqctl import_definitions ./definitions.json
Importing definitions in JSON from a file at "./definitions.json" ...
Error:
Validation failed

definition must be a dictionary: [[<<"expires">>,1800000]]
 (vhost: "/" / component: "operator_policy" / key: "transient-queue-ttl-1")

Expected behavior

Importing definition should succeed.

Additional context

There's a comment on export_definitions_command.ex at serialise/2 before the JSON.encode/1 of the definitions saying rabbit_definitions already takes care of transforming all proplists into maps, but I don't think it does.

My immediate solution was to change this line but based on the mentioned comment maybe the change should be in rabbit_definitions around here.

[LoisSotoLopez]

Creating an issue just to clarify which of the two solution strategies specified in Additional context is preferred.

[michaelklishin]

@LoisSotoLopez thanks. Using rabbit_data_coercion in either or both places sounds good. I don't think we have a test that runs this round trip using CLI tools but as long as

cd deps/rabbit
gmake ct-definition_import

and so do rabbitmqadmin suites (I can run them against a RabbitMQ server PR if you want me to), we should be good to go.

[michaelklishin]

@LoisSotoLopez I think I see the difference now. Option 1 would introduce a potential incompatibility with plugins that might list runtime parameters. I can think of at least one such plugin.

So updating the "export" code path is the way to go, I am experimenting with it.

[michaelklishin]

Example of what export_definitions produces:

  "parameters": [
    {
      "component": "operator_policy",
      "name": "transient-queue-ttl-1",
      "value": {
        "apply-to": "queues",
        "definition": [
          [
            "expires",
            1800000
          ]
        ],
        "pattern": "^amq\\.",
        "priority": 1
      },
      "vhost": "/"
    }
  ],

So a proplist in definition stays a proplist, despite rabbit_definitions:maybe_map/1.

[michaelklishin]

Right, so the issue seems to be this: rabbit_definitions:maybe_map/1 does a shallow conversion, while the underlying key, definitions, also needs to be converted.

I am close to being done with a PR.

[michaelklishin]

@LoisSotoLopez thank you for doing all the key investigative work. That code path now has much better coverage (even though it generally was good anyway, just not for runtime parameter exports).