[master] Merge branch 'trac2575'

src/lib/datasrc/client.cc

@@ -28,20 +28,22 @@ namespace datasrc {
 
 ZoneIteratorPtr
 DataSourceClient::getIterator(const isc::dns::Name&, bool) const {
-    isc_throw(isc::NotImplemented,
-              "Data source doesn't support iteration");
+    isc_throw(isc::NotImplemented, "Data source doesn't support iteration");
 }
 
 unsigned int
 DataSourceClient::getZoneCount() const {
-    isc_throw(isc::NotImplemented,
-              "Data source doesn't support getZoneCount");
+    isc_throw(isc::NotImplemented, "Data source doesn't support getZoneCount");
 }
 
 bool
 DataSourceClient::createZone(const dns::Name&) {
-    isc_throw(isc::NotImplemented,
-              "Data source doesn't support addZone");
+    isc_throw(isc::NotImplemented, "Data source doesn't support createZone");
+}
+
+bool
+DataSourceClient::deleteZone(const dns::Name&) {
+    isc_throw(isc::NotImplemented, "Data source doesn't support deleteZone");
 }
 
 } // end namespace datasrc

src/lib/datasrc/client.h

@@ -385,9 +385,41 @@ public:
     ///                       direct zone creation.
     /// \throw DataSourceError If something goes wrong in the data source
     ///                        while creating the zone.
-    /// \param name The (fully qualified) name of the zone to create
+    /// \param zone_name The (fully qualified) name of the zone to create
     /// \return True if the zone was added, false if it already existed
-    virtual bool createZone(const dns::Name& name);
+    virtual bool createZone(const dns::Name& zone_name);
+
+    /// \brief Delete a zone from the data source
+    ///
+    /// This method also checks if the specified zone exists in the data
+    /// source, and returns true/false depending on whether the zone
+    /// existed/not existed, respectively.  In either case, on successful
+    /// return it ensures the data source does not contain the specified
+    /// name of the zone.
+    ///
+    /// \note This is a tentative API, and this method is likely to change
+    /// or be removed in the near future. For that reason, it currently
+    /// provides a default implementation that throws NotImplemented.
+    /// Note also that this method does not delete other database records
+    /// related to the zone, such as zone's resource records or differences
+    /// corresponding to updates made in the zone.  This is primarily for
+    /// implementation simplicity (in the currently intended usage there
+    /// wouldn't be such other data at the time of this call anyway) and due
+    /// to the fact that details of managing zones is still in flux.  Once
+    /// the design in this area is fixed we may revisit the behavior.
+    ///
+    /// Apart from the two exceptions mentioned below, in theory this
+    /// call can throw anything, depending on the implementation of
+    /// the datasource backend.
+    ///
+    /// \throw NotImplemented If the datasource backend does not support
+    ///                       direct zone deletion.
+    /// \throw DataSourceError If something goes wrong in the data source
+    ///                        while deleting the zone.
+    /// \param zone_name The (fully qualified) name of the zone to be deleted
+    /// \return true if the zone previously existed and has been deleted by
+    /// this method; false if the zone didn't exist.
+    virtual bool deleteZone(const dns::Name& zone_name);
 };
 }
 }

src/lib/datasrc/database.cc

@@ -117,13 +117,25 @@ DatabaseClient::findZone(const Name& name) const {
 }
 
 bool
-DatabaseClient::createZone(const Name& name) {
+DatabaseClient::createZone(const Name& zone_name) {
     TransactionHolder transaction(*accessor_);
-    std::pair<bool, int> zone(accessor_->getZone(name.toText()));
+    const std::pair<bool, int> zone(accessor_->getZone(zone_name.toText()));
     if (zone.first) {
         return (false);
     }
-    accessor_->addZone(name.toText());
+    accessor_->addZone(zone_name.toText());
+    transaction.commit();
+    return (true);
+}
+
+bool
+DatabaseClient::deleteZone(const Name& zone_name) {
+    TransactionHolder transaction(*accessor_);
+    const std::pair<bool, int> zinfo(accessor_->getZone(zone_name.toText()));
+    if (!zinfo.first) {         // if it doesn't exist just return false
+        return (false);
+    }
+    accessor_->deleteZone(zinfo.second);
     transaction.commit();
     return (true);
 }

src/lib/datasrc/database.h

@@ -155,7 +155,7 @@ public:
     ///
     /// It is empty, but needs a virtual one, since we will use the derived
     /// classes in polymorphic way.
-    virtual ~DatabaseAccessor() { }
+    virtual ~DatabaseAccessor() {}
 
     /// \brief Retrieve a zone identifier
     ///
@@ -164,8 +164,8 @@ public:
     /// apex), as the DatabaseClient will loop trough the labels itself and
     /// find the most suitable zone.
     ///
-    /// It is not specified if and what implementation of this method may throw,
-    /// so code should expect anything.
+    /// It is not specified if and what implementation of this method may
+    /// throw, so code should expect anything.
     ///
     /// \param name The (fully qualified) domain name of the zone's apex to be
     ///             looked up.
@@ -195,6 +195,23 @@ public:
     ///         or was created by this call).
     virtual int addZone(const std::string& name) = 0;
 
+    /// \brief Delete a zone from the database
+    ///
+    /// Like for deleteRecordToZone, implementations are not required to
+    /// check for the existence of the given zone name, it is the
+    /// responsibility of the caller to do so.
+    ///
+    /// Callers must also start a transaction before calling this method.
+    /// Implementations should throw InvalidOperation if this has not been
+    /// done. Callers should also expect DataSourceError for other potential
+    /// problems specific to the database.
+    ///
+    /// \note This method does not delete other database records related to
+    /// the zone.  See \c DataSourceClient::deleteZone for the rationale.
+    ///
+    /// \param zone_id The ID of the zone, that would be returned by getZone().
+    virtual void deleteZone(int zone_id) = 0;
+
     /// \brief This holds the internal context of ZoneIterator for databases
     ///
     /// While the ZoneIterator implementation from DatabaseClient does all the
@@ -212,15 +229,15 @@ public:
         /// \brief Destructor
         ///
         /// Virtual destructor, so any descendand class is destroyed correctly.
-        virtual ~IteratorContext() { }
+        virtual ~IteratorContext() {}
 
         /// \brief Function to provide next resource record
         ///
         /// This function should provide data about the next resource record
         /// from the data that is searched. The data is not converted yet.
         ///
-        /// Depending on how the iterator was constructed, there is a difference
-        /// in behaviour; for a 'full zone iterator', created with
+        /// Depending on how the iterator was constructed, there is a
+        /// difference in behaviour; for a 'full zone iterator', created with
         /// getAllRecords(), all COLUMN_COUNT elements of the array are
         /// overwritten.
         /// For a 'name iterator', created with getRecords(), the column
@@ -1399,7 +1416,9 @@ public:
     /// does not, creates it, commits, and returns true. If the zone
     /// does exist already, it does nothing (except abort the transaction)
     /// and returns false.
-    virtual bool createZone(const isc::dns::Name& name);
+    virtual bool createZone(const isc::dns::Name& zone_name);
+
+    virtual bool deleteZone(const isc::dns::Name& zone_name);
 
     /// \brief Get the zone iterator
     ///

src/lib/datasrc/sqlite3_accessor.cc

@@ -79,7 +79,8 @@ enum StatementID {
     DEL_ZONE_NSEC3_RECORDS = 20,
     DEL_NSEC3_RECORD = 21,
     ADD_ZONE = 22,
-    NUM_STATEMENTS = 23
+    DELETE_ZONE = 23,
+    NUM_STATEMENTS = 24
 };
 
 const char* const text_statements[NUM_STATEMENTS] = {
@@ -165,7 +166,9 @@ const char* const text_statements[NUM_STATEMENTS] = {
     "AND rdtype=?3 AND rdata=?4",
 
     // ADD_ZONE: add a zone to the zones table
-    "INSERT INTO zones (name, rdclass) VALUES (?1, ?2)" // ADD_ZONE
+    "INSERT INTO zones (name, rdclass) VALUES (?1, ?2)", // ADD_ZONE
+    // DELETE_ZONE: delete a zone from the zones table
+    "DELETE FROM zones WHERE id=?1" // DELETE_ZONE
 };
 
 struct SQLite3Parameters {
@@ -643,6 +646,19 @@ SQLite3Accessor::addZone(const std::string& name) {
     return (getzone_result.second);
 }
 
+void
+SQLite3Accessor::deleteZone(int zone_id) {
+    // Transaction should have been started by the caller
+    if (!dbparameters_->in_transaction) {
+        isc_throw(InvalidOperation, "performing deleteZone on SQLite3 "
+                  "data source without transaction");
+    }
+
+    StatementProcessor proc(*dbparameters_, DELETE_ZONE, "delete zone");
+    proc.bindInt(1, zone_id);
+    proc.exec();
+}
+
 namespace {
 
 // Conversion to plain char

src/lib/datasrc/sqlite3_accessor.h

@@ -139,6 +139,10 @@ public:
     /// \return the id of the zone that has been added
     virtual int addZone(const std::string& name);
 
+    // Nothing special to add for this implementation (the base class
+    // description is sufficient).
+    virtual void deleteZone(int zone_id);
+
     /// \brief Look up all resource records for a name
     ///
     /// This implements the getRecords() method from DatabaseAccessor

src/lib/datasrc/tests/database_unittest.cc

@@ -55,6 +55,7 @@ namespace {
 
 // Imaginary zone IDs used in the mock accessor below.
 const int READONLY_ZONE_ID = 42;
+const int NEW_ZONE_ID = 420;
 const int WRITABLE_ZONE_ID = 4200;
 
 // Commonly used test data
@@ -257,26 +258,43 @@ const char* TEST_NSEC3_RECORDS[][5] = {
  */
 class NopAccessor : public DatabaseAccessor {
 public:
-    NopAccessor() : database_name_("mock_database")
-    { }
+    NopAccessor() : database_name_("mock_database") {
+        zones_["example.org."] = READONLY_ZONE_ID;
+        zones_["null.example.org."] = 13;
+        zones_["empty.example.org."] = 0;
+        zones_["bad.example.org."] = -1;
+    }
 
     virtual std::pair<bool, int> getZone(const std::string& name) const {
-        if (name == "example.org.") {
-            return (std::pair<bool, int>(true, READONLY_ZONE_ID));
-        } else if (name == "null.example.org.") {
-            return (std::pair<bool, int>(true, 13));
-        } else if (name == "empty.example.org.") {
-            return (std::pair<bool, int>(true, 0));
-        } else if (name == "bad.example.org.") {
-            return (std::pair<bool, int>(true, -1));
+        std::map<std::string, int>::const_iterator found = zones_.find(name);
+        if (found != zones_.end()) {
+            return (std::pair<bool, int>(true, found->second));
         } else {
             return (std::pair<bool, int>(false, 0));
         }
     }
 
-    virtual int addZone(const std::string&) {
-        isc_throw(isc::NotImplemented,
-                  "This database datasource can't add zones");
+    // A simple implementation of addZone.
+    virtual int addZone(const std::string& zone_name) {
+        if (zone_name == "example.com.") {
+            zones_[zone_name] = NEW_ZONE_ID;
+        }
+
+        // for simplicity we assume zone_name is in zones_ at this point
+        return (zones_[zone_name]);
+    }
+
+    // A simple implementation of deleteZone.
+    virtual void deleteZone(int zone_id) {
+        std::map<std::string, int>::iterator it = zones_.begin();
+        std::map<std::string, int>::iterator end = zones_.end();
+        while (it != end) {
+            if (it->second == zone_id) {
+                zones_.erase(it);
+                return;
+            }
+            ++it;
+        }
     }
 
     virtual boost::shared_ptr<DatabaseAccessor> clone() {
@@ -337,7 +355,7 @@ public:
 
 private:
     const std::string database_name_;
-
+    std::map<std::string, int> zones_;
 };
 
 /**
@@ -438,7 +456,7 @@ public:
 
         // Check any attempt of multiple transactions
         if (did_transaction_) {
-            isc_throw(isc::Unexpected, "MockAccessor::startTransaction() "
+            isc_throw(DataSourceError, "MockAccessor::startTransaction() "
                       "called multiple times - likely a bug in the test");
         }
 
@@ -447,6 +465,14 @@ public:
         did_transaction_ = true;
     }
 
+    // If the test needs multiple calls to startTransaction() and knows it's
+    // safe, it can use this method to disable the safeguard check in
+    // startTransaction(); the test can also use this method by emulating a
+    // lock conflict by setting is_allowed to false.
+    void allowMoreTransaction(bool is_allowed) {
+        did_transaction_ = !is_allowed;
+    }
+
 private:
     class DomainIterator : public IteratorContext {
     public:
@@ -1293,6 +1319,14 @@ public:
         }
     }
 
+    // Mock-only; control whether to allow subsequent transaction.
+    void allowMoreTransaction(bool is_allowed) {
+        if (is_mock_) {
+            dynamic_cast<MockAccessor&>(*current_accessor_).
+                allowMoreTransaction(is_allowed);
+        }
+    }
+
     // Some tests only work for MockAccessor.  We remember whether our accessor
     // is of that type.
     bool is_mock_;
@@ -4104,51 +4138,86 @@ TYPED_TEST(DatabaseClientTest, createZone) {
         zone(this->client_->findZone(new_name));
     ASSERT_EQ(result::NOTFOUND, zone.code);
 
-    // The mock implementation does not do createZone,
-    // in which case it should throw NotImplemented (from
-    // the base class)
-    if (this->is_mock_) {
-        ASSERT_THROW(this->client_->createZone(new_name), isc::NotImplemented);
-    } else {
-        // But in the real case, it should work and return true
-        ASSERT_TRUE(this->client_->createZone(new_name));
-        const DataSourceClient::FindResult
-            zone2(this->client_->findZone(new_name));
-        ASSERT_EQ(result::SUCCESS, zone2.code);
-        // And the second call should return false since
-        // it already exists
-        ASSERT_FALSE(this->client_->createZone(new_name));
-    }
+    // Adding a new zone; it should work and return true
+    ASSERT_TRUE(this->client_->createZone(new_name));
+    const DataSourceClient::FindResult
+        zone2(this->client_->findZone(new_name));
+    ASSERT_EQ(result::SUCCESS, zone2.code);
+    // And the second call should return false since
+    // it already exists
+    this->allowMoreTransaction(true);
+    ASSERT_FALSE(this->client_->createZone(new_name));
 }
 
 TYPED_TEST(DatabaseClientTest, createZoneRollbackOnLocked) {
-    // skip test for mock
-    if (this->is_mock_) {
-        return;
-    }
-
     const Name new_name("example.com");
     isc::datasrc::ZoneUpdaterPtr updater =
         this->client_->getUpdater(this->zname_, true);
+    this->allowMoreTransaction(false);
     ASSERT_THROW(this->client_->createZone(new_name), DataSourceError);
     // createZone started a transaction as well, but since it failed,
     // it should have been rolled back. Roll back the other one as
     // well, and the next attempt should succeed
     updater.reset();
+    this->allowMoreTransaction(true);
     ASSERT_TRUE(this->client_->createZone(new_name));
 }
 
 TYPED_TEST(DatabaseClientTest, createZoneRollbackOnExists) {
-    // skip test for mock
-    if (this->is_mock_) {
-        return;
-    }
-
     const Name new_name("example.com");
     ASSERT_FALSE(this->client_->createZone(this->zname_));
-    // createZone started a transaction, but since it failed,
-    // it should have been rolled back, and the next attempt should succeed
+
+    // deleteZone started a transaction, but since the zone didn't even exist
+    // the transaction was not committed but should have been rolled back.
+    // The first transaction shouldn't leave any state, lock, etc, that
+    // would hinder the second attempt.
+    this->allowMoreTransaction(true);
     ASSERT_TRUE(this->client_->createZone(new_name));
 }
 
+TYPED_TEST(DatabaseClientTest, deleteZone) {
+    // Check the zone currently exists.
+    EXPECT_EQ(result::SUCCESS, this->client_->findZone(this->zname_).code);
+
+    // Deleting an existing zone; it should work and return true (previously
+    // existed and is now deleted)
+    EXPECT_TRUE(this->client_->deleteZone(this->zname_));
+
+    // Now it's not found by findZone
+    EXPECT_EQ(result::NOTFOUND, this->client_->findZone(this->zname_).code);
+
+    // And the second call should return false since it doesn't exist any more
+    this->allowMoreTransaction(true);
+    EXPECT_FALSE(this->client_->deleteZone(this->zname_));
+}
+
+TYPED_TEST(DatabaseClientTest, deleteZoneRollbackOnLocked) {
+    isc::datasrc::ZoneUpdaterPtr updater =
+        this->client_->getUpdater(this->zname_, true);
+
+    // updater locks the DB so deleteZone() will fail.
+    this->allowMoreTransaction(false);
+    EXPECT_THROW(this->client_->deleteZone(this->zname_), DataSourceError);
+
+    // deleteZone started a transaction as well, but since it failed,
+    // it should have been rolled back. Roll back the other one as
+    // well, and the next attempt should succeed
+    updater.reset();
+    this->allowMoreTransaction(true);
+    EXPECT_TRUE(this->client_->deleteZone(this->zname_));
+}
+
+TYPED_TEST(DatabaseClientTest, deleteZoneRollbackOnNotFind) {
+    // attempt of deleting non-existent zone.  result in false
+    const Name new_name("example.com");
+    EXPECT_FALSE(this->client_->deleteZone(new_name));
+
+    // deleteZone started a transaction, but since the zone didn't even exist
+    // the transaction was not committed but should have been rolled back.
+    // The first transaction shouldn't leave any state, lock, etc, that
+    // would hinder the second attempt.
+    this->allowMoreTransaction(true);
+    EXPECT_TRUE(this->client_->deleteZone(this->zname_));
+}
+
 }

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -20,6 +20,8 @@
 
 #include <dns/rrclass.h>
 
+#include <exceptions/exceptions.h>
+
 #include <sqlite3.h>
 
 #include <gtest/gtest.h>
@@ -1615,4 +1617,50 @@ TEST_F(SQLite3Update, addZoneWhileLocked) {
     EXPECT_FALSE(accessor->getZone(new_zone).first);
 }
 
+//
+// Tests for deleteZone() follow.
+//
+TEST_F(SQLite3Update, deleteZone) {
+    const std::pair<bool, int> zone_info(accessor->getZone("example.com."));
+    ASSERT_TRUE(zone_info.first);
+    zone_id = zone_info.second;
+
+    // Calling deleteZone without transaction should fail
+    EXPECT_THROW(accessor->deleteZone(zone_info.first), isc::InvalidOperation);
+
+    // Delete the zone.  Then confirm it, both before and after commit.
+    accessor->startTransaction();
+    accessor->deleteZone(zone_info.second);
+    EXPECT_FALSE(accessor->getZone("example.com.").first);
+    accessor->commit();
+    EXPECT_FALSE(accessor->getZone("example.com.").first);
+
+    // Records are not deleted.
+    std::string data[DatabaseAccessor::COLUMN_COUNT];
+    EXPECT_TRUE(accessor->getRecords("example.com.", zone_id, false)
+                ->getNext(data));
+}
+
+TEST_F(SQLite3Update, deleteZoneWhileLocked) {
+    const std::pair<bool, int> zone_info(accessor->getZone("example.com."));
+    ASSERT_TRUE(zone_info.first);
+    zone_id = zone_info.second;
+
+    // Adding another (not commit yet), it should lock the db
+    const std::string new_zone = "new.example.com.";
+    accessor->startTransaction();
+    zone_id = accessor->addZone(new_zone);
+
+    // deleteZone should throw an exception that it is locked
+    another_accessor->startTransaction();
+    EXPECT_THROW(another_accessor->deleteZone(zone_id), DataSourceError);
+    // Commit should do nothing, but not fail
+    another_accessor->commit();
+
+    accessor->rollback();
+
+    // The zone should still exist.
+    EXPECT_TRUE(accessor->getZone("example.com.").first);
+}
+
 } // end anonymous namespace