QPID-3087: fail rather than ignoring attempts to declare queues with bad arguments; ensure qpid-config can deal with different types of argument.

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

6 files changed, 67 insertions, 17 deletions

diff --git a/qpid/cpp/src/qpid/broker/Broker.cpp b/qpid/cpp/src/qpid/broker/Broker.cpp
index 695a926f74..4e7e78c961 100644
--- a/qpid/cpp/src/qpid/broker/Broker.cpp
+++ b/qpid/cpp/src/qpid/broker/Broker.cpp
@@ -790,15 +790,13 @@ std::pair<boost::shared_ptr<Queue>, bool> Broker::createQueue(
         if (!alternate) throw framing::NotFoundException(QPID_MSG("Alternate exchange does not exist: " << alternateExchange));
     }
 
-    std::pair<Queue::shared_ptr, bool> result = queues.declare(name, durable, autodelete, owner);
+    std::pair<Queue::shared_ptr, bool> result = queues.declare(name, durable, autodelete, owner, arguments);
     if (result.second) {
         if (alternate) {
             result.first->setAlternateExchange(alternate);
             alternate->incAlternateUsers();
         }
 
-        //apply settings & create persistent record if required
-        result.first->create(arguments);
         //add default binding:
         result.first->bind(exchanges.getDefault(), name);
 
diff --git a/qpid/cpp/src/qpid/broker/QueuePolicy.cpp b/qpid/cpp/src/qpid/broker/QueuePolicy.cpp
index 4168221ad0..b39ea3614b 100644
--- a/qpid/cpp/src/qpid/broker/QueuePolicy.cpp
+++ b/qpid/cpp/src/qpid/broker/QueuePolicy.cpp
@@ -136,11 +136,10 @@ uint32_t QueuePolicy::getCapacity(const FieldTable& settings, const std::string&
         string s(v->get<string>());
         QPID_LOG(debug, "Got string value for " << key << ": " << s);
         std::istringstream convert(s);
-        if (convert >> result && result >= 0) return result;
+        if (convert >> result && result >= 0 && convert.eof()) return result;
     }
 
-    QPID_LOG(warning, "Cannot convert " << key << " to unsigned integer, using default (" << defaultValue << ")");
-    return defaultValue;
+    throw IllegalArgumentException(QPID_MSG("Cannot convert " << key << " to unsigned integer: " << *v));
 }
 
 std::string QueuePolicy::getType(const FieldTable& settings)
diff --git a/qpid/cpp/src/qpid/broker/QueueRegistry.cpp b/qpid/cpp/src/qpid/broker/QueueRegistry.cpp
index ea2531dae7..cdd1d87e63 100644
--- a/qpid/cpp/src/qpid/broker/QueueRegistry.cpp
+++ b/qpid/cpp/src/qpid/broker/QueueRegistry.cpp
@@ -36,7 +36,8 @@ QueueRegistry::~QueueRegistry(){}
 
 std::pair<Queue::shared_ptr, bool>
 QueueRegistry::declare(const string& declareName, bool durable, 
-                       bool autoDelete, const OwnershipToken* owner)
+                       bool autoDelete, const OwnershipToken* owner,
+                       const framing::FieldTable& arguments)
 {
     RWlock::ScopedWlock locker(lock);
     string name = declareName.empty() ? generateName() : declareName;
@@ -45,6 +46,8 @@ QueueRegistry::declare(const string& declareName, bool durable,
 
     if (i == queues.end()) {
         Queue::shared_ptr queue(new Queue(name, autoDelete, durable ? store : 0, owner, parent, broker));
+        //apply settings & create persistent record if required
+        queue->create(arguments);
         queues[name] = queue;
         if (lastNode) queue->setLastNodeFailure();
 
diff --git a/qpid/cpp/src/qpid/broker/QueueRegistry.h b/qpid/cpp/src/qpid/broker/QueueRegistry.h
index 57859fe639..90ee924ba4 100644
--- a/qpid/cpp/src/qpid/broker/QueueRegistry.h
+++ b/qpid/cpp/src/qpid/broker/QueueRegistry.h
@@ -24,6 +24,7 @@
 #include "qpid/broker/BrokerImportExport.h"
 #include "qpid/sys/Mutex.h"
 #include "qpid/management/Manageable.h"
+#include "qpid/framing/FieldTable.h"
 #include <boost/bind.hpp>
 #include <boost/shared_ptr.hpp>
 #include <algorithm>
@@ -60,7 +61,8 @@ class QueueRegistry {
         const std::string& name,
         bool durable = false,
         bool autodelete = false, 
-        const OwnershipToken* owner = 0);
+        const OwnershipToken* owner = 0,
+        const qpid::framing::FieldTable& args = framing::FieldTable());
 
     /**
      * Destroy the named queue.
diff --git a/qpid/tests/src/py/qpid_tests/broker_0_10/extensions.py b/qpid/tests/src/py/qpid_tests/broker_0_10/extensions.py
index 26ea3cb0e9..d9eeb8d1ea 100644
--- a/qpid/tests/src/py/qpid_tests/broker_0_10/extensions.py
+++ b/qpid/tests/src/py/qpid_tests/broker_0_10/extensions.py
@@ -20,6 +20,7 @@ from qpid.client import Client, Closed
 from qpid.queue import Empty
 from qpid.content import Content
 from qpid.testlib import TestBase010
+from qpid.session import SessionException
 from time import sleep
 
 class ExtensionTests(TestBase010):
@@ -35,3 +36,50 @@ class ExtensionTests(TestBase010):
         sleep(5)
         result = session.queue_query(queue="my-queue")
         self.assert_(not result.queue)
+
+    def valid_policy_args(self, args, name="test-queue"):
+        try:
+            self.session.queue_declare(queue=name, arguments=args)
+            self.session.queue_delete(queue=name) # cleanup
+        except SessionException, e:
+            self.fail("declare with valid policy args failed: %s" % (args))
+            self.session = self.conn.session("replacement", 2)
+
+    def invalid_policy_args(self, args, name="test-queue"):
+        # go through invalid declare attempts twice to make sure that
+        # the queue doesn't actually get created first time around
+        # even if exception is thrown
+        for i in range(1, 3):
+            try:
+                self.session.queue_declare(queue=name, arguments=args)
+                #self.session.queue_delete(queue=name) # cleanup
+                self.fail("declare with invalid policy args suceeded: %s (iteration %d)" % (args, i))
+            except SessionException, e:
+                self.session = self.conn.session("replacement", 2)
+
+    def test_policy_max_size_as_valid_string(self):
+        self.valid_policy_args({"qpid.max_size":"3"})
+
+    def test_policy_max_count_as_valid_string(self):
+        self.valid_policy_args({"qpid.max_count":"3"})
+
+    def test_policy_max_count_and_size_as_valid_strings(self):
+        self.valid_policy_args({"qpid.max_count":"3","qpid.max_size":"0"})
+
+    def test_policy_negative_count(self):
+        self.invalid_policy_args({"qpid.max_count":-1})
+
+    def test_policy_negative_size(self):
+        self.invalid_policy_args({"qpid.max_size":-1})
+
+    def test_policy_size_as_invalid_string(self):
+        self.invalid_policy_args({"qpid.max_size":"foo"})
+
+    def test_policy_count_as_invalid_string(self):
+        self.invalid_policy_args({"qpid.max_count":"foo"})
+
+    def test_policy_size_as_float(self):
+        self.invalid_policy_args({"qpid.max_size":3.14159})
+
+    def test_policy_count_as_float(self):
+        self.invalid_policy_args({"qpid.max_count":"2222222.22222"})
diff --git a/qpid/tools/src/py/qpid-config b/qpid/tools/src/py/qpid-config
index 9ff405541c..b02bd2c1e1 100755
--- a/qpid/tools/src/py/qpid-config
+++ b/qpid/tools/src/py/qpid-config
@@ -405,20 +405,20 @@ class BrokerManager:
                 if CLUSTER_DURABLE in args and args[CLUSTER_DURABLE] == 1: print "--cluster-durable",
                 if q.autoDelete: print "auto-del",
                 if q.exclusive:  print "excl",
-                if FILESIZE in args: print "--file-size=%d" % args[FILESIZE],
-                if FILECOUNT in args: print "--file-count=%d" % args[FILECOUNT],
-                if MAX_QUEUE_SIZE in args: print "--max-queue-size=%d" % args[MAX_QUEUE_SIZE],
-                if MAX_QUEUE_COUNT in args: print "--max-queue-count=%d" % args[MAX_QUEUE_COUNT],
+                if FILESIZE in args: print "--file-size=%s" % args[FILESIZE],
+                if FILECOUNT in args: print "--file-count=%s" % args[FILECOUNT],
+                if MAX_QUEUE_SIZE in args: print "--max-queue-size=%s" % args[MAX_QUEUE_SIZE],
+                if MAX_QUEUE_COUNT in args: print "--max-queue-count=%s" % args[MAX_QUEUE_COUNT],
                 if POLICY_TYPE in args: print "--limit-policy=%s" % args[POLICY_TYPE].replace("_", "-"),
                 if LVQ in args and args[LVQ] == 1: print "--order lvq",
                 if LVQNB in args and args[LVQNB] == 1: print "--order lvq-no-browse",
-                if QUEUE_EVENT_GENERATION in args: print "--generate-queue-events=%d" % args[QUEUE_EVENT_GENERATION],
+                if QUEUE_EVENT_GENERATION in args: print "--generate-queue-events=%s" % args[QUEUE_EVENT_GENERATION],
                 if q.altExchange:
                     print "--alternate-exchange=%s" % q._altExchange_.name,
-                if FLOW_STOP_SIZE in args: print "--flow-stop-size=%d" % args[FLOW_STOP_SIZE],
-                if FLOW_RESUME_SIZE in args: print "--flow-resume-size=%d" % args[FLOW_RESUME_SIZE],
-                if FLOW_STOP_COUNT in args: print "--flow-stop-count=%d" % args[FLOW_STOP_COUNT],
-                if FLOW_RESUME_COUNT in args: print "--flow-resume-count=%d" % args[FLOW_RESUME_COUNT],
+                if FLOW_STOP_SIZE in args: print "--flow-stop-size=%s" % args[FLOW_STOP_SIZE],
+                if FLOW_RESUME_SIZE in args: print "--flow-resume-size=%s" % args[FLOW_RESUME_SIZE],
+                if FLOW_STOP_COUNT in args: print "--flow-stop-count=%s" % args[FLOW_STOP_COUNT],
+                if FLOW_RESUME_COUNT in args: print "--flow-resume-count=%s" % args[FLOW_RESUME_COUNT],
                 print
 
     def QueueListRecurse(self, filter):