[1791] fixed one remaining regression: 'separate_rrs' case was broken.

added a test for that case to confirm the regression and the fix.
also introduced some more cleanups: removed rdata_txt_ so we simplify the
code further, made isSameType static class function (it doesn't refer to
any class attributes), constified separate_rrs_.

src/lib/datasrc/database.cc

@@ -1011,12 +1011,8 @@ public:
         const RRType rtype(rtype_txt_);
         RRsetPtr rrset(new RRset(Name(name_txt_), class_, rtype,
                                  RRTTL(ttl_txt_)));
-        // For the first RR, rdata_ is null, so we need to create it here.
-        // After that, rdata_ will be updated in the while loop below.
-        if (!rdata_) {
-            rdata_ = rdata::createRdata(rtype, class_, rdata_txt_);
-        }
-        const ConstRdataPtr rdata_base = rdata_; // remember it for comparison
+        // Remember the first RDATA of the RRset for comparison:
+        const ConstRdataPtr rdata_base = rdata_;
         while (true) {
             // Extend the RRset with the new RDATA.
             rrset->addRdata(rdata_);
@@ -1030,13 +1026,11 @@ public:
             }
 
             // Check if the next record belongs to the same RRset.  If not,
-            // we are done.  The next RDATA is stored in rdata_, which is used
-            // within this loop (if it belongs to the same RRset) or in the
-            // next call.
-            const RRType next_rtype(rtype_txt_);
-            rdata_ = rdata::createRdata(next_rtype, class_, rdata_txt_);
+            // we are done.  The next RDATA has been stored in rdata_, which
+            // is used within this loop (if it belongs to the same RRset) or
+            // in the next call.
             if (Name(name_txt_) != rrset->getName() ||
-                !isSameType(rtype, rdata_base, next_rtype, rdata_)) {
+                !isSameType(rtype, rdata_base, RRType(rtype_txt_), rdata_)) {
                 break;
             }
 
@@ -1059,8 +1053,8 @@ private:
     // Check two RDATA types are equivalent.  Basically it's a trivial
     // comparison, but if both are of RRSIG, we should also compare the types
     // covered.
-    bool isSameType(RRType type1, ConstRdataPtr rdata1,
-                    RRType type2, ConstRdataPtr rdata2)
+    static bool isSameType(RRType type1, ConstRdataPtr rdata1,
+                           RRType type2, ConstRdataPtr rdata2)
     {
         if (type1 != type2) {
             return (false);
@@ -1077,10 +1071,13 @@ private:
     void getData() {
         string data[DatabaseAccessor::COLUMN_COUNT];
         data_ready_ = context_->getNext(data);
-        name_txt_ = data[DatabaseAccessor::NAME_COLUMN];
-        rtype_txt_ = data[DatabaseAccessor::TYPE_COLUMN];
-        ttl_txt_ = data[DatabaseAccessor::TTL_COLUMN];
-        rdata_txt_= data[DatabaseAccessor::RDATA_COLUMN];
+        if (data_ready_) {
+            name_txt_ = data[DatabaseAccessor::NAME_COLUMN];
+            rtype_txt_ = data[DatabaseAccessor::TYPE_COLUMN];
+            ttl_txt_ = data[DatabaseAccessor::TTL_COLUMN];
+            rdata_ = rdata::createRdata(RRType(rtype_txt_), class_,
+                                        data[DatabaseAccessor::RDATA_COLUMN]);
+        }
     }
 
     // The dedicated accessor
@@ -1094,12 +1091,12 @@ private:
     // Status
     bool ready_, data_ready_;
     // Data of the next row
-    string name_txt_, rtype_txt_, rdata_txt_, ttl_txt_;
-    // RDATA of the next row; created from rdata_txt_.
+    string name_txt_, rtype_txt_, ttl_txt_;
+    // RDATA of the next row
     ConstRdataPtr rdata_;
     // Whether to modify differing TTL values, or treat a different TTL as
     // a different RRset
-    bool separate_rrs_;
+    const bool separate_rrs_;
 };
 
 }

src/lib/datasrc/tests/database_unittest.cc

@@ -1333,7 +1333,7 @@ checkRRset(isc::dns::ConstRRsetPtr rrset,
     isc::testutils::rrsetCheck(expected_rrset, rrset);
 }
 
-// Iterate through a zone
+// Iterate through a zone, common case
 TYPED_TEST(DatabaseClientTest, iterator) {
     ZoneIteratorPtr it(this->client_->getIterator(Name("example.org")));
     ConstRRsetPtr rrset(it->getNextRRset());
@@ -1341,71 +1341,100 @@ TYPED_TEST(DatabaseClientTest, iterator) {
 
     // The first name should be the zone origin.
     EXPECT_EQ(this->zname_, rrset->getName());
+}
 
-    // The rest of the checks work only for the mock accessor.
-    if (!this->is_mock_) {
-        return;
-    }
-
-    this->expected_rdatas_.clear();
-    this->expected_rdatas_.push_back("192.0.2.1");
-    checkRRset(rrset, Name("example.org"), this->qclass_, RRType::A(),
-               this->rrttl_, this->expected_rdatas_);
-
-    rrset = it->getNextRRset();
-    this->expected_rdatas_.clear();
-    this->expected_rdatas_.push_back("ns1.example.org. admin.example.org. "
-                                     "1234 3600 1800 2419200 7200");
-    checkRRset(rrset, Name("example.org"), this->qclass_, RRType::SOA(),
-               this->rrttl_, this->expected_rdatas_);
-
-    rrset = it->getNextRRset();
-    this->expected_rdatas_.clear();
-    this->expected_rdatas_.push_back("192.0.2.1");
-    this->expected_rdatas_.push_back("192.0.2.2");
-    checkRRset(rrset, Name("x.example.org"), this->qclass_, RRType::A(),
-               RRTTL(300), this->expected_rdatas_);
-
-    rrset = it->getNextRRset();
-    this->expected_rdatas_.clear();
-    this->expected_rdatas_.push_back("2001:db8::1");
-    this->expected_rdatas_.push_back("2001:db8::2");
-    checkRRset(rrset, Name("x.example.org"), this->qclass_, RRType::AAAA(),
-               RRTTL(300), this->expected_rdatas_);
-
-    rrset = it->getNextRRset();
-    this->expected_rdatas_.clear();
-    this->expected_rdatas_.push_back(
-        "A 5 3 3600 20000101000000 20000201000000 "
-        "12345 example.org. FAKEFAKEFAKE");
-    checkRRset(rrset, Name("x.example.org"), this->qclass_, RRType::RRSIG(),
-               RRTTL(300), this->expected_rdatas_);
-
-    rrset = it->getNextRRset();
-    this->expected_rdatas_.clear();
-    this->expected_rdatas_.push_back(
-        "AAAA 5 3 3600 20000101000000 20000201000000 "
-        "12345 example.org. FAKEFAKEFAKEFAKE");
-    checkRRset(rrset, Name("x.example.org"), this->qclass_, RRType::RRSIG(),
-               RRTTL(300), this->expected_rdatas_);
-
-    rrset = it->getNextRRset();
-    ASSERT_NE(ConstRRsetPtr(), rrset);
-    this->expected_rdatas_.clear();
-    this->expected_rdatas_.push_back("192.0.2.1");
-    this->expected_rdatas_.push_back("192.0.2.2");
-    checkRRset(rrset, Name("ttldiff.example.org"), this->qclass_, RRType::A(),
-               RRTTL(300), this->expected_rdatas_);
-
-    rrset = it->getNextRRset();
-    ASSERT_NE(ConstRRsetPtr(), rrset);
-    this->expected_rdatas_.clear();
-    this->expected_rdatas_.push_back("2001:db8::1");
-    this->expected_rdatas_.push_back("2001:db8::2");
-    checkRRset(rrset, Name("ttldiff2.example.org"), this->qclass_,
-               RRType::AAAA(), RRTTL(300), this->expected_rdatas_);
+// Supplemental structure used in the couple of tests below.  It represents
+// parameters of an expected RRset containing up to two RDATAs.  If it contains
+// only one RDATA, rdata2 is NULL.
+struct ExpectedRRset {
+    const char* const name;
+    const RRType rrtype;
+    const RRTTL rrttl;
+    const char* const rdata1;
+    const char* const rdata2;
+};
 
-    EXPECT_EQ(ConstRRsetPtr(), it->getNextRRset());
+// Common checker for the iterator tests below.  It extracts RRsets from the
+// give iterator and compare them to the expected sequence.
+void
+checkIteratorSequence(ZoneIterator& it, ExpectedRRset expected_sequence[],
+                      size_t num_rrsets)
+{
+    vector<string> expected_rdatas;
+    for (size_t i = 0; i < num_rrsets; ++i) {
+        const ConstRRsetPtr rrset = it.getNextRRset();
+        ASSERT_TRUE(rrset);
+
+        expected_rdatas.clear();
+        expected_rdatas.push_back(expected_sequence[i].rdata1);
+        if (expected_sequence[i].rdata2 != NULL) {
+            expected_rdatas.push_back(expected_sequence[i].rdata2);
+        }
+        checkRRset(rrset, Name(expected_sequence[i].name), RRClass::IN(),
+                   expected_sequence[i].rrtype, expected_sequence[i].rrttl,
+                   expected_rdatas);
+    }
+    EXPECT_FALSE(it.getNextRRset());
+}
+
+TEST_F(MockDatabaseClientTest, iterator) {
+    // This version of test creates an iterator that combines same types of
+    // RRs into single RRsets.
+    ExpectedRRset expected_sequence[] = {
+        {"example.org", RRType::A(), rrttl_, "192.0.2.1", NULL},
+        {"example.org", RRType::SOA(), rrttl_,
+         "ns1.example.org. admin.example.org. 1234 3600 1800 2419200 7200",
+         NULL},
+        {"x.example.org", RRType::A(), RRTTL(300), "192.0.2.1", "192.0.2.2"},
+        {"x.example.org", RRType::AAAA(), RRTTL(300),
+         "2001:db8::1", "2001:db8::2"},
+        {"x.example.org", RRType::RRSIG(), RRTTL(300),
+         "A 5 3 3600 20000101000000 20000201000000 12345 example.org. "
+         "FAKEFAKEFAKE", NULL},
+        {"x.example.org", RRType::RRSIG(), RRTTL(300),
+         "AAAA 5 3 3600 20000101000000 20000201000000 12345 example.org. "
+         "FAKEFAKEFAKEFAKE", NULL},
+        {"ttldiff.example.org", RRType::A(), RRTTL(300),
+         "192.0.2.1", "192.0.2.2"},
+        {"ttldiff2.example.org", RRType::AAAA(), RRTTL(300),
+         "2001:db8::1", "2001:db8::2"}
+    };
+    checkIteratorSequence(*client_->getIterator(Name("example.org")),
+                          expected_sequence,
+                          sizeof(expected_sequence) /
+                          sizeof(expected_sequence[0]));
+}
+
+TEST_F(MockDatabaseClientTest, iteratorSeparateRRs) {
+    // This version of test creates an iterator that separates all RRs as
+    // individual RRsets.  In particular, it preserves the TTLs of an RRset
+    // even if they are different.
+    ExpectedRRset expected_sequence[] = {
+        {"example.org", RRType::A(), rrttl_, "192.0.2.1", NULL},
+        {"example.org", RRType::SOA(), rrttl_,
+         "ns1.example.org. admin.example.org. 1234 3600 1800 2419200 7200",
+         NULL},
+        {"x.example.org", RRType::A(), RRTTL(300), "192.0.2.1", NULL},
+        {"x.example.org", RRType::A(), RRTTL(300), "192.0.2.2", NULL},
+        {"x.example.org", RRType::AAAA(), RRTTL(300), "2001:db8::1", NULL},
+        {"x.example.org", RRType::AAAA(), RRTTL(300), "2001:db8::2", NULL},
+        {"x.example.org", RRType::RRSIG(), RRTTL(300),
+         "A 5 3 3600 20000101000000 20000201000000 12345 example.org. "
+         "FAKEFAKEFAKE", NULL},
+        {"x.example.org", RRType::RRSIG(), RRTTL(300),
+         "AAAA 5 3 3600 20000101000000 20000201000000 12345 example.org. "
+         "FAKEFAKEFAKEFAKE", NULL},
+        {"ttldiff.example.org", RRType::A(), RRTTL(300), "192.0.2.1", NULL},
+        {"ttldiff.example.org", RRType::A(), RRTTL(600), "192.0.2.2", NULL},
+        {"ttldiff2.example.org", RRType::AAAA(), RRTTL(600), "2001:db8::1",
+         NULL},
+        {"ttldiff2.example.org", RRType::AAAA(), RRTTL(300), "2001:db8::2",
+         NULL}
+    };
+    checkIteratorSequence(*client_->getIterator(Name("example.org"), true),
+                          expected_sequence,
+                          sizeof(expected_sequence) /
+                          sizeof(expected_sequence[0]));
 }
 
 // This has inconsistent TTL in the set (the rest, like nonsense in