QPID-6027 : [Java Broker] Enforce that all records being added into the Json store have a name which is a String

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

2 files changed, 125 insertions, 38 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/store/JsonFileConfigStore.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/store/JsonFileConfigStore.java
index 70dc2344db..1f5665a0a5 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/store/JsonFileConfigStore.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/store/JsonFileConfigStore.java
@@ -37,8 +37,6 @@ import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.SortedSet;
-import java.util.TreeSet;
 import java.util.UUID;
 
 import org.apache.log4j.Logger;
@@ -60,6 +58,28 @@ public class JsonFileConfigStore implements DurableConfigurationStore
 {
     private static final Logger _logger = Logger.getLogger(JsonFileConfigStore.class);
 
+    private static final Comparator<Class<? extends ConfiguredObject>> CATEGORY_CLASS_COMPARATOR =
+            new Comparator<Class<? extends ConfiguredObject>>()
+            {
+                @Override
+                public int compare(final Class<? extends ConfiguredObject> left,
+                                   final Class<? extends ConfiguredObject> right)
+                {
+                    return left.getSimpleName().compareTo(right.getSimpleName());
+                }
+            };
+    private static final Comparator<ConfiguredObjectRecord> CONFIGURED_OBJECT_RECORD_COMPARATOR =
+            new Comparator<ConfiguredObjectRecord>()
+            {
+                @Override
+                public int compare(final ConfiguredObjectRecord left, final ConfiguredObjectRecord right)
+                {
+                    String leftName = (String) left.getAttributes().get(ConfiguredObject.NAME);
+                    String rightName = (String) right.getAttributes().get(ConfiguredObject.NAME);
+                    return leftName.compareTo(rightName);
+                }
+            };
+
     private final Map<UUID, ConfiguredObjectRecord> _objectsById = new HashMap<UUID, ConfiguredObjectRecord>();
     private final Map<String, List<UUID>> _idsByType = new HashMap<String, List<UUID>>();
     private final ObjectMapper _objectMapper = new ObjectMapper();
@@ -316,6 +336,14 @@ public class JsonFileConfigStore implements DurableConfigurationStore
         {
             throw new StoreException("Cannot create object of unknown type " + record.getType());
         }
+        else if(record.getAttributes() == null || !(record.getAttributes().get(ConfiguredObject.NAME) instanceof String))
+        {
+            throw new StoreException("The record " + record.getId()
+                                     + " of type " + record.getType()
+                                     + " does not have an attribute '"
+                                     + ConfiguredObject.NAME
+                                     + "' of type String");
+        }
         else
         {
             record = new ConfiguredObjectRecordImpl(record);
@@ -410,14 +438,7 @@ public class JsonFileConfigStore implements DurableConfigurationStore
         List<Class<? extends ConfiguredObject>> childClasses =
                 new ArrayList<Class<? extends ConfiguredObject>>(_parent.getModel().getChildTypes(type));
 
-        Collections.sort(childClasses, new Comparator<Class<? extends ConfiguredObject>>()
-        {
-            @Override
-            public int compare(final Class<? extends ConfiguredObject> o1, final Class<? extends ConfiguredObject> o2)
-            {
-                return o1.getSimpleName().compareTo(o2.getSimpleName());
-            }
-        });
+        Collections.sort(childClasses, CATEGORY_CLASS_COMPARATOR);
 
         for(Class<? extends ConfiguredObject> childClass : childClasses)
         {
@@ -429,18 +450,7 @@ public class JsonFileConfigStore implements DurableConfigurationStore
                 if(childIds != null)
                 {
                     List<Map<String,Object>> entities = new ArrayList<Map<String, Object>>();
-                    SortedSet<ConfiguredObjectRecord> sortedChildren = new TreeSet<>(new Comparator<ConfiguredObjectRecord>()
-                    {
-                        @Override
-                        public int compare(final ConfiguredObjectRecord left, final ConfiguredObjectRecord right)
-                        {
-                            String leftName = (String) left.getAttributes().get(ConfiguredObject.NAME);
-                            String rightName = (String) right.getAttributes().get(ConfiguredObject.NAME);
-                            return leftName == null
-                                    ?  -1
-                                    : rightName == null ? 1 : leftName.compareTo(rightName);
-                        }
-                    });
+                    List<ConfiguredObjectRecord> sortedChildren = new ArrayList<>();
                     for(UUID childId : childIds)
                     {
                         ConfiguredObjectRecord childRecord = _objectsById.get(childId);
@@ -452,6 +462,9 @@ public class JsonFileConfigStore implements DurableConfigurationStore
                             sortedChildren.add(childRecord);
                         }
                     }
+
+                    Collections.sort(sortedChildren, CONFIGURED_OBJECT_RECORD_COMPARATOR);
+
                     for(ConfiguredObjectRecord childRecord : sortedChildren)
                     {
                         entities.add(build(childClass, childRecord.getId()));
@@ -505,6 +518,13 @@ public class JsonFileConfigStore implements DurableConfigurationStore
             final UUID id = record.getId();
             final String type = record.getType();
 
+            if(record.getAttributes() == null || !(record.getAttributes().get(ConfiguredObject.NAME) instanceof String))
+            {
+                throw new StoreException("The record " + id + " of type " + type + " does not have an attribute '"
+                                         + ConfiguredObject.NAME
+                                         + "' of type String");
+            }
+
             if(_objectsById.containsKey(id))
             {
                 final ConfiguredObjectRecord existingRecord = _objectsById.get(id);
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/store/JsonFileConfigStoreTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/store/JsonFileConfigStoreTest.java
index ee8f6497bc..b652992021 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/store/JsonFileConfigStoreTest.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/store/JsonFileConfigStoreTest.java
@@ -38,6 +38,7 @@ import org.mockito.ArgumentMatcher;
 import org.mockito.InOrder;
 
 import org.apache.qpid.server.model.BrokerModel;
+import org.apache.qpid.server.model.ConfiguredObject;
 import org.apache.qpid.server.model.ConfiguredObjectFactory;
 import org.apache.qpid.server.model.ConfiguredObjectFactoryImpl;
 import org.apache.qpid.server.model.Queue;
@@ -248,10 +249,14 @@ public class JsonFileConfigStoreTest extends QpidTestCase
         createRootRecord();
 
         final UUID id = UUID.randomUUID();
-        _store.create(new ConfiguredObjectRecordImpl(id, "Queue", Collections.<String, Object>emptyMap(), getRootAsParentMap()));
+        _store.create(new ConfiguredObjectRecordImpl(id, "Queue",
+                                                     Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "queue"),
+                                                     getRootAsParentMap()));
         try
         {
-            _store.create(new ConfiguredObjectRecordImpl(id, "Exchange", Collections.<String, Object>emptyMap(), getRootAsParentMap()));
+            _store.create(new ConfiguredObjectRecordImpl(id, "Exchange",
+                                                         Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "exchange"),
+                                                         getRootAsParentMap()));
             fail("Should not be able to create two objects with same id");
         }
         catch (StoreException e)
@@ -261,19 +266,61 @@ public class JsonFileConfigStoreTest extends QpidTestCase
     }
 
 
+    public void testObjectWithoutName() throws Exception
+    {
+        _store.openConfigurationStore(_parent, false);
+        createRootRecord();
+
+        final UUID id = UUID.randomUUID();
+        try
+        {
+            _store.create(new ConfiguredObjectRecordImpl(id, "Exchange",
+                                                         Collections.<String, Object>emptyMap(),
+                                                         getRootAsParentMap()));
+            fail("Should not be able to create an object without a name");
+        }
+        catch (StoreException e)
+        {
+            // pass
+        }
+    }
+
+    public void testObjectWithNonStringName() throws Exception
+    {
+        _store.openConfigurationStore(_parent, false);
+        createRootRecord();
+
+        final UUID id = UUID.randomUUID();
+        try
+        {
+            _store.update(true, new ConfiguredObjectRecordImpl(id, "Exchange",
+                                                         Collections.<String, Object>singletonMap(ConfiguredObject.NAME, 3),
+                                                         getRootAsParentMap()));
+            fail("Should not be able to create an object without a name");
+        }
+        catch (StoreException e)
+        {
+            // pass
+        }
+    }
+
     public void testChangeTypeOfObject() throws Exception
     {
         _store.openConfigurationStore(_parent, false);
         createRootRecord();
 
         final UUID id = UUID.randomUUID();
-        _store.create(new ConfiguredObjectRecordImpl(id, "Queue", Collections.<String, Object>emptyMap(), getRootAsParentMap()));
+        _store.create(new ConfiguredObjectRecordImpl(id, "Queue",
+                                                     Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "queue"),
+                                                     getRootAsParentMap()));
         _store.closeConfigurationStore();
         _store.openConfigurationStore(_parent, false);
 
         try
         {
-            _store.update(false, new ConfiguredObjectRecordImpl(id, "Exchange", Collections.<String, Object>emptyMap(), getRootAsParentMap()));
+            _store.update(false, new ConfiguredObjectRecordImpl(id, "Exchange",
+                                                                Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "exchange"),
+                                                                getRootAsParentMap()));
             fail("Should not be able to update object to different type");
         }
         catch (StoreException e)
@@ -329,40 +376,57 @@ public class JsonFileConfigStoreTest extends QpidTestCase
         final UUID queueId = new UUID(0, 1);
         final UUID queue2Id = new UUID(1, 1);
 
-        final Map<String, Object> EMPTY_ATTR = Collections.emptyMap();
         final UUID exchangeId = new UUID(0, 2);
 
         final UUID bindingId = new UUID(0, 3);
         final UUID binding2Id = new UUID(1, 3);
 
         Map<String, UUID> parents = getRootAsParentMap();
-        final ConfiguredObjectRecordImpl queueRecord = new ConfiguredObjectRecordImpl(queueId, "Queue", EMPTY_ATTR, parents);
+        Map<String, Object> queueAttr = Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "queue");
+        final ConfiguredObjectRecordImpl queueRecord =
+                new ConfiguredObjectRecordImpl(queueId, "Queue",
+                                               queueAttr,
+                                               parents);
         _store.create(queueRecord);
-        final ConfiguredObjectRecordImpl queue2Record = new ConfiguredObjectRecordImpl(queue2Id, "Queue", EMPTY_ATTR, parents);
+        Map<String, Object> queue2Attr = Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "queue2");
+        final ConfiguredObjectRecordImpl queue2Record =
+                new ConfiguredObjectRecordImpl(queue2Id, "Queue",
+                                               queue2Attr,
+                                               parents);
         _store.create(queue2Record);
-        final ConfiguredObjectRecordImpl exchangeRecord = new ConfiguredObjectRecordImpl(exchangeId, "Exchange", EMPTY_ATTR, parents);
+        Map<String, Object> exchangeAttr = Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "exchange");
+        final ConfiguredObjectRecordImpl exchangeRecord =
+                new ConfiguredObjectRecordImpl(exchangeId, "Exchange",
+                                               exchangeAttr,
+                                               parents);
         _store.create(exchangeRecord);
         Map<String,UUID> bindingParents = new HashMap();
         bindingParents.put("Exchange", exchangeRecord.getId());
         bindingParents.put("Queue", queueRecord.getId());
+        Map<String, Object> bindingAttr = Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "binding");
         final ConfiguredObjectRecordImpl bindingRecord =
-                new ConfiguredObjectRecordImpl(bindingId, "Binding", EMPTY_ATTR, bindingParents);
+                new ConfiguredObjectRecordImpl(bindingId, "Binding",
+                                               bindingAttr,
+                                               bindingParents);
 
 
         Map<String,UUID> binding2Parents = new HashMap();
         binding2Parents.put("Exchange", exchangeRecord.getId());
         binding2Parents.put("Queue", queue2Record.getId());
+        Map<String, Object> binding2Attr = Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "binding2");
         final ConfiguredObjectRecordImpl binding2Record =
-                new ConfiguredObjectRecordImpl(binding2Id, "Binding", EMPTY_ATTR, binding2Parents);
+                new ConfiguredObjectRecordImpl(binding2Id, "Binding",
+                                               binding2Attr,
+                                               binding2Parents);
         _store.update(true, bindingRecord, binding2Record);
         _store.closeConfigurationStore();
         _store.openConfigurationStore(_parent, false);
         _store.visitConfiguredObjectRecords(_handler);
-        verify(_handler).handle(matchesRecord(queueId, "Queue", EMPTY_ATTR));
-        verify(_handler).handle(matchesRecord(queue2Id, "Queue", EMPTY_ATTR));
-        verify(_handler).handle(matchesRecord(exchangeId, "Exchange", EMPTY_ATTR));
-        verify(_handler).handle(matchesRecord(bindingId, "Binding", EMPTY_ATTR));
-        verify(_handler).handle(matchesRecord(binding2Id, "Binding", EMPTY_ATTR));
+        verify(_handler).handle(matchesRecord(queueId, "Queue", queueAttr));
+        verify(_handler).handle(matchesRecord(queue2Id, "Queue", queue2Attr));
+        verify(_handler).handle(matchesRecord(exchangeId, "Exchange", exchangeAttr));
+        verify(_handler).handle(matchesRecord(bindingId, "Binding", bindingAttr));
+        verify(_handler).handle(matchesRecord(binding2Id, "Binding", binding2Attr));
         _store.closeConfigurationStore();
 
     }
@@ -371,7 +435,10 @@ public class JsonFileConfigStoreTest extends QpidTestCase
     private void createRootRecord()
     {
         UUID rootRecordId = UUID.randomUUID();
-        _rootRecord = new ConfiguredObjectRecordImpl(rootRecordId, VIRTUAL_HOST_TYPE, Collections.<String, Object>emptyMap());
+        _rootRecord =
+                new ConfiguredObjectRecordImpl(rootRecordId,
+                                               VIRTUAL_HOST_TYPE,
+                                               Collections.<String, Object>singletonMap(ConfiguredObject.NAME, "root"));
         _store.create(_rootRecord);
     }