Qpid
  1. Qpid
  2. QPID-3769

NPE in client AMQDestination.equals()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.12
    • Fix Version/s: 0.22
    • Component/s: Java Client
    • Labels:

      Description

      Code of org.apache.qpid.client.AMQDestination.equals(Object) is buggy, it should test for null on _exchangeClass and _exchangeName before dereferencing them, lines 522 and 526.

        Activity

        Hide
        Rajith Attapattu added a comment -

        There are two options for implementing the equals method (and hashcode).

        1. Check the "type" and "name" to ensure they both point to the same "destination" (which could be a queue or an exchange in pre 1.0 terms).

        2. Do a comprehensive check on the internal address data structure to ensure that all fields, properties, options are the same.

        (Please note that comparing two address strings is not a good idea as there can whitespaces).

        I prefer option one and have attached a patch for that.
        What do others think?

                 final AMQDestination that = (AMQDestination) o;
         
        -        if (!_exchangeClass.equals(that._exchangeClass))
        +        if (_destSyntax != that.getDestSyntax())
                 {
                     return false;
                 }
        -        if (!_exchangeName.equals(that._exchangeName))
        +
        +        if (_destSyntax == DestSyntax.ADDR)
                 {
        -            return false;
        +            if (_addressType != that.getAddressType())
        +            {
        +                return false;
        +            }
        +            if (!_name.equals(that.getAddressName()))
        +            {
        +                return false;
        +            }
                 }
        -        if ((_queueName == null && that._queueName != null) ||
        -            (_queueName != null && !_queueName.equals(that._queueName)))
        +        else
                 {
        -            return false;
        +            if (!_exchangeClass.equals(that._exchangeClass))
        +            {
        +                return false;
        +            }
        +            if (!_exchangeName.equals(that._exchangeName))
        +            {
        +                return false;
        +            }
        +            if ((_queueName == null && that._queueName != null) ||
        +                (_queueName != null && !_queueName.equals(that._queueName)))
        +            {
        +                return false;
        +            }
                 }
        -
                 return true;
             }
         
        
        Show
        Rajith Attapattu added a comment - There are two options for implementing the equals method (and hashcode). 1. Check the "type" and "name" to ensure they both point to the same "destination" (which could be a queue or an exchange in pre 1.0 terms). 2. Do a comprehensive check on the internal address data structure to ensure that all fields, properties, options are the same. (Please note that comparing two address strings is not a good idea as there can whitespaces). I prefer option one and have attached a patch for that. What do others think? final AMQDestination that = (AMQDestination) o; - if (!_exchangeClass.equals(that._exchangeClass)) + if (_destSyntax != that.getDestSyntax()) { return false ; } - if (!_exchangeName.equals(that._exchangeName)) + + if (_destSyntax == DestSyntax.ADDR) { - return false ; + if (_addressType != that.getAddressType()) + { + return false ; + } + if (!_name.equals(that.getAddressName())) + { + return false ; + } } - if ((_queueName == null && that._queueName != null ) || - (_queueName != null && !_queueName.equals(that._queueName))) + else { - return false ; + if (!_exchangeClass.equals(that._exchangeClass)) + { + return false ; + } + if (!_exchangeName.equals(that._exchangeName)) + { + return false ; + } + if ((_queueName == null && that._queueName != null ) || + (_queueName != null && !_queueName.equals(that._queueName))) + { + return false ; + } } - return true ; }
        Hide
        Rajith Attapattu added a comment -

        http://svn.apache.org/r1456007
        http://svn.apache.org/r1456008

        Committed a modified version of Siddhesh Poyarekar's patch.
        Added a fix to hashcode as well and added a test case.

        Show
        Rajith Attapattu added a comment - http://svn.apache.org/r1456007 http://svn.apache.org/r1456008 Committed a modified version of Siddhesh Poyarekar's patch. Added a fix to hashcode as well and added a test case.
        Hide
        Alex Rudyy added a comment -

        Hi Rajith,

        I believe that topic address based destination should also compare the subject.
        For instance the following test fails because really different destinations are considered to be the same:

            public void testEqualsForTopicDestinationsWithDifferentSubjects() throws Exception
            {
                AMQDestination dest1 = new AMQAnyDestination("ADDR:Foo/subjectOne; {node :{type:topic}}");
                AMQDestination dest2 = new AMQAnyDestination("ADDR:Foo/subjectTwo; {node :{type:topic}}");
        
                assertFalse("Destination with different subjects cannot be equal", dest1.equals(dest2));
            }
        

        Also, I would like to point out that AMQDestinationTest#testEqaulsAndHashCodeForAddressBasedDestinations fails if it is ran from Eclipse because the client defaults to address based syntax and parsing of binding URL as address fails. I would like to suggest to prefix the binding destination URL in test with BURL prefix:

        BURL:direct://amq.direct/test-route/Foo?routingkey='Foo'

        I also noticed that both topic and queue destinations in your test are created using AMQQueue class. Should it possibly use AMQTopic or AMQAnyDestination?

        Show
        Alex Rudyy added a comment - Hi Rajith, I believe that topic address based destination should also compare the subject. For instance the following test fails because really different destinations are considered to be the same: public void testEqualsForTopicDestinationsWithDifferentSubjects() throws Exception { AMQDestination dest1 = new AMQAnyDestination( "ADDR:Foo/subjectOne; {node :{type:topic}}" ); AMQDestination dest2 = new AMQAnyDestination( "ADDR:Foo/subjectTwo; {node :{type:topic}}" ); assertFalse( "Destination with different subjects cannot be equal" , dest1.equals(dest2)); } Also, I would like to point out that AMQDestinationTest#testEqaulsAndHashCodeForAddressBasedDestinations fails if it is ran from Eclipse because the client defaults to address based syntax and parsing of binding URL as address fails. I would like to suggest to prefix the binding destination URL in test with BURL prefix: BURL:direct://amq.direct/test-route/Foo?routingkey='Foo' I also noticed that both topic and queue destinations in your test are created using AMQQueue class. Should it possibly use AMQTopic or AMQAnyDestination?
        Hide
        Alex Rudyy added a comment -

        Re-opened issue as per comments

        Show
        Alex Rudyy added a comment - Re-opened issue as per comments
        Hide
        Rajith Attapattu added a comment -

        I discussed this with Rafi a fair bit before arriving at a solution.

        I don't think comparing the subject is the right approach for every situation.
        Comparing the subjects might make sense for Topics, but not for Queues. Also at the time equals is called we may not know the 'type'. We could look at the resolved property to determine this. Otherwise what are we going to do ?

        What should we do for exchanges with more complicated routing algorithms like Headers or XML exchange where the subject is not a factor at all ? It gets complicated real soon when you look beyond the simple cases.

        If you look at it from a high level pov, we send messages to either a Queue or an Exchange. They are the "destinations". So one could argue that if you are sending to the same "exchange" then you are sending to the same destination. I think that has a stronger argument that than what you are suggesting.

        How messages are matched onto queues is a function of the exchange. And it's something that we should not try to factor into our equals implementation.

        Show
        Rajith Attapattu added a comment - I discussed this with Rafi a fair bit before arriving at a solution. I don't think comparing the subject is the right approach for every situation. Comparing the subjects might make sense for Topics, but not for Queues. Also at the time equals is called we may not know the 'type'. We could look at the resolved property to determine this. Otherwise what are we going to do ? What should we do for exchanges with more complicated routing algorithms like Headers or XML exchange where the subject is not a factor at all ? It gets complicated real soon when you look beyond the simple cases. If you look at it from a high level pov, we send messages to either a Queue or an Exchange. They are the "destinations". So one could argue that if you are sending to the same "exchange" then you are sending to the same destination. I think that has a stronger argument that than what you are suggesting. How messages are matched onto queues is a function of the exchange. And it's something that we should not try to factor into our equals implementation.
        Hide
        Rajith Attapattu added a comment -

        Alex I will fix the test cases by using the right class and will prefix with BURL for the destination that uses that syntax.

        Show
        Rajith Attapattu added a comment - Alex I will fix the test cases by using the right class and will prefix with BURL for the destination that uses that syntax.
        Hide
        Rajith Attapattu added a comment -
        Show
        Rajith Attapattu added a comment - Fixed the immediate concerns. http://svn.apache.org/r1461324 http://svn.apache.org/r1461329
        Hide
        Robbie Gemmell added a comment -

        Reopening for a couple reasons.

        Half the changes were committed before the 0.22 branch, and half after. The remainder need to be requested for merge if they check out ok. We should then assign the fix-version appropriately.

        I think this needs more tests to ensure the changes check out ok:

        In the test that exists, some bits use AMQTopic destination objects, and that has its own equals() implementation and so isn't really affected by these changes.

        There doesn't seem to be any testing using AMQAnyDestination, which gets used for all the Address syntax based destinations created via JNDI and so is affected by these changes.

        Show
        Robbie Gemmell added a comment - Reopening for a couple reasons. Half the changes were committed before the 0.22 branch, and half after. The remainder need to be requested for merge if they check out ok. We should then assign the fix-version appropriately. I think this needs more tests to ensure the changes check out ok: In the test that exists, some bits use AMQTopic destination objects, and that has its own equals() implementation and so isn't really affected by these changes. There doesn't seem to be any testing using AMQAnyDestination, which gets used for all the Address syntax based destinations created via JNDI and so is affected by these changes.
        Hide
        Rajith Attapattu added a comment -

        I was going to request a port, but I should have kept this open until then.

        I also agree on the comments about the tests for this.

        Show
        Rajith Attapattu added a comment - I was going to request a port, but I should have kept this open until then. I also agree on the comments about the tests for this.
        Hide
        Rajith Attapattu added a comment -

        Adjusted the hashcode and equals impl for AMQTopic to take the new implementation (in AMQDestination) into account while preserving the old BURL based logic. Addressed concerns regarding the tests.
        http://svn.apache.org/r1463158

        Show
        Rajith Attapattu added a comment - Adjusted the hashcode and equals impl for AMQTopic to take the new implementation (in AMQDestination) into account while preserving the old BURL based logic. Addressed concerns regarding the tests. http://svn.apache.org/r1463158
        Hide
        Justin Ross added a comment -

        Reviewed by Robbie. Approved for 0.22.

        Show
        Justin Ross added a comment - Reviewed by Robbie. Approved for 0.22.
        Hide
        Rajith Attapattu added a comment -
        Show
        Rajith Attapattu added a comment - Ported the relevant changes to 0.22 branch. http://svn.apache.org/r1463215 http://svn.apache.org/r1463216 http://svn.apache.org/r1463217
        Hide
        Rajith Attapattu added a comment -

        Committed to both trunk and 0.22 branch.

        Show
        Rajith Attapattu added a comment - Committed to both trunk and 0.22 branch.

          People

          • Assignee:
            Rajith Attapattu
            Reporter:
            Jan Bareš
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development