[1068] Merge branch 'trac1068review2' into trac1068review3

with fixing conflicts.  The conflicts were only in database_unittest.cc,
and the necessary fix is straighforward updates to make newly added tests
work as TYPED_TESTs.

src/lib/datasrc/database.cc

@@ -618,8 +618,7 @@ public:
         committed_(false), accessor_(accessor), zone_id_(zone_id),
         db_name_(accessor->getDBName()), zone_name_(zone_name.toText()),
         zone_class_(zone_class),
-        finder_(new DatabaseClient::Finder::Finder(accessor_, zone_id_,
-                                                   zone_name))
+        finder_(new DatabaseClient::Finder(accessor_, zone_id_, zone_name))
     {
         logger.debug(DBG_TRACE_DATA, DATASRC_DATABASE_UPDATER_CREATED)
             .arg(zone_name_).arg(zone_class_).arg(db_name_);
@@ -660,9 +659,7 @@ private:
     const string db_name_;
     const string zone_name_;
     const RRClass zone_class_;
-    boost::scoped_ptr<DatabaseClient::Finder::Finder> finder_;
-    string add_columns_[DatabaseAccessor::ADD_COLUMN_COUNT];
-    string del_params_[DatabaseAccessor::DEL_PARAM_COUNT];
+    boost::scoped_ptr<DatabaseClient::Finder> finder_;
 };
 
 void
@@ -676,6 +673,11 @@ DatabaseUpdater::addRRset(const RRset& rrset) {
                   << "added to " << zone_name_ << "/" << zone_class_ << ": "
                   << rrset.toText());
     }
+    if (rrset.getRRsig()) {
+        isc_throw(DataSourceError, "An RRset with RRSIG is being added to "
+                  << zone_name_ << "/" << zone_class_ << ": "
+                  << rrset.toText());
+    }
 
     RdataIteratorPtr it = rrset.getRdataIterator();
     if (it->isLast()) {
@@ -684,11 +686,12 @@ DatabaseUpdater::addRRset(const RRset& rrset) {
                   << rrset.getType());
     }
 
-    add_columns_[DatabaseAccessor::ADD_NAME] = rrset.getName().toText();
-    add_columns_[DatabaseAccessor::ADD_REV_NAME] =
+    string columns[DatabaseAccessor::ADD_COLUMN_COUNT]; // initialized with ""
+    columns[DatabaseAccessor::ADD_NAME] = rrset.getName().toText();
+    columns[DatabaseAccessor::ADD_REV_NAME] =
         rrset.getName().reverse().toText();
-    add_columns_[DatabaseAccessor::ADD_TTL] = rrset.getTTL().toText();
-    add_columns_[DatabaseAccessor::ADD_TYPE] = rrset.getType().toText();
+    columns[DatabaseAccessor::ADD_TTL] = rrset.getTTL().toText();
+    columns[DatabaseAccessor::ADD_TYPE] = rrset.getType().toText();
     for (; !it->isLast(); it->next()) {
         if (rrset.getType() == RRType::RRSIG()) {
             // XXX: the current interface (based on the current sqlite3
@@ -698,11 +701,11 @@ DatabaseUpdater::addRRset(const RRset& rrset) {
             // the interface, but until then we have to conform to the schema.
             const generic::RRSIG& rrsig_rdata =
                 dynamic_cast<const generic::RRSIG&>(it->getCurrent());
-            add_columns_[DatabaseAccessor::ADD_SIGTYPE] =
+            columns[DatabaseAccessor::ADD_SIGTYPE] =
                 rrsig_rdata.typeCovered().toText();
         }
-        add_columns_[DatabaseAccessor::ADD_RDATA] = it->getCurrent().toText();
-        accessor_->addRecordToZone(add_columns_);
+        columns[DatabaseAccessor::ADD_RDATA] = it->getCurrent().toText();
+        accessor_->addRecordToZone(columns);
     }
 }
 
@@ -717,6 +720,11 @@ DatabaseUpdater::deleteRRset(const RRset& rrset) {
                   << "deleted from " << zone_name_ << "/" << zone_class_
                   << ": " << rrset.toText());
     }
+    if (rrset.getRRsig()) {
+        isc_throw(DataSourceError, "An RRset with RRSIG is being deleted from "
+                  << zone_name_ << "/" << zone_class_ << ": "
+                  << rrset.toText());
+    }
 
     RdataIteratorPtr it = rrset.getRdataIterator();
     if (it->isLast()) {
@@ -725,12 +733,12 @@ DatabaseUpdater::deleteRRset(const RRset& rrset) {
                   << rrset.getType());
     }
 
-    del_params_[DatabaseAccessor::DEL_NAME] = rrset.getName().toText();
-    del_params_[DatabaseAccessor::DEL_TYPE] = rrset.getType().toText();
+    string params[DatabaseAccessor::DEL_PARAM_COUNT]; // initialized with ""
+    params[DatabaseAccessor::DEL_NAME] = rrset.getName().toText();
+    params[DatabaseAccessor::DEL_TYPE] = rrset.getType().toText();
     for (; !it->isLast(); it->next()) {
-        del_params_[DatabaseAccessor::DEL_RDATA] =
-            it->getCurrent().toText();
-        accessor_->deleteRecordInZone(del_params_);
+        params[DatabaseAccessor::DEL_RDATA] = it->getCurrent().toText();
+        accessor_->deleteRecordInZone(params);
     }
 }
 
@@ -742,13 +750,12 @@ DatabaseUpdater::commit() {
                   << db_name_);
     }
     accessor_->commitUpdateZone();
+    committed_ = true; // make sure the destructor won't trigger rollback
 
     // We release the accessor immediately after commit is completed so that
     // we don't hold the possible internal resource any longer.
     accessor_.reset();
 
-    committed_ = true;
-
     logger.debug(DBG_TRACE_DATA, DATASRC_DATABASE_UPDATER_COMMIT)
         .arg(zone_name_).arg(zone_class_).arg(db_name_);
 }
@@ -763,8 +770,8 @@ DatabaseClient::getUpdater(const isc::dns::Name& name, bool replace) const {
         return (ZoneUpdaterPtr());
     }
 
-     return (ZoneUpdaterPtr(new DatabaseUpdater(update_accessor, zone.second,
-                                                name, rrclass_)));
+    return (ZoneUpdaterPtr(new DatabaseUpdater(update_accessor, zone.second,
+                                               name, rrclass_)));
 }
 }
 }

src/lib/datasrc/memory_datasrc.h

@@ -267,7 +267,13 @@ public:
     virtual ZoneIteratorPtr getIterator(const isc::dns::Name& name) const;
 
     /// In-memory data source is read-only, so this derived method will
-    /// result in a NotImplemented (once merged) exception.
+    /// result in a NotImplemented exception.
+    ///
+    /// \note We plan to use a database-based data source as a backend
+    /// persistent storage for an in-memory data source.  When it's
+    /// implemented we may also want to allow the user of the in-memory client
+    /// to update via its updater (this may or may not be a good idea and
+    /// is subject to further discussions).
     virtual ZoneUpdaterPtr getUpdater(const isc::dns::Name& name,
                                       bool replace) const;
 

src/lib/datasrc/tests/database_unittest.cc

@@ -531,7 +531,7 @@ public:
 private:
     // The following member variables are storage and/or update work space
     // of the test zone.  The "master"s are the real objects that contain
-    // the data, and they are shared among by all accessors cloned from
+    // the data, and they are shared among all accessors cloned from
     // an initially created one.  The pointer members allow the sharing.
     // "readonly" is for normal lookups.  "update" is the workspace for
     // updates.  When update starts it will be initialized either as an
@@ -545,7 +545,6 @@ private:
     const Domains* empty_records_;
 
     // used as temporary storage during the building of the fake data
-    //Domains records;
 
     // used as temporary storage after searchForRecord() and during
     // getNextRecord() calls, as well as during the building of the
@@ -1679,6 +1678,15 @@ TYPED_TEST(DatabaseClientTest, addRRsetToNewZone) {
                    this->qtype_, this->rrttl_, ZoneFinder::SUCCESS,
                    this->expected_rdatas_, this->expected_sig_rdatas_);
     }
+
+    // Add the non RRSIG RRset again, to see the attempt of adding RRSIG
+    // causes any unexpected effect, in particular, whether the SIGTYPE
+    // field might remain.
+    this->updater_->addRRset(*this->rrset_);
+    const char* const rrset_added[] = {
+        "www.example.org.", "org.example.www.", "3600", "A", "", "192.0.2.2"
+    };
+    this->checkLastAdded(rrset_added);
 }
 
 TYPED_TEST(DatabaseClientTest, addRRsetToCurrentZone) {
@@ -1833,6 +1841,12 @@ TYPED_TEST(DatabaseClientTest, addAfterCommit) {
    EXPECT_THROW(this->updater_->addRRset(*this->rrset_), DataSourceError);
 }
 
+TYPED_TEST(DatabaseClientTest, addRRsetWithRRSIG) {
+    this->updater_ = this->client_->getUpdater(this->zname_, false);
+    this->rrset_->addRRsig(*this->rrsigset_);
+    EXPECT_THROW(this->updater_->addRRset(*this->rrset_), DataSourceError);
+}
+
 TYPED_TEST(DatabaseClientTest, deleteRRset) {
     shared_ptr<DatabaseClient::Finder> finder(this->getFinder());
 
@@ -2025,4 +2039,99 @@ TYPED_TEST(DatabaseClientTest, deleteEmptyRRset) {
                                  this->rrttl_));
     EXPECT_THROW(this->updater_->deleteRRset(*this->rrset_), DataSourceError);
 }
+
+TYPED_TEST(DatabaseClientTest, deleteRRsetWithRRSIG) {
+    this->updater_ = this->client_->getUpdater(this->zname_, false);
+    this->rrset_->addRRsig(*this->rrsigset_);
+    EXPECT_THROW(this->updater_->deleteRRset(*this->rrset_), DataSourceError);
+}
+
+TYPED_TEST(DatabaseClientTest, compoundUpdate) {
+    // This test case performs an arbitrary chosen add/delete operations
+    // in a single update transaction.  Essentially there is nothing new to
+    // test here, but there may be some bugs that was overlooked and can
+    // only happen in the compound update scenario, so we test it.
+
+    this->updater_ = this->client_->getUpdater(this->zname_, false);
+
+    // add a new RR to an existing RRset
+    this->updater_->addRRset(*this->rrset_);
+    this->expected_rdatas_.clear();
+    this->expected_rdatas_.push_back("192.0.2.1");
+    this->expected_rdatas_.push_back("192.0.2.2");
+    doFindTest(this->updater_->getFinder(), this->qname_, this->qtype_,
+               this->qtype_, this->rrttl_, ZoneFinder::SUCCESS,
+               this->expected_rdatas_, this->empty_rdatas_);
+
+    // delete an existing RR
+    this->rrset_.reset(new RRset(Name("www.example.org"), this->qclass_,
+                                 this->qtype_, this->rrttl_));
+    this->rrset_->addRdata(rdata::createRdata(this->rrset_->getType(),
+                                              this->rrset_->getClass(),
+                                              "192.0.2.1"));
+    this->updater_->deleteRRset(*this->rrset_);
+    this->expected_rdatas_.clear();
+    this->expected_rdatas_.push_back("192.0.2.2");
+    doFindTest(this->updater_->getFinder(), this->qname_, this->qtype_,
+               this->qtype_, this->rrttl_, ZoneFinder::SUCCESS,
+               this->expected_rdatas_, this->empty_rdatas_);
+
+    // re-add it
+    this->updater_->addRRset(*this->rrset_);
+    this->expected_rdatas_.push_back("192.0.2.1");
+    doFindTest(this->updater_->getFinder(), this->qname_, this->qtype_,
+               this->qtype_, this->rrttl_, ZoneFinder::SUCCESS,
+               this->expected_rdatas_, this->empty_rdatas_);
+
+    // add a new RR with a new name
+    const Name newname("newname.example.org");
+    const RRType newtype(RRType::AAAA());
+    doFindTest(this->updater_->getFinder(), newname, newtype, newtype,
+               this->rrttl_, ZoneFinder::NXDOMAIN, this->empty_rdatas_,
+               this->empty_rdatas_);
+    this->rrset_.reset(new RRset(newname, this->qclass_, newtype,
+                                 this->rrttl_));
+    this->rrset_->addRdata(rdata::createRdata(this->rrset_->getType(),
+                                              this->rrset_->getClass(),
+                                              "2001:db8::10"));
+    this->rrset_->addRdata(rdata::createRdata(this->rrset_->getType(),
+                                              this->rrset_->getClass(),
+                                              "2001:db8::11"));
+    this->updater_->addRRset(*this->rrset_);
+    this->expected_rdatas_.clear();
+    this->expected_rdatas_.push_back("2001:db8::10");
+    this->expected_rdatas_.push_back("2001:db8::11");
+    doFindTest(this->updater_->getFinder(), newname, newtype, newtype,
+               this->rrttl_, ZoneFinder::SUCCESS, this->expected_rdatas_,
+               this->empty_rdatas_);
+
+    // delete one RR from the previous set
+    this->rrset_.reset(new RRset(newname, this->qclass_, newtype,
+                                 this->rrttl_));
+    this->rrset_->addRdata(rdata::createRdata(this->rrset_->getType(),
+                                              this->rrset_->getClass(),
+                                              "2001:db8::11"));
+    this->updater_->deleteRRset(*this->rrset_);
+    this->expected_rdatas_.clear();
+    this->expected_rdatas_.push_back("2001:db8::10");
+    doFindTest(this->updater_->getFinder(), newname, newtype, newtype,
+               this->rrttl_, ZoneFinder::SUCCESS, this->expected_rdatas_,
+               this->empty_rdatas_);
+
+    // Commit the changes, confirm the entire changes applied.
+    this->updater_->commit();
+    shared_ptr<DatabaseClient::Finder> finder(this->getFinder());
+    this->expected_rdatas_.clear();
+    this->expected_rdatas_.push_back("192.0.2.2");
+    this->expected_rdatas_.push_back("192.0.2.1");
+    doFindTest(*finder, this->qname_, this->qtype_, this->qtype_, this->rrttl_,
+               ZoneFinder::SUCCESS, this->expected_rdatas_,
+               this->empty_rdatas_);
+
+    this->expected_rdatas_.clear();
+    this->expected_rdatas_.push_back("2001:db8::10");
+    doFindTest(*finder, newname, newtype, newtype, this->rrttl_,
+               ZoneFinder::SUCCESS, this->expected_rdatas_,
+               this->empty_rdatas_);
+}
 }

src/lib/datasrc/zone.h

@@ -214,7 +214,7 @@ typedef boost::shared_ptr<const ZoneFinder> ConstZoneFinderPtr;
 /// On construction, each derived class object will start a "transaction"
 /// for making updates to a specific zone (this means a constructor of
 /// a derived class would normally take parameters to identify the zone
-/// to be updated).  The underlying realization of a "transaction" will defer
+/// to be updated).  The underlying realization of a "transaction" will differ
 /// for different derived classes; if it uses a general purpose database
 /// as a backend, it will involve performing some form of "begin transaction"
 /// statement for the database.
@@ -279,6 +279,8 @@ public:
     /// It performs minimum level of validation on the specified RRset:
     /// - Whether the RR class is identical to that for the zone to be updated
     /// - Whether the RRset is not empty, i.e., it has at least one RDATA
+    /// - Whether the RRset is not associated with an RRSIG, i.e.,
+    ///   whether \c getRRsig() on the RRset returns a NULL pointer.
     ///
     /// and otherwise does not check any oddity.  For example, it doesn't
     /// check whether the owner name of the specified RRset is a subdomain
@@ -289,6 +291,12 @@ public:
     /// the existing data beforehand using the \c ZoneFinder returned by
     /// \c getFinder().
     ///
+    /// The validation requirement on the associated RRSIG is temporary.
+    /// If we find it more reasonable and useful to allow adding a pair of
+    /// RRset and its RRSIG RRset as we gain experiences with the interface,
+    /// we may remove this restriction.  Until then we explicitly check it
+    /// to prevent accidental misuse.
+    ///
     /// Conceptually, on successful call to this method, the zone will have
     /// the specified RRset, and if there is already an RRset of the same
     /// name and RR type, these two sets will be "merged".  "Merged" means
@@ -368,6 +376,8 @@ public:
     /// RRset:
     /// - Whether the RR class is identical to that for the zone to be updated
     /// - Whether the RRset is not empty, i.e., it has at least one RDATA
+    /// - Whether the RRset is not associated with an RRSIG, i.e.,
+    ///   whether \c getRRsig() on the RRset returns a NULL pointer.
     ///
     /// This method must not be called once commit() is performed.  If it
     /// calls after \c commit() the implementation must throw a