QPID-4093: Prevent NullPointerException from ExchangeMBean when target queue does not exist

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

2 files changed, 251 insertions, 27 deletions

diff --git a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ExchangeMBean.java b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ExchangeMBean.java
index eb7e716af8..56802d0403 100644
--- a/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ExchangeMBean.java
+++ b/qpid/java/broker-plugins/jmx/src/main/java/org/apache/qpid/server/jmx/mbeans/ExchangeMBean.java
@@ -22,8 +22,6 @@
 package org.apache.qpid.server.jmx.mbeans;
 
 import org.apache.qpid.management.common.mbeans.ManagedExchange;
-import org.apache.qpid.management.common.mbeans.ManagedQueue;
-import org.apache.qpid.management.common.mbeans.annotations.MBeanOperationParameter;
 import org.apache.qpid.server.jmx.AMQManagedObject;
 import org.apache.qpid.server.jmx.ManagedObject;
 import org.apache.qpid.server.model.Binding;
@@ -35,6 +33,7 @@ import org.apache.qpid.server.model.VirtualHost;
 import javax.management.JMException;
 import javax.management.MalformedObjectNameException;
 import javax.management.ObjectName;
+import javax.management.OperationsException;
 import javax.management.openmbean.ArrayType;
 import javax.management.openmbean.CompositeData;
 import javax.management.openmbean.CompositeDataSupport;
@@ -56,6 +55,9 @@ import java.util.Map;
 public class ExchangeMBean extends AMQManagedObject implements ManagedExchange
 {
 
+    public static final String FANOUT_EXCHANGE_TYPE = "fanout";
+    public static final String HEADERS_EXCHANGE_TYPE  = "headers";
+
     private static final String[] TABULAR_UNIQUE_INDEX_ARRAY =
             TABULAR_UNIQUE_INDEX.toArray(new String[TABULAR_UNIQUE_INDEX.size()]);
 
@@ -79,7 +81,6 @@ public class ExchangeMBean extends AMQManagedObject implements ManagedExchange
             HEADERS_COMPOSITE_ITEM_DESC.toArray(new String[HEADERS_COMPOSITE_ITEM_DESC.size()]);
     private static final  String[] HEADERS_TABULAR_UNIQUE_INDEX_ARRAY =
             HEADERS_TABULAR_UNIQUE_INDEX.toArray(new String[HEADERS_TABULAR_UNIQUE_INDEX.size()]);
-    public static final   String   HEADERS_EXCHANGE_TYPE              = "headers";
 
     static
     {
@@ -179,7 +180,6 @@ public class ExchangeMBean extends AMQManagedObject implements ManagedExchange
         }
     }
 
-
     private TabularData getHeadersBindings(Collection<Binding> bindings) throws OpenDataException
     {
         TabularType bindinglistDataType =
@@ -234,7 +234,7 @@ public class ExchangeMBean extends AMQManagedObject implements ManagedExchange
 
         for (Binding binding : bindings)
         {
-            String key = "fanout".equals(_exchange.getExchangeType()) ? "*" : binding.getName();
+            String key = FANOUT_EXCHANGE_TYPE.equals(_exchange.getExchangeType()) ? "*" : binding.getName();
             List<String> queueList = bindingMap.get(key);
             if(queueList == null)
             {
@@ -269,7 +269,7 @@ public class ExchangeMBean extends AMQManagedObject implements ManagedExchange
                 final String[] keyAndValue = bindings[i].split("=");
                 if (keyAndValue == null || keyAndValue.length == 0 || keyAndValue.length > 2 || keyAndValue[0].length() == 0)
                 {
-                    throw new JMException("Format for headers binding should be \"<attribute1>=<value1>,<attribute2>=<value2>\" ");
+                    throw new JMException("Format for headers binding should be \"<attribute1>=<value1>,<attribute2>=<value2>\"");
                 }
 
                 if(keyAndValue.length == 1)
@@ -284,40 +284,30 @@ public class ExchangeMBean extends AMQManagedObject implements ManagedExchange
             }
         }
 
-        Queue queue = null;
-        VirtualHost vhost = _exchange.getParent(VirtualHost.class);
-        for(Queue aQueue : vhost.getQueues())
-        {
-            if(aQueue.getName().equals(queueName))
-            {
-                queue = aQueue;
-                break;
-            }
-        }
+        VirtualHost virtualHost = _exchange.getParent(VirtualHost.class);
+        Queue queue = MBeanUtils.findQueueFromQueueName(virtualHost, queueName);
         _exchange.createBinding(binding, queue, arguments, Collections.EMPTY_MAP);
     }
 
     public void removeBinding(String queueName, String bindingKey)
             throws IOException, JMException
     {
-        Queue queue = null;
-        VirtualHost vhost = _exchange.getParent(VirtualHost.class);
-        for(Queue aQueue : vhost.getQueues())
-        {
-            if(aQueue.getName().equals(queueName))
-            {
-                queue = aQueue;
-                break;
-            }
-        }
+        VirtualHost virtualHost = _exchange.getParent(VirtualHost.class);
+        Queue queue = MBeanUtils.findQueueFromQueueName(virtualHost, queueName);;
 
+        boolean deleted = false;
         for(Binding binding : _exchange.getBindings())
         {
             if(queue.equals(binding.getParent(Queue.class)) && bindingKey.equals(binding.getName()))
             {
                 binding.delete();
+                deleted = true;
             }
         }
-        
+
+        if (!deleted)
+        {
+            throw new OperationsException("No such binding \"" + bindingKey + "\" on queue \"" + queueName + "\"");
+        }
     }
 }
diff --git a/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ExchangeMBeanTest.java b/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ExchangeMBeanTest.java
new file mode 100644
index 0000000000..e350f80a25
--- /dev/null
+++ b/qpid/java/broker-plugins/jmx/src/test/java/org/apache/qpid/server/jmx/mbeans/ExchangeMBeanTest.java
@@ -0,0 +1,234 @@
+/*
+ * 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.jmx.mbeans;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.never;
+import static org.mockito.Matchers.isNull;
+import static org.mockito.Matchers.argThat;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.anyMap;
+
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.TreeMap;
+
+import javax.management.JMException;
+import javax.management.ListenerNotFoundException;
+import javax.management.Notification;
+import javax.management.NotificationListener;
+import javax.management.OperationsException;
+
+import org.apache.qpid.server.jmx.ManagedObjectRegistry;
+import org.apache.qpid.server.model.Binding;
+import org.apache.qpid.server.model.Exchange;
+import org.apache.qpid.server.model.LifetimePolicy;
+import org.apache.qpid.server.model.Queue;
+import org.apache.qpid.server.model.Statistics;
+import org.apache.qpid.server.model.VirtualHost;
+import org.apache.qpid.server.queue.NotificationCheck;
+import org.mockito.ArgumentMatcher;
+
+import junit.framework.TestCase;
+
+public class ExchangeMBeanTest extends TestCase
+{
+    private static final String EXCHANGE_NAME = "EXCHANGE_NAME";
+    private static final String EXCHANGE_TYPE = "EXCHANGE_TYPE";
+    private static final String QUEUE1_NAME = "QUEUE1_NAME";
+    private static final String QUEUE2_NAME = "QUEUE2_NAME";
+    private static final String BINDING1 = "BINDING1";
+    private static final String BINDING2 = "BINDING2";
+
+    private Exchange _mockExchange;
+    private VirtualHostMBean _mockVirtualHostMBean;
+    private ManagedObjectRegistry _mockManagedObjectRegistry;
+    private ExchangeMBean _exchangeMBean;
+    private Queue _mockQueue1;
+    private Queue _mockQueue2;
+    private Exchange _mockHeadersExchange;
+
+    @Override
+    protected void setUp() throws Exception
+    {
+        _mockExchange = mock(Exchange.class);
+        when(_mockExchange.getName()).thenReturn(EXCHANGE_NAME);
+        when(_mockExchange.getExchangeType()).thenReturn(EXCHANGE_TYPE);
+        _mockVirtualHostMBean = mock(VirtualHostMBean.class);
+
+        _mockManagedObjectRegistry = mock(ManagedObjectRegistry.class);
+        when(_mockVirtualHostMBean.getRegistry()).thenReturn(_mockManagedObjectRegistry);
+
+        _mockQueue1 = createMockQueue(QUEUE1_NAME);
+        _mockQueue2 = createMockQueue(QUEUE2_NAME);
+
+        VirtualHost mockVirtualHost = mock(VirtualHost.class);
+        when(mockVirtualHost.getQueues()).thenReturn(Arrays.asList(new Queue[] {_mockQueue1, _mockQueue2}));
+        when(_mockExchange.getParent(VirtualHost.class)).thenReturn(mockVirtualHost);
+
+        _exchangeMBean = new ExchangeMBean(_mockExchange, _mockVirtualHostMBean);
+
+        _mockHeadersExchange = mock(Exchange.class);
+        when(_mockHeadersExchange.getExchangeType()).thenReturn(ExchangeMBean.HEADERS_EXCHANGE_TYPE);
+        when(_mockHeadersExchange.getParent(VirtualHost.class)).thenReturn(mockVirtualHost);
+
+    }
+
+    public void testExchangeName()
+    {
+        assertEquals(EXCHANGE_NAME, _exchangeMBean.getName());
+    }
+
+    public void testExchangeType()
+    {
+        assertEquals(EXCHANGE_TYPE, _exchangeMBean.getExchangeType());
+    }
+
+    public void testNonHeadersExchangeCreateNewBinding() throws Exception
+    {
+        _exchangeMBean.createNewBinding(QUEUE1_NAME, BINDING1);
+        verify(_mockExchange).createBinding(BINDING1, _mockQueue1, Collections.EMPTY_MAP, Collections.EMPTY_MAP);
+    }
+
+    public void testCreateNewBindingWhereQueueIsUnknown() throws Exception
+    {
+        try
+        {
+            _exchangeMBean.createNewBinding("unknown", BINDING1);
+        }
+        catch (OperationsException oe)
+        {
+            // PASS
+            assertEquals("No such queue \"unknown\"", oe.getMessage());
+        }
+        verify(_mockExchange, never()).createBinding(anyString(), any(Queue.class), anyMap(), anyMap());
+    }
+
+    public void testRemoveBinding() throws Exception
+    {
+        Binding mockBinding1 = createBindingOnQueue(BINDING1, _mockQueue1);
+        Binding mockBinding2 = createBindingOnQueue(BINDING2, _mockQueue1);
+        when(_mockExchange.getBindings()).thenReturn(Arrays.asList(new Binding[] {mockBinding1, mockBinding2}));
+
+        _exchangeMBean.removeBinding(QUEUE1_NAME, BINDING1);
+        verify(mockBinding1).delete();
+    }
+
+    public void testRemoveBindingWhereQueueIsUnknown() throws Exception
+    {
+        Binding mockBinding1 = createBindingOnQueue(BINDING1, _mockQueue1);
+        when(_mockExchange.getBindings()).thenReturn(Arrays.asList(new Binding[] {mockBinding1}));
+
+        try
+        {
+            _exchangeMBean.removeBinding("unknown", BINDING1);
+            fail("Exception not thrown");
+        }
+        catch (OperationsException oe)
+        {
+            // PASS
+            assertEquals("No such queue \"unknown\"", oe.getMessage());
+        }
+        verify(mockBinding1, never()).delete();
+    }
+
+    public void testRemoveBindingWhereBindingNameIsUnknown() throws Exception
+    {
+        Binding mockBinding1 = createBindingOnQueue(BINDING1, _mockQueue1);
+        when(_mockExchange.getBindings()).thenReturn(Arrays.asList(new Binding[] {mockBinding1}));
+
+        try
+        {
+            _exchangeMBean.removeBinding(QUEUE1_NAME, "unknown");
+            fail("Exception not thrown");
+        }
+        catch (OperationsException oe)
+        {
+            // PASS
+            assertEquals("No such binding \"unknown\" on queue \"" + QUEUE1_NAME + "\"", oe.getMessage());
+        }
+        verify(mockBinding1, never()).delete();
+    }
+
+    public void testHeadersExchangeCreateNewBinding() throws Exception
+    {
+        String binding = "key1=binding1,key2=binding2";
+        Map<String, Object> expectedBindingMap = new HashMap<String, Object>()
+        {{
+            put("key1", "binding1");
+            put("key2", "binding2");
+        }};
+        _exchangeMBean = new ExchangeMBean(_mockHeadersExchange, _mockVirtualHostMBean);
+
+        _exchangeMBean.createNewBinding(QUEUE1_NAME, binding);
+        verify(_mockHeadersExchange).createBinding(binding, _mockQueue1, expectedBindingMap, Collections.EMPTY_MAP);
+    }
+
+    public void testHeadersExchangeCreateNewBindingWithFieldWithoutValue() throws Exception
+    {
+        String binding = "key1=binding1,key2=";
+        Map<String, Object> expectedBindingMap = new HashMap<String, Object>()
+        {{
+            put("key1", "binding1");
+            put("key2", "");
+        }};
+        _exchangeMBean = new ExchangeMBean(_mockHeadersExchange, _mockVirtualHostMBean);
+
+        _exchangeMBean.createNewBinding(QUEUE1_NAME, binding);
+        verify(_mockHeadersExchange).createBinding(binding, _mockQueue1, expectedBindingMap, Collections.EMPTY_MAP);
+    }
+
+    public void testHeadersExchangeCreateNewBindingMalformed() throws Exception
+    {
+        String binding = "=binding1,=";
+       _exchangeMBean = new ExchangeMBean(_mockHeadersExchange, _mockVirtualHostMBean);
+
+       try
+       {
+           _exchangeMBean.createNewBinding(QUEUE1_NAME, binding);
+           fail("Exception not thrown");
+       }
+       catch (JMException e)
+       {
+           assertEquals("Format for headers binding should be \"<attribute1>=<value1>,<attribute2>=<value2>\"", e.getMessage());
+       }
+    }
+
+    private Binding createBindingOnQueue(String bindingName, Queue queue)
+    {
+        Binding mockBinding = mock(Binding.class);
+        when(mockBinding.getParent(Queue.class)).thenReturn(queue);
+        when(mockBinding.getName()).thenReturn(bindingName);
+        return mockBinding;
+    }
+
+    private Queue createMockQueue(String queueName)
+    {
+        Queue mockQueue = mock(Queue.class);
+        when(mockQueue.getName()).thenReturn(queueName);
+        return mockQueue;
+    }
+
+}