QPID-5709 : [Java Broker] address review comments from Keith Wall

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

5 files changed, 25 insertions, 190 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/exchange/TopicExchange.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/exchange/TopicExchange.java
index d46ea44239..0c0311903b 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/exchange/TopicExchange.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/exchange/TopicExchange.java
@@ -73,7 +73,7 @@ public class TopicExchange extends AbstractExchange<TopicExchange>
     }
 
     @Override
-    protected void onBindingUpdated(final BindingImpl binding, final Map<String, Object> oldArguments)
+    protected synchronized void onBindingUpdated(final BindingImpl binding, final Map<String, Object> oldArguments)
     {
         final String bindingKey = binding.getBindingKey();
         AMQQueue queue = binding.getAMQQueue();
@@ -82,7 +82,7 @@ public class TopicExchange extends AbstractExchange<TopicExchange>
         assert queue != null;
         assert bindingKey != null;
 
-        _logger.debug("Registering queue " + queue.getName() + " with routing key " + bindingKey);
+        _logger.debug("Updating binding of queue " + queue.getName() + " with routing key " + bindingKey);
 
 
         String routingKey = TopicNormalizer.normalize(bindingKey);
@@ -253,7 +253,7 @@ public class TopicExchange extends AbstractExchange<TopicExchange>
 
     }
 
-    private boolean deregisterQueue(final BindingImpl binding)
+    private synchronized boolean deregisterQueue(final BindingImpl binding)
     {
         if(_bindings.containsKey(binding))
         {
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
index 3bf9c6cda1..4860d72e26 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java
@@ -212,13 +212,10 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
         _type = Model.getType(getClass());
         _bestFitInterface = calculateBestFitInterface();
 
-        if(attributes.get(TYPE) != null)
+        if(attributes.get(TYPE) != null && !_type.equals(attributes.get(TYPE)))
         {
-            if(!_type.equals(attributes.get(TYPE)))
-            {
-                throw new IllegalConfigurationException("Provided type is " + attributes.get(TYPE)
-                                                        + " but calculated type is " + _type);
-            }
+            throw new IllegalConfigurationException("Provided type is " + attributes.get(TYPE)
+                                                    + " but calculated type is " + _type);
         }
 
         for (Class<? extends ConfiguredObject> childClass : getModel().getChildTypes(getCategoryClass()))
@@ -244,18 +241,14 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
         Object durableObj = attributes.get(DURABLE);
         _durable = AttributeValueConverter.BOOLEAN_CONVERTER.convert(durableObj == null ? _attributeTypes.get(DURABLE).getAnnotation().defaultValue() : durableObj, this);
 
-        Collection<String> names = getAttributeNames();
-        if(names!=null)
+        for (String name : getAttributeNames())
         {
-            for (String name : names)
+            if (attributes.containsKey(name))
             {
-                if (attributes.containsKey(name))
+                final Object value = attributes.get(name);
+                if (value != null)
                 {
-                    final Object value = attributes.get(name);
-                    if(value != null)
-                    {
-                        _attributes.put(name, value);
-                    }
+                    _attributes.put(name, value);
                 }
             }
         }
@@ -800,10 +793,7 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
 
     public <T extends ConfiguredObject> T getParent(final Class<T> clazz)
     {
-        synchronized (_parents)
-        {
-            return (T) _parents.get(clazz);
-        }
+        return (T) _parents.get(clazz);
     }
 
     private <T extends ConfiguredObject> void addParent(Class<T> clazz, T parent)
@@ -924,33 +914,16 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
         {
             throw new DuplicateIdException(child);
         }
-        if(_childrenByName.get(categoryClass).containsKey(name))
+        if(getModel().getParentTypes(categoryClass).size() == 1)
         {
-            Collection<Class<? extends ConfiguredObject>> parentTypes =
-                    new ArrayList<Class<? extends ConfiguredObject>>(child.getModel().getParentTypes(categoryClass));
-            parentTypes.remove(getCategoryClass());
-            boolean duplicate = true;
-
-            C existing = (C) _childrenByName.get(categoryClass).get(name);
-            for(Class<? extends ConfiguredObject> parentType : parentTypes)
-            {
-                ConfiguredObject existingParent = existing.getParent(parentType);
-                ConfiguredObject childParent = child.getParent(parentType);
-                duplicate =  existingParent == childParent;
-                if(!duplicate)
-                {
-                    break;
-                }
-            }
-
-            if(duplicate)
+            if (_childrenByName.get(categoryClass).containsKey(name))
             {
                 throw new DuplicateNameException(child);
             }
+            _childrenByName.get(categoryClass).put(name, child);
         }
         _children.get(categoryClass).add(child);
         _childrenById.get(categoryClass).put(childId,child);
-        _childrenByName.get(categoryClass).put(name, child);
 
     }
 
@@ -986,7 +959,13 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
     @Override
     public final <C extends ConfiguredObject> C getChildByName(final Class<C> clazz, final String name)
     {
-        return (C) _childrenByName.get(Model.getCategory(clazz)).get(name);
+        Class<? extends ConfiguredObject> categoryClass = Model.getCategory(clazz);
+        if(getModel().getParentTypes(categoryClass).size() != 1)
+        {
+            throw new UnsupportedOperationException("Cannot use getChildByName for objects of category "
+                                                    + categoryClass.getSimpleName() + " as it has more than one parent");
+        }
+        return (C) _childrenByName.get(categoryClass).get(name);
     }
 
     @Override
@@ -1072,8 +1051,9 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im
 
     private ConfiguredObject<?> createProxyForValidation(final Map<String, Object> attributes)
     {
-        return (ConfiguredObject<?>) Proxy.newProxyInstance(getClass().getClassLoader(),new Class<?>[]{_bestFitInterface},
-                                      new AttributeGettingHandler(attributes));
+        return (ConfiguredObject<?>) Proxy.newProxyInstance(getClass().getClassLoader(),
+                                                            new Class<?>[]{_bestFitInterface},
+                                                            new AttributeGettingHandler(attributes));
     }
 
     protected void authoriseSetDesiredState(State currentState, State desiredState) throws AccessControlException
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/queue/DefaultQueueRegistry.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/queue/DefaultQueueRegistry.java
deleted file mode 100644
index a54b3ae7f0..0000000000
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/queue/DefaultQueueRegistry.java
+++ /dev/null
@@ -1,90 +0,0 @@
-/*
- *
- * 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.queue;
-
-import java.util.Collection;
-import java.util.UUID;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-
-import org.apache.qpid.server.virtualhost.VirtualHostImpl;
-
-public class DefaultQueueRegistry implements QueueRegistry
-{
-    private ConcurrentMap<String, AMQQueue<?>> _queueMap = new ConcurrentHashMap<String, AMQQueue<?>>();
-
-    private final VirtualHostImpl _virtualHost;
-
-    public DefaultQueueRegistry(VirtualHostImpl virtualHost)
-    {
-        _virtualHost = virtualHost;
-    }
-
-    public VirtualHostImpl getVirtualHost()
-    {
-        return _virtualHost;
-    }
-
-    public void registerQueue(AMQQueue queue)
-    {
-        _queueMap.put(queue.getName(), queue);
-    }
-
-    public void unregisterQueue(String name)
-    {
-        AMQQueue q = _queueMap.remove(name);
-    }
-
-
-    public Collection<AMQQueue<?>> getQueues()
-    {
-        return _queueMap.values();
-    }
-
-    public AMQQueue getQueue(String queue)
-    {
-        return queue == null ? null : _queueMap.get(queue);
-    }
-
-    @Override
-    public void close()
-    {
-        for (final AMQQueue queue : getQueues())
-        {
-            queue.stop();
-        }
-        _queueMap.clear();
-    }
-
-    @Override
-    public synchronized AMQQueue getQueue(UUID queueId)
-    {
-        Collection<AMQQueue<?>> queues = _queueMap.values();
-        for (AMQQueue<?> queue : queues)
-        {
-            if (queue.getId().equals(queueId))
-            {
-                return queue;
-            }
-        }
-        return null;
-    }
-}
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/queue/QueueRegistry.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/queue/QueueRegistry.java
deleted file mode 100644
index 5a90536476..0000000000
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/queue/QueueRegistry.java
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
- *
- * 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.queue;
-
-import java.util.Collection;
-import java.util.UUID;
-
-import org.apache.qpid.server.virtualhost.VirtualHostImpl;
-
-public interface QueueRegistry
-{
-    VirtualHostImpl getVirtualHost();
-
-    void registerQueue(AMQQueue<?> queue);
-
-    void unregisterQueue(String name);
-
-    Collection<AMQQueue<?>> getQueues();
-
-    AMQQueue<?> getQueue(String queue);
-
-    void close();
-
-    AMQQueue<?> getQueue(UUID queueId);
-
-    interface RegistryChangeListener
-    {
-        void queueRegistered(AMQQueue queue);
-        void queueUnregistered(AMQQueue queue);
-    }
-}
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/virtualhost/MockVirtualHost.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/virtualhost/MockVirtualHost.java
index 3f104de77b..43cf54ae3e 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/virtualhost/MockVirtualHost.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/virtualhost/MockVirtualHost.java
@@ -45,7 +45,6 @@ import org.apache.qpid.server.model.State;
 import org.apache.qpid.server.model.VirtualHostAlias;
 import org.apache.qpid.server.protocol.LinkRegistry;
 import org.apache.qpid.server.queue.AMQQueue;
-import org.apache.qpid.server.queue.QueueRegistry;
 import org.apache.qpid.server.security.SecurityManager;
 import org.apache.qpid.server.security.auth.manager.AuthenticationManager;
 import org.apache.qpid.server.stats.StatisticsCounter;
@@ -197,11 +196,6 @@ public class MockVirtualHost implements VirtualHostImpl<MockVirtualHost, AMQQueu
         return null;
     }
 
-    public QueueRegistry getQueueRegistry()
-    {
-        return null;
-    }
-
     @Override
     public AMQQueue<?> getQueue(String name)
     {