[2470] handled some advanced/unusual cases of RRCollator.

src/lib/dns/rrcollator.cc

@@ -13,6 +13,7 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <dns/name.h>
+#include <dns/rdataclass.h>
 #include <dns/rrcollator.h>
 #include <dns/rrclass.h>
 #include <dns/rrtype.h>
@@ -22,8 +23,11 @@
 
 #include <boost/bind.hpp>
 
+#include <algorithm>
+
 namespace isc {
 namespace dns {
+using namespace rdata;
 
 class RRCollator::Impl {
 public:
@@ -31,24 +35,47 @@ public:
 
     void addRR(const Name& name, const RRClass& rrclass,
                const RRType& rrtype, const RRTTL& rrttl,
-               const rdata::RdataPtr& rdata);
+               const RdataPtr& rdata);
 
     RRsetPtr current_rrset_;
     AddRRsetCallback callback_;
 };
 
+namespace {
+inline bool
+isSameType(RRType type1, const ConstRdataPtr& rdata1,
+           const ConstRRsetPtr& rrset)
+{
+    if (type1 != rrset->getType()) {
+        return (false);
+    }
+    if (type1 == RRType::RRSIG()) {
+        RdataIteratorPtr rit = rrset->getRdataIterator();
+        return (dynamic_cast<const generic::RRSIG&>(*rdata1).typeCovered()
+                == dynamic_cast<const generic::RRSIG&>(
+                    rit->getCurrent()).typeCovered());
+    }
+    return (true);
+}
+}
+
 void
 RRCollator::Impl::addRR(const Name& name, const RRClass& rrclass,
                         const RRType& rrtype, const RRTTL& rrttl,
-                        const rdata::RdataPtr& rdata)
+                        const RdataPtr& rdata)
 {
-    if (current_rrset_ && current_rrset_->getType() != rrtype) {
+    if (current_rrset_ && (!isSameType(rrtype, rdata, current_rrset_) ||
+                           current_rrset_->getClass() != rrclass ||
+                           current_rrset_->getName() != name)) {
         callback_(current_rrset_);
         current_rrset_.reset();
     }
 
     if (!current_rrset_) {
         current_rrset_ = RRsetPtr(new RRset(name, rrclass, rrtype, rrttl));
+    } else if (current_rrset_->getTTL() != rrttl) {
+        // RRs with different TTLs are given.  Smaller TTL should win.
+        current_rrset_->setTTL(std::min(current_rrset_->getTTL(), rrttl));
     }
     current_rrset_->addRdata(rdata);
 }
@@ -69,8 +96,10 @@ RRCollator::getCallback() {
 
 void
 RRCollator::finish() {
-    // TODO: case when !current_rrset_
-    impl_->callback_(impl_->current_rrset_);
+    if (impl_->current_rrset_) {
+        impl_->callback_(impl_->current_rrset_);
+        impl_->current_rrset_.reset();
+    }
 }
 
 } // end namespace dns

src/lib/dns/tests/rrcollator_unittest.cc

@@ -45,21 +45,25 @@ protected:
         collator_(boost::bind(addRRset, _1, &rrsets_)),
         rr_callback_(collator_.getCallback())
     {
-        RRsetPtr rrset(new RRset(origin_, rrclass_, RRType::A(), rrttl_));
         a_rdata1_ = createRdata(RRType::A(), rrclass_, "192.0.2.1");
         a_rdata2_ = createRdata(RRType::A(), rrclass_, "192.0.2.2");
-        rrset->addRdata(a_rdata1_);
-        rrset->addRdata(a_rdata2_);
-        a_rrset_ = rrset;
 
-        rrset = RRsetPtr(new RRset(origin_, rrclass_, RRType::AAAA(), rrttl_));
-        aaaa_rdata_ = createRdata(RRType::AAAA(), rrclass_, "2001:db8::1");
-        aaaa_rrset_ = rrset;
+        txt_rdata_ = createRdata(RRType::TXT(), rrclass_, "test");
+
+        sig_rdata1_ = createRdata(RRType::RRSIG(), rrclass_,
+                                  "A 5 3 3600 20000101000000 20000201000000 "
+                                  "12345 example.com. FAKE\n");
+        sig_rdata2_ = createRdata(RRType::RRSIG(), rrclass_,
+                                  "NS 5 3 3600 20000101000000 20000201000000 "
+                                  "12345 example.com. FAKE\n");
     }
 
     void checkRRset(const Name& expected_name, const RRClass& expected_class,
                     const RRType& expected_type, const RRTTL& expected_ttl,
                     const vector<ConstRdataPtr>& expected_rdatas) {
+        SCOPED_TRACE(expected_name.toText(true) + "/" +
+                     expected_class.toText() + "/" + expected_type.toText());
+
         // This test always clears rrsets_ to confirm RRsets are added
         // one-by-one
         ASSERT_EQ(1, rrsets_.size());
@@ -85,15 +89,13 @@ protected:
     const RRClass rrclass_;
     const RRTTL rrttl_;
     vector<ConstRRsetPtr> rrsets_;
-    RdataPtr a_rdata1_, a_rdata2_, aaaa_rdata_;
-    ConstRRsetPtr a_rrset_, aaaa_rrset_;
+    RdataPtr a_rdata1_, a_rdata2_, txt_rdata_, sig_rdata1_, sig_rdata2_;
     RRCollator collator_;
     AddRRCallback rr_callback_;
+    vector<ConstRdataPtr> rdatas_; // placeholder for expected data
 };
 
-TEST_F(RRCollatorTest, add) {
-    vector<ConstRdataPtr> rdatas;
-
+TEST_F(RRCollatorTest, basicCases) {
     // Add two RRs belonging to the same RRset.  These will be buffered.
     rr_callback_(origin_, rrclass_, RRType::A(), rrttl_, a_rdata1_);
     EXPECT_TRUE(rrsets_.empty()); // not yet given as an RRset
@@ -102,16 +104,72 @@ TEST_F(RRCollatorTest, add) {
 
     // Add another type of RR.  This completes the construction of the A RRset,
     // which will be given via the callback.
-    rr_callback_(origin_, rrclass_, RRType::AAAA(), rrttl_, aaaa_rdata_);
-    rdatas.push_back(a_rdata1_);
-    rdatas.push_back(a_rdata2_);
-    checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas);
+    rr_callback_(origin_, rrclass_, RRType::TXT(), rrttl_, txt_rdata_);
+    rdatas_.push_back(a_rdata1_);
+    rdatas_.push_back(a_rdata2_);
+    checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas_);
+
+    // Add the same type of RR but of different name.  This should make another
+    // callback for the previous TXT RR.
+    rr_callback_(Name("txt.example.com"), rrclass_, RRType::TXT(), rrttl_,
+                 txt_rdata_);
+    rdatas_.clear();
+    rdatas_.push_back(txt_rdata_);
+    checkRRset(origin_, rrclass_, RRType::TXT(), rrttl_, rdatas_);
+
+    // Add the same type and name of RR but of different class (rare case
+    // in practice)
+    rr_callback_(Name("txt.example.com"), RRClass::CH(), RRType::TXT(), rrttl_,
+                 txt_rdata_);
+    rdatas_.clear();
+    rdatas_.push_back(txt_rdata_);
+    checkRRset(Name("txt.example.com"), rrclass_, RRType::TXT(), rrttl_,
+               rdatas_);
+
+    // Tell the collator we are done, then we'll see the last RR as an RRset.
+    collator_.finish();
+    checkRRset(Name("txt.example.com"), RRClass::CH(), RRType::TXT(), rrttl_,
+               rdatas_);
+
+    // Redundant finish() will be no-op.
+    collator_.finish();
+    EXPECT_TRUE(rrsets_.empty());
+}
+
+TEST_F(RRCollatorTest, minTTLFirst) {
+    // RRs of the same RRset but has different TTLs.  The first RR has
+    // the smaller TTL, which should be used for the TTL of the RRset.
+    rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(10), a_rdata1_);
+    rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(20), a_rdata2_);
+    rdatas_.push_back(a_rdata1_);
+    rdatas_.push_back(a_rdata2_);
+    collator_.finish();
+    checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_);
+}
+
+TEST_F(RRCollatorTest, maxTTLFirst) {
+    // RRs of the same RRset but has different TTLs.  The second RR has
+    // the smaller TTL, which should be used for the TTL of the RRset.
+    rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(20), a_rdata1_);
+    rr_callback_(origin_, rrclass_, RRType::A(), RRTTL(10), a_rdata2_);
+    rdatas_.push_back(a_rdata1_);
+    rdatas_.push_back(a_rdata2_);
+    collator_.finish();
+    checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_);
+}
+
+TEST_F(RRCollatorTest, addRRSIGs) {
+    // RRSIG is special; they are also distinguished by their covered types.
+    rr_callback_(origin_, rrclass_, RRType::RRSIG(), rrttl_, sig_rdata1_);
+    rr_callback_(origin_, rrclass_, RRType::RRSIG(), rrttl_, sig_rdata2_);
+
+    rdatas_.push_back(sig_rdata1_);
+    checkRRset(origin_, rrclass_, RRType::RRSIG(), rrttl_, rdatas_);
+}
 
-    // Let the collator we are done, then we'll see the AAAA RR as an RRset.
+TEST_F(RRCollatorTest, emptyFinish) {
     collator_.finish();
-    rdatas.clear();
-    rdatas.push_back(aaaa_rdata_);
-    checkRRset(origin_, rrclass_, RRType::AAAA(), rrttl_, rdatas);
+    EXPECT_TRUE(rrsets_.empty());
 }
 
 }