[1688] suggested reorg: merge response building to the inserter class.

that way we don't have to expose mutable member variables of `Query`.
now the responsibility of the "inserter" is enlarged, also renamed it
to represent the new job more appropriately.

src/bin/auth/query.cc

@@ -54,14 +54,14 @@ namespace isc {
 namespace auth {
 
 void
-Query::RRsetInserter::addRRset(isc::dns::Message& message,
-                               const isc::dns::Message::Section section,
-                               const ConstRRsetPtr& rrset, const bool dnssec)
+Query::ResponseCreator::addRRset(isc::dns::Message& message,
+                                 const isc::dns::Message::Section section,
+                                 const ConstRRsetPtr& rrset, const bool dnssec)
 {
     /// Is this RRset already in the list of RRsets added to the message?
     const std::vector<const AbstractRRset*>::const_iterator i =
         std::find_if(added_.begin(), added_.end(),
-                     std::bind1st(Query::RRsetInserter::IsSameKind(),
+                     std::bind1st(Query::ResponseCreator::IsSameKind(),
                                   rrset.get()));
     if (i == added_.end()) {
         // No - add it to both the message and the list of RRsets processed.
@@ -73,6 +73,35 @@ Query::RRsetInserter::addRRset(isc::dns::Message& message,
     }
 }
 
+void
+Query::ResponseCreator::create(Message& response,
+                               const vector<ConstRRsetPtr>& answers,
+                               const vector<ConstRRsetPtr>& authorities,
+                               const vector<ConstRRsetPtr> additionals,
+                               const bool dnssec)
+{
+    // Inserter should be reset each time the query is reset, so should be
+    // empty at this point.
+    assert(added_.empty());
+
+    // 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;
+    for (i = answers.begin(); i != answers.end(); ++i) {
+        addRRset(response, Message::SECTION_ANSWER, *i, dnssec);
+    }
+
+    for (i = authorities.begin(); i != authorities.end(); ++i) {
+        addRRset(response, Message::SECTION_AUTHORITY, *i, dnssec);
+    }
+
+    for (i = additionals.begin(); i != additionals.end(); ++i) {
+        addRRset(response, Message::SECTION_ADDITIONAL, *i, dnssec);
+    }
+}
 
 void
 Query::addSOA(ZoneFinder& finder) {
@@ -512,7 +541,8 @@ Query::process(datasrc::DataSourceClient& datasrc_client,
             break;
     }
 
-    createResponse();
+    response_creator_.create(*response_, answers_, authorities_, additionals_,
+                             dnssec_);
 }
 
 void
@@ -530,31 +560,6 @@ Query::initialize(datasrc::DataSourceClient& datasrc_client,
 }
 
 void
-Query::createResponse() {
-    // Inserter should be reset each time the query is reset, so should be
-    // empty at this point.
-    assert(inserter_.empty());
-
-    // 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;
-    for (i = answers_.begin(); i != answers_.end(); ++i) {
-        inserter_.addRRset(*response_, Message::SECTION_ANSWER, *i, dnssec_);
-    }
-
-    for (i = authorities_.begin(); i != authorities_.end(); ++i) {
-        inserter_.addRRset(*response_, Message::SECTION_AUTHORITY, *i, dnssec_);
-    }
-
-    for (i = additionals_.begin(); i != additionals_.end(); ++i) {
-        inserter_.addRRset(*response_, Message::SECTION_ADDITIONAL, *i, dnssec_);
-    }
-}
-
-void
 Query::reset() {
     datasrc_client_ = NULL;
     qname_ = NULL;
@@ -563,7 +568,7 @@ Query::reset() {
     answers_.clear();
     authorities_.clear();
     additionals_.clear();
-    inserter_.clear();
+    response_creator_.clear();
 }
 
 bool
@@ -595,7 +600,8 @@ Query::processDSAtChild() {
         }
     }
 
-    createResponse();
+    response_creator_.create(*response_, answers_, authorities_, additionals_,
+                             dnssec_);
     return (true);
 }
 

src/bin/auth/query.h

@@ -20,6 +20,7 @@
 
 #include <boost/noncopyable.hpp>
 
+#include <functional>
 #include <vector>
 
 namespace isc {
@@ -256,70 +257,6 @@ private:
     /// Called by the QueryCleaner object upon its destruction
     void reset();
 
-    /// \brief Inserter Class
-    ///
-    /// Used during the construction of the response message, this performs
-    /// the duplicate RRset detection check.  It keeps a list of RRsets added
-    /// to the message and does not add an RRset if it is the same as one
-    /// already added.
-    class RRsetInserter {
-    public:
-        // \brief RRset comparison functor.
-        struct IsSameKind : public std::binary_function<
-                            const isc::dns::AbstractRRset*,
-                            const isc::dns::AbstractRRset*,
-                            bool> {
-            bool operator()(const isc::dns::AbstractRRset* r1,
-                            const isc::dns::AbstractRRset* r2) const {
-                return (r1->isSameKind(*r2));
-            }
-        };
-
-        /// \brief Constructor
-        ///
-        /// Reserves space for the list of RRsets.  Although the RRInserter
-        /// will be used to create a message from the contents of the Query
-        /// object's answers_, authorities_ and additionals_ elements, and
-        /// each of these are sized to RESERVE_RRSETS, it is _extremely_
-        /// unlikely that all three will be filled to capacity.  So we reserve
-        /// more elements than in each of these components, but not three
-        /// times the amount.
-        ///
-        /// As with the answers_, authorities_ and additionals_ elements, the
-        /// reservation is made in the constructor to avoid dynamic allocation
-        /// of memory.  The RRsetInserter is a member variable of the Query
-        /// object so is constructed once and lasts as long as that object.
-        /// Internal state is cleared through the clear() method.
-        RRsetInserter() {
-            added_.reserve(2 * RESERVE_RRSETS);
-        }
-
-        /// \brief Reset internal state
-        void clear() {
-            added_.clear();
-        }
-
-        /// \brief Return true if empty
-        bool empty() const {
-            return (added_.empty());
-        }
-
-        /// Insertion operation
-        ///
-        /// \param message Message to which the RRset is to be added
-        /// \param section Section of the message in which the RRset is put
-        /// \param rrset Pointer to RRset to be added to the message
-        /// \param dnssec Whether RRSIG records should be added as well
-        void addRRset(isc::dns::Message& message,
-                      const isc::dns::Message::Section section,
-                      const isc::dns::ConstRRsetPtr& rrset, const bool dnssec);
-
-    private:
-        /// List of RRsets already added to the message
-        std::vector<const isc::dns::AbstractRRset*> added_;
-    };
-
-
     /// \brief Internal class used for cleanup of Query members
     ///
     /// The process() call creates an object of this class, which
@@ -337,18 +274,6 @@ 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:
@@ -471,21 +396,105 @@ public:
         {}
     };
 
+    /// \brief Response Creator Class
+    ///
+    /// This is a helper class of Query, and is expected to be used during the
+    /// construction of the response message. This class performs the
+    /// duplicate RRset detection check.  It keeps a list of RRsets added
+    /// to the message and does not add an RRset if it is the same as one
+    /// already added.
+    ///
+    /// This class is essentially private to Query, but is visible to public
+    /// for testing purposes.  It's not expected to be used from a normal
+    /// application.
+    class ResponseCreator {
+    public:
+        /// \brief Constructor
+        ///
+        /// Reserves space for the list of RRsets.  Although the
+        /// ResponseCreator will be used to create a message from the
+        /// contents of the Query object's answers_, authorities_ and
+        /// additionals_ elements, and each of these are sized to
+        /// RESERVE_RRSETS, it is _extremely_ unlikely that all three will be
+        /// filled to capacity.  So we reserve more elements than in each of
+        /// these components, but not three times the amount.
+        ///
+        /// As with the answers_, authorities_ and additionals_ elements, the
+        /// reservation is made in the constructor to avoid dynamic allocation
+        /// of memory.  The ResponseCreator is a member variable of the Query
+        /// object so is constructed once and lasts as long as that object.
+        /// Internal state is cleared through the clear() method.
+        ResponseCreator() {
+            added_.reserve(2 * RESERVE_RRSETS);
+        }
+
+        /// \brief Reset internal state
+        void clear() {
+            added_.clear();
+        }
+
+        /// \brief Complete the response message with filling in the
+        /// response sections.
+        ///
+        /// This is the final step of the Query::process() method, and within
+        /// that method, it should be called before it returns (if any
+        /// response data is to be added)
+        ///
+        /// This will take a message to build and each RRsets for the answer,
+        /// authority, and additional sections, and add them to their
+        /// corresponding sections in the given message.  The RRsets are
+        /// filtered such that a particular RRset appears only once in the
+        /// message.
+        ///
+        /// If \c dnssec is true, it tells the message to include any RRSIGs
+        /// attached to the RRsets.
+        void create(
+            isc::dns::Message& message,
+            const std::vector<isc::dns::ConstRRsetPtr>& answers_,
+            const std::vector<isc::dns::ConstRRsetPtr>& authorities_,
+            const std::vector<isc::dns::ConstRRsetPtr> additionals_,
+            const bool dnssec);
+
+    private:
+        // \brief RRset comparison functor.
+        struct IsSameKind : public std::binary_function<
+                            const isc::dns::AbstractRRset*,
+                            const isc::dns::AbstractRRset*,
+                            bool> {
+            bool operator()(const isc::dns::AbstractRRset* r1,
+                            const isc::dns::AbstractRRset* r2) const {
+                return (r1->isSameKind(*r2));
+            }
+        };
+
+        /// Insertion operation
+        ///
+        /// \param message Message to which the RRset is to be added
+        /// \param section Section of the message in which the RRset is put
+        /// \param rrset Pointer to RRset to be added to the message
+        /// \param dnssec Whether RRSIG records should be added as well
+        void addRRset(isc::dns::Message& message,
+                      const isc::dns::Message::Section section,
+                      const isc::dns::ConstRRsetPtr& rrset, const bool dnssec);
+
+
+    private:
+        /// List of RRsets already added to the message
+        std::vector<const isc::dns::AbstractRRset*> added_;
+    };
+
 private:
     const isc::datasrc::DataSourceClient* datasrc_client_;
     const isc::dns::Name* qname_;
     const isc::dns::RRType* qtype_;
     bool dnssec_;
     isc::datasrc::ZoneFinder::FindOptions dnssec_opt_;
+    ResponseCreator response_creator_;
 
-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_;
-    RRsetInserter inserter_;
 };
 
 }

src/bin/auth/tests/query_unittest.cc

@@ -2369,37 +2369,6 @@ TEST_F(QueryTest, emptyNameWithNSEC3) {
     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;
@@ -2474,19 +2443,19 @@ TEST_F(QueryTest, DuplicateNameRemoval) {
               (sizeof(expected_section) / sizeof(Message::Section)));
 
     // Create the vectors of RRsets (with the RRsets in a semi-random order).
-    std::vector<RRsetPtr> answer;
+    std::vector<ConstRRsetPtr> answer;
     answer.insert(answer.end(), rrset_vector.begin() + 2,
                   rrset_vector.begin() + 4);
     answer.insert(answer.end(), rrset_vector.begin() + 0,
                   rrset_vector.begin() + 2);
 
-    std::vector<RRsetPtr> authority;
+    std::vector<ConstRRsetPtr> authority;
     authority.insert(authority.end(), rrset_vector.begin() + 3,
                      rrset_vector.begin() + 8);
     authority.push_back(rrset_vector[2]);
     authority.push_back(rrset_vector[5]);
 
-    std::vector<RRsetPtr> additional;
+    std::vector<ConstRRsetPtr> additional;
     additional.insert(additional.end(), rrset_vector.begin() + 7,
                       rrset_vector.end());
     additional.push_back(rrset_vector[3]);
@@ -2499,8 +2468,8 @@ TEST_F(QueryTest, DuplicateNameRemoval) {
     EXPECT_EQ(0, message.getRRCount(Message::SECTION_ADDITIONAL));
 
     // ... and fill it.
-    DuplicateQuery query(&message, answer, authority, additional);
-    query.produceResponse();
+    Query::ResponseCreator().create(message, answer, authority, additional,
+                                    false);
 
     // Check counts in each section.  Note that these are RR counts,
     // not RRset counts.