[1068] implemented deleting RRset

src/lib/datasrc/database.cc

@@ -338,7 +338,8 @@ DatabaseClient::Updater::Updater(shared_ptr<DatabaseAccessor> accessor,
     db_name_(accessor->getDBName()), zone_name_(zone_name),
     zone_class_(zone_class),
     finder_(new Finder(accessor_, zone_id_)),
-    add_columns_(DatabaseAccessor::ADD_COLUMN_COUNT)
+    add_columns_(DatabaseAccessor::ADD_COLUMN_COUNT),
+    del_params_(DatabaseAccessor::DEL_PARAM_COUNT)
 {
     logger.debug(DBG_TRACE_DATA, DATASRC_DATABASE_UPDATER_CREATED)
         .arg(zone_name_).arg(zone_class_).arg(db_name_);
@@ -379,12 +380,12 @@ DatabaseClient::Updater::addRRset(const RRset& rrset) {
                   << rrset.getType());
     }
 
-    add_columns_.clear();
-    add_columns_[DatabaseAccessor::ADD_NAME] = rrset.getName().toText();
-    add_columns_[DatabaseAccessor::ADD_REV_NAME] =
+    add_columns_.assign(DatabaseAccessor::ADD_COLUMN_COUNT, "");
+    add_columns_.at(DatabaseAccessor::ADD_NAME) = rrset.getName().toText();
+    add_columns_.at(DatabaseAccessor::ADD_REV_NAME) =
         rrset.getName().reverse().toText();
-    add_columns_[DatabaseAccessor::ADD_TTL] = rrset.getTTL().toText();
-    add_columns_[DatabaseAccessor::ADD_TYPE] = rrset.getType().toText();
+    add_columns_.at(DatabaseAccessor::ADD_TTL) = rrset.getTTL().toText();
+    add_columns_.at(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
@@ -394,15 +395,45 @@ DatabaseClient::Updater::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] =
+            add_columns_.at(DatabaseAccessor::ADD_SIGTYPE) =
                 rrsig_rdata.typeCovered().toText();
         }
-        add_columns_[DatabaseAccessor::ADD_RDATA] = it->getCurrent().toText();
+        add_columns_.at(DatabaseAccessor::ADD_RDATA) =
+            it->getCurrent().toText();
         accessor_->addRecordToZone(add_columns_);
     }
 }
 
 void
+DatabaseClient::Updater::deleteRRset(const RRset& rrset) {
+    if (committed_) {
+        isc_throw(DataSourceError, "Delete attempt after commit on zone: "
+                  << zone_name_ << "/" << zone_class_);
+    }
+    if (rrset.getClass() != zone_class_) {
+        isc_throw(DataSourceError, "An RRset of a different class is being "
+                  << "deleted from " << zone_name_ << "/" << zone_class_
+                  << ": " << rrset.toText());
+    }
+
+    RdataIteratorPtr it = rrset.getRdataIterator();
+    if (it->isLast()) {
+        isc_throw(DataSourceError, "An empty RRset is being deleted for "
+                  << rrset.getName() << "/" << zone_class_ << "/"
+                  << rrset.getType());
+    }
+
+    del_params_.assign(DatabaseAccessor::DEL_PARAM_COUNT, "");
+    del_params_.at(DatabaseAccessor::DEL_NAME) = rrset.getName().toText();
+    del_params_.at(DatabaseAccessor::DEL_TYPE) = rrset.getType().toText();
+    for (; !it->isLast(); it->next()) {
+        del_params_.at(DatabaseAccessor::DEL_RDATA) =
+            it->getCurrent().toText();
+        accessor_->deleteRecordInZone(del_params_);
+    }
+}
+
+void
 DatabaseClient::Updater::commit() {
     if (committed_) {
         isc_throw(DataSourceError, "Duplicate commit attempt for "

src/lib/datasrc/database.h

@@ -165,6 +165,12 @@ public:
         ADD_RDATA = 5    ///< Full text representation of the record's RDATA
     };
 
+    enum DeleteRecordParams {
+        DEL_NAME = 0, ///< The owner name of the record (a domain name)
+        DEL_TYPE = 1, ///< The RRType of the record (A/NS/TXT etc.)
+        DEL_RDATA = 2 ///< Full text representation of the record's RDATA
+    };
+
     /// TBD
     virtual std::pair<bool, int> startUpdateZone(const std::string& zone_name,
                                                  bool replace) = 0;
@@ -362,6 +368,7 @@ public:
         ~Updater();
         virtual ZoneFinder& getFinder();
         virtual void addRRset(const isc::dns::RRset& rrset);
+        virtual void deleteRRset(const isc::dns::RRset& rrset);
         virtual void commit();
 
     private:
@@ -373,6 +380,7 @@ public:
         isc::dns::RRClass zone_class_;
         boost::scoped_ptr<Finder::Finder> finder_;
         std::vector<std::string> add_columns_;
+        std::vector<std::string> del_params_;
     };
 
     /**

src/lib/datasrc/tests/database_unittest.cc

@@ -28,7 +28,10 @@
 
 #include <testutils/dnsmessage_test.h>
 
+#include <algorithm>
 #include <map>
+#include <vector>
+#include <string>
 
 using namespace isc::datasrc;
 using namespace std;
@@ -158,7 +161,33 @@ public:
         // remember this one so that test cases can check it.
         columns_lastadded = columns;
     }
-    virtual void deleteRecordInZone(const vector<string>&) {}
+
+    // Helper predicate class used in deleteRecordInZone().
+    struct deleteMatch {
+        deleteMatch(const string& type, const string& rdata) :
+            type_(type), rdata_(rdata)
+        {}
+        bool operator()(const vector<string>& row) const {
+            return (row[0] == type_ && row[3] == rdata_);
+        }
+        const string& type_;
+        const string& rdata_;
+    };
+
+    virtual void deleteRecordInZone(const vector<string>& params) {
+        vector<vector<string> > records =
+            update_records[params[DatabaseAccessor::DEL_NAME]];
+        records.erase(remove_if(records.begin(), records.end(),
+                                deleteMatch(
+                                    params[DatabaseAccessor::DEL_TYPE],
+                                    params[DatabaseAccessor::DEL_RDATA])),
+                      records.end());
+        if (records.empty()) {
+            update_records.erase(params[DatabaseAccessor::DEL_NAME]);
+        } else {
+            update_records[params[DatabaseAccessor::DEL_NAME]] = records;
+        }
+    }
 
     bool searchRunning() const {
         return (search_running_);
@@ -356,7 +385,7 @@ protected:
 
         rrset.reset(new RRset(qname, RRClass::IN(), qtype, rrttl));
         // Adding an IN/A RDATA.  Intentionally using different data
-        // than the initial data configured MockAccessor::fillData().
+        // than the initial data configured in MockAccessor::fillData().
         rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
                                            "192.0.2.2"));
 
@@ -931,6 +960,24 @@ TEST_F(DatabaseClientTest, addRRsetToCurrentZone) {
     }
 }
 
+TEST_F(DatabaseClientTest, addMultipleRRs) {
+    // Similar to the previous case, but the added RRset contains multiple
+    // RRs.
+    updater = client_->startUpdateZone(Name("example.org"), false);
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "192.0.2.3"));
+    updater->addRRset(*rrset);
+    expected_rdatas.clear();
+    expected_rdatas.push_back("192.0.2.1");
+    expected_rdatas.push_back("192.0.2.2");
+    expected_rdatas.push_back("192.0.2.3");
+    {
+        SCOPED_TRACE("add multiple RRs");
+        doFindTest(updater->getFinder(), qname, qtype, qtype, rrttl,
+                   ZoneFinder::SUCCESS, expected_rdatas, empty_rdatas);
+    }
+}
+
 TEST_F(DatabaseClientTest, addRRsetOfLargerTTL) {
     // Similar to the previous one, but the TTL of the added RRset is larger
     // than that of the existing record.  The finder should use the smaller
@@ -1025,7 +1072,169 @@ TEST_F(DatabaseClientTest, addAfterCommit) {
    EXPECT_THROW(updater->addRRset(*rrset), DataSourceError);
 }
 
-// delete rrset without rdata; not necessarily harmful, but treat it as an error.
-// delete after commit.  should error
+TEST_F(DatabaseClientTest, deleteRRset) {
+    rrset.reset(new RRset(qname, RRClass::IN(), qtype, rrttl));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "192.0.2.1"));
+
+    // Delete one RR from an RRset
+    updater = client_->startUpdateZone(Name("example.org"), false);
+    updater->deleteRRset(*rrset);
+
+    // Delete the only RR of a name
+    rrset.reset(new RRset(Name("cname.example.org"), RRClass::IN(),
+                          RRType::CNAME(), rrttl));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "www.example.org"));
+    updater->deleteRRset(*rrset);
+
+    // The updater finder should immediately see the deleted results.
+    {
+        SCOPED_TRACE("delete RRset");
+        doFindTest(updater->getFinder(), qname, qtype, qtype, rrttl,
+                   ZoneFinder::NXRRSET, empty_rdatas, empty_rdatas);
+        doFindTest(updater->getFinder(), Name("cname.example.org"),
+                   qtype, qtype, rrttl, ZoneFinder::NXDOMAIN,
+                   empty_rdatas, empty_rdatas);
+    }
+
+    // before committing the change, the original finder should see the
+    // original record.
+    {
+        SCOPED_TRACE("delete RRset before commit");
+        expected_rdatas.push_back("192.0.2.1");
+        doFindTest(*finder, qname, qtype, qtype, rrttl,
+                   ZoneFinder::SUCCESS, expected_rdatas, empty_rdatas);
+
+        expected_rdatas.clear();
+        expected_rdatas.push_back("www.example.org.");
+        doFindTest(*finder, Name("cname.example.org"), qtype,
+                   RRType::CNAME(), rrttl,
+                   ZoneFinder::CNAME, expected_rdatas, empty_rdatas);
+    }
+
+    // once committed, the record should be removed from the original finder's
+    // view, too.
+    updater->commit();
+    {
+        SCOPED_TRACE("delete RRset after commit");
+        doFindTest(*finder, qname, qtype, qtype, rrttl,
+                   ZoneFinder::NXRRSET, empty_rdatas, empty_rdatas);
+        doFindTest(*finder, Name("cname.example.org"),
+                   qtype, qtype, rrttl, ZoneFinder::NXDOMAIN,
+                   empty_rdatas, empty_rdatas);
+    }
+}
+
+TEST_F(DatabaseClientTest, deleteRRsetToNXDOMAIN) {
+    // similar to the previous case, but it removes the only record of the
+    // given name.  a subsequent find() should result in NXDOMAIN.
+    rrset.reset(new RRset(Name("cname.example.org"), RRClass::IN(),
+                          RRType::CNAME(), rrttl));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "www.example.org"));
+
+    updater = client_->startUpdateZone(Name("example.org"), false);
+    updater->deleteRRset(*rrset);
+    {
+        SCOPED_TRACE("delete RRset to NXDOMAIN");
+        doFindTest(updater->getFinder(), Name("cname.example.org"), qtype,
+                   qtype, rrttl, ZoneFinder::NXDOMAIN, empty_rdatas,
+                   empty_rdatas);
+    }
+}
+
+TEST_F(DatabaseClientTest, deleteMultipleRRs) {
+    rrset.reset(new RRset(qname, RRClass::IN(), RRType::AAAA(), rrttl));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "2001:db8::1"));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "2001:db8::2"));
+
+    updater = client_->startUpdateZone(Name("example.org"), false);
+    updater->deleteRRset(*rrset);
+
+    {
+        SCOPED_TRACE("delete multiple RRs");
+        doFindTest(updater->getFinder(), qname, RRType::AAAA(), qtype, rrttl,
+                   ZoneFinder::NXRRSET, empty_rdatas, empty_rdatas);
+    }
+}
+
+TEST_F(DatabaseClientTest, partialDelete) {
+    rrset.reset(new RRset(qname, RRClass::IN(), RRType::AAAA(), rrttl));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "2001:db8::1"));
+    // This does not exist in the test data source:
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "2001:db8::3"));
+
+    // deleteRRset should succeed "silently", and subsequent find() should
+    // find the remaining RR.
+    updater = client_->startUpdateZone(Name("example.org"), false);
+    updater->deleteRRset(*rrset);
+    {
+        SCOPED_TRACE("partial delete");
+        expected_rdatas.push_back("2001:db8::2");
+        doFindTest(updater->getFinder(), qname, RRType::AAAA(),
+                   RRType::AAAA(), rrttl, ZoneFinder::SUCCESS,
+                   expected_rdatas, empty_rdatas);
+    }
+}
+
+TEST_F(DatabaseClientTest, deleteNoMatch) {
+    // similar to the previous test, but there's not even a match in the
+    // specified RRset.  Essentially there's no difference in the result.
+    updater = client_->startUpdateZone(Name("example.org"), false);
+    updater->deleteRRset(*rrset);
+    {
+        SCOPED_TRACE("delete no match");
+        expected_rdatas.push_back("192.0.2.1");
+        doFindTest(updater->getFinder(), qname, qtype, qtype, rrttl,
+                   ZoneFinder::SUCCESS, expected_rdatas, empty_rdatas);
+    }
+}
 
+TEST_F(DatabaseClientTest, deleteWithDifferentTTL) {
+    // Our delete interface simply ignores TTL (may change in a future version)
+    rrset.reset(new RRset(qname, RRClass::IN(), qtype, RRTTL(1800)));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "192.0.2.1"));
+    updater = client_->startUpdateZone(Name("example.org"), false);
+    updater->deleteRRset(*rrset);
+    {
+        SCOPED_TRACE("delete RRset with a different TTL");
+        doFindTest(updater->getFinder(), qname, qtype, qtype, rrttl,
+                   ZoneFinder::NXRRSET, empty_rdatas, empty_rdatas);
+    }
+}
+
+TEST_F(DatabaseClientTest, deleteDeviantRR) {
+    updater = client_->startUpdateZone(Name("example.org"), false);
+
+    // RR class mismatch.  This should be detected and rejected.
+    rrset.reset(new RRset(qname, RRClass::CH(), RRType::TXT(), rrttl));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "test text"));
+    EXPECT_THROW(updater->deleteRRset(*rrset), DataSourceError);
+
+    // Out-of-zone owner name.  At a higher level this should be rejected,
+    // but it doesn't happen in this interface.
+    rrset.reset(new RRset(Name("example.com"), RRClass::IN(), qtype, rrttl));
+    rrset->addRdata(rdata::createRdata(rrset->getType(), rrset->getClass(),
+                                       "192.0.2.100"));
+    EXPECT_NO_THROW(updater->deleteRRset(*rrset));
+}
+
+TEST_F(DatabaseClientTest, deleteAfterCommit) {
+   updater = client_->startUpdateZone(Name("example.org"), false);
+   updater->commit();
+   EXPECT_THROW(updater->deleteRRset(*rrset), DataSourceError);
+}
+
+TEST_F(DatabaseClientTest, deleteEmptyRRset) {
+    updater = client_->startUpdateZone(Name("example.org"), false);
+    rrset.reset(new RRset(qname, RRClass::IN(), qtype, rrttl));
+    EXPECT_THROW(updater->deleteRRset(*rrset), DataSourceError);
+}
 }

src/lib/datasrc/zone.h

@@ -239,6 +239,11 @@ public:
 
     /// TBD
     ///
+    /// how to handle TTL?
+    virtual void deleteRRset(const isc::dns::RRset& rrset) = 0;
+
+    /// TBD
+    ///
     /// This operation can only be performed at most once.  A duplicate call
     /// must result in a DatasourceError exception.
     virtual void commit() = 0;