[1605] Other changes as result of review

src/lib/datasrc/rbnode_rrset.h

@@ -42,11 +42,11 @@ namespace internal {
 /// - Calls to methods that change attributes of the underlying RRset (such as
 ///   TTL or Name) cause an exception to be thrown.  The in-memory data source
 ///   does not allow modification of these attributes.
-/// - Calls that modify the associated RRSIGs of the RRset are allowed (even
-///   though the pointer is to a "const" object).  The reason here is because
-///   RRSIGs are added to the in-memory data source after the RBNodeRRset
-///   objects have been created.  Thus there has to be the capability of
-///   modifying this information.
+/// - Calls that add the pointer to the associated RRSIG to the RRset are
+///   allowed (even though the pointer is to a "const" RRset).  The reason here
+///   is that RRSIGs are added to the in-memory data source after the
+///   RBNodeRRset objects have been created.  Thus there has to be the
+///   capability of modifying this information.
 ///
 /// The class is not derived from RRset itself to simplify coding: part of the
 /// loading of the memory data source is handled in the BIND 10 "libdns++"
@@ -61,8 +61,8 @@ class RBNodeRRset : public isc::dns::AbstractRRset {
 private:
     // Note: The copy constructor and the assignment operator are intentionally
     // defined as private as we would normally not duplicate a RBNodeRRset.
-    // (We use the "private" method instead of inheriting from boost::noncopyable
-    // so as to avoid multiple inheritance.)
+    // (We use the "private" method instead of inheriting from
+    // boost::noncopyable so as to avoid multiple inheritance.)
     RBNodeRRset(const RBNodeRRset& source);
     RBNodeRRset& operator=(const RBNodeRRset& source);
 
@@ -72,7 +72,8 @@ public:
     /// Creates an RBNodeRRset from the pointer to the RRset passed to it.
     ///
     /// \param rrset Pointer to underlying RRset encapsulated by this object.
-    RBNodeRRset(const isc::dns::ConstRRsetPtr& rrset) : rrset_(rrset) {}
+    explicit RBNodeRRset(const isc::dns::ConstRRsetPtr& rrset) : rrset_(rrset)
+    {}
 
     /// \brief Destructor
     virtual ~RBNodeRRset() {}
@@ -181,7 +182,7 @@ public:
     /// \brief Return underlying RRset pointer
     ///
     /// ... mainly for testing.
-    virtual isc::dns::ConstRRsetPtr getUnderlyingRRset() const {
+    isc::dns::ConstRRsetPtr getUnderlyingRRset() const {
         return (rrset_);
     }
 

src/lib/datasrc/tests/memory_datasrc_unittest.cc

@@ -172,19 +172,25 @@ TEST_F(InMemoryClientTest, iterator) {
     EXPECT_EQ(result::SUCCESS, zone->add(aRRsetA));
     EXPECT_EQ(result::SUCCESS, zone->add(aRRsetAAAA));
     EXPECT_EQ(result::SUCCESS, zone->add(subRRsetA));
-    // Check it with full zone, one by one.
-    // It should be in ascending order in case of InMemory data source
-    // (isn't guaranteed in general)
-    // Since the in-memory data source uses objects that encapsulate the
-    // RRsets stored, the iterator does not iterate over the RRsets stored -
-    // it iterates over the encapsulating objects.  This means that we cannot
-    // directly compare pointer values: instead, we compare the actual data
-    // stored.
+
+    // Check it with full zone.
+    vector<ConstRRsetPtr> expected_rrsets;
+    expected_rrsets.push_back(aRRsetA);
+    expected_rrsets.push_back(aRRsetAAAA);
+    expected_rrsets.push_back(subRRsetA);
+
     iterator = memory_client.getIterator(Name("a"));
-    EXPECT_EQ(aRRsetA->toText(), iterator->getNextRRset()->toText());
-    EXPECT_EQ(aRRsetAAAA->toText(), iterator->getNextRRset()->toText());
-    EXPECT_EQ(subRRsetA->toText(), iterator->getNextRRset()->toText());
+    vector<ConstRRsetPtr> actual_rrsets;
+    for (int i = 0; i < 3; ++i) {
+        ConstRRsetPtr actual = iterator->getNextRRset();
+        ASSERT_NE(ConstRRsetPtr(), actual);
+        actual_rrsets.push_back(actual);
+    }
     EXPECT_EQ(ConstRRsetPtr(), iterator->getNextRRset());
+
+    rrsetsCheck(expected_rrsets.begin(), expected_rrsets.end(),
+                actual_rrsets.begin(), actual_rrsets.end());
+
 }
 
 TEST_F(InMemoryClientTest, iterator_separate_rrs) {

src/lib/datasrc/tests/rbnode_rrset_unittest.cc

@@ -17,6 +17,7 @@
 #include <exceptions/exceptions.h>
 #include <dns/rdataclass.h>
 #include <datasrc/rbnode_rrset.h>
+#include <testutils/dnsmessage_test.h>
 
 #include <gtest/gtest.h>
 
@@ -24,13 +25,14 @@
 
 using isc::UnitTestUtil;
 
-using namespace std;
 using namespace isc;
 using namespace isc::datasrc;
 using namespace isc::datasrc::internal;
 using namespace isc::dns;
 using namespace isc::dns::rdata;
+using namespace isc::testutils;
 using namespace isc::util;
+using namespace std;
 
 // These tests are very similar to those for RRset - indeed, this file was
 // created from those tests.  However, the significant difference in behaviour
@@ -136,33 +138,36 @@ TEST_F(RBNodeRRsetTest, toText) {
     EXPECT_THROW(rrset_a_empty.toText(), EmptyRRset);
 }
 
-TEST_F(RBNodeRRsetTest, toWireRenderer) {
-    OutputBuffer buffer(0);
-    MessageRenderer renderer(buffer);
-    rrset_a.toWire(renderer);
+// Note: although the next two tests are essentially the same and used common
+// test code, they use different test data: the MessageRenderer produces
+// compressed wire data whereas the OutputBuffer does not.
+
+template <typename T>
+void
+performToWireTest(T& dataHolder, const RBNodeRRset& rrset,
+                  const RBNodeRRset& rrset_empty, const char* testdata)
+{
+    rrset.toWire(dataHolder);
 
     std::vector<unsigned char> wiredata;
-    UnitTestUtil::readWireData("rrset_toWire2", wiredata);
-    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData, buffer.getData(),
-                        buffer.getLength(), &wiredata[0], wiredata.size());
+    UnitTestUtil::readWireData(testdata, wiredata);
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData, dataHolder.getData(),
+                        dataHolder.getLength(), &wiredata[0], wiredata.size());
 
     // toWire() cannot be performed for an empty RRset.
-    renderer.clear();
-    EXPECT_THROW(rrset_a_empty.toWire(renderer), EmptyRRset);
+    dataHolder.clear();
+    EXPECT_THROW(rrset_empty.toWire(dataHolder), EmptyRRset);
 }
 
-TEST_F(RBNodeRRsetTest, toWireBuffer) {
+TEST_F(RBNodeRRsetTest, toWireRenderer) {
     OutputBuffer buffer(0);
-    rrset_a.toWire(buffer);
-
-    std::vector<unsigned char> wiredata;
-    UnitTestUtil::readWireData("rrset_toWire1", wiredata);
-    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData, buffer.getData(),
-                        buffer.getLength(), &wiredata[0], wiredata.size());
+    MessageRenderer renderer(buffer);
+    performToWireTest(renderer, rrset_a, rrset_a_empty, "rrset_toWire2");
+}
 
-    // toWire() cannot be performed for an empty RRset.
-    buffer.clear();
-    EXPECT_THROW(rrset_a_empty.toWire(buffer), EmptyRRset);
+TEST_F(RBNodeRRsetTest, toWireBuffer) {
+    OutputBuffer buffer(0);
+    performToWireTest(buffer, rrset_a, rrset_a_empty, "rrset_toWire1");
 }
 
 TEST_F(RBNodeRRsetTest, addRdata) {
@@ -205,24 +210,13 @@ TEST_F(RBNodeRRsetTest, LeftShiftOperator) {
               "test.example.com. 3600 IN A 192.0.2.2\n", oss.str());
 }
 
-// General RRSIG check function.  Get the RRSIG from the RRset and check that
-// the RDATA is what we expect.
-void
-checkSignature(const RBNodeRRset& rrset) {
-    RRsetPtr rrsig = rrset.getRRsig();
-    ASSERT_TRUE(rrsig);
-    RdataIteratorPtr it = rrsig->getRdataIterator();
-    EXPECT_EQ(RRSIG_TXT, it->getCurrent().toText());
-}
-
 // addRRSIG tests.
 TEST_F(RBNodeRRsetTest, addRRsigConstRdataPointer) {
     EXPECT_FALSE(rrset_a.getRRsig());
-    RdataPtr data = createRdata(rrset_siga->getType(), rrset_siga->getClass(),
-                                RRSIG_TXT);
-    ConstRdataPtr cdata(data);
-    rrset_a.addRRsig(cdata);
-    checkSignature(rrset_a);
+    ConstRdataPtr data = createRdata(rrset_siga->getType(),
+                                     rrset_siga->getClass(), RRSIG_TXT);
+    rrset_a.addRRsig(data);
+    rrsetCheck(rrset_siga, rrset_a.getUnderlyingRRset()->getRRsig());
 }
 
 TEST_F(RBNodeRRsetTest, addRRsigRdataPointer) {
@@ -230,19 +224,19 @@ TEST_F(RBNodeRRsetTest, addRRsigRdataPointer) {
     RdataPtr data = createRdata(rrset_siga->getType(), rrset_siga->getClass(),
                                 RRSIG_TXT);
     rrset_a.addRRsig(data);
-    checkSignature(rrset_a);
+    rrsetCheck(rrset_siga, rrset_a.getUnderlyingRRset()->getRRsig());
 }
 
 TEST_F(RBNodeRRsetTest, addRRsigAbstractRRset) {
     EXPECT_FALSE(rrset_a.getRRsig());
     rrset_a.addRRsig(*(rrset_siga.get()));
-    checkSignature(rrset_a);
+    rrsetCheck(rrset_siga, rrset_a.getUnderlyingRRset()->getRRsig());
 }
 
 TEST_F(RBNodeRRsetTest, addRRsigConstantRRsetPointer) {
     EXPECT_FALSE(rrset_a.getRRsig());
     rrset_a.addRRsig(rrset_siga);
-    checkSignature(rrset_a);
+    rrsetCheck(rrset_siga, rrset_a.getUnderlyingRRset()->getRRsig());
 }
 
 TEST_F(RBNodeRRsetTest, addRRsigRRsetPointer) {
@@ -251,7 +245,7 @@ TEST_F(RBNodeRRsetTest, addRRsigRRsetPointer) {
                    RRTTL(3600)));
     rrsig->addRdata(generic::RRSIG(RRSIG_TXT));
     rrset_a.addRRsig(rrsig);
-    checkSignature(rrset_a);
+    rrsetCheck(rrset_siga, rrset_a.getUnderlyingRRset()->getRRsig());
 }
 
 TEST_F(RBNodeRRsetTest, removeRRsig) {