[2470] added a framework to report different TTLs via an optional callback.

the constructor was extended to have the additional callbacks.

src/lib/dns/rrcollator.cc

@@ -31,7 +31,10 @@ using namespace rdata;
 
 class RRCollator::Impl {
 public:
-    Impl(const AddRRsetCallback& callback) : callback_(callback) {}
+    Impl(const AddRRsetCallback& callback,
+         const MasterLoaderCallbacks& issue_callback) :
+        callback_(callback), issue_callback_(issue_callback)
+    {}
 
     void addRR(const Name& name, const RRClass& rrclass,
                const RRType& rrtype, const RRTTL& rrttl,
@@ -39,6 +42,8 @@ public:
 
     RRsetPtr current_rrset_;
     AddRRsetCallback callback_;
+private:
+    MasterLoaderCallbacks issue_callback_;
 };
 
 namespace {
@@ -75,13 +80,20 @@ RRCollator::Impl::addRR(const Name& name, const RRClass& rrclass,
         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));
+        const RRTTL min_ttl(std::min(current_rrset_->getTTL(), rrttl));
+        issue_callback_.warning("<unknown source>", 0,
+                                "Different TTLs for the same RRset: " +
+                                name.toText(true) + "/" +
+                                rrclass.toText() + "/" + rrtype.toText() +
+                                ", set to " + min_ttl.toText());
+        current_rrset_->setTTL(min_ttl);
     }
     current_rrset_->addRdata(rdata);
 }
 
-RRCollator::RRCollator(const AddRRsetCallback& callback) :
-    impl_(new Impl(callback))
+RRCollator::RRCollator(const AddRRsetCallback& callback,
+                       const MasterLoaderCallbacks& issue_callback) :
+    impl_(new Impl(callback, issue_callback))
 {}
 
 RRCollator::~RRCollator() {

src/lib/dns/rrcollator.h

@@ -63,12 +63,27 @@ public:
 
     /// \brief Constructor.
     ///
+    /// If the optional issue_callback parameter is given, it will be used
+    /// to report any errors and non fatal warnings found in the collator's
+    /// operation.  By default special callbacks that do nothing are used.
+    ///
+    /// \note Since the \c RRCollator does not have any information on the
+    /// source of the given RRs (which is normally a DNS master file in the
+    /// intended usage) it cannot provide the actual source name or the line
+    /// number via the callback.  Instead, it passes a special string of
+    /// "<unknown source>" for the source name and the line number of 0 via
+    /// the callback.
+    ///
     /// \throw std::bad_alloc Internal memory allocation fails.  This should
     /// be very rare.
     ///
     /// \param callback The callback functor to be called for each collated
     /// RRset.
-    RRCollator(const AddRRsetCallback& callback);
+    /// \param issue_callback The callbacks to be called on any issue found
+    /// in the collator.
+    RRCollator(const AddRRsetCallback& callback,
+               const MasterLoaderCallbacks& issue_callback =
+               MasterLoaderCallbacks::getNullCallbacks());
 
     /// \brief Destructor.
     ///
@@ -85,7 +100,7 @@ public:
 
     /// \brief Call the callback on the remaining RRset, if any.
     ///
-    /// This method is expected to be called that it's supposed all RRs have
+    /// This method is expected to be called when it's supposed all RRs have
     /// been passed to this class object.  Since there is no explicit
     /// indicator of the end of the stream, the user of this class needs to
     /// explicitly call this method to call the callback for the last buffered

src/lib/dns/tests/rrcollator_unittest.cc

@@ -25,12 +25,16 @@
 
 #include <gtest/gtest.h>
 
+#include <boost/lexical_cast.hpp>
 #include <boost/bind.hpp>
 
+#include <string>
 #include <sstream>
 #include <vector>
 
+using std::string;
 using std::vector;
+using boost::lexical_cast;
 using namespace isc::dns;
 using namespace isc::dns::rdata;
 
@@ -50,9 +54,14 @@ addRRset(const RRsetPtr& rrset, vector<ConstRRsetPtr>* to_append,
 class RRCollatorTest : public ::testing::Test {
 protected:
     RRCollatorTest() :
+        issue_callbacks_(boost::bind(&RRCollatorTest::issueCallback, this,
+                                     _1, _2, _3, true),
+                         boost::bind(&RRCollatorTest::issueCallback, this,
+                                     _1, _2, _3, false)),
         origin_("example.com"), rrclass_(RRClass::IN()), rrttl_(3600),
         throw_from_callback_(false),
-        collator_(boost::bind(addRRset, _1, &rrsets_, &throw_from_callback_)),
+        collator_(boost::bind(addRRset, _1, &rrsets_, &throw_from_callback_),
+                  issue_callbacks_),
         rr_callback_(collator_.getCallback()),
         a_rdata1_(createRdata(RRType::A(), rrclass_, "192.0.2.1")),
         a_rdata2_(createRdata(RRType::A(), rrclass_, "192.0.2.2")),
@@ -65,6 +74,12 @@ protected:
                                 "12345 example.com. FAKE\n"))
     {}
 
+    virtual void TearDown() {
+        // (The current implementation of) RRCollator should never report an
+        // error.
+        EXPECT_TRUE(errors_.empty());
+    }
+
     void checkRRset(const Name& expected_name, const RRClass& expected_class,
                     const RRType& expected_type, const RRTTL& expected_ttl,
                     const vector<ConstRdataPtr>& expected_rdatas) {
@@ -92,6 +107,20 @@ protected:
         rrsets_.clear();
     }
 
+    void issueCallback(const string& src_name, size_t src_line,
+                       const string& reason, bool is_error) {
+        const string msg(src_name + ":" + lexical_cast<string>(src_line) +
+                         ": " + reason);
+        if (is_error) {
+            errors_.push_back(msg);
+        } else {
+            warnings_.push_back(msg);
+        }
+    }
+
+private:
+    MasterLoaderCallbacks issue_callbacks_;
+protected:
     const Name origin_;
     const RRClass rrclass_;
     const RRTTL rrttl_;
@@ -101,6 +130,7 @@ protected:
     AddRRCallback rr_callback_;
     const RdataPtr a_rdata1_, a_rdata2_, txt_rdata_, sig_rdata1_, sig_rdata2_;
     vector<ConstRdataPtr> rdatas_; // placeholder for expected data
+    vector<string> warnings_, errors_;
 };
 
 TEST_F(RRCollatorTest, basicCases) {
@@ -134,6 +164,9 @@ TEST_F(RRCollatorTest, basicCases) {
     checkRRset(Name("txt.example.com"), rrclass_, RRType::TXT(), rrttl_,
                rdatas_);
 
+    // There should have been no warnings.
+    EXPECT_EQ(0, warnings_.size());
+
     // Tell the collator we are done, then we'll see the last RR as an RRset.
     collator_.flush();
     checkRRset(Name("txt.example.com"), RRClass::CH(), RRType::TXT(), rrttl_,
@@ -147,12 +180,16 @@ TEST_F(RRCollatorTest, basicCases) {
 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.
+    // There should be a warning callback about this.
     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_.flush();
     checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_);
+    EXPECT_EQ(1, warnings_.size());
+    EXPECT_EQ("<unknown source>:0: Different TTLs for the same RRset: "
+              "example.com/IN/A, set to 10", warnings_.at(0));
 }
 
 TEST_F(RRCollatorTest, maxTTLFirst) {
@@ -164,6 +201,9 @@ TEST_F(RRCollatorTest, maxTTLFirst) {
     rdatas_.push_back(a_rdata2_);
     collator_.flush();
     checkRRset(origin_, rrclass_, RRType::A(), RRTTL(10), rdatas_);
+    EXPECT_EQ(1, warnings_.size());
+    EXPECT_EQ("<unknown source>:0: Different TTLs for the same RRset: "
+              "example.com/IN/A, set to 10", warnings_.at(0));
 }
 
 TEST_F(RRCollatorTest, addRRSIGs) {
@@ -200,13 +240,17 @@ TEST_F(RRCollatorTest, throwFromCallback) {
 TEST_F(RRCollatorTest, withMasterLoader) {
     // Test a simple case with MasterLoader.  There shouldn't be anything
     // special, but that's the mainly intended usage of the collator, so we
-    // check it explicitly.
+    // check it explicitly.  Also, this test uses a different local collator,
+    // just for checking it works with omitting the issue callback (using
+    // the default).
+    RRCollator collator(boost::bind(addRRset, _1, &rrsets_,
+                                    &throw_from_callback_));
     std::istringstream ss("example.com. 3600 IN A 192.0.2.1\n");
     MasterLoader loader(ss, origin_, rrclass_,
                         MasterLoaderCallbacks::getNullCallbacks(),
-                        collator_.getCallback());
+                        collator.getCallback());
     loader.load();
-    collator_.flush();
+    collator.flush();
     rdatas_.push_back(a_rdata1_);
     checkRRset(origin_, rrclass_, RRType::A(), rrttl_, rdatas_);
 }