[1068] supported commit and (implicit) rollback operation.

added some log messages.  to make the log/exception messages useful,
also added an RRClass parameter to the Client constructor.

src/lib/datasrc/database.cc

@@ -12,12 +12,14 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
+#include <string>
 #include <vector>
 
 #include <datasrc/database.h>
 
 #include <exceptions/exceptions.h>
 #include <dns/name.h>
+#include <dns/rrclass.h>
 #include <dns/rrttl.h>
 #include <dns/rdata.h>
 #include <dns/rdataclass.h>
@@ -27,15 +29,18 @@
 
 #include <boost/foreach.hpp>
 
+using namespace std;
 using boost::shared_ptr;
 using isc::dns::Name;
+using isc::dns::RRClass;
 
 namespace isc {
 namespace datasrc {
 
-DatabaseClient::DatabaseClient(boost::shared_ptr<DatabaseAccessor>
+DatabaseClient::DatabaseClient(RRClass rrclass,
+                               boost::shared_ptr<DatabaseAccessor>
                                accessor) :
-    accessor_(accessor)
+    rrclass_(rrclass), accessor_(accessor)
 {
     if (!accessor_) {
         isc_throw(isc::InvalidParameter,
@@ -321,21 +326,55 @@ DatabaseClient::startUpdateZone(const isc::dns::Name& name,
         return (ZoneUpdaterPtr());
     }
 
-    // At this moment this one cannot anything except giving a finder.
-    return (ZoneUpdaterPtr(new DatabaseClient::Updater(accessor_,
-                                                       zone.second)));
+     return (ZoneUpdaterPtr(new Updater(accessor_, zone.second,
+                                        name.toText(), rrclass_.toText())));
 }
 
 DatabaseClient::Updater::Updater(shared_ptr<DatabaseAccessor> accessor,
-                                 int zone_id) :
-    accessor_(accessor), zone_id_(zone_id),
-    finder_(new Finder::Finder(accessor_, zone_id_))
+                                 int zone_id, const string& zone_name,
+                                 const string& class_name) :
+    committed_(false), accessor_(accessor), zone_id_(zone_id),
+    db_name_(accessor->getDBName()), zone_name_(zone_name),
+    class_name_(class_name),
+    finder_(new Finder(accessor_, zone_id_))
 {
+    logger.debug(DBG_TRACE_DATA, DATASRC_DATABASE_UPDATER_CREATED)
+        .arg(zone_name_).arg(class_name_).arg(db_name_);
+}
+
+DatabaseClient::Updater::~Updater() {
+    if (!committed_) {
+        accessor_->rollbackUpdateZone();
+        logger.info(DATASRC_DATABASE_UPDATER_ROLLBACK)
+            .arg(zone_name_).arg(class_name_).arg(db_name_);
+    }
+
+    logger.debug(DBG_TRACE_DATA, DATASRC_DATABASE_UPDATER_DESTROYED)
+        .arg(zone_name_).arg(class_name_).arg(db_name_);
 }
 
 ZoneFinder&
 DatabaseClient::Updater::getFinder() {
     return (*finder_);
 }
+
+void
+DatabaseClient::Updater::commit() {
+    if (committed_) {
+        isc_throw(DataSourceError, "Duplicate commit attempt for "
+                  << zone_name_ << "/" << class_name_ << " on "
+                  << db_name_);
+    }
+    accessor_->commitUpdateZone();
+
+    // 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(class_name_).arg(db_name_);
+}
 }
 }

src/lib/datasrc/database.h

@@ -15,8 +15,12 @@
 #ifndef __DATABASE_DATASRC_H
 #define __DATABASE_DATASRC_H
 
+#include <string>
+
 #include <boost/scoped_ptr.hpp>
 
+#include <dns/rrclass.h>
+
 #include <datasrc/client.h>
 
 namespace isc {
@@ -230,11 +234,14 @@ public:
      * \exception isc::InvalidParameter if database is NULL. It might throw
      * standard allocation exception as well, but doesn't throw anything else.
      *
+     * \param rrclass The RR class of the zones that this client will handle.
      * \param database The database to use to get data. As the parameter
      *     suggests, the client takes ownership of the database and will
      *     delete it when itself deleted.
      */
-    DatabaseClient(boost::shared_ptr<DatabaseAccessor> database);
+    DatabaseClient(isc::dns::RRClass rrclass,
+                   boost::shared_ptr<DatabaseAccessor> database);
+
     /**
      * \brief Corresponding ZoneFinder implementation
      *
@@ -337,12 +344,19 @@ public:
 
     class Updater : public ZoneUpdater {
     public:
-        Updater(boost::shared_ptr<DatabaseAccessor> database, int zone_id);
+        Updater(boost::shared_ptr<DatabaseAccessor> database, int zone_id,
+                const std::string& zone_name, const std::string& class_name);
+        ~Updater();
         virtual ZoneFinder& getFinder();
+        virtual void commit();
 
     private:
+        bool committed_;
         boost::shared_ptr<DatabaseAccessor> accessor_;
         const int zone_id_;
+        std::string db_name_;
+        std::string zone_name_;
+        std::string class_name_;
         boost::scoped_ptr<Finder::Finder> finder_;
     };
 
@@ -367,6 +381,9 @@ public:
                                            bool replace) const;
 
 private:
+    /// \brief The RR class that this client handles.
+    const isc::dns::RRClass rrclass_;
+
     /// \brief The accessor to our database.
     const boost::shared_ptr<DatabaseAccessor> accessor_;
 };

src/lib/datasrc/datasrc_messages.mes

@@ -549,3 +549,25 @@ data source.
 % DATASRC_UNEXPECTED_QUERY_STATE unexpected query state
 This indicates a programming error. An internal task of unknown type was
 generated.
+
+% DATASRC_DATABASE_UPDATER_CREATED zone updater created for '%1/%2' on %3
+Debug information.  A zone updater object is created to make updates to
+the shown zone on the shown backend database.
+
+% DATASRC_DATABASE_UPDATER_DESTROYED zone updater destroyed for '%1/%2' on %3
+Debug information.  A zone updater object is destroyed, either successfully
+or after failure of, making updates to the shown zone on the shown backend
+database.
+
+%DATASRC_DATABASE_UPDATER_ROLLBACK zone updates roll-backed for '%1/%2' on %3
+A zone updater is being destroyed without committing the changes.
+This would typically mean the update attempt was aborted due to some
+error, but may also be a bug of the application that forgets committing
+the changes.  The intermediate changes made through the updater won't
+be applied to the underlying database.  The zone name, its class, and
+the underlying database name are shown in the log message.
+
+% DATASRC_DATABASE_UPDATER_COMMIT updates committed for '%1/%2' on %3
+Debug information.  A set of updates to a zone has been successfully
+committed to the corresponding database backend.  The zone name,
+its class and the database name are printed.

src/lib/datasrc/tests/database_unittest.cc

@@ -32,6 +32,7 @@ using namespace std;
 using namespace boost;
 using isc::dns::Name;
 using isc::dns::RRType;
+using isc::dns::RRClass;
 using isc::dns::RRTTL;
 
 namespace {
@@ -47,8 +48,8 @@ class MockAccessor : public DatabaseAccessor {
     typedef std::map<std::string, std::vector< std::vector<std::string> > >
     RECORDS;
 public:
-    MockAccessor() : search_running_(false),
-                       database_name_("mock_database")
+    MockAccessor() : search_running_(false), rollbacked_(false),
+                     database_name_("mock_database")
     {
         fillData();
     }
@@ -112,16 +113,20 @@ public:
             resetSearch();
             return (false);
         }
-    };
+    }
 
     virtual void resetSearch() {
         search_running_ = false;
-    };
+    }
 
     bool searchRunning() const {
         return (search_running_);
     }
 
+    bool isRollbacked() const {
+        return (rollbacked_);
+    }
+
     virtual const std::string& getDBName() const {
         return (database_name_);
     }
@@ -141,6 +146,10 @@ private:
     // when it encounters an error
     bool search_running_;
 
+    // Whether rollback operation has been performed for the database.
+    // Not useful except for purely testing purpose.
+    bool rollbacked_;
+
     // We store the name passed to searchForRecords, so we can
     // hardcode some exceptions into getNextRecord
     std::string searched_name_;
@@ -303,15 +312,19 @@ private:
 
         return (pair<bool, int>(true, WRITABLE_ZONE_ID));
     }
-    virtual void commitUpdateZone() {}
-    virtual void rollbackUpdateZone() {}
+    virtual void commitUpdateZone() {
+        readonly_records = update_records;
+    }
+    virtual void rollbackUpdateZone() {
+        rollbacked_ = true;
+    }
     virtual void addRecordToZone(const vector<string>&) {}
     virtual void deleteRecordInZone(const vector<string>&) {}
 };
 
 class DatabaseClientTest : public ::testing::Test {
 protected:
-    DatabaseClientTest() {
+    DatabaseClientTest() : qname("www.example.org"), qtype("A") {
         createClient();
     }
     /*
@@ -320,14 +333,19 @@ protected:
      */
     void createClient() {
         current_accessor_ = new MockAccessor();
-        client_.reset(new DatabaseClient(shared_ptr<DatabaseAccessor>(
-             current_accessor_)));
+        client_.reset(new DatabaseClient(RRClass::IN(),
+                                         shared_ptr<DatabaseAccessor>(
+                                             current_accessor_)));
     }
     // Will be deleted by client_, just keep the current value for comparison.
     MockAccessor* current_accessor_;
     shared_ptr<DatabaseClient> client_;
     const std::string database_name_;
 
+    const Name qname;           // commonly used name to be found
+    const RRType qtype;         // commonly used RR type with qname
+
+    ZoneUpdaterPtr updater;
     const std::vector<std::string> empty_rdatas; // for NXRRSET/NXDOMAIN
     std::vector<std::string> expected_rdatas;
     std::vector<std::string> expected_sig_rdatas;
@@ -366,9 +384,7 @@ TEST_F(DatabaseClientTest, superZone) {
 }
 
 TEST_F(DatabaseClientTest, noAccessorException) {
-    // We need a dummy variable here; some compiler would regard it a mere
-    // declaration instead of an instantiation and make the test fail.
-    EXPECT_THROW(DatabaseClient dummy((shared_ptr<DatabaseAccessor>())),
+    EXPECT_THROW(DatabaseClient(RRClass::IN(), shared_ptr<DatabaseAccessor>()),
                  isc::InvalidParameter);
 }
 
@@ -740,8 +756,7 @@ TEST_F(DatabaseClientTest, find) {
 }
 
 TEST_F(DatabaseClientTest, updaterFinder) {
-    ZoneUpdaterPtr updater = client_->startUpdateZone(Name("example.org"),
-                                                      false);
+    updater = client_->startUpdateZone(Name("example.org"), false);
     ASSERT_TRUE(updater);
 
     // If this update isn't replacing the zone, the finder should work
@@ -765,4 +780,55 @@ TEST_F(DatabaseClientTest, updaterFinder) {
                empty_rdatas, empty_rdatas);
 }
 
+TEST_F(DatabaseClientTest, flushZone) {
+    // A simple update case: flush the entire zone
+
+    // Before update, the name exists.
+    ZoneFinderPtr finder = client_->findZone(Name("example.org")).zone_finder;
+    EXPECT_EQ(ZoneFinder::SUCCESS, finder->find(qname, qtype).code);
+
+    // start update in the replace mode.  the normal finder should still
+    // be able to see the record, but the updater's finder shouldn't.
+    updater = client_->startUpdateZone(Name("example.org"), true);
+    EXPECT_EQ(ZoneFinder::SUCCESS, finder->find(qname, qtype).code);
+    EXPECT_EQ(ZoneFinder::NXDOMAIN,
+              updater->getFinder().find(qname, qtype).code);
+
+    // commit the update.  now the normal finder shouldn't see it.
+    updater->commit();
+    EXPECT_EQ(ZoneFinder::NXDOMAIN, finder->find(qname, qtype).code);
+
+    // Check rollback wasn't accidentally performed.
+    EXPECT_FALSE(current_accessor_->isRollbacked());
+}
+
+TEST_F(DatabaseClientTest, updateCancel) {
+    // similar to the previous test, but destruct the updater before commit.
+
+    ZoneFinderPtr finder = client_->findZone(Name("example.org")).zone_finder;
+    EXPECT_EQ(ZoneFinder::SUCCESS, finder->find(qname, qtype).code);
+
+    updater = client_->startUpdateZone(Name("example.org"), true);
+    EXPECT_EQ(ZoneFinder::NXDOMAIN,
+              updater->getFinder().find(qname, qtype).code);
+    // DB should not have been rolled back yet.
+    EXPECT_FALSE(current_accessor_->isRollbacked());
+    updater.reset();            // destruct without commit
+
+    // reset() should have triggered rollback (although it doesn't affect
+    // anything to the mock accessor implementation except for the result of
+    // isRollbacked())
+    EXPECT_TRUE(current_accessor_->isRollbacked());
+    EXPECT_EQ(ZoneFinder::SUCCESS, finder->find(qname, qtype).code);
+}
+
+TEST_F(DatabaseClientTest, duplicateCommit) {
+    // duplicate commit.  should result in exception.
+    updater = client_->startUpdateZone(Name("example.org"), true);
+    updater->commit();
+    EXPECT_THROW(updater->commit(), DataSourceError);
+}
+
+// add/delete after commit.  should error
+
 }

src/lib/datasrc/zone.h

@@ -209,9 +209,20 @@ typedef boost::shared_ptr<const ZoneFinder> ConstZoneFinderPtr;
 
 /// The base class to make updates to a single zone.
 class ZoneUpdater {
+protected:
+    ZoneUpdater() {}
+
 public:
+    virtual ~ZoneUpdater() {}
+
     /// TBD
     virtual ZoneFinder& getFinder() = 0;
+
+    /// TBD
+    ///
+    /// This operation can only be performed at most once.  A duplicate call
+    /// must result in a DatasourceError exception.
+    virtual void commit() = 0;
 };
 
 /// \brief A pointer-like type pointing to a \c ZoneUpdater object.