doxygen for createAnswer and family of helper functions

added sanity checks within those functions (and tests for that)
changes createAnswer(int rcode) to createAnswer() since rcode should only be 0 if there is not argument



git-svn-id: svn://bind10.isc.org/svn/bind10/trunk@1368 e5f2f494-b856-4b98-b285-d166d9295462

src/bin/auth/auth_srv.cc

@@ -248,7 +248,7 @@ AuthSrvImpl::setDbFile(const isc::data::ElementPtr config) {
     // delete and swap operations which should not fail.
     DataSrcPtr datasrc_ptr(DataSrcPtr(new Sqlite3DataSrc));
     datasrc_ptr->init(config);
-    ElementPtr answer = isc::config::createAnswer(0);
+    ElementPtr answer = isc::config::createAnswer();
     data_sources_.addDataSrc(datasrc_ptr);
 
     // The following code should be exception free.
@@ -263,7 +263,7 @@ AuthSrvImpl::setDbFile(const isc::data::ElementPtr config) {
 ElementPtr
 AuthSrv::updateConfig(isc::data::ElementPtr new_config) {
     try {
-        ElementPtr answer = isc::config::createAnswer(0);
+        ElementPtr answer = isc::config::createAnswer();
         if (new_config != NULL) {
             // the ModuleCCSession has already checked if we have
             // the correct ElementPtr type as specified in our .spec file

src/bin/auth/main.cc

@@ -90,7 +90,7 @@ my_config_handler(ElementPtr new_config) {
 
 static ElementPtr
 my_command_handler(const string& command, const ElementPtr args) {
-    ElementPtr answer = createAnswer(0);
+    ElementPtr answer = createAnswer();
 
     if (command == "print_message") {
         cout << args << endl;

src/lib/config/ccsession.cc

@@ -56,17 +56,20 @@ namespace config {
 
 /// Creates a standard config/command protocol answer message
 ElementPtr
-createAnswer(const int rcode)
+createAnswer()
 {
     ElementPtr answer = Element::createFromString("{\"result\": [] }");
     ElementPtr answer_content = answer->get("result");
-    answer_content->add(Element::create(rcode));
+    answer_content->add(Element::create(0));
     return answer;
 }
 
 ElementPtr
 createAnswer(const int rcode, const ElementPtr arg)
 {
+    if (rcode != 0 && (!arg || arg->getType() != Element::string)) {
+        isc_throw(CCSessionError, "Bad or no argument for rcode != 0");
+    }
     ElementPtr answer = Element::createFromString("{\"result\": [] }");
     ElementPtr answer_content = answer->get("result");
     answer_content->add(Element::create(rcode));
@@ -87,26 +90,41 @@ createAnswer(const int rcode, const std::string& arg)
 ElementPtr
 parseAnswer(int &rcode, const ElementPtr msg)
 {
-    if (!msg->contains("result")) {
-        // TODO: raise CCSessionError exception
-        isc_throw(CCSessionError, "No result in answer message");
-    } else {
+    if (msg &&
+        msg->getType() == Element::map &&
+        msg->contains("result")) {
         ElementPtr result = msg->get("result");
-        if (result->get(0)->getType() != Element::integer) {
+        if (result->getType() != Element::list) {
+            isc_throw(CCSessionError, "Result element in answer message is not a list");
+        } else if (result->get(0)->getType() != Element::integer) {
             isc_throw(CCSessionError, "First element of result is not an rcode in answer message");
-        } else if (result->get(0)->intValue() != 0 && result->get(1)->getType() != Element::string) {
-            isc_throw(CCSessionError, "Rcode in answer message is non-zero, but other argument is not a StringElement");
         }
         rcode = result->get(0)->intValue();
         if (result->size() > 1) {
-            return result->get(1);
+            if (rcode == 0 || result->get(1)->getType() == Element::string) {
+                return result->get(1);
+            } else {
+                isc_throw(CCSessionError, "Error description in result with rcode != 0 is not a string");
+            }
         } else {
-            return ElementPtr();
+            if (rcode == 0) {
+                return ElementPtr();
+            } else {
+                isc_throw(CCSessionError, "Result with rcode != 0 does not have an error description");
+            }
         }
+    } else {
+        isc_throw(CCSessionError, "No result part in answer message");
     }
 }
 
 ElementPtr
+createCommand(const std::string& command)
+{
+    return createCommand(command, ElementPtr());
+}
+
+ElementPtr
 createCommand(const std::string& command, ElementPtr arg)
 {
     ElementPtr cmd = Element::createFromString("{}");
@@ -124,7 +142,8 @@ createCommand(const std::string& command, ElementPtr arg)
 const std::string
 parseCommand(ElementPtr& arg, const ElementPtr command)
 {
-    if (command->getType() == Element::map &&
+    if (command &&
+        command->getType() == Element::map &&
         command->contains("command")) {
         ElementPtr cmd = command->get("command");
         if (cmd->getType() == Element::list &&
@@ -136,10 +155,12 @@ parseCommand(ElementPtr& arg, const ElementPtr command)
                 arg = ElementPtr();
             }
             return cmd->get(0)->stringValue();
+        } else {
+            isc_throw(CCSessionError, "Command part in command message missing, empty, or not a list");
         }
+    } else {
+        isc_throw(CCSessionError, "Command Element empty or not a map with \"command\"");
     }
-    arg = ElementPtr();
-    return "";
 }
 
 ModuleSpec
@@ -302,29 +323,33 @@ ModuleCCSession::checkCommand()
         
         /* ignore result messages (in case we're out of sync, to prevent
          * pingpongs */
-        if (!data->getType() == Element::map || data->contains("result")) {
+        if (data->getType() != Element::map || data->contains("result")) {
             return 0;
         }
         ElementPtr arg;
-        std::string cmd_str = parseCommand(arg, data);
         ElementPtr answer;
-        if (cmd_str == "config_update") {
-            std::string target_module = routing->get("group")->stringValue();
-            if (target_module == module_name_) {
-                answer = handleConfigUpdate(arg);
-            } else {
-                // ok this update is not for us, if we have this module
-                // in our remote config list, update that
-                updateRemoteConfig(target_module, arg);
-                // we're not supposed to answer to this, so return
-                return 0;
-            }
-        } else {
-            if (command_handler_) {
-                answer = command_handler_(cmd_str, arg);
+        try {
+            std::string cmd_str = parseCommand(arg, data);
+            if (cmd_str == "config_update") {
+                std::string target_module = routing->get("group")->stringValue();
+                if (target_module == module_name_) {
+                    answer = handleConfigUpdate(arg);
+                } else {
+                    // ok this update is not for us, if we have this module
+                    // in our remote config list, update that
+                    updateRemoteConfig(target_module, arg);
+                    // we're not supposed to answer to this, so return
+                    return 0;
+                }
             } else {
-                answer = createAnswer(1, "Command given but no command handler for module");
+                if (command_handler_) {
+                    answer = command_handler_(cmd_str, arg);
+                } else {
+                    answer = createAnswer(1, "Command given but no command handler for module");
+                }
             }
+        } catch (CCSessionError re) {
+            answer = createAnswer(1, re.what());
         }
         session_.reply(routing, answer);
     }

src/lib/config/ccsession.h

@@ -33,12 +33,72 @@ class io_service;
 namespace isc {
 namespace config {
 
-ElementPtr createAnswer(const int rcode);
+///
+/// \brief Creates a standard config/command level success answer message
+///        (i.e. of the form { "result": [ 0 ] }
+/// \return Standard command/config success answer message
+ElementPtr createAnswer();
+
+///
+/// \brief Creates a standard config/command level answer message
+///        (i.e. of the form { "result": [ rcode, arg ] }
+/// If rcode != 0, arg must be a StringElement
+///
+/// \param rcode The return code (0 for success)
+/// \param arg For rcode == 0, this is an optional argument of any
+///            Element type. For rcode == 1, this argument is mandatory,
+///            and must be a StringElement containing an error description
+/// \return Standard command/config answer message
 ElementPtr createAnswer(const int rcode, const ElementPtr arg);
+
+///
+/// \brief Creates a standard config/command level answer message
+/// (i.e. of the form { "result": [ rcode, arg ] }
+///
+/// \param rcode The return code (0 for success)
+/// \param arg A string to put into the StringElement argument
+/// \return Standard command/config answer message
 ElementPtr createAnswer(const int rcode, const std::string& arg);
+
+///
+/// Parses a standard config/command level answer message
+/// 
+/// \param rcode This value will be set to the return code contained in
+///              the message
+/// \param msg The message to parse
+/// \return The optional argument in the message, or an empty ElementPtr
+///         if there was no argument. If rcode != 0, this contains a
+///         StringElement with the error description.
 ElementPtr parseAnswer(int &rcode, const ElementPtr msg);
 
+
+///
+/// \brief Creates a standard config/command command message with no
+/// argument (of the form { "command": [ "my_command" ] }
+/// 
+/// \param command The command string
+/// \return The created message
+ElementPtr createCommand(const std::string& command);
+
+///
+/// \brief Creates a standard config/command command message with the
+/// given argument (of the form { "command": [ "my_command", arg ] }
+/// 
+/// \param command The command string
+/// \param arg The optional argument for the command. This can be of 
+///        any Element type, but it should conform to the .spec file.
+/// \return The created message
 ElementPtr createCommand(const std::string& command, ElementPtr arg);
+
+///
+/// \brief Parses the given command into a string containing the actual
+///        command and an ElementPtr containing the optional argument.
+///
+/// \param arg This value will be set to the ElementPtr pointing to
+///        the argument, or to an empty ElementPtr if there was none.
+/// \param command The command message containing the command (as made
+///        by createCommand()
+/// \return The command string
 const std::string parseCommand(ElementPtr& arg, const ElementPtr command);
 
 

src/lib/config/tests/ccsession_unittests.cc

@@ -49,7 +49,7 @@ initFakeSession()
     initial_messages = el("[]");
     msg_queue = el("[]");
     subscriptions = el("[]");
-    initial_messages->add(createAnswer(0));
+    initial_messages->add(createAnswer());
 }
 
 void
@@ -63,14 +63,13 @@ endFakeSession()
 TEST(CCSession, createAnswer)
 {
     ElementPtr answer;
-    // todo: this should fail (actually int rcode should probably be removed)
-    answer = createAnswer(1);
-    answer = createAnswer(0);
+    answer = createAnswer();
     EXPECT_EQ("{\"result\": [ 0 ]}", answer->str());
     answer = createAnswer(1, "error");
     EXPECT_EQ("{\"result\": [ 1, \"error\" ]}", answer->str());
-    // todo: this should fail
-    answer = createAnswer(0, "error");
+
+    EXPECT_THROW(createAnswer(1, ElementPtr()), CCSessionError);
+    EXPECT_THROW(createAnswer(1, Element::create(1)), CCSessionError);
 
     ElementPtr arg = el("[ \"just\", \"some\", \"data\" ]");
     answer = createAnswer(0, arg);
@@ -83,20 +82,21 @@ TEST(CCSession, parseAnswer)
     ElementPtr arg;
     int rcode;
 
-    // todo: these should throw CCSessionError
-    //answer = el("1");
-    //arg = parseAnswer(rcode, ElementPtr());
-    //arg = parseAnswer(rcode, answer);
-    // etc
+    EXPECT_THROW(parseAnswer(rcode, ElementPtr()), CCSessionError);
+    EXPECT_THROW(parseAnswer(rcode, el("1")), CCSessionError);
+    EXPECT_THROW(parseAnswer(rcode, el("[]")), CCSessionError);
+    EXPECT_THROW(parseAnswer(rcode, el("{}")), CCSessionError);
+    EXPECT_THROW(parseAnswer(rcode, el("{ \"something\": 1 }")), CCSessionError);
+    EXPECT_THROW(parseAnswer(rcode, el("{ \"result\": 0 }")), CCSessionError);
+    EXPECT_THROW(parseAnswer(rcode, el("{ \"result\": 1 }")), CCSessionError);
+    EXPECT_THROW(parseAnswer(rcode, el("{ \"result\": [ 1 ]}")), CCSessionError);
+    EXPECT_THROW(parseAnswer(rcode, el("{ \"result\": [ 1, 1 ]}")), CCSessionError);
     
     answer = el("{\"result\": [ 0 ]}");
     arg = parseAnswer(rcode, answer);
     EXPECT_EQ(0, rcode);
     EXPECT_TRUE(isNull(arg));
 
-    // todo: this should throw
-    answer = el("{\"result\": [ 1 ]}");
-
     answer = el("{\"result\": [ 1, \"error\"]}");
     arg = parseAnswer(rcode, answer);
     EXPECT_EQ(1, rcode);
@@ -113,9 +113,8 @@ TEST(CCSession, createCommand)
     ElementPtr command;
     ElementPtr arg;
 
-    // todo: add overload for no arg
-    //command = createCommand("my_command");
-    //ASSERT_EQ("{\"command\": [ \"my_command\" ]", command->str());
+    command = createCommand("my_command");
+    ASSERT_EQ("{\"command\": [ \"my_command\" ]}", command->str());
 
     arg = el("1");
     command = createCommand("my_command", arg);
@@ -136,14 +135,17 @@ TEST(CCSession, parseCommand)
     std::string cmd;
 
     // should throw
-    //parseCommand(arg, ElementPtr());
-    parseCommand(arg, el("1"));
-    parseCommand(arg, el("{}"));
-    parseCommand(arg, el("{\"not a command\": 1 }"));
-    parseCommand(arg, el("{\"command\": 1 }"));
-    parseCommand(arg, el("{\"command\": [] }"));
-    parseCommand(arg, el("{\"command\": [ 1 ] }"));
-    parseCommand(arg, el("{\"command\": [ \"command\" ] }"));
+    EXPECT_THROW(parseCommand(arg, ElementPtr()), CCSessionError);
+    EXPECT_THROW(parseCommand(arg, el("1")), CCSessionError);
+    EXPECT_THROW(parseCommand(arg, el("{}")), CCSessionError);
+    EXPECT_THROW(parseCommand(arg, el("{\"not a command\": 1 }")), CCSessionError);
+    EXPECT_THROW(parseCommand(arg, el("{\"command\": 1 }")), CCSessionError);
+    EXPECT_THROW(parseCommand(arg, el("{\"command\": [] }")), CCSessionError);
+    EXPECT_THROW(parseCommand(arg, el("{\"command\": [ 1 ] }")), CCSessionError);
+
+    cmd = parseCommand(arg, el("{\"command\": [ \"my_command\" ] }"));
+    EXPECT_EQ("my_command", cmd);
+    EXPECT_TRUE(isNull(arg));
 
     cmd = parseCommand(arg, el("{\"command\": [ \"my_command\", 1 ] }"));
     EXPECT_EQ("my_command", cmd);
@@ -197,13 +199,13 @@ ElementPtr my_config_handler(ElementPtr new_config)
         new_config->get("item1")->intValue() == 5) {
         return createAnswer(6, "I do not like the number 5");
     }
-    return createAnswer(0);
+    return createAnswer();
 }
 
 ElementPtr my_command_handler(const std::string& command, ElementPtr arg UNUSED_PARAM)
 {
     if (command == "good_command") {
-        return createAnswer(0);
+        return createAnswer();
     } else if (command == "command_with_arg") {
         if (arg) {
             if (arg->getType() == Element::integer) {
@@ -266,10 +268,10 @@ TEST(CCSession, checkCommand)
     result = mccs.checkCommand();
     EXPECT_EQ(0, result);
 
-    // todo: type check in message handling in checkCommand
-    //addMessage(el("1"), "Spec2", "*");
-    //result = mccs.checkCommand();
-    //EXPECT_EQ(0, result);
+    // not a command, should be ignored
+    addMessage(el("1"), "Spec2", "*");
+    result = mccs.checkCommand();
+    EXPECT_EQ(0, result);
 
     addMessage(el("{ \"command\": [ \"good_command\" ] }"), "Spec2", "*");
     result = mccs.checkCommand();
@@ -282,6 +284,13 @@ TEST(CCSession, checkCommand)
     result = mccs.checkCommand();
     EXPECT_EQ(1, msg_queue->size());
     msg = getFirstMessage(group, to);
+    EXPECT_EQ("{\"result\": [ 1, \"Command part in command message missing, empty, or not a list\" ]}", msg->str());
+    EXPECT_EQ(0, result);
+
+    addMessage(el("{ \"command\": [ \"bad_command\" ] }"), "Spec2", "*");
+    result = mccs.checkCommand();
+    EXPECT_EQ(1, msg_queue->size());
+    msg = getFirstMessage(group, to);
     EXPECT_EQ("{\"result\": [ 1, \"bad command\" ]}", msg->str());
     EXPECT_EQ(0, result);