Absent port in URI resolves in invalid port

[philprime]

Expected behaviour

The README.ME states in Transport Options the following:

AMQP related options may be indicated either by the connection URI or by passing additional parameters to the seneca#client() or seneca#listen() functions.

where connection URI links to the RabbitMQ URI Specification.

This specification states the following:

2.2. Port

The port number to which the underlying TCP connection is made is determined from the port component according to RFC3986. The port component may be absent, indicated by the lack of the ":" character separating it from the host. If it is absent, then the IANA-assigned port number for AMQP 0-9-1, 5672, should be substituted instead.

This means, if an example AMQP URI does not contain a port number, it should use 5672, e.g. amqp://guest:guest@rabbitmq.host/seneca?locale=es_AR should be handled as amqp://guest:guest@rabbitmq.host:5672/seneca?locale=es_AR.

Actual behaviour

My RabbitMQ provider gave me an AMQP URI without a port number and I was experiencing connection issues, as of it didn't connect at all.

After a lot of debugging I found the following method in lib/common/hooker.js:

function hook(options) {
  [...]
  return (args, done) => {
    const pluginOptions = buildPluginOptions(this.seneca, args, options);
    return amqp.connect(pluginOptions.url, pluginOptions.socketOptions)
      .then((conn) => conn.createChannel())
      .then((ch) => {
         [...]
      }).catch(done);
  };
}

If you didn't define a AMQP URI with a port, it will create use the port 10101 instead, and pluginOptions.url will be amqp://guest:guest@rabbitmq.host:10101/seneca?locale=es_AR!

Proposed solution

To conform with the linked specification, it should use the default port 5672 instead of the port 10101

[nfantone]

@techprimate-phil Hi and thanks for taking the time to open an issue!

This looks like a bug, indeed. However, I don't think the port should be enforced by the plugin, but rather be left blank so your AMQP broker resolves the connection according to the spec you cited.

Sorry that it took me so long to come back to you.

[nfantone]

@techprimate-phil I was not able to replicate this on the most basic of tests.

Try this on your node REPL:

> const amqpuri = require('amqpuri');

> function buildPluginOptions(seneca, args, options) {
...   const { clean, deepextend } = seneca.util;
...   const amqpUrl = amqpuri.format(args);
...   return Object.assign(clean(deepextend(options[args.type], args)), {
.....     url: amqpUrl
.....   });
... }

> buildPluginOptions({ util: { clean: a => a, deepextend: Object.assign } }, { url: 'amqp://guest:guest@rabbitmq.host/seneca?locale=es_AR', type: 'client' }, { client: {} });

{ url: 'amqp://guest:guest@rabbitmq.host/seneca?locale=es_AR',
  type: 'client' }

The url appears ok for me. I'm guessing your options are different? Could you please elaborate a bit more on what your setup is? Also, which version of the plugin are you on?

[philprime]

Example to reproduce:

package.json

{
  "name": "seneca-amqp-test",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "seneca": "^3.6.0",
    "seneca-amqp-transport": "^2.2.0"
  }
}

index.js

const seneca = require('seneca')()
        .use('seneca-amqp-transport');

const client = seneca.client({
        type: 'amqp',
        url: 'amqp://guest:guest@localhost',
});

const listener = seneca.listen({
        type: 'amqp',
        url: 'amqp://guest:guest@localhost'
});

Command Just run it using node index.js

This is very basic code but still it should connect to RabbitMQ, instead it throws this error:

STACK     :::  Error: connect ECONNREFUSED 127.0.0.1:10101
               at Object._errnoException (util.js:1022:11)
               at _exceptionWithHostPort (util.js:1044:20)
               at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1198:14)

and you can see here it uses 10101 as the default port if none is given, which is not correct!

If I use amqp://guest:guest@localhost:5672 as the URL it still fails because the connection is closed, but at least it is connecting to the correct URL port!

Proposed solution Either add a note to the documentation that it is necessary to set a port in the URL, or use the correct default port, as explained above.

[nfantone]

@techprimate-phil Ok, found the culprit. For reasons unknown to me there's a default port property in global Seneca options with the value 10101. And since the plugin merges Seneca options into its own, it gets picked if one is not provided.

In light of this, I think we could:

1) Follow this issue upstream and open a new one at https://github.com/senecajs/seneca/issues. I don't believe it's a good idea to decide on an arbitrary numbered port as default for all transports.

2) As a workaround, we could set a default value for port that overrides 10101 with 5672.

[philprime]

Solution 1 would give us more insight into why they set this arbitrary number, but might not resolve the issue. Solution 2 would keep the port number to the default RabbitMQ port as desired by this transport - seems like this is a more applicable as you can keep the URI handling inside this project.

In my opinion solution 2 sounds better, even though it could be considered as a workaround.