[2522] MINFO & RP review updates

src/lib/dns/rdata/generic/minfo_14.cc

@@ -31,13 +31,6 @@ using isc::dns::rdata::generic::detail::createNameFromLexer;
 // BEGIN_ISC_NAMESPACE
 // BEGIN_RDATA_NAMESPACE
 
-// helper function for string and lexer constructors
-void
-MINFO::constructFromLexer(MasterLexer& lexer, const Name* origin) {
-    rmailbox_ = createNameFromLexer(lexer, origin);
-    emailbox_ = createNameFromLexer(lexer, origin);
-}
-
 /// \brief Constructor from string.
 ///
 /// \c minfo_str must be formatted as follows:
@@ -64,7 +57,8 @@ MINFO::MINFO(const std::string& minfo_str) :
         MasterLexer lexer;
         lexer.pushSource(ss);
 
-        constructFromLexer(lexer, NULL);
+	rmailbox_ = createNameFromLexer(lexer, NULL);
+	emailbox_ = createNameFromLexer(lexer, NULL);
 
         if (lexer.getNextToken().getType() != MasterToken::END_OF_FILE) {
             isc_throw(InvalidRdataText, "extra input text for MINFO: "
@@ -92,10 +86,10 @@ MINFO::MINFO(const std::string& minfo_str) :
 /// \param origin If non NULL, specifies the origin of SERVER when it
 /// is non-absolute.
 MINFO::MINFO(MasterLexer& lexer, const Name* origin,
-       MasterLoader::Options, MasterLoaderCallbacks&) :
-    rmailbox_(Name::ROOT_NAME()), emailbox_(Name::ROOT_NAME())
+             MasterLoader::Options, MasterLoaderCallbacks&) :
+    rmailbox_(createNameFromLexer(lexer, origin)),
+    emailbox_(createNameFromLexer(lexer, origin))
 {
-    constructFromLexer(lexer, origin);
 }
 
 /// \brief Constructor from wire-format data.

src/lib/dns/rdata/generic/minfo_14.h

@@ -69,9 +69,6 @@ public:
     Name getEmailbox() const { return (emailbox_); }
 
 private:
-    // helper function for string and lexer constructors
-    void constructFromLexer(MasterLexer& lexer, const Name* origin);
-
     Name rmailbox_;
     Name emailbox_;
 };

src/lib/dns/rdata/generic/rp_17.cc

@@ -31,13 +31,6 @@ using isc::dns::rdata::generic::detail::createNameFromLexer;
 // BEGIN_ISC_NAMESPACE
 // BEGIN_RDATA_NAMESPACE
 
-// helper function for string and lexer constructors
-void
-RP::constructFromLexer(MasterLexer& lexer, const Name* origin) {
-    mailbox_ = createNameFromLexer(lexer, origin);
-    text_ = createNameFromLexer(lexer, origin);
-}
-
 /// \brief Constructor from string.
 ///
 /// \c rp_str must be formatted as follows:
@@ -61,7 +54,8 @@ RP::RP(const std::string& rp_str) :
         MasterLexer lexer;
         lexer.pushSource(ss);
 
-        constructFromLexer(lexer, NULL);
+        mailbox_ = createNameFromLexer(lexer, NULL);
+        text_ = createNameFromLexer(lexer, NULL);
 
         if (lexer.getNextToken().getType() != MasterToken::END_OF_FILE) {
             isc_throw(InvalidRdataText, "extra input text for RP: "
@@ -89,9 +83,9 @@ RP::RP(const std::string& rp_str) :
 /// is non-absolute.
 RP::RP(MasterLexer& lexer, const Name* origin,
        MasterLoader::Options, MasterLoaderCallbacks&) :
-    mailbox_(Name::ROOT_NAME()), text_(Name::ROOT_NAME())
+    mailbox_(createNameFromLexer(lexer, origin)),
+    text_(createNameFromLexer(lexer, origin))
 {
-    constructFromLexer(lexer, origin);
 }
 
 /// \brief Constructor from wire-format data.

src/lib/dns/rdata/generic/rp_17.h

@@ -73,9 +73,6 @@ public:
     Name getText() const { return (text_); }
 
 private:
-    // helper function for string and lexer constructors
-    void constructFromLexer(MasterLexer& lexer, const Name* origin);
-
     Name mailbox_;
     Name text_;
 };

src/lib/dns/tests/rdata_minfo_unittest.cc

@@ -12,6 +12,8 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#include <string>
+
 #include <util/buffer.h>
 #include <dns/exceptions.h>
 #include <dns/messagerenderer.h>
@@ -31,17 +33,18 @@ using namespace isc::util;
 using namespace isc::dns;
 using namespace isc::dns::rdata;
 
-// minfo text
-const char* const minfo_txt = "rmailbox.example.com. emailbox.example.com.";
-const char* const minfo_txt2 = "root.example.com. emailbox.example.com.";
-const char* const too_long_label = "01234567890123456789012345678901234567"
-                                   "89012345678901234567890123.";
-
 namespace {
+const string too_long_label =
+    "0123456789012345678901234567890123456789012345678901234567890123.";
+
 class Rdata_MINFO_Test : public RdataTest {
 protected:
     Rdata_MINFO_Test():
-        rdata_minfo(string(minfo_txt)), rdata_minfo2(string(minfo_txt2)) {}
+        minfo_txt("rmailbox.example.com. emailbox.example.com."),
+        minfo_txt2("root.example.com. emailbox.example.com."),
+        rdata_minfo(minfo_txt),
+        rdata_minfo2(minfo_txt2)
+    {}
 
     void checkFromText_None(const string& rdata_str) {
         checkFromText<generic::MINFO, isc::Exception, isc::Exception>(
@@ -51,7 +54,7 @@ protected:
     void checkFromText_LexerError(const string& rdata_str) {
         checkFromText
             <generic::MINFO, InvalidRdataText, MasterLexer::LexerError>(
-                rdata_str, rdata_minfo, true, true);
+            rdata_str, rdata_minfo, true, true);
     }
 
     void checkFromText_TooLongLabel(const string& rdata_str) {
@@ -59,6 +62,29 @@ protected:
             rdata_str, rdata_minfo, true, true);
     }
 
+    void checkFromText_BadString(const string& rdata_str) {
+        checkFromText<generic::MINFO, InvalidRdataText, isc::Exception>(
+            rdata_str, rdata_minfo, true, false);
+    }
+
+    void checkFromText_EmptyLabel(const string& rdata_str) {
+        checkFromText<generic::MINFO, EmptyLabel, EmptyLabel>(
+            rdata_str, rdata_minfo, true, true);
+    }
+
+    void checkFromText_MissingOrigin(const string& rdata_str) {
+        checkFromText
+            <generic::MINFO, MissingNameOrigin, MissingNameOrigin>(
+                rdata_str, rdata_minfo, true, true);
+    }
+
+    void checkFromText_Origin(const string& rdata_str, const Name* origin) {
+        checkFromText<generic::MINFO, MissingNameOrigin, isc::Exception>(
+            rdata_str, rdata_minfo, true, false, origin);
+    }
+
+    const string minfo_txt;
+    const string minfo_txt2;
     const generic::MINFO rdata_minfo;
     const generic::MINFO rdata_minfo2;
 };
@@ -72,21 +98,32 @@ TEST_F(Rdata_MINFO_Test, createFromText) {
     EXPECT_EQ(Name("emailbox.example.com."), rdata_minfo2.getEmailbox());
 
     checkFromText_None(minfo_txt);
+
+    // origin defined for lexer constructor, but not string constructor
+    Name origin("example.com");
+    checkFromText_Origin("rmailbox emailbox", &origin);
+
+    // lexer constructor accepts extra text, but string constructor doesn't
+    checkFromText_BadString("rmailbox.example.com. emailbox.example.com. "
+                            "extra.example.com.");
 }
 
 TEST_F(Rdata_MINFO_Test, badText) {
-    // incomplete text
-    checkFromText_LexerError("root.example.com.");
-    // number of fields (must be 2) is incorrect
-    EXPECT_THROW(generic::MINFO("root.example.com. emailbox.example.com. "
-                                "example.com."),
-                 InvalidRdataText);
-    // bad rmailbox name
-    checkFromText_TooLongLabel("root.example.com. emailbox.example.com." +
-                               string(too_long_label));
-    // bad emailbox name
-    checkFromText_TooLongLabel("root.example.com."  + string(too_long_label) +
+    // too long names
+    checkFromText_TooLongLabel("root.example.com."  + too_long_label +
                                " emailbox.example.com.");
+    checkFromText_TooLongLabel("root.example.com. emailbox.example.com." +
+                               too_long_label);
+
+    // invalid names
+    checkFromText_EmptyLabel("root..example.com. emailbox.example.com.");
+    checkFromText_EmptyLabel("root.example.com. emailbox..example.com.");
+
+    // missing name
+    checkFromText_LexerError("root.example.com.");
+
+    // missing origin
+    checkFromText_MissingOrigin("root.example.com emailbox.example.com");
 }
 
 TEST_F(Rdata_MINFO_Test, createFromWire) {
@@ -118,21 +155,6 @@ TEST_F(Rdata_MINFO_Test, createFromWire) {
                  DNSMessageFORMERR);
 }
 
-TEST_F(Rdata_MINFO_Test, createFromLexer) {
-    EXPECT_EQ(0, rdata_minfo.compare(
-        *test::createRdataUsingLexer(RRType::MINFO(), RRClass::IN(),
-                                     minfo_txt)));
-
-    // test::createRdataUsingLexer() constructs relative to
-    // "example.org." origin.
-    EXPECT_EQ(0, generic::MINFO("r.example.org. e.example.org.").compare(
-        *test::createRdataUsingLexer(RRType::MINFO(), RRClass::IN(), "r e")));
-
-    // Extra text at end of line
-    EXPECT_FALSE(test::createRdataUsingLexer(RRType::MINFO(), RRClass::IN(),
-             "rmailbox.example.com. emailbox.example.com. extra.example.com."));
-}
-
 TEST_F(Rdata_MINFO_Test, assignment) {
     generic::MINFO copy((string(minfo_txt2)));
     copy = rdata_minfo;

src/lib/dns/tests/rdata_rp_unittest.cc

@@ -41,21 +41,36 @@ protected:
         obuffer(0)
     {}
 
+    void checkFromText_None(const string& rdata_str) {
+        checkFromText<generic::RP, isc::Exception, isc::Exception>(
+            rdata_str, rdata_rp, false, false);
+    }
+
     void checkFromText_LexerError(const string& rdata_str) {
         checkFromText
             <generic::RP, InvalidRdataText, MasterLexer::LexerError>(
                 rdata_str, rdata_rp, true, true);
     }
 
+    void checkFromText_BadString(const string& rdata_str) {
+        checkFromText<generic::RP, InvalidRdataText, isc::Exception>(
+            rdata_str, rdata_rp, true, false);
+    }
+
+    void checkFromText_EmptyLabel(const string& rdata_str) {
+        checkFromText<generic::RP, EmptyLabel, EmptyLabel>(
+            rdata_str, rdata_rp, true, true);
+    }
+
     void checkFromText_MissingOrigin(const string& rdata_str) {
         checkFromText
             <generic::RP, MissingNameOrigin, MissingNameOrigin>(
                 rdata_str, rdata_rp, true, true);
     }
 
-    void checkFromText_EmptyLabel(const string& rdata_str) {
-        checkFromText<generic::RP, EmptyLabel, EmptyLabel>(
-            rdata_str, rdata_rp, true, true);
+    void checkFromText_Origin(const string& rdata_str, const Name* origin) {
+        checkFromText<generic::RP, MissingNameOrigin, isc::Exception>(
+            rdata_str, rdata_rp, true, false, origin);
     }
 
     const Name mailbox_name, text_name;
@@ -69,18 +84,27 @@ TEST_F(Rdata_RP_Test, createFromText) {
     EXPECT_EQ(mailbox_name, rdata_rp.getMailbox());
     EXPECT_EQ(text_name, rdata_rp.getText());
 
-    // Invalid textual input cases follow:
-    // names are invalid
-    checkFromText_EmptyLabel("bad..name. rp-text.example.com.");
-    checkFromText_EmptyLabel("root.example.com. bad..name.");
-    checkFromText_MissingOrigin("root.example.com rp-text.example.com");
+    checkFromText_None("root.example.com. rp-text.example.com.");
+
+    // origin defined for lexer constructor, but not string constructor
+    Name origin("example.com");
+    checkFromText_Origin("root rp-text", &origin);
+
+    // lexer constructor accepts extra text, but string constructor doesn't
+    checkFromText_BadString("root.example.com. rp-text.example.com. "
+                            "extra.example.com.");
+}
+
+TEST_F(Rdata_RP_Test, badText) {
+    // invalid names
+    checkFromText_EmptyLabel("root..example.com. rp-text.example.com.");
+    checkFromText_EmptyLabel("root.example.com. rp-text..example.com.");
 
     // missing field
     checkFromText_LexerError("root.example.com.");
 
-    // redundant field
-    EXPECT_THROW(generic::RP("root.example.com. rp-text.example.com. "
-                             "redundant.example."), InvalidRdataText);
+    // missing origin
+    checkFromText_MissingOrigin("root.example.com rp-text.example.com");
 }
 
 TEST_F(Rdata_RP_Test, createFromWire) {
@@ -124,29 +148,6 @@ TEST_F(Rdata_RP_Test, createFromParams) {
     EXPECT_EQ(text_name, generic::RP(mailbox_name, text_name).getText());
 }
 
-TEST_F(Rdata_RP_Test, createFromLexer) {
-    EXPECT_EQ(0, rdata_rp.compare(
-        *test::createRdataUsingLexer(RRType::RP(), RRClass::IN(),
-                                     "root.example.com. "
-                                     "rp-text.example.com.")));
-
-    // test::createRdataUsingLexer() constructs relative to
-    // "example.org." origin.
-    EXPECT_EQ(0, generic::RP("root.example.org. rp-text.example.org.").compare(
-        *test::createRdataUsingLexer(RRType::RP(), RRClass::IN(),
-                                     "root rp-text")));
-
-    // Exceptions cause NULL to be returned.
-    EXPECT_FALSE(test::createRdataUsingLexer(RRType::RP(), RRClass::IN(),
-                                             "root.example.com."));
-
-    // Extra text at end of line
-    EXPECT_FALSE(test::createRdataUsingLexer(RRType::RP(), RRClass::IN(),
-                                                "root.example.com. "
-                                                "rp-text.example.com. "
-                                                "redundant.example.com."));
-}
-
 TEST_F(Rdata_RP_Test, toWireBuffer) {
     // construct expected data
     UnitTestUtil::readWireData("rdata_rp_toWire1.wire", expected_wire);

src/lib/dns/tests/rdata_sshfp_unittest.cc

@@ -123,7 +123,7 @@ TEST_F(Rdata_SSHFP_Test, badText) {
     checkFromText_LexerError("1");
     checkFromText_LexerError("ONE 2 123456789abcdef67890123456789abcdef67890");
     checkFromText_LexerError("1 TWO 123456789abcdef67890123456789abcdef67890");
-    checkFromText_BadValue("1 2 BUCKLEMYSHOES");
+    checkFromText_BadValue("1 2 BUCKLEMYSHOE");
     checkFromText_BadString(sshfp_txt + " extra text");
 }
 
@@ -181,7 +181,7 @@ TEST_F(Rdata_SSHFP_Test, createFromWire) {
                  InvalidBufferPosition);
 }
 
-TEST_F(Rdata_SSHFP_Test, createFromComponents) {
+TEST_F(Rdata_SSHFP_Test, createFromParams) {
     const generic::SSHFP rdata_sshfp2(2, 1, "123456789abcdef67890123456789abcdef67890");
     EXPECT_EQ(0, rdata_sshfp2.compare(rdata_sshfp));
 }