QPID-2873: ensure that queues in the configuration file are always bound to the default exchange with their name, unknown exchanges properly cause exception to be thrown, and you cant use custom bindings against the default exchange

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

4 files changed, 248 insertions, 35 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
index 7c804fc1fd..bfdb30764a 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
@@ -568,7 +568,7 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
 
     public VirtualHost createVirtualHost(final VirtualHostConfiguration vhostConfig) throws Exception
     {
-        VirtualHostImpl virtualHost = new VirtualHostImpl(this, vhostConfig);
+        VirtualHostImpl virtualHost = new VirtualHostImpl(this, vhostConfig, null);
         _virtualHostRegistry.registerVirtualHost(virtualHost);
         getBroker().addVirtualHost(virtualHost);
         return virtualHost;
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
index 52acd9085b..5f3446236c 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
@@ -20,7 +20,6 @@
  */
 package org.apache.qpid.server.virtualhost;
 
-import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
@@ -178,22 +177,11 @@ public class VirtualHostImpl implements VirtualHost
         }
     }
 
-    public VirtualHostImpl(IApplicationRegistry appRegistry, VirtualHostConfiguration hostConfig) throws Exception
-    {
-        this(appRegistry, hostConfig, null);
-    }
-
-
-    public VirtualHostImpl(VirtualHostConfiguration hostConfig, MessageStore store) throws Exception
-    {
-        this(ApplicationRegistry.getInstance(),hostConfig,store);
-    }
-
-    private VirtualHostImpl(IApplicationRegistry appRegistry, VirtualHostConfiguration hostConfig, MessageStore store) throws Exception
+    public VirtualHostImpl(IApplicationRegistry appRegistry, VirtualHostConfiguration hostConfig, MessageStore store) throws Exception
     {
 		if (hostConfig == null)
 		{
-			throw new IllegalAccessException("HostConfig and MessageStore cannot be null");
+			throw new IllegalArgumentException("HostConfig cannot be null");
 		}
 		
         _appRegistry = appRegistry;
@@ -462,46 +450,57 @@ public class VirtualHostImpl implements VirtualHost
     private void configureQueue(QueueConfiguration queueConfiguration) throws AMQException, ConfigurationException
     {
     	AMQQueue queue = AMQQueueFactory.createAMQQueueImpl(queueConfiguration, this);
+        String queueName = queue.getName();
 
     	if (queue.isDurable())
     	{
     		getDurableConfigurationStore().createQueue(queue);
     	}
 
+        //get the exchange name (returns default exchange name if none was specified)
     	String exchangeName = queueConfiguration.getExchange();
 
-    	Exchange exchange = _exchangeRegistry.getExchange(exchangeName == null ? null : new AMQShortString(exchangeName));
-
-        if (exchange == null)
-        {
-            exchange = _exchangeRegistry.getDefaultExchange();
-        }
-
+        Exchange exchange = _exchangeRegistry.getExchange(exchangeName);
     	if (exchange == null)
     	{
-    		throw new ConfigurationException("Attempt to bind queue to unknown exchange:" + exchangeName);
+            throw new ConfigurationException("Attempt to bind queue '" + queueName + "' to unknown exchange:" + exchangeName);
     	}
 
-        List routingKeys = queueConfiguration.getRoutingKeys();
-        if (routingKeys == null || routingKeys.isEmpty())
-        {
-            routingKeys = Collections.singletonList(queue.getNameShortString());
-        }
+        Exchange defaultExchange = _exchangeRegistry.getDefaultExchange();
+
+        //get routing keys in configuration (returns empty list if none are defined)
+        List<?> routingKeys = queueConfiguration.getRoutingKeys();
 
         for (Object routingKeyNameObj : routingKeys)
         {
-            AMQShortString routingKey = new AMQShortString(String.valueOf(routingKeyNameObj));
-            if (_logger.isInfoEnabled())
+            String routingKey = String.valueOf(routingKeyNameObj);
+
+            if (exchange.equals(defaultExchange) && !queueName.equals(routingKey))
             {
-                _logger.info("Binding queue:" + queue + " with routing key '" + routingKey + "' to exchange:" + this);
+                throw new ConfigurationException("Illegal attempt to bind queue '" + queueName +
+                        "' to the default exchange with a key other than the queue name: " + routingKey);
             }
-            _bindingFactory.addBinding(routingKey.toString(), queue, exchange, null);
+
+            configureBinding(queue, exchange, routingKey);
         }
 
-        if (exchange != _exchangeRegistry.getDefaultExchange())
+        if (!exchange.equals(defaultExchange))
+        {
+            //bind the queue to the named exchange using its name
+            configureBinding(queue, exchange, queueName);
+        }
+
+        //ensure the queue is bound to the default exchange using its name
+        configureBinding(queue, defaultExchange, queueName);
+    }
+
+    private void configureBinding(AMQQueue queue, Exchange exchange, String routingKey) throws AMQException
+    {
+        if (_logger.isInfoEnabled())
         {
-            _bindingFactory.addBinding(queue.getNameShortString().toString(), queue, exchange, null);
+            _logger.info("Binding queue:" + queue + " with routing key '" + routingKey + "' to exchange:" + exchange.getName());
         }
+        _bindingFactory.addBinding(routingKey, queue, exchange, null);
     }
 
     public String getName()
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/queue/SimpleAMQQueueTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/queue/SimpleAMQQueueTest.java
index 0e136c523f..f4cdbbe02c 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/queue/SimpleAMQQueueTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/queue/SimpleAMQQueueTest.java
@@ -107,7 +107,7 @@ public class SimpleAMQQueueTest extends InternalBrokerBaseCase
         ApplicationRegistry applicationRegistry = (ApplicationRegistry)ApplicationRegistry.getInstance();
 
         PropertiesConfiguration env = new PropertiesConfiguration();
-        _virtualHost = new VirtualHostImpl(new VirtualHostConfiguration(getClass().getName(), env), _store);
+        _virtualHost = new VirtualHostImpl(ApplicationRegistry.getInstance(), new VirtualHostConfiguration(getClass().getName(), env), _store);
         applicationRegistry.getVirtualHostRegistry().registerVirtualHost(_virtualHost);
 
         _queue = (SimpleAMQQueue) AMQQueueFactory.createAMQQueueImpl(_qname, false, _owner, false, false, _virtualHost, _arguments);
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/virtualhost/VirtualHostImplTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/virtualhost/VirtualHostImplTest.java
new file mode 100644
index 0000000000..c87e5a1648
--- /dev/null
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/virtualhost/VirtualHostImplTest.java
@@ -0,0 +1,214 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.server.virtualhost;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+
+import org.apache.commons.configuration.ConfigurationException;
+import org.apache.commons.configuration.XMLConfiguration;
+import org.apache.qpid.server.configuration.ServerConfiguration;
+import org.apache.qpid.server.exchange.Exchange;
+import org.apache.qpid.server.queue.AMQQueue;
+import org.apache.qpid.server.registry.ApplicationRegistry;
+import org.apache.qpid.server.store.TestableMemoryMessageStore;
+import org.apache.qpid.server.util.TestApplicationRegistry;
+import org.apache.qpid.test.utils.QpidTestCase;
+
+public class VirtualHostImplTest extends QpidTestCase
+{
+    private ServerConfiguration _configuration;
+    private ApplicationRegistry _registry;
+
+    @Override
+    public void tearDown() throws Exception
+    {
+        super.tearDown();
+
+        ApplicationRegistry.remove();
+    }
+
+    /**
+     * Tests that custom routing keys for the queue specified in the configuration
+     * file are correctly bound to the exchange (in addition to the queue name)
+     */
+    public void testSpecifyingCustomBindings() throws Exception
+    {
+        customBindingTestImpl(new String[]{"custom1","custom2"});
+    }
+
+    /**
+     * Tests that a queue specified in the configuration file to be bound to a
+     * specified(non-default) direct exchange is a correctly bound to the exchange
+     * and the default exchange using the queue name.
+     */
+    public void testQueueSpecifiedInConfigurationIsBoundToDefaultExchange() throws Exception
+    {
+        customBindingTestImpl(new String[0]);
+    }
+
+    private void customBindingTestImpl(final String[] routingKeys) throws Exception
+    {
+        String exchangeName = getName() +".direct";
+        String vhostName = getName();
+        String queueName = getName();
+
+        File config = writeConfigFile(vhostName, queueName, exchangeName, false, routingKeys);
+        VirtualHost vhost = createVirtualHost(vhostName, config);
+        assertNotNull("virtualhost should exist", vhost);
+
+        AMQQueue queue = vhost.getQueueRegistry().getQueue(queueName);
+        assertNotNull("queue should exist", queue);
+
+        Exchange defaultExch = vhost.getExchangeRegistry().getDefaultExchange();
+        assertTrue("queue should have been bound to default exchange with its name", defaultExch.isBound(queueName, queue));
+
+        Exchange exch = vhost.getExchangeRegistry().getExchange(exchangeName);
+        assertTrue("queue should have been bound to " + exchangeName + " with its name", exch.isBound(queueName, queue));
+
+        for(String key: routingKeys)
+        {
+            assertTrue("queue should have been bound to " + exchangeName + " with key " + key, exch.isBound(key, queue));
+        }
+    }
+
+    /**
+     * Tests that specifying custom routing keys for a queue in the configuration file results in failure
+     * to create the vhost (since this is illegal, only queue names are used with the default exchange)
+     */
+    public void testSpecifyingCustomBindingForDefaultExchangeThrowsException() throws Exception
+    {
+        File config = writeConfigFile(getName(), getName(), null, false, new String[]{"custom-binding"});
+
+        try
+        {
+            createVirtualHost(getName(), config);
+            fail("virtualhost creation should have failed due to illegal configuration");
+        }
+        catch (ConfigurationException e)
+        {
+            //expected
+        }
+    }
+
+    /**
+     * Tests that specifying an unknown exchange to bind the queue to results in failure to create the vhost
+     */
+    public void testSpecifyingUnknownExchangeThrowsException() throws Exception
+    {
+        File config = writeConfigFile(getName(), getName(), "made-up-exchange", true, new String[0]);
+
+        try
+        {
+            createVirtualHost(getName(), config);
+            fail("virtualhost creation should have failed due to illegal configuration");
+        }
+        catch (ConfigurationException e)
+        {
+            //expected
+        }
+    }
+
+    private VirtualHost createVirtualHost(String vhostName, File config) throws Exception
+    {
+        _configuration = new ServerConfiguration(new XMLConfiguration(config));
+
+        _registry = new TestApplicationRegistry(_configuration);
+        ApplicationRegistry.initialise(_registry);
+
+        return _registry.getVirtualHostRegistry().getVirtualHost(vhostName);
+    }
+
+    /**
+     * Create a configuration file for testing virtualhost creation
+     * 
+     * @param vhostName name of the virtualhost
+     * @param queueName name of the queue
+     * @param exchangeName name of a direct exchange to declare (unless dontDeclare = true) and bind the queue to (null = none)
+     * @param dontDeclare if true then dont declare the exchange, even if its name is non-null
+     * @param routingKeys routingKeys to bind the queue with (empty array = none)
+     * @return
+     */
+    private File writeConfigFile(String vhostName, String queueName, String exchangeName, boolean dontDeclare, String[] routingKeys)
+    {
+        File tmpFile = null;
+        try
+        {
+            tmpFile = File.createTempFile(getName(), ".tmp");
+            tmpFile.deleteOnExit();
+
+            FileWriter fstream = new FileWriter(tmpFile);
+            BufferedWriter writer = new BufferedWriter(fstream);
+
+            //extra outer tag to please Commons Configuration
+            writer.write("<configuration>");
+
+            writer.write("<virtualhosts>");
+            writer.write("  <default>" + vhostName + "</default>");
+            writer.write("  <virtualhost>");
+            writer.write("      <store>");
+            writer.write("          <class>" + TestableMemoryMessageStore.class.getName() + "</class>");
+            writer.write("      </store>");
+            writer.write("      <name>" + vhostName + "</name>");
+            writer.write("      <" + vhostName + ">");
+            if(exchangeName != null && !dontDeclare)
+            {
+                writer.write("          <exchanges>");
+                writer.write("              <exchange>");
+                writer.write("                  <type>direct</type>");
+                writer.write("                  <name>" + exchangeName + "</name>");
+                writer.write("              </exchange>");
+                writer.write("          </exchanges>");
+            }
+            writer.write("          <queues>");
+            writer.write("              <queue>");
+            writer.write("                  <name>" + queueName + "</name>");
+            writer.write("                  <" + queueName + ">");
+            if(exchangeName != null)
+            {
+                writer.write("                      <exchange>" + exchangeName + "</exchange>");
+            }
+            for(String routingKey: routingKeys)
+            {
+                writer.write("                      <routingKey>" + routingKey + "</routingKey>");
+            }
+            writer.write("                  </" + queueName + ">");
+            writer.write("              </queue>");
+            writer.write("          </queues>");
+            writer.write("      </" + vhostName + ">");
+            writer.write("  </virtualhost>");
+            writer.write("</virtualhosts>");
+
+            writer.write("</configuration>");
+
+            writer.flush();
+            writer.close();
+        }
+        catch (IOException e)
+        {
+            fail("Unable to create virtualhost configuration");
+        }
+
+        return tmpFile;
+    }
+}