QPID-5601 : Address review comments from Robbie Gemmell

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1575335 13f79535-47bb-0310-9956-ffa450edef68

9 files changed, 59 insertions, 27 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java
index c5eb70c1f0..2eb2790382 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java
@@ -34,6 +34,7 @@ import java.util.concurrent.TimeUnit;
 
 import org.apache.commons.configuration.ConfigurationException;
 import org.apache.log4j.Logger;
+import org.apache.qpid.exchange.ExchangeDefaults;
 import org.apache.qpid.server.configuration.updater.TaskExecutor;
 import org.apache.qpid.server.exchange.AMQUnknownExchangeType;
 import org.apache.qpid.server.exchange.ExchangeImpl;
@@ -382,7 +383,7 @@ public abstract class AbstractVirtualHost implements VirtualHost, IConnectionReg
         String exchangeName = queueConfiguration.getExchange();
 
 
-        if("".equals(exchangeName))
+        if(ExchangeDefaults.DEFAULT_EXCHANGE_NAME.equals(exchangeName))
         {
             //get routing keys in configuration (returns empty list if none are defined)
             List<?> routingKeys = queueConfiguration.getRoutingKeys();
diff --git a/qpid/java/broker-plugins/amqp-0-10-protocol/src/main/java/org/apache/qpid/server/protocol/v0_10/ServerSessionDelegate.java b/qpid/java/broker-plugins/amqp-0-10-protocol/src/main/java/org/apache/qpid/server/protocol/v0_10/ServerSessionDelegate.java
index d21da824e9..e258a27255 100644
--- a/qpid/java/broker-plugins/amqp-0-10-protocol/src/main/java/org/apache/qpid/server/protocol/v0_10/ServerSessionDelegate.java
+++ b/qpid/java/broker-plugins/amqp-0-10-protocol/src/main/java/org/apache/qpid/server/protocol/v0_10/ServerSessionDelegate.java
@@ -685,8 +685,9 @@ public class ServerSessionDelegate extends SessionDelegate
                 return;
             }
         }
-        if(method.getExchange() == null || method.getExchange().equals(""))
+        if(nameNullOrEmpty(method.getExchange()))
         {
+            // special case handling to fake the existence of the default exchange for 0-10
             if(!DirectExchange.TYPE.getType().equals(method.getType()))
             {
                 exception(session, method, ExecutionErrorCode.NOT_ALLOWED,
@@ -694,7 +695,7 @@ public class ServerSessionDelegate extends SessionDelegate
                           + " of type " + DirectExchange.TYPE.getType()
                           + " to " + method.getType() +".");
             }
-            if(method.hasAlternateExchange() && !"".equals(method.getAlternateExchange()))
+            if(!nameNullOrEmpty(method.getAlternateExchange()))
             {
                 exception(session, method, ExecutionErrorCode.NOT_ALLOWED,
                           "Attempt to set alternate exchange of the default exchange "
@@ -901,8 +902,9 @@ public class ServerSessionDelegate extends SessionDelegate
 
         final String exchangeName = method.getName();
 
-        if(exchangeName == null || exchangeName.equals(""))
+        if(nameNullOrEmpty(exchangeName))
         {
+            // Fake the existence of the "default" exchange for 0-10
             result.setDurable(true);
             result.setType(DirectExchange.TYPE.getType());
             result.setNotFound(false);
@@ -1046,7 +1048,7 @@ public class ServerSessionDelegate extends SessionDelegate
         ExchangeImpl exchange;
         AMQQueue queue;
         boolean isDefaultExchange;
-        if(method.hasExchange() && !method.getExchange().equals(""))
+        if(!nameNullOrEmpty(method.getExchange()))
         {
             isDefaultExchange = false;
             exchange = virtualHost.getExchange(method.getExchange());
@@ -1064,6 +1066,7 @@ public class ServerSessionDelegate extends SessionDelegate
 
         if(isDefaultExchange)
         {
+            // fake the existence of the "default" exchange for 0-10
             if(method.hasQueue())
             {
                 queue = getQueue(session, method.getQueue());
diff --git a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/BasicConsumeMethodHandler.java b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/BasicConsumeMethodHandler.java
index cb4f758fb7..37e531bd2a 100644
--- a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/BasicConsumeMethodHandler.java
+++ b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/BasicConsumeMethodHandler.java
@@ -129,7 +129,7 @@ public class BasicConsumeMethodHandler implements StateAwareMethodListener<Basic
                     }
                     else
                     {
-                        AMQShortString msg = new AMQShortString("Non-unique consumer tag, '" + body.getConsumerTag() + "'");
+                        AMQShortString msg = AMQShortString.validValueOf("Non-unique consumer tag, '" + body.getConsumerTag() + "'");
 
                         MethodRegistry methodRegistry = protocolConnection.getMethodRegistry();
                         AMQMethodBody responseBody = methodRegistry.createConnectionCloseBody(AMQConstant.NOT_ALLOWED.getCode(),    // replyCode
@@ -146,7 +146,7 @@ public class BasicConsumeMethodHandler implements StateAwareMethodListener<Basic
 
                     MethodRegistry methodRegistry = protocolConnection.getMethodRegistry();
                     AMQMethodBody responseBody = methodRegistry.createChannelCloseBody(AMQConstant.ARGUMENT_INVALID.getCode(),
-                                                                                       new AMQShortString(ise.getMessage()),
+                                                                                       AMQShortString.validValueOf(ise.getMessage()),
                                                                                        body.getClazz(),
                                                                                        body.getMethod());
                     protocolConnection.writeFrame(responseBody.generateFrame(channelId));
diff --git a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeBoundHandler.java b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeBoundHandler.java
index fce1260155..d30961b13c 100644
--- a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeBoundHandler.java
+++ b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeBoundHandler.java
@@ -83,7 +83,8 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
         AMQShortString queueName = body.getQueue();
         AMQShortString routingKey = body.getRoutingKey();
         ExchangeBoundOkBody response;
-        if(exchangeName == null || exchangeName.equals(AMQShortString.EMPTY_STRING))
+
+        if(isDefaultExchange(exchangeName))
         {
             if(routingKey == null)
             {
@@ -98,7 +99,7 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
                     {
 
                         response = methodRegistry.createExchangeBoundOkBody(QUEUE_NOT_FOUND,	// replyCode
-                                                                            new AMQShortString("Queue '" + queueName + "' not found"));	// replyText
+                                                                            AMQShortString.validValueOf("Queue '" + queueName + "' not found"));	// replyText
                     }
                     else
                     {
@@ -119,7 +120,7 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
                     {
 
                         response = methodRegistry.createExchangeBoundOkBody(QUEUE_NOT_FOUND,	// replyCode
-                                                                            new AMQShortString("Queue '" + queueName + "' not found"));	// replyText
+                                                                            AMQShortString.validValueOf("Queue '" + queueName + "' not found"));	// replyText
                     }
                     else
                     {
@@ -136,7 +137,7 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
 
 
                 response = methodRegistry.createExchangeBoundOkBody(EXCHANGE_NOT_FOUND,
-                                                                    new AMQShortString("Exchange '" + exchangeName + "' not found"));
+                                                                    AMQShortString.validValueOf("Exchange '" + exchangeName + "' not found"));
             }
             else if (routingKey == null)
             {
@@ -161,7 +162,7 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
                     {
 
                         response = methodRegistry.createExchangeBoundOkBody(QUEUE_NOT_FOUND,	// replyCode
-                            new AMQShortString("Queue '" + queueName + "' not found"));	// replyText
+                            AMQShortString.validValueOf("Queue '" + queueName + "' not found"));	// replyText
                     }
                     else
                     {
@@ -175,7 +176,7 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
                         {
 
                             response = methodRegistry.createExchangeBoundOkBody(QUEUE_NOT_BOUND,	// replyCode
-                                new AMQShortString("Queue '" + queueName + "' not bound to exchange '" + exchangeName + "'"));	// replyText
+                                AMQShortString.validValueOf("Queue '" + queueName + "' not bound to exchange '" + exchangeName + "'"));	// replyText
                         }
                     }
                 }
@@ -187,7 +188,7 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
                 {
 
                     response = methodRegistry.createExchangeBoundOkBody(QUEUE_NOT_FOUND,	// replyCode
-                        new AMQShortString("Queue '" + queueName + "' not found"));	// replyText
+                        AMQShortString.validValueOf("Queue '" + queueName + "' not found"));	// replyText
                 }
                 else
                 {
@@ -204,12 +205,8 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
                         String message = "Queue '" + queueName + "' not bound with routing key '" +
                                             body.getRoutingKey() + "' to exchange '" + exchangeName + "'";
 
-                        if(message.length()>255)
-                        {
-                            message = message.substring(0,254);
-                        }
                         response = methodRegistry.createExchangeBoundOkBody(SPECIFIC_QUEUE_NOT_BOUND_WITH_RK,	// replyCode
-                            new AMQShortString(message));	// replyText
+                            AMQShortString.validValueOf(message));	// replyText
                     }
                 }
             }
@@ -225,11 +222,17 @@ public class ExchangeBoundHandler implements StateAwareMethodListener<ExchangeBo
                 {
 
                     response = methodRegistry.createExchangeBoundOkBody(NO_QUEUE_BOUND_WITH_RK,	// replyCode
-                        new AMQShortString("No queue bound with routing key '" + body.getRoutingKey() +
+                        AMQShortString.validValueOf("No queue bound with routing key '" + body.getRoutingKey() +
                         "' to exchange '" + exchangeName + "'"));	// replyText
                 }
             }
         }
         session.writeFrame(response.generateFrame(channelId));
     }
+
+    protected boolean isDefaultExchange(final AMQShortString exchangeName)
+    {
+        return exchangeName == null || exchangeName.equals(AMQShortString.EMPTY_STRING);
+    }
+
 }
diff --git a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeDeclareHandler.java b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeDeclareHandler.java
index 78d47aaa52..fd7d9c10ff 100644
--- a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeDeclareHandler.java
+++ b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeDeclareHandler.java
@@ -79,7 +79,7 @@ public class ExchangeDeclareHandler implements StateAwareMethodListener<Exchange
 
         ExchangeImpl exchange;
 
-        if(exchangeName == null || exchangeName.equals(AMQShortString.EMPTY_STRING))
+        if(isDefaultExchange(exchangeName))
         {
             if(!new AMQShortString(DirectExchange.TYPE.getType()).equals(body.getType()))
             {
@@ -171,4 +171,9 @@ public class ExchangeDeclareHandler implements StateAwareMethodListener<Exchange
             session.writeFrame(responseBody.generateFrame(channelId));
         }
     }
+
+    protected boolean isDefaultExchange(final AMQShortString exchangeName)
+    {
+        return exchangeName == null || exchangeName.equals(AMQShortString.EMPTY_STRING);
+    }
 }
diff --git a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeDeleteHandler.java b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeDeleteHandler.java
index bc723fc3dd..9af383c3ca 100644
--- a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeDeleteHandler.java
+++ b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/ExchangeDeleteHandler.java
@@ -21,6 +21,7 @@
 package org.apache.qpid.server.protocol.v0_8.handler;
 
 import org.apache.qpid.AMQException;
+import org.apache.qpid.framing.AMQShortString;
 import org.apache.qpid.framing.ExchangeDeleteBody;
 import org.apache.qpid.framing.ExchangeDeleteOkBody;
 import org.apache.qpid.protocol.AMQConstant;
@@ -60,13 +61,14 @@ public class ExchangeDeleteHandler implements StateAwareMethodListener<ExchangeD
         channel.sync();
         try
         {
-            final String exchangeName = body.getExchange() == null ? null : body.getExchange().toString();
 
-            if(exchangeName == null || "".equals(exchangeName))
+            if(isDefaultExchange(body.getExchange()))
             {
                 throw body.getConnectionException(AMQConstant.NOT_ALLOWED, "Default Exchange cannot be deleted");
             }
 
+            final String exchangeName = body.getExchange().toString();
+
             final ExchangeImpl exchange = virtualHost.getExchange(exchangeName);
             if(exchange == null)
             {
@@ -94,4 +96,10 @@ public class ExchangeDeleteHandler implements StateAwareMethodListener<ExchangeD
             throw body.getConnectionException(AMQConstant.ACCESS_REFUSED, e.getMessage());
         }
     }
+
+
+    protected boolean isDefaultExchange(final AMQShortString exchangeName)
+    {
+        return exchangeName == null || exchangeName.equals(AMQShortString.EMPTY_STRING);
+    }
 }
diff --git a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/QueueBindHandler.java b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/QueueBindHandler.java
index 7dc76d13d0..c14ac72818 100644
--- a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/QueueBindHandler.java
+++ b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/QueueBindHandler.java
@@ -101,13 +101,14 @@ public class QueueBindHandler implements StateAwareMethodListener<QueueBindBody>
         {
             throw body.getChannelException(AMQConstant.NOT_FOUND, "Queue " + queueName + " does not exist.");
         }
-        final String exchangeName = body.getExchange() == null ? null : body.getExchange().toString();
 
-        if(exchangeName == null || "".equals(exchangeName))
+        if(isDefaultExchange(body.getExchange()))
         {
             throw body.getConnectionException(AMQConstant.NOT_ALLOWED, "Cannot bind the queue " + queueName + " to the default exchange");
         }
 
+        final String exchangeName = body.getExchange().toString();
+
         final ExchangeImpl exch = virtualHost.getExchange(exchangeName);
         if (exch == null)
         {
@@ -148,4 +149,9 @@ public class QueueBindHandler implements StateAwareMethodListener<QueueBindBody>
 
         }
     }
+
+    protected boolean isDefaultExchange(final AMQShortString exchangeName)
+    {
+        return exchangeName == null || exchangeName.equals(AMQShortString.EMPTY_STRING);
+    }
 }
diff --git a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/QueueUnbindHandler.java b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/QueueUnbindHandler.java
index abc9c8541c..f1bcce2ade 100644
--- a/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/QueueUnbindHandler.java
+++ b/qpid/java/broker-plugins/amqp-0-8-protocol/src/main/java/org/apache/qpid/server/protocol/v0_8/handler/QueueUnbindHandler.java
@@ -94,7 +94,7 @@ public class QueueUnbindHandler implements StateAwareMethodListener<QueueUnbindB
             throw body.getChannelException(AMQConstant.NOT_FOUND, "Queue " + body.getQueue() + " does not exist.");
         }
 
-        if(body.getExchange() == null || body.getExchange().equals(AMQShortString.EMPTY_STRING))
+        if(isDefaultExchange(body.getExchange()))
         {
             throw body.getConnectionException(AMQConstant.NOT_ALLOWED, "Cannot unbind the queue " + queue.getName() + " from the default exchange");
         }
@@ -145,4 +145,10 @@ public class QueueUnbindHandler implements StateAwareMethodListener<QueueUnbindB
         channel.sync();
         session.writeFrame(responseBody.generateFrame(channelId));
     }
+
+    protected boolean isDefaultExchange(final AMQShortString exchangeName)
+    {
+        return exchangeName == null || exchangeName.equals(AMQShortString.EMPTY_STRING);
+    }
+
 }
diff --git a/qpid/java/broker-plugins/amqp-msg-conv-0-8-to-0-10/src/main/java/org/apache/qpid/server/protocol/converter/v0_8_v0_10/MessageConverter_0_10_to_0_8.java b/qpid/java/broker-plugins/amqp-msg-conv-0-8-to-0-10/src/main/java/org/apache/qpid/server/protocol/converter/v0_8_v0_10/MessageConverter_0_10_to_0_8.java
index d385f684ea..bde7d1446b 100644
--- a/qpid/java/broker-plugins/amqp-msg-conv-0-8-to-0-10/src/main/java/org/apache/qpid/server/protocol/converter/v0_8_v0_10/MessageConverter_0_10_to_0_8.java
+++ b/qpid/java/broker-plugins/amqp-msg-conv-0-8-to-0-10/src/main/java/org/apache/qpid/server/protocol/converter/v0_8_v0_10/MessageConverter_0_10_to_0_8.java
@@ -137,7 +137,7 @@ public class MessageConverter_0_10_to_0_8 implements MessageConverter<MessageTra
                 {
                     try
                     {
-                        ft.put(new AMQShortString(entry.getKey()), entry.getValue());
+                        ft.put(AMQShortString.validValueOf(entry.getKey()), entry.getValue());
                     }
                     catch (AMQPInvalidClassException e)
                     {