Skip to content

Commit

Permalink
feat(parseLinkAddress): add utility for parsing a link address
Browse files Browse the repository at this point in the history
  • Loading branch information
@mbroadst
mbroadst committed Aug 3, 2015
1 parent 9b02351 commit c50f497
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 0 deletions.
8 changes: 8 additions & 0 deletions lib/utilities.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,14 @@ function parseAddress(address) {

module.exports.parseAddress = parseAddress;

// @todo: this "parsing" should be far more rigorous
module.exports.parseLinkAddress = function(address) {
var parts = address.split('/');
var result = { name: parts.shift() };
if (parts.length) result.subject = parts.shift();
return result;
};

function deepMerge() {
var args = Array.prototype.slice.call(arguments);
var helper = function(tgt, src, key) {
Expand Down
20 changes: 20 additions & 0 deletions test/unit/test_utilities.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,26 @@ describe('Utilities', function() {

});

describe('#parseLinkAddress', function() {
[
{
description: 'a simple link name',
address: 'amq.topic',
expected: { name: 'amq.topic' }
},
{
description: 'link name with subject',
address: 'amq.topic/news',
expected: { name: 'amq.topic', subject: 'news' }
}
].forEach(function(testCase) {
it('should match ' + testCase.description, function() {
var result = u.parseLinkAddress(testCase.address);
expect(result).to.eql(testCase.expected);
});
});
});

describe('#generateTimeouts', function() {
it('should generate a fibonacci sequence of timeouts', function() {
var options = {
Expand Down

15 comments on commit c50f497

@playachronix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am connecting to an amqps queue and I wish to create a receiver on a topic. In a previous version I was able to use createReceiver('topic://topicname') but it appears after passing through this, that will no longer work. I can pass in the topic name, but if I add "DEBUG=*" in the environment, I get an error:
Thu, 29 Oct 2015 15:18:36 GMT amqp10:client session error: { descriptor: [Int64 value:29 octets:00 00 00 00 00 00 00 1d],
value: undefined,
condition: { contents: 'amqp:unauthorized-access' },
description: 'User username is not authorized to read from: queue://topicname',
errorInfo: undefined }

I really want it to connect to topic://topicname not queue://topicname and I haven't found in the source how to go about doing that.

Thanks for the help!

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@playachronix as an initial attempt to get it working more quickly for you, could you try passing defaultSubjects: false to your client's policy? I'll write up some tests quickly so we can fix this for good without the workaround

@playachronix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbroadst I added defaultSubjects: false to my configuration but I still get the same error message in the logs. Let me know if I am missing something else. If you get a patch in for this. I'll try to watch and I can make the change locally for now until it goes into a published version. Thanks for the help!

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@playachronix can you possibly provide me with a small code sample of what you're running. If it's converting topic://topicname to queue://topicname thats incredibly strange. Just trying to write a better test on my side

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@playachronix also, are you using ActiveMQ or one of the JMS brokers? It looks like it from what you're trying to specify as a link name. I do see that I overlooked the parsing for link names when I refactored the connect parsing to point to a policy so that needs to be fixed. AFAICT this would be specific to an ActiveMQ-esque parsing scheme

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@playachronix also I was wrong about my initial suggestion for a "quick fix", you should instead try doing:
createReceiver('topic://topicname', { name: 'topic://topicname' })

@playachronix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbroadst That appeared to work (your last suggestion). I don't really know how the broker is set up, it is run by another company, and I am reading off of their queues. I am going to troubleshoot with them to see if I am now properly connected.

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@playachronix okay, we've been primarily developing against SB/EH (for @noodlefrenzy) and apache qpid (for me), so there are known issues with brokers like ActiveMQ that support these types of naming schemes. I'm putting together some tests to ensure we can support the whole range through policies.

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@playachronix actually, I've added these basic tests: 67c2903, and can verify that we were indeed parsing the addresses correctly.

The difference with the suggestion I added above is that the link name (not the target of the link) would be forced to be whatever you wanted (e.g. topic://mytopic') vs using our default behavior of adding a uuid to the end of the link (e.g.topic://mytopic_some-really-long-uuid`). This shouldn't have caused a problem.

Would you be willing to retry your test with the latest code in master, and to provide me with a trace log of a test run where it fails?

@playachronix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbroadst I can pull latest and give it a shot. Perhaps connecting to topics just needs to be covered better in the documentation :)

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we could add some documentation about it, but it's varied depending on what broker you're using.. AMQP 1.0 itself has no concepts of exchanges, topics, queues, etc. It only deals with "links". Beyond that there are a slew of differing opinions on how to handle the broker side of the equation between the major vendors in this market, so we're sort of stuck playing catchup to their rules :)

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you could send me the code you use to generate the result that would be great (sensitive information redacted of course)

@playachronix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbroadst On latest, I can createReceiver('topic://topicname') and it works as intended. (still need the defaultSubjects:false part though).

Here is the basics. I'm using when promise library but it works well with this other one.

var policy = AMQP.Policy.merge({
    defaultSubjects:false,
    connect: {
        options: {
            sslOptions: {
                keyFile: './certs/client-key.pem',
                certFile: './certs/client-crt.pem',
                caFile: './certs/broker.pem',
                rejectUnauthorized:false
            }
        }
    }
});
var client = new AMQP.Client(policy);
client.connect('amqps://user:password@<ipaddress>:5671')
    .then(function() {
        console.log('Connected');
        return client.createReceiver('topic://mytopic');
    })
    .then(function(receiver) {
        console.log('Receiver created');
        receiver.on('errorReceived', function(err) {
            console.log("errorReceived: ", err);
        });
        receiver.on('message', function(message) {
            console.log("Message Received: ", message);
        });
    })
    .catch(function(err) {
        console.log("error: ", err);
    })
    .done();

@mbroadst
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@playachronix that's the weird part.. I didn't actually change any code, I just added tests! Also, when writing the tests I could confirm that there wasn't a need for defaultSubjects (although the test I left in there uses the ActiveMQ policy which will indeed turn those off).

Perhaps there was some issue on the broker setup side of the equation. I'm glad that it seems to be working though.

@playachronix
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbroadst Perhaps it just needs an example with the topic connection, if it were in an existing test I would have found it as well. Thanks for your help.

Please sign in to comment.