[1688] Remove duplicate RRsets when producing output message

When Query::createResponse() creates a message, this change ensures
that any duplicate RRsets are removed.

src/bin/auth/query.cc

@@ -15,6 +15,7 @@
 #include <dns/message.h>
 #include <dns/rcode.h>
 #include <dns/rrtype.h>
+#include <dns/rrset.h>
 #include <dns/rdataclass.h>
 
 #include <datasrc/client.h>
@@ -26,6 +27,7 @@
 #include <boost/function.hpp>
 
 #include <algorithm>            // for std::max
+#include <set>
 #include <vector>
 
 using namespace std;
@@ -35,27 +37,64 @@ using namespace isc::dns::rdata;
 
 // Commonly used helper callback object for vector<ConstRRsetPtr> to
 // insert them to (the specified section of) a message.
+//
+// One feature is that it maintains an internal set of raw pointers to the
+// RRsets as they are added (this is safe - the object is only in scope in
+// the createResponse() method and during this time, all RRsets referred to
+// remain in existence due to the presence of the ConstRRsetPtr objects
+// from which the raw objects were derived).  The set is used to detect
+// and discard duplicates.
 namespace {
 class RRsetInserter {
+private:
+    // \brief RRset comparison functor.
+    struct RRsetLthan {
+        bool operator()(const AbstractRRset* r1, const AbstractRRset* r2) {
+            return (r1->lthan(*r2));
+        }
+    };
+
+    typedef std::set<const AbstractRRset*, RRsetLthan> AddedRRsets;
+
 public:
-    RRsetInserter(Message& msg, Message::Section section, bool dnssec) :
-        msg_(msg), section_(section), dnssec_(dnssec)
+    RRsetInserter(Message& msg, const Message::Section section,
+                  const bool dnssec) :
+                  msg_(msg), section_(section), dnssec_(dnssec)
     {}
-    void operator()(const ConstRRsetPtr& rrset) {
-        /*
-         * FIXME:
-         * The const-cast is wrong, but the Message interface seems
-         * to insist.
-         */
-        msg_.addRRset(section_,
-                      boost::const_pointer_cast<AbstractRRset>(rrset),
-                      dnssec_);
+
+    // \brief Set Target Section
+    // 
+    // Sets the section into which the information added by addRRset will be
+    // inserted.
+    //
+    // \param section New section number
+    void setSection(Message::Section section) {
+        section_ = section;
+    }
+
+    // Insertion operation
+    //
+    // \param rrset Pointer to RRset to be added to the message
+    void addRRset(const ConstRRsetPtr& rrset) {
+        // Has the RRset already been added to this message?
+        std::pair<AddedRRsets::iterator, bool> result =
+            rrsets_added_.insert(rrset.get());
+        if (result.second) {
+            // Were able to add the pointer to the RRset to rrsets_added_, so
+            // the RRset has not already been seen.  Add it to the message.
+            // The const-cast is wrong, but the Message interface seems
+            // to insist.
+            msg_.addRRset(section_,
+                          boost::const_pointer_cast<AbstractRRset>(rrset),
+                          dnssec_);
+        }
     }
 
 private:
     Message& msg_;
-    const Message::Section section_;
+    Message::Section section_;
     const bool dnssec_;
+    AddedRRsets rrsets_added_;
 };
 
 // This is a "constant" vector storing desired RR types for the additional
@@ -553,12 +592,26 @@ Query::initialize(datasrc::DataSourceClient& datasrc_client,
 
 void
 Query::createResponse() {
-    for_each(answers_.begin(), answers_.end(),
-             RRsetInserter(*response_, Message::SECTION_ANSWER, dnssec_));
-    for_each(authorities_.begin(), authorities_.end(),
-             RRsetInserter(*response_, Message::SECTION_AUTHORITY, dnssec_));
-    for_each(additionals_.begin(), additionals_.end(),
-             RRsetInserter(*response_, Message::SECTION_ADDITIONAL, dnssec_));
+    // Add the RRsets to the message.  The order of sections is important,
+    // as the RRsetInserter remembers RRsets added and will not add
+    // duplicates.  Adding in the order answer, authory, additional will
+    // guarantee that if there are duplicates, the single RRset added will
+    // appear in the most important section.
+    std::vector<isc::dns::ConstRRsetPtr>::const_iterator i;
+    RRsetInserter inserter(*response_, Message::SECTION_ANSWER, dnssec_);
+    for (i = answers_.begin(); i != answers_.end(); ++i) {
+        inserter.addRRset(*i);
+    }
+
+    inserter.setSection(Message::SECTION_AUTHORITY);
+    for (i = authorities_.begin(); i != authorities_.end(); ++i) {
+        inserter.addRRset(*i);
+    }
+
+    inserter.setSection(Message::SECTION_ADDITIONAL);
+    for (i = additionals_.begin(); i != additionals_.end(); ++i) {
+        inserter.addRRset(*i);
+    }
 }
 
 void

src/bin/auth/query.h

@@ -251,19 +251,6 @@ private:
                     const isc::dns::Name& qname, const isc::dns::RRType& qtype,
                     isc::dns::Message& response, bool dnssec = false);
 
-    /// \brief Fill in the response sections
-    ///
-    /// This is the final step of the process() method, and within
-    /// that method, it should be called before it returns (if any
-    /// response data is to be added)
-    ///
-    /// This will take each RRset collected in answers_, authorities_, and
-    /// additionals_, and add them to their corresponding sections in
-    /// the response message.
-    ///
-    /// After they are added, the vectors are cleared.
-    void createResponse();
-
     /// \brief Resets any partly built response data, and internal pointers
     ///
     /// Called by the QueryCleaner object upon its destruction
@@ -282,6 +269,24 @@ private:
         isc::auth::Query& query_;
     };
 
+protected:
+    // Following methods declared protected so they can be accessed
+    // by unit tests.
+
+    /// \brief Fill in the response sections
+    ///
+    /// This is the final step of the process() method, and within
+    /// that method, it should be called before it returns (if any
+    /// response data is to be added)
+    ///
+    /// This will take each RRset collected in answers_, authorities_, and
+    /// additionals_, and add them to their corresponding sections in
+    /// the response message.  The RRsets are filtered such that a
+    /// particular RRset appears only once in the message.
+    ///
+    /// After they are added, the vectors are cleared.
+    void createResponse();
+
 public:
     /// Default constructor.
     ///
@@ -289,8 +294,8 @@ public:
     ///
     Query() :
         datasrc_client_(NULL), qname_(NULL), qtype_(NULL),
-        response_(NULL), dnssec_(false),
-        dnssec_opt_(isc::datasrc::ZoneFinder::FIND_DEFAULT)
+        dnssec_(false), dnssec_opt_(isc::datasrc::ZoneFinder::FIND_DEFAULT),
+        response_(NULL)
     {
         answers_.reserve(RESERVE_RRSETS);
         authorities_.reserve(RESERVE_RRSETS);
@@ -406,10 +411,13 @@ private:
     const isc::datasrc::DataSourceClient* datasrc_client_;
     const isc::dns::Name* qname_;
     const isc::dns::RRType* qtype_;
-    isc::dns::Message* response_;
     bool dnssec_;
     isc::datasrc::ZoneFinder::FindOptions dnssec_opt_;
 
+protected:
+    // Following members declared protected to allow them to be accessed
+    // by unit tests.
+    isc::dns::Message* response_;
     std::vector<isc::dns::ConstRRsetPtr> answers_;
     std::vector<isc::dns::ConstRRsetPtr> authorities_;
     std::vector<isc::dns::ConstRRsetPtr> additionals_;

src/bin/auth/tests/query_unittest.cc

@@ -12,12 +12,13 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#include <map>
 #include <sstream>
 #include <vector>
-#include <map>
 
 #include <boost/bind.hpp>
 #include <boost/scoped_ptr.hpp>
+#include <boost/static_assert.hpp>
 
 #include <exceptions/exceptions.h>
 
@@ -2367,4 +2368,172 @@ TEST_F(QueryTest, emptyNameWithNSEC3) {
     EXPECT_TRUE(result->isNSEC3Signed());
     EXPECT_FALSE(result->isWildcard());
 }
+
+// Class to allow checking of duplication removal in messages resulting from.
+// the query.  This class allows the setting of the answers, authorities and
+// additionals vector in the Query class, as well as the ability to call the
+// createResponse() method.
+
+class DuplicateQuery : public isc::auth::Query {
+public:
+    // \brief Constructor
+    //
+    // Fill in the parts of Query that we test in the DuplicateRemoval test.
+    DuplicateQuery(isc::dns::Message* message,
+                   const vector<RRsetPtr>& answers,
+                   const vector<RRsetPtr>& authorities,
+                   const vector<RRsetPtr>& additionals) : Query() {
+        response_ = message;
+        copy(answers.begin(), answers.end(),
+             back_inserter(answers_));
+        copy(authorities.begin(), authorities.end(),
+             back_inserter(authorities_));
+        copy(additionals.begin(), additionals.end(),
+             back_inserter(additionals_));
+    }
+
+    // \brief Create Response
+    //
+    // Public interface to the (protected) Query::createResponse() method.
+    void produceResponse() {
+        createResponse();
+    }
+};
+
+// Vector of RRsets used for the test.   Having this external to functions and
+// classes used for the testing simplifies the code.
+std::vector<RRsetPtr> rrset_vector;
+
+// Callback function for masterLoad.
+void
+loadRRsetVectorCallback(RRsetPtr rrsetptr) {
+    rrset_vector.push_back(rrsetptr);
+}
+
+// Load a set of RRsets into a vector for use in the duplicate RRset test.
+// They don't make a lot of sense as a zone, they are just different.  The
+// variables used in the stringstream input have been chosen so that each
+// represents just one RRset.
+void
+loadRRsetVector() {
+    stringstream ss;
+
+    // Comments indicate offset in the rrset_vector (when loaded) and the
+    // number of RRs in that RRset.
+    ss << soa_txt               // 0(1)
+       << zone_ns_txt           // 1(3)
+       << delegation_txt        // 2(4)
+       << delegation_ds_txt     // 3(1)
+       << mx_txt                // 4(3)
+       << www_a_txt             // 5(1)
+       << cname_txt             // 6(1)
+       << cname_nxdom_txt       // 7(1)
+       << cname_out_txt;        // 8(1)
+    rrset_vector.clear();
+    masterLoad(ss, Name("example.com."), RRClass::IN(), loadRRsetVectorCallback);
+}
+
+TEST_F(QueryTest, DuplicateNameRemoval) {
+
+    // Load some RRsets into the master vector.
+    loadRRsetVector();
+    EXPECT_EQ(9, rrset_vector.size());
+
+    // Create an answer, authority and authority vector with some overlapping
+    // entries.  The following indicate which elements from rrset_vector
+    // go into each section vector.  (The values have been separated to show
+    // the overlap.)
+    //
+    // answer     = 0 1 2 3
+    // authority  =     2 3 4 5 6 7
+    // additional = 0     3       7 8
+    //
+    // If the duplicate removal works, we should end up with the following in
+    // the message created from the three vectors:
+    //
+    // answer     = 0 1 2 3
+    // authority  =         4 5 6 7
+    // additional =                 8
+    //
+    // The expected section into which each RRset is placed is indicated in the
+    // array below.
+    const Message::Section expected_section[] = {
+        Message::SECTION_ANSWER,
+        Message::SECTION_ANSWER,
+        Message::SECTION_ANSWER,
+        Message::SECTION_ANSWER,
+        Message::SECTION_AUTHORITY,
+        Message::SECTION_AUTHORITY,
+        Message::SECTION_AUTHORITY,
+        Message::SECTION_AUTHORITY,
+        Message::SECTION_ADDITIONAL
+    };
+    EXPECT_EQ(rrset_vector.size(),
+              (sizeof(expected_section) / sizeof(Message::Section)));
+
+    // Create the vectors of RRsets (with the RRsets in a semi-random order).
+    std::vector<RRsetPtr> answer;
+    copy(rrset_vector.begin() + 2, rrset_vector.begin() + 4,
+         back_inserter(answer));
+    copy(rrset_vector.begin() + 0, rrset_vector.begin() + 2,
+         back_inserter(answer));
+
+    std::vector<RRsetPtr> authority;
+    copy(rrset_vector.begin() + 3, rrset_vector.begin() + 8,
+         back_inserter(authority));
+    authority.push_back(rrset_vector[2]);
+
+    std::vector<RRsetPtr> additional;
+    copy(rrset_vector.begin() + 7, rrset_vector.begin() + 9,
+         back_inserter(additional));
+    additional.push_back(rrset_vector[3]);
+    additional.push_back(rrset_vector[0]);
+
+    // Create the message object into which the RRsets are put
+    Message message(Message::RENDER);
+    EXPECT_EQ(0, message.getRRCount(Message::SECTION_ANSWER));
+    EXPECT_EQ(0, message.getRRCount(Message::SECTION_AUTHORITY));
+    EXPECT_EQ(0, message.getRRCount(Message::SECTION_ADDITIONAL));
+
+    // ... and fill it.
+    DuplicateQuery query(&message, answer, authority, additional);
+    query.produceResponse();
+
+    // Check counts in each section.  Note that these are RR counts,
+    // not RRset counts.
+    EXPECT_EQ(9, message.getRRCount(Message::SECTION_ANSWER));
+    EXPECT_EQ(6, message.getRRCount(Message::SECTION_AUTHORITY));
+    EXPECT_EQ(1, message.getRRCount(Message::SECTION_ADDITIONAL));
+
+    // ... and check that the RRsets are in the correct section
+    BOOST_STATIC_ASSERT(Message::SECTION_QUESTION == 0);
+    BOOST_STATIC_ASSERT(Message::SECTION_ANSWER == 1);
+    BOOST_STATIC_ASSERT(Message::SECTION_AUTHORITY == 2);
+    BOOST_STATIC_ASSERT(Message::SECTION_ADDITIONAL == 3);
+    Message::Section sections[] = {
+        Message::SECTION_QUESTION,
+        Message::SECTION_ANSWER,
+        Message::SECTION_AUTHORITY,
+        Message::SECTION_ADDITIONAL
+    };
+    for (int section = 1; section <= 3; ++section) {
+        for (int vecindex = 0; vecindex < rrset_vector.size(); ++vecindex) {
+            // Prepare error message in case an assertion fails (as the default
+            // message will only refer to the loop indexes).
+            stringstream ss;
+            ss << "section " << section << ", name "
+               << rrset_vector[vecindex]->getName().toText();
+            SCOPED_TRACE(ss.str());
+
+            // Check RRset is in the right section and not in the wrong section.
+            if (sections[section] == expected_section[vecindex]) {
+                EXPECT_TRUE(message.hasRRset(sections[section],
+                            rrset_vector[vecindex]));
+            } else {
+                EXPECT_FALSE(message.hasRRset(sections[section],
+                             rrset_vector[vecindex]));
+            }
+        }
+    }
+}
 }