> On 2011-05-10 09:25:19, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, > > line 1081 > > <https://reviews.apache.org/r/706/diff/1/?file=18458#file18458line1081> > > > > As above, is it possible the routing key is null? If so does that > > matter here? > > rajith attapattu wrote: > If the address is resolved it can't be null as a default would have been > assigned. > If BURL is used, then we'd be getting a AMQTopic object which should not > have a null routing key. > (I actually need to verify that to be sure.)
Worth a comment in the code making those assumptions clear perhaps. > On 2011-05-10 09:25:19, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAnyDestination.java, > > line 87 > > <https://reviews.apache.org/r/706/diff/1/?file=18457#file18457line87> > > > > Not a major issue, but this seems a bit convoluted to me as compared > > with the more direct: > > > > if (getRoutingKey() != null) { > > return getRoutingKey().toString(); > > } else if (getSubject() != null) { > > return getSubject(); > > } else { > > return ""; > > } > > > > Also why use 'super' here? > > > > Finally, is returning the empty string the right default? Might it be > > better to simply throw an exception? What does it mean if the topic has a > > null routing key (and null subject)? > > rajith attapattu wrote: > Agreed about the simpler form and that using supper is unnecessary. > > Let me start with some background info before I explain the reasons. > The AMQAnyDestination is actually a hybrid class that represents both > Topics and Queues. A bad idea (by me) in hindsight, that pre dates the > introduction of addressing. At the time in our JMS implementation, Topics > reffered to amq.topic (or any other Topic exchange) and Queues referred to > amq.direct (or any other Direct exchange). The design at that time was a > Destination type for each exchange type. I conceived an idea of representing > any exchange type with a single destination implementation, and the result > was AMQAnyDestination. With the binding URL's and existing design model this > worked well. > > However with the introduction of addressing and it's definition of Topics > and Queues (which was more aligned with the JMS notion of Queues and Topics) > this quickly became a bad idea. I should have dropped this class in favour of > something like QpidQueue and QpidTopic which would have made a lot more > sense. However I struggled a bit with merging the existing behaviour with the > new behaviour and instead opted for keeping this class. > > When addressing is used a null routing key (or subject) means that a > subject isn't specified in the address string. For queues this should simply > mean "" (empty string) and for topics it's the same except when the > underlying exchange type was a Topic exchange. In which case the subject was > defaulted to "#". > The address resolution code is responsible for figuring this out and > updates the fields (both subject and routing key) with the right defaults. > But until it does so, this fields aren't populated. > > When BURL's are used the routing key could be null when using the fanout > exchange. > > So clearly we can't throw an exception, so the safe approach is to use "". > Also the routing key is of type AMQShortString and in the code there > could be instances of just calling getRoutingKey().asString() without > checking for null which will result in an NPE. > > So IMO it's best to use "" as the default until it's updated properly. "Also the routing key is of type AMQShortString and in the code there could be instances of just calling getRoutingKey().asString() without checking for null which will result in an NPE." - this patch doesn't do anything to affect that however, right? It sounds to me like the role of getTopicName() and its implementation for address based destinations (perhaps AMQAnyDestinations more widely) needs reviewed. I'm not sure "" is any more meaningful to users than null. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/706/#review654 ----------------------------------------------------------- On 2011-05-10 03:38:41, rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/706/ > ----------------------------------------------------------- > > (Updated 2011-05-10 03:38:41) > > > Review request for qpid. > > > Summary > ------- > > The attached patch adds code to resolve the address to determine the correct > defualt for the subject field and also to populate the legacy fields. > Also added null checks in both AMQAnyDestination and AMQTopic to prevent any > NPE. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAnyDestination.java > 1099057 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java > 1099288 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQTopic.java > 1099057 > > Diff: https://reviews.apache.org/r/706/diff > > > Testing > ------- > > > Thanks, > > rajith > >
