[2441] Update in-memory data source so it can load RRs in any order

The ZoneDataLoader can still be simplified to remove the vectors
in it (as the RdataSet now merges for us). This will be done
in a future commit.

* In some tests, the RdataSet list sequence changed (as there's no
  order enforced for this data). Tests were adjusted for it.
* Some test data was adjusted to add missing NS records.
* Some tests now pass and don't throw exceptions as duplicate
  records for a type are now accepted.

src/lib/datasrc/memory/zone_data_loader.cc

@@ -77,8 +77,7 @@ public:
     void flushNodeRRsets();
 
 private:
-    typedef std::map<isc::dns::RRType, isc::dns::ConstRRsetPtr> NodeRRsets;
-    typedef NodeRRsets::value_type NodeRRsetsVal;
+    typedef std::vector<isc::dns::ConstRRsetPtr> NodeRRsets;
 
     // A helper to identify the covered type of an RRSIG.
     const isc::dns::Name& getCurrentName() const;
@@ -103,13 +102,9 @@ ZoneDataLoader::addFromLoad(const ConstRRsetPtr& rrset) {
     // once, so we check the "duplicate" here.
     const bool is_rrsig = rrset->getType() == RRType::RRSIG();
     NodeRRsets& node_rrsets = is_rrsig ? node_rrsigsets_ : node_rrsets_;
-    const RRType& rrtype = is_rrsig ? getCoveredType(rrset) : rrset->getType();
-    if (!node_rrsets.insert(NodeRRsetsVal(rrtype, rrset)).second) {
-        isc_throw(ZoneDataUpdater::AddError,
-                  "Duplicate add of the same type of"
-                  << (is_rrsig ? " RRSIG" : "") << " RRset: "
-                  << rrset->getName() << "/" << rrtype);
-    }
+
+    // Store this RRset until it can be added to the zone.
+    node_rrsets.insert(node_rrsets.begin(), rrset);
 
     if (rrset->getRRsig()) {
         addFromLoad(rrset->getRRsig());
@@ -118,23 +113,12 @@ ZoneDataLoader::addFromLoad(const ConstRRsetPtr& rrset) {
 
 void
 ZoneDataLoader::flushNodeRRsets() {
-    BOOST_FOREACH(NodeRRsetsVal val, node_rrsets_) {
-        // Identify the corresponding RRSIG for the RRset, if any.  If
-        // found add both the RRset and its RRSIG at once.
-        ConstRRsetPtr sig_rrset;
-        NodeRRsets::iterator sig_it = node_rrsigsets_.find(val.first);
-        if (sig_it != node_rrsigsets_.end()) {
-            sig_rrset = sig_it->second;
-            node_rrsigsets_.erase(sig_it);
-        }
-        updater_.add(val.second, sig_rrset);
+    BOOST_FOREACH(ConstRRsetPtr rrset, node_rrsets_) {
+        updater_.add(rrset, ConstRRsetPtr());
     }
 
-    // Normally rrsigsets map should be empty at this point, but it's still
-    // possible that an RRSIG that don't has covered RRset is added; they
-    // still remain in the map.  We add them to the zone separately.
-    BOOST_FOREACH(NodeRRsetsVal val, node_rrsigsets_) {
-        updater_.add(ConstRRsetPtr(), val.second);
+    BOOST_FOREACH(ConstRRsetPtr rrset, node_rrsigsets_) {
+        updater_.add(ConstRRsetPtr(), rrset);
     }
 
     node_rrsets_.clear();
@@ -144,10 +128,10 @@ ZoneDataLoader::flushNodeRRsets() {
 const Name&
 ZoneDataLoader::getCurrentName() const {
     if (!node_rrsets_.empty()) {
-        return (node_rrsets_.begin()->second->getName());
+        return (node_rrsets_.front()->getName());
     }
     assert(!node_rrsigsets_.empty());
-    return (node_rrsigsets_.begin()->second->getName());
+    return (node_rrsigsets_.front()->getName());
 }
 
 void

src/lib/datasrc/memory/zone_data_updater.cc

@@ -270,8 +270,12 @@ ZoneDataUpdater::addNSEC3(const Name& name, const ConstRRsetPtr rrset,
     ZoneNode* node;
     nsec3_data->insertName(mem_sgmt_, name, &node);
 
-    RdataSet* rdataset = RdataSet::create(mem_sgmt_, encoder_, rrset, rrsig);
-    RdataSet* old_rdataset = node->setData(rdataset);
+    // Create a new RdataSet, merging any existing existing NSEC3 data
+    // for this name.
+    RdataSet* old_rdataset = node->getData();
+    RdataSet* rdataset = RdataSet::create(mem_sgmt_, encoder_, rrset, rrsig,
+                                          old_rdataset);
+    old_rdataset = node->setData(rdataset);
     if (old_rdataset != NULL) {
         RdataSet::destroy(mem_sgmt_, old_rdataset, rrclass_);
     }
@@ -298,16 +302,34 @@ ZoneDataUpdater::addRdataSet(const Name& name, const RRType& rrtype,
             contextCheck(*rrset, rdataset_head);
         }
 
-        if (RdataSet::find(rdataset_head, rrtype, true) != NULL) {
-            isc_throw(AddError,
-                      "RRset of the type already exists: "
-                      << name << " (type: " << rrtype << ")");
-        }
-
+        // Create a new RdataSet, merging any existing data for this
+        // type.
+        RdataSet* old_rdataset = RdataSet::find(rdataset_head, rrtype, true);
         RdataSet* rdataset_new = RdataSet::create(mem_sgmt_, encoder_,
-                                                  rrset, rrsig);
-        rdataset_new->next = rdataset_head;
-        node->setData(rdataset_new);
+                                                  rrset, rrsig, old_rdataset);
+        if (old_rdataset == NULL) {
+            // There is no existing RdataSet. Prepend the new RdataSet
+            // to the list.
+            rdataset_new->next = rdataset_head;
+            node->setData(rdataset_new);
+        } else {
+            // Replace the old RdataSet in the list with the newly
+            // created one, and destroy the old one.
+            for (RdataSet* cur = rdataset_head, *prev = NULL;
+                 cur != NULL;
+                 prev = cur, cur = cur->getNext()) {
+                if (cur == old_rdataset) {
+                    rdataset_new->next = cur->getNext();
+                    if (prev == NULL) {
+                        node->setData(rdataset_new);
+                    } else {
+                        prev->next = rdataset_new;
+                    }
+                    break;
+                }
+            }
+            RdataSet::destroy(mem_sgmt_, old_rdataset, rrclass_);
+        }
 
         // Ok, we just put it in.
 

src/lib/datasrc/tests/memory/memory_client_unittest.cc

@@ -74,6 +74,7 @@ const char* rrset_data[] = {
 const char* rrset_data_separated[] = {
     "example.org. 3600 IN SOA ns1.example.org. bugs.x.w.example.org. "
     "68 3600 300 3600000 3600",
+    "example.org. 3600 IN NS ns1.example.org.",
     "a.example.org. 3600 IN A 192.168.0.1", // these two belong to the same
     "a.example.org. 3600 IN A 192.168.0.2", // RRset, but are separated.
     NULL
@@ -83,6 +84,7 @@ const char* rrset_data_separated[] = {
 const char* rrset_data_sigseparated[] = {
     "example.org. 3600 IN SOA ns1.example.org. bugs.x.w.example.org. "
     "68 3600 300 3600000 3600",
+    "example.org. 3600 IN NS ns1.example.org.",
     "a.example.org. 3600 IN A 192.168.0.1",
     "a.example.org. 3600 IN RRSIG A 5 3 3600 20150420235959 20051021000000 "
     "40430 example.org. FAKEFAKE",
@@ -254,20 +256,20 @@ TEST_F(MemoryClientTest, loadFromIterator) {
     // First we have the SOA
     ConstRRsetPtr rrset(iterator->getNextRRset());
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::SOA(), rrset->getType());
+    EXPECT_EQ(RRType::NS(), rrset->getType());
 
     // RRType::NS() RRset
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::NS(), rrset->getType());
+    EXPECT_EQ(RRType::SOA(), rrset->getType());
 
-    // RRType::MX() RRset
+    // RRType::A() RRset
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
     EXPECT_EQ(RRType::MX(), rrset->getType());
     EXPECT_EQ(1, rrset->getRRsigDataCount()); // this RRset is signed
 
-    // RRType::A() RRset
+    // RRType::MX() RRset
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
     EXPECT_EQ(RRType::A(), rrset->getType());
@@ -279,24 +281,22 @@ TEST_F(MemoryClientTest, loadFromIterator) {
     // Iterating past the end should result in an exception
     EXPECT_THROW(iterator->getNextRRset(), isc::Unexpected);
 
-    // Loading the zone with an iterator separating RRs of the same RRset
-    // will fail because the resulting sequence doesn't meet assumptions of
-    // the (current) in-memory implementation.
-    EXPECT_THROW(client_->load(Name("example.org"),
-                               *MockIterator::makeIterator(
-                                   rrset_data_separated)),
-                 ZoneDataUpdater::AddError);
+    // Loading the zone with an iterator separating RRs of the same
+    // RRset should not fail. It is acceptable to load RRs of the same
+    // type again.
+    client_->load(Name("example.org"),
+                  *MockIterator::makeIterator(
+                      rrset_data_separated));
 
     // Similar to the previous case, but with separated RRSIGs.
-    EXPECT_THROW(client_->load(Name("example.org"),
-                               *MockIterator::makeIterator(
-                                   rrset_data_sigseparated)),
-                 ZoneDataUpdater::AddError);
+    client_->load(Name("example.org"),
+                  *MockIterator::makeIterator(
+                      rrset_data_sigseparated));
 
     // Emulating bogus iterator implementation that passes empty RRSIGs.
     EXPECT_THROW(client_->load(Name("example.org"),
                                *MockIterator::makeIterator(rrset_data, true)),
-                 isc::Unexpected);
+                 ZoneDataUpdater::AddError);
 }
 
 TEST_F(MemoryClientTest, loadMemoryAllocationFailures) {
@@ -380,11 +380,11 @@ TEST_F(MemoryClientTest, loadReloadZone) {
 
     const RdataSet* set = node->getData();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::SOA(), set->type);
+    EXPECT_EQ(RRType::NS(), set->type);
 
     set = set->getNext();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::NS(), set->type);
+    EXPECT_EQ(RRType::SOA(), set->type);
 
     set = set->getNext();
     EXPECT_EQ(static_cast<const RdataSet*>(NULL), set);
@@ -409,11 +409,11 @@ TEST_F(MemoryClientTest, loadReloadZone) {
 
     set = node->getData();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::SOA(), set->type);
+    EXPECT_EQ(RRType::NS(), set->type);
 
     set = set->getNext();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::NS(), set->type);
+    EXPECT_EQ(RRType::SOA(), set->type);
 
     set = set->getNext();
     EXPECT_EQ(static_cast<const RdataSet*>(NULL), set);
@@ -439,15 +439,18 @@ TEST_F(MemoryClientTest, loadReloadZone) {
 }
 
 TEST_F(MemoryClientTest, loadDuplicateType) {
-    // This should not result in any exceptions:
+    // This should not result in any exceptions (multiple records of the
+    // same name, type are present, one after another in sequence).
     client_->load(Name("example.org"),
                   TEST_DATA_DIR "/example.org-duplicate-type.zone");
 
-    // This should throw:
-    EXPECT_THROW(client_->load(Name("example.org"),
-                               TEST_DATA_DIR
-                               "/example.org-duplicate-type-bad.zone"),
-                 ZoneDataUpdater::AddError);
+    // This should not result in any exceptions (multiple records of the
+    // same name, type are present, but not one after another in
+    // sequence).
+    client_->load(Name("example.org"),
+                  TEST_DATA_DIR
+                  "/example.org-duplicate-type-bad.zone");
+
     // Teardown checks for memory segment leaks
 }
 
@@ -649,15 +652,15 @@ TEST_F(MemoryClientTest, getIterator) {
     client_->load(Name("example.org"), TEST_DATA_DIR "/example.org-empty.zone");
     ZoneIteratorPtr iterator(client_->getIterator(Name("example.org")));
 
-    // First we have the SOA
+    // First we have the NS
     ConstRRsetPtr rrset(iterator->getNextRRset());
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::SOA(), rrset->getType());
+    EXPECT_EQ(RRType::NS(), rrset->getType());
 
-    // Then the NS
+    // Then the SOA
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::NS(), rrset->getType());
+    EXPECT_EQ(RRType::SOA(), rrset->getType());
 
     // There's nothing else in this iterator
     EXPECT_EQ(ConstRRsetPtr(), iterator->getNextRRset());
@@ -673,15 +676,15 @@ TEST_F(MemoryClientTest, getIteratorSeparateRRs) {
     // separate_rrs = false
     ZoneIteratorPtr iterator(client_->getIterator(Name("example.org")));
 
-    // First we have the SOA
+    // First we have the NS
     ConstRRsetPtr rrset(iterator->getNextRRset());
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::SOA(), rrset->getType());
+    EXPECT_EQ(RRType::NS(), rrset->getType());
 
-    // Then, the NS
+    // Then, the SOA
     rrset = iterator->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::NS(), rrset->getType());
+    EXPECT_EQ(RRType::SOA(), rrset->getType());
 
     // Only one RRType::A() RRset
     rrset = iterator->getNextRRset();
@@ -694,15 +697,15 @@ TEST_F(MemoryClientTest, getIteratorSeparateRRs) {
     // separate_rrs = true
     ZoneIteratorPtr iterator2(client_->getIterator(Name("example.org"), true));
 
-    // First we have the SOA
+    // First we have the NS
     rrset = iterator2->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::SOA(), rrset->getType());
+    EXPECT_EQ(RRType::NS(), rrset->getType());
 
-    // Then, the NS
+    // Then, the SOA
     rrset = iterator2->getNextRRset();
     EXPECT_TRUE(rrset);
-    EXPECT_EQ(RRType::NS(), rrset->getType());
+    EXPECT_EQ(RRType::SOA(), rrset->getType());
 
     // First RRType::A() RRset
     rrset = iterator2->getNextRRset();
@@ -777,12 +780,12 @@ TEST_F(MemoryClientTest, findZoneData) {
 
     const RdataSet* set = node->getData();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::SOA(), set->type);
+    EXPECT_EQ(RRType::NS(), set->type);
 
     /* Check NS */
     set = set->getNext();
     EXPECT_NE(static_cast<const RdataSet*>(NULL), set);
-    EXPECT_EQ(RRType::NS(), set->type);
+    EXPECT_EQ(RRType::SOA(), set->type);
 
     set = set->getNext();
     EXPECT_EQ(static_cast<const RdataSet*>(NULL), set);

src/lib/datasrc/tests/memory/testdata/example.org-duplicate-type-bad.zone

@@ -1,4 +1,6 @@
-example.org. 3600 IN SOA	ns1.example.org. bugs.x.w.example.org. 77 3600 300 3600000 3600
+example.org. 3600 IN SOA	ns1.example.org. bugs.x.w.example.org. 78 3600 300 3600000 3600
+example.org.			      3600 IN NS	ns1.example.org.
+example.org.			      3600 IN NS	ns2.example.org.
 ns1.example.org.		      3600 IN A	 	192.168.0.1
 ns1.example.org.		      3600 IN AAAA 	::1
 ns1.example.org.		      3600 IN A	 	192.168.0.2

src/lib/datasrc/tests/memory/zone_data_updater_unittest.cc

@@ -111,12 +111,10 @@ TEST_F(ZoneDataUpdaterTest, rrsigOnly) {
     EXPECT_EQ(0, rdset->getRdataCount());
     EXPECT_EQ(1, rdset->getSigRdataCount());
 
-    // The RRSIG covering A prohibits an actual A RRset from being added.
-    // This should be loosened in future version, but we check the current
-    // behavior.
-    EXPECT_THROW(updater_->add(
-                     textToRRset("www.example.org. 3600 IN A 192.0.2.1"),
-                     ConstRRsetPtr()), ZoneDataUpdater::AddError);
+    // The RRSIG covering A must not prohibit an actual A RRset from
+    // being added later.
+    updater_->add(textToRRset("www.example.org. 3600 IN A 192.0.2.1"),
+                  ConstRRsetPtr());
 
     // The special "wildcarding" node mark should be added for the RRSIG-only
     // case, too.