completed Question class with doc and more test cases

git-svn-id: svn://bind10.isc.org/svn/bind10/trunk@943 e5f2f494-b856-4b98-b285-d166d9295462

src/lib/dns/cpp/question.cc

@@ -25,7 +25,6 @@
 #include "rrtype.h"
 
 using namespace std;
-using namespace isc::dns;
 
 namespace isc {
 namespace dns {
@@ -69,13 +68,6 @@ Question::toWire(MessageRenderer& renderer) const
     return (1);                 // number of "entries"
 }
 
-bool
-Question::operator==(const Question& other) const
-{
-    return (name_ == other.name_ && rrtype_ == other.rrtype_ &&
-            rrclass_ == other.rrclass_);
-}
-
 ostream&
 operator<<(std::ostream& os, const Question& question)
 {

src/lib/dns/cpp/question.h

@@ -18,6 +18,7 @@
 #define __QUESTION_H 1
 
 #include <iostream>
+#include <string>
 
 #include <boost/shared_ptr.hpp>
 
@@ -29,46 +30,225 @@ namespace isc {
 namespace dns {
 
 class InputBuffer;
+class MessageRenderer;
 class Question;
+
+/// \brief A pointer-like type pointing to an \c Question object.
 typedef boost::shared_ptr<Question> QuestionPtr;
 
+/// \brief A pointer-like type pointing to an (immutable) \c Question object.
+typedef boost::shared_ptr<const Question> ConstQuestionPtr;
+
+/// \brief The \c Question class encapsulates the common search key of DNS
+/// lookup, consisting of owner name, RR type and RR class.
+///
+/// The primarily intended use case of this class is an entry of the question
+/// section of DNS messages.
+/// This could also be used as a general purpose lookup key, e.g., in a
+/// custom implementation of DNS database.
+///
+/// In this initial implementation, the \c Question class is defined as
+/// a <em>concrete class</em>; it's not expected to be inherited by
+/// a user-defined customized class.
+/// It may be worth noting that it's different from the design of the
+/// RRset classes (\c AbstractRRset and its derived classes).
+/// The RRset classes form an inheritance hierarchy from the base abstract
+/// class.
+/// This may look odd in that an "RRset" and "Question" are similar from the
+/// protocol point of view: Both are used as a semantics unit of DNS messages;
+/// both share the same set of components, name, RR type and RR class.
+///
+/// In fact, BIND9 didn't introduce a separate data structure for Questions,
+/// and use the same \c "rdataset" structure for both RRsets and Questions.
+/// We could take the same approach, but chose to adopt the different design.
+/// One reason for that is because a Question and an RRset are still
+/// different, and a Question might not be cleanly defined if (e.g.) it were
+/// a derived class of some "RRset-like" class.
+/// For example, we couldn't give a reasonable semantics for \c %getTTL() or
+/// \c %setTTL() methods for a Question, since it's not associated with the
+/// TTL.
+/// In fact, the BIND9 implementation ended up often separating the case where
+/// a \c "rdataset" is from the Question section of a DNS message and the
+/// case where it comes from other sections.
+/// If we cannot treat them completely transparently anyway, separating the
+/// class (type) would make more sense because we can exploit compilation
+/// time type checks.
+///
+/// On the other hand, we do not expect a strong need for customizing the
+/// \c Question class, unlike the RRset.
+/// Handling the Question section of a DNS message is relatively a
+/// simple work comparing to RRset-involved operations, so a unified
+/// straightforward implementation should suffice for any use cases
+/// including performance sensitive ones.
 ///
-/// This is a straightforward implementation of a "DNS question", modeling
-/// an entry of the question section of DNS messages.
-/// This could also be used as an input parameter for lookups into internal
-/// data sources.
+/// We may, however, still want to have customized version of Question
+/// for, e.g, highly optimized behavior, and may revisit this design choice
+/// as we have more experiences with this implementation.
 ///
-/// Open issue: we may want to introduce an abstract base class so that we
-/// can have multiple concrete implementations (e.g. for better performance)
-/// of the base class in a polymorphic way.
+/// One disadvantage of defining RRsets and Questions as unrelated classes
+/// is that we cannot handle them in a polymorphic way.
+/// For example, we might want to iterate over DNS message sections and
+/// render the data in the wire format, whether it's an RRset or a Question.
+/// If a \c Question were a derived class of some common RRset-like class,
+/// we could do this by calling <code>rrset_or_question->%toWire()</code>.
+/// But the actual design doesn't allow this approach, which may require
+/// duplicate code for almost the same operation.
+/// To mitigate this problem, we intentionally used the same names
+/// with the same signature for some common methods of \c Question and
+/// \c AbstractRRset such as \c %getName() or \c %toWire().
+/// So the user class may use a template function that is applicable to both
+/// \c Question and \c RRset to avoid writing duplicate code logic.
 class Question {
+    ///
+    /// \name Constructors and Destructor
+    ///
+    /// We use the default versions of destructor, copy constructor,
+    /// and assignment operator.
+    ///
+    /// The default constructor is hidden as a result of defining the other
+    /// constructors.  This is intentional; we don't want to allow a
+    /// \c Question object to be constructed with an invalid state.
+    //@{
 public:
+    /// \brief Constructor from wire-format data.
+    ///
+    /// It simply constructs a set of \c Name, \c RRType, and \c RRClass
+    /// object from the \c buffer in this order, and constructs a
+    /// \c Question object in a straightforward way.
+    ///
+    /// It may throw an exception if the construction of these component
+    /// classes fails.
+    ///
+    /// \param buffer A buffer storing the wire format data.
     Question(InputBuffer& buffer);
+
+    /// \brief Constructor from fixed parameters of the \c Question.
+    ///
+    /// This constructor is basically expected to be exception free, but
+    /// copying the name may involve resource allocation, and if it fails
+    /// the corresponding standard exception will be thrown.
+    ///
+    /// \param name The owner name of the \c Question.
+    /// \param rrclass The RR class of the \c Question.
+    /// \param rrtype The RR type of the \c Question.
     Question(const Name& name, const RRClass& rrclass, const RRType& rrtype) :
         name_(name), rrtype_(rrtype), rrclass_(rrclass)
     {}
+    //@}
 
     ///
-    /// \name Getter methods
+    /// \name Getter Methods
+    ///
     //@{
+    /// \brief Returns the owner name of the \c Question.
+    ///
+    /// This method never throws an exception.
+    ///
+    /// \return A reference to a \c Name class object corresponding to the
+    /// \c Question owner name.
     const Name& getName() const { return (name_); }
+
+    /// \brief Returns the RR Class of the \c Question.
+    ///
+    /// This method never throws an exception.
+    ///
+    /// \return A reference to a \c RRClass class object corresponding to the
+    /// RR class of the \c Question.
     const RRType& getType() const { return (rrtype_); }
+
+    /// \brief Returns the RR Type of the \c Question.
+    ///
+    /// This method never throws an exception.
+    ///
+    /// \return A reference to a \c RRType class object corresponding to the
+    /// RR type of the \c Question.
     const RRClass& getClass() const { return (rrclass_); }
     //@}
 
+    ///
+    /// \name Converter Methods
+    ///
+    //@{
+    /// \brief Convert the Question to a string.
+    ///
+    /// Unlike other similar methods of this library, this method terminates
+    /// the resulting string with a trailing newline character
+    /// (following the BIND9 convention).
+    ///
+    /// This method simply calls the \c %toText() methods of the corresponding
+    /// \c Name, \c RRType and \c RRClass classes for this \c Question, and
+    /// these methods may throw an exception.
+    /// In particular, if resource allocation fails, a corresponding standard
+    /// exception will be thrown.
+    ///
+    /// \return A string representation of the \c Question.
     std::string toText() const;
-    unsigned int toWire(OutputBuffer& buffer) const;
+
+    /// \brief Render the Question in the wire format with name compression.
+    ///
+    /// This method simply calls the \c %toWire() methods of the corresponding
+    /// \c Name, \c RRType and \c RRClass classes for this \c Question, and
+    /// these methods may throw an exception.
+    /// In particular, if resource allocation fails, a corresponding standard
+    /// exception will be thrown.
+    ///
+    /// This method returns 1, which is the number of "questions" contained
+    /// in the \c Question.
+    /// This is a meaningless value, but is provided to be consistent with
+    /// the corresponding method of \c AbstractRRset (see the detailed
+    /// class description).
+    ///
+    /// The owner name will be compressed if possible, although it's an
+    /// unlikely event in practice because the %Question section a DNS
+    /// message normally doesn't contain multiple question entries and
+    /// it's located right after the Header section.
+    /// Nevertheless, \c renderer records the information of the owner name
+    /// so that it can be pointed by other RRs in other sections (which is
+    /// more likely to happen).
+    ///
+    /// In theory, an attempt to render a Question may cause truncation
+    /// (when the Question section contains a large number of entries),
+    /// but this implementation doesn't catch that situation.
+    /// It would make the code unnecessarily complicated (though perhaps
+    /// slightly) for almost impossible case in practice.
+    /// An upper layer will handle the pathological case as a general error.
+    ///
+    /// \param renderer DNS message rendering context that encapsulates the
+    /// output buffer and name compression information.
+    /// \return 1
     unsigned int toWire(MessageRenderer& renderer) const;
 
-    bool operator==(const Question& other) const;
-    bool operator!=(const Question& other) const
-    { return !(*this == other); }
+    /// \brief Render the Question in the wire format without name compression.
+    ///
+    /// This method behaves like the render version except it doesn't compress
+    /// the owner name.
+    /// See \c toWire(MessageRenderer& renderer)const.
+    ///
+    /// \param buffer An output buffer to store the wire data.
+    /// \return 1
+    unsigned int toWire(OutputBuffer& buffer) const;
+    //@}
+
 private:
     Name name_;
     RRType rrtype_;
     RRClass rrclass_;
 };
 
+/// \brief Insert the \c Question as a string into stream.
+///
+/// This method convert the \c question into a string and inserts it into the
+/// output stream \c os.
+///
+/// This function overloads the global \c operator<< to behave as described in
+/// \c %ostream::%operator<< but applied to Question objects.
+///
+/// \param os A \c std::ostream object on which the insertion operation is
+/// performed.
+/// \param question A reference to a \c Question object output by the
+/// operation.
+/// \return A reference to the same \c std::ostream object referenced by
+/// parameter \c os after the insertion operation.
 std::ostream& operator<<(std::ostream& os, const Question& question);
 } // end of namespace dns
 } // end of namespace isc

src/lib/dns/cpp/question_unittest.cc

@@ -14,6 +14,11 @@
 
 // $Id$
 
+#include <vector>
+#include <sstream>
+
+#include <exceptions/exceptions.h>
+
 #include "buffer.h"
 #include "messagerenderer.h"
 #include "name.h"
@@ -33,36 +38,86 @@ namespace {
 class QuestionTest : public ::testing::Test {
 protected:
     QuestionTest() : obuffer(0), renderer(obuffer),
-                     test_question(Name("example.com"), RRClass::IN(),
-                                   RRType::NS()) 
+                     example_name1(Name("foo.example.com")),
+                     example_name2(Name("bar.example.com")),
+                     test_question1(example_name1, RRClass::IN(),
+                                    RRType::NS()),
+                     test_question2(example_name2, RRClass::CH(),
+                                    RRType::A())
     {}
     OutputBuffer obuffer;
     MessageRenderer renderer;
-    Question test_question;
-    static const uint8_t wiredata[];
+    Name example_name1;
+    Name example_name2;
+    Question test_question1;
+    Question test_question2;
+    vector<unsigned char> wiredata;
 };
 
-// wire-format representation of "example.com. NS IN"
-const uint8_t QuestionTest::wiredata[] = { 0x07, 0x65, 0x78, 0x61, 0x6d, 0x70,
-                                           0x6c, 0x65, 0x03, 0x63, 0x6f, 0x6d,
-                                           0x00, 0x00, 0x02, 0x00, 0x01 };
+Question
+questionFromWire(const char* datafile, size_t position = 0)
+{
+    vector<unsigned char> data;
+    UnitTestUtil::readWireData(datafile, data);
+
+    InputBuffer buffer(&data[0], data.size());
+    buffer.setPosition(position);
+
+    return (Question(buffer));
+}
 
 TEST_F(QuestionTest, fromWire)
 {
-    InputBuffer ibuffer(wiredata, sizeof(wiredata));
-    Question q(ibuffer);
-    EXPECT_EQ(test_question, q);
+    Question q = questionFromWire("testdata/question_fromWire");
+
+    EXPECT_EQ(example_name1, q.getName());
+    EXPECT_EQ(RRClass::IN(), q.getClass());
+    EXPECT_EQ(RRType::NS(), q.getType());
+
+    // owner name of the second Question is compressed.  It's uncommon
+    // (to have multiple questions), but isn't prohibited by the protocol.
+    q = questionFromWire("testdata/question_fromWire", 21);
+    EXPECT_EQ(example_name2, q.getName());
+    EXPECT_EQ(RRClass::CH(), q.getClass());
+    EXPECT_EQ(RRType::A(), q.getType());
+
+    // Pathological cases: Corresponding exceptions will be thrown from
+    // the underlying parser.
+    EXPECT_THROW(questionFromWire("testdata/question_fromWire", 31),
+                 BadPointer);
+    EXPECT_THROW(questionFromWire("testdata/question_fromWire", 36),
+                 IncompleteRRClass);
 }
 
 TEST_F(QuestionTest, toText)
 {
-    EXPECT_EQ("example.com. IN NS\n", test_question.toText());
+    EXPECT_EQ("foo.example.com. IN NS\n", test_question1.toText());
+    EXPECT_EQ("bar.example.com. CH A\n", test_question2.toText());
+}
+
+TEST_F(QuestionTest, toWireBuffer)
+{
+    test_question1.toWire(obuffer);
+    test_question2.toWire(obuffer);
+    UnitTestUtil::readWireData("testdata/question_toWire1", wiredata);
+    EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData, obuffer.getData(),
+                        obuffer.getLength(), &wiredata[0], wiredata.size());
 }
 
-TEST_F(QuestionTest, toWire)
+TEST_F(QuestionTest, toWireRenderer)
 {
-    test_question.toWire(obuffer);
+    test_question1.toWire(renderer);
+    test_question2.toWire(renderer);
+    UnitTestUtil::readWireData("testdata/question_toWire2", wiredata);
     EXPECT_PRED_FORMAT4(UnitTestUtil::matchWireData, obuffer.getData(),
-                        obuffer.getLength(), wiredata, sizeof(wiredata));
+                        obuffer.getLength(), &wiredata[0], wiredata.size());
+}
+
+// test operator<<.  We simply confirm it appends the result of toText().
+TEST_F(QuestionTest, LeftShiftOperator)
+{
+    ostringstream oss;
+    oss << test_question1;
+    EXPECT_EQ(test_question1.toText(), oss.str());
 }
 }

src/lib/dns/cpp/testdata/question_fromWire

@@ -0,0 +1,33 @@
+#
+# Wire-format data of a sequence of DNS questions.
+# foo.example.com. IN NS
+# bar.example.com. CH A (owner name is compressed)
+# and some pathological cases
+#
+# 0  1  2  3  4  5  6  7  8  9 10  1  2  3  4  5  6 (-th byte)
+#(3) f  o  o (7) e  x  a  m  p  l  e (3) c  o  m  .
+ 03 66 6f 6f 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00
+#7  8  9 20
+# type/class: NS = 2, IN = 1
+00 02 00 01
+
+# 1  2  3  4  5  6
+#(3) b  a  r [ptr=0x04]
+ 03 62 61 72 c0 04
+#7  8  9 30
+# type/class: A = 1, CH = 3
+00 01 00 03
+
+# owner name is broken
+#1
+# invalid label type
+ff
+#2  3  4  5
+#type/class IN/A
+00 01 00 01
+
+# short buffer
+# (root name)
+00
+#class is missing
+00 01

src/lib/dns/cpp/testdata/question_toWire1

@@ -0,0 +1,14 @@
+#
+# Rendering two DNS Questions without name compression
+# foo.example.com. IN NS
+# bar.example.com. CH A
+#
+#(3) f  o  o (7) e  x  a  m  p  l  e (3) c  o  m  .
+ 03 66 6f 6f 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00
+# type/class: NS = 2, IN = 1
+00 02 00 01
+#(3) b  a  r (7) e  x  a  m  p  l  e (3) c  o  m  .
+ 03 62 61 72 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00
+#7  8  9 30
+# type/class: A = 1, CH = 3
+00 01 00 03

src/lib/dns/cpp/testdata/question_toWire2

@@ -0,0 +1,14 @@
+#
+# Rendering two DNS Questions with name compression
+# foo.example.com. IN NS
+# bar.example.com. CH A
+#
+# 0  1  2  3  4 ... (-th byte)
+#(3) f  o  o (7) e  x  a  m  p  l  e (3) c  o  m  .
+ 03 66 6f 6f 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00
+# type/class: NS = 2, IN = 1
+00 02 00 01
+#(3) b  a  r [ptr=0x04]
+ 03 62 61 72 c0 04
+# type/class: A = 1, CH = 3
+00 01 00 03