withDefaultConfig does not work properly with url connection strings

matt1097 commented 1 year ago

Description

Using the Promise example code and the following two config files, I can get drastically different behavior.

url based connection string:

{
  "vhosts": {
    "/": {
      "connection": {
        "url": "amqp://guest:guest@HOST:5672/"
      }
    }
  }
}

parameters based connection string:

{
  "vhosts": {
    "/": {
      "connection": {
        "slashes": true,
        "protocol": "amqp",
        "hostname": "HOST",
        "user": "guest",
        "password": "guest",
        "port": 5672,
        "vhost": "/"
      }
    }
  }
}

When using the first file my connection details looks like:

and with the second it is more like what i expect:

Notice when using the URL based connection, that heart beat and channel max do not appear to be getting set properly from the default config.

When using the parameter based connection, they are properly set to 10s and 100.

That is the observable behavior, I am unsure on if it is missing the other default options as well.

Expected Behavior

Both connection methods should be properly using the default config

Actual Behavior

Default config does not appear to be getting used when using the all-in-one url string connection method

Steps to Reproduce

Start with your promises example
Run it once with each config and observe the connection details in your broker

Context

Was troubleshooting unrelated heartbeat failures and noticed two applications had different values even though neither explicitly specified a heartbeat value in their configs and both were using withDefaultConfig(). Determined it was due to the difference in the connection method.

Your Environment

Version used: Seen in several versions including the latest 16.3.0
Environment name and version (e.g. Chrome 39, node.js 5.4): Nodejs 18.9.0
Operating System and version (desktop or mobile): Seen on Windows, Ubuntu, docker containers, etc.. does not seem to matter.
Broker: RabbitMQ running in a container. completely stock fresh container from image: rabbitmq:3-management-alpine

cressie176 commented 1 year ago

Thanks @matt1097, I'll take a look

cressie176 commented 1 year ago

It's a very long time since I wrote this part of Rascal (8 years), however this was intended behaviour, and there's a test to assert it.

I think my reasoning was that if you specify a URL, then it overrides any explicitly specified individual connection attributes. The presence or absence of options in the url is taken as what was intended. I can see how it is counter intuitive though.

I will give some thought as to whether it might be better to merge the specified connection options with any query parameters specified in the url.

cressie176 commented 1 year ago

I've updated Rascal's configuration to merge values source from a connection url with those provided as individual attributes, favouring the connection url if both are present.

At the same time I've made a few other hygiene changes, such as removing attributes from the connection properties that aren't directly related to amqp connections (e.g. auth, query, pathname), and regenerating the URL from the derived properties so that the url is consistence once all the defaults have been applied.

I'll release as v17 shortly.

onebeyond / rascal