[1287] a small refactoring: removed "Update" from commit/rollback so that

they can be used in more generic context.  We'll use them in subsequent
changes.  other than the method name change no behavior change yet.

src/lib/datasrc/database.cc

@@ -819,13 +819,13 @@ public:
     virtual ~DatabaseUpdater() {
         if (!committed_) {
             try {
-                accessor_->rollbackUpdateZone();
+                accessor_->rollback();
                 logger.info(DATASRC_DATABASE_UPDATER_ROLLBACK)
                     .arg(zone_name_).arg(zone_class_).arg(db_name_);
             } catch (const DataSourceError& e) {
                 // We generally expect that rollback always succeeds, and
                 // it should in fact succeed in a way we execute it.  But
-                // as the public API allows rollbackUpdateZone() to fail and
+                // as the public API allows rollback() to fail and
                 // throw, we should expect it.  Obviously we cannot re-throw
                 // it.  The best we can do is to log it as a critical error.
                 logger.error(DATASRC_DATABASE_UPDATER_ROLLBACKFAIL)
@@ -941,7 +941,7 @@ DatabaseUpdater::commit() {
                   << zone_name_ << "/" << zone_class_ << " on "
                   << db_name_);
     }
-    accessor_->commitUpdateZone();
+    accessor_->commit();
     committed_ = true; // make sure the destructor won't trigger rollback
 
     // We release the accessor immediately after commit is completed so that

src/lib/datasrc/database.h

@@ -382,12 +382,12 @@ public:
     virtual void deleteRecordInZone(
         const std::string (&params)[DEL_PARAM_COUNT]) = 0;
 
-    /// Commit updates to the zone.
+    /// Commit a transaction.
     ///
-    /// This method completes a transaction of making updates to the zone
-    /// in the context started by startUpdateZone.
+    /// This method completes a transaction started by \c startTransaction
+    /// or \c startUpdateZone.
     ///
-    /// A successful call to \c startUpdateZone() must have preceded to
+    /// A successful call to one of the "start" methods must have preceded to
     /// this call; otherwise a \c DataSourceError exception will be thrown.
     /// Once this method successfully completes, the transaction isn't
     /// considered to exist any more.  So a new transaction can now be
@@ -403,17 +403,16 @@ public:
     ///
     /// \exception DataSourceError Call without a transaction, duplicate call
     /// to the method or internal database error.
-    virtual void commitUpdateZone() = 0;
+    virtual void commit() = 0;
 
-    /// Rollback updates to the zone made so far.
+    /// Rollback any changes in a transaction made so far.
     ///
-    /// This method rollbacks a transaction of making updates to the zone
-    /// in the context started by startUpdateZone.  When it succeeds
-    /// (it normally should, but see below), the underlying database should
-    /// be reverted to the point before performing the corresponding
-    /// \c startUpdateZone().
+    /// This method rollbacks a transaction started by \c startTransaction or
+    /// \c startUpdateZone.  When it succeeds (it normally should, but see
+    /// below), the underlying database should be reverted to the point
+    /// before performing the corresponding "start" method.
     ///
-    /// A successful call to \c startUpdateZone() must have preceded to
+    /// A successful call to one of the "start" method must have preceded to
     /// this call; otherwise a \c DataSourceError exception will be thrown.
     /// Once this method successfully completes, the transaction isn't
     /// considered to exist any more.  So a new transaction can now be
@@ -430,7 +429,7 @@ public:
     ///
     /// \exception DataSourceError Call without a transaction, duplicate call
     /// to the method or internal database error.
-    virtual void rollbackUpdateZone() = 0;
+    virtual void rollback() = 0;
 
     /// Clone the accessor with the same configuration.
     ///

src/lib/datasrc/sqlite3_accessor.cc

@@ -86,7 +86,8 @@ const char* const text_statements[NUM_STATEMENTS] = {
 
 struct SQLite3Parameters {
     SQLite3Parameters() :
-        db_(NULL), version_(-1), updating_zone(false), updated_zone_id(-1)
+        db_(NULL), version_(-1), in_transaction(false), updating_zone(false),
+        updated_zone_id(-1)
     {
         for (int i = 0; i < NUM_STATEMENTS; ++i) {
             statements_[i] = NULL;
@@ -96,8 +97,9 @@ struct SQLite3Parameters {
     sqlite3* db_;
     int version_;
     sqlite3_stmt* statements_[NUM_STATEMENTS];
-    bool updating_zone;         // whether or not updating the zone
-    int updated_zone_id;        // valid only when updating_zone is true
+    bool in_transaction; // whether or not a transaction has been started
+    bool updating_zone;          // whether or not updating the zone
+    int updated_zone_id;        // valid only when in_transaction is true
 };
 
 // This is a helper class to encapsulate the code logic of executing
@@ -539,7 +541,7 @@ SQLite3Accessor::getAllRecords(int id) const {
 
 pair<bool, int>
 SQLite3Accessor::startUpdateZone(const string& zone_name, const bool replace) {
-    if (dbparameters_->updating_zone) {
+    if (dbparameters_->in_transaction || dbparameters_->updating_zone) {
         isc_throw(DataSourceError,
                   "duplicate zone update on SQLite3 data source");
     }
@@ -577,6 +579,7 @@ SQLite3Accessor::startUpdateZone(const string& zone_name, const bool replace) {
         }
     }
 
+    dbparameters_->in_transaction = true;
     dbparameters_->updating_zone = true;
     dbparameters_->updated_zone_id = zone_info.second;
 
@@ -584,28 +587,28 @@ SQLite3Accessor::startUpdateZone(const string& zone_name, const bool replace) {
 }
 
 void
-SQLite3Accessor::commitUpdateZone() {
-    if (!dbparameters_->updating_zone) {
-        isc_throw(DataSourceError, "committing zone update on SQLite3 "
+SQLite3Accessor::commit() {
+    if (!dbparameters_->in_transaction) {
+        isc_throw(DataSourceError, "performing commit on SQLite3 "
                   "data source without transaction");
     }
 
     StatementProcessor(*dbparameters_, COMMIT,
                        "commit an SQLite3 transaction").exec();
-    dbparameters_->updating_zone = false;
+    dbparameters_->in_transaction = false;
     dbparameters_->updated_zone_id = -1;
 }
 
 void
-SQLite3Accessor::rollbackUpdateZone() {
-    if (!dbparameters_->updating_zone) {
-        isc_throw(DataSourceError, "rolling back zone update on SQLite3 "
+SQLite3Accessor::rollback() {
+    if (!dbparameters_->in_transaction) {
+        isc_throw(DataSourceError, "performing rollback on SQLite3 "
                   "data source without transaction");
     }
 
     StatementProcessor(*dbparameters_, ROLLBACK,
                        "rollback an SQLite3 transaction").exec();
-    dbparameters_->updating_zone = false;
+    dbparameters_->in_transaction = false;
     dbparameters_->updated_zone_id = -1;
 }
 

src/lib/datasrc/sqlite3_accessor.h

@@ -139,7 +139,7 @@ public:
     /// attempt and/or increase timeout before giving up the COMMIT, even
     /// if it still doesn't guarantee 100% success.  Right now this
     /// implementation throws a \c DataSourceError exception in such a case.
-    virtual void commitUpdateZone();
+    virtual void commit();
 
     /// \note In SQLite3 rollback can fail if there's another unfinished
     /// statement is performed for the same database structure.
@@ -147,7 +147,7 @@ public:
     /// guaranteed to be prevented at the API level.  If it ever happens, this
     /// method throws a \c DataSourceError exception.  It should be
     /// considered a bug of the higher level application program.
-    virtual void rollbackUpdateZone();
+    virtual void rollback();
 
     virtual void addRecordToZone(
         const std::string (&columns)[ADD_COLUMN_COUNT]);

src/lib/datasrc/tests/database_unittest.cc

@@ -223,8 +223,8 @@ public:
         // return dummy value.  unused anyway.
         return (pair<bool, int>(true, 0));
     }
-    virtual void commitUpdateZone() {}
-    virtual void rollbackUpdateZone() {}
+    virtual void commit() {}
+    virtual void rollback() {}
     virtual void addRecordToZone(const string (&)[ADD_COLUMN_COUNT]) {}
     virtual void deleteRecordInZone(const string (&)[DEL_PARAM_COUNT]) {}
 
@@ -486,10 +486,10 @@ public:
 
         return (pair<bool, int>(true, WRITABLE_ZONE_ID));
     }
-    virtual void commitUpdateZone() {
+    virtual void commit() {
         *readonly_records_ = *update_records_;
     }
-    virtual void rollbackUpdateZone() {
+    virtual void rollback() {
         // Special hook: if something with a name of "throw.example.org"
         // has been added, trigger an imaginary unexpected event with an
         // exception.
@@ -860,7 +860,7 @@ public:
 
             addRecordToZone(columns);
         }
-        commitUpdateZone();
+        commit();
     }
 };
 

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -550,7 +550,7 @@ TEST_F(SQLite3Update, emptyUpdate) {
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
     zone_id = accessor->startUpdateZone("example.com.", false).second;
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
-    accessor->commitUpdateZone();
+    accessor->commit();
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
 }
 
@@ -561,7 +561,7 @@ TEST_F(SQLite3Update, flushZone) {
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
     zone_id = accessor->startUpdateZone("example.com.", true).second;
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
-    accessor->commitUpdateZone();
+    accessor->commit();
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
 }
 
@@ -575,7 +575,7 @@ TEST_F(SQLite3Update, readWhileUpdate) {
 
     // Once the changes are committed, the other accessor will see the new
     // data.
-    accessor->commitUpdateZone();
+    accessor->commit();
     checkRecords(*another_accessor, zone_id, "foo.bar.example.com.",
                  empty_stored);
 }
@@ -585,7 +585,7 @@ TEST_F(SQLite3Update, rollback) {
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
 
     // Rollback will revert the change made by startUpdateZone(, true).
-    accessor->rollbackUpdateZone();
+    accessor->rollback();
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
 }
 
@@ -599,7 +599,7 @@ TEST_F(SQLite3Update, rollbackFailure) {
     EXPECT_TRUE(iterator->getNext(columns));
 
     accessor->startUpdateZone("example.com.", true);
-    EXPECT_THROW(accessor->rollbackUpdateZone(), DataSourceError);
+    EXPECT_THROW(accessor->rollback(), DataSourceError);
 }
 
 TEST_F(SQLite3Update, commitConflict) {
@@ -612,8 +612,8 @@ TEST_F(SQLite3Update, commitConflict) {
     // which will prevent commit.
     zone_id = accessor->startUpdateZone("example.com.", true).second;
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
-    EXPECT_THROW(accessor->commitUpdateZone(), DataSourceError);
-    accessor->rollbackUpdateZone();   // rollback should still succeed
+    EXPECT_THROW(accessor->commit(), DataSourceError);
+    accessor->rollback();   // rollback should still succeed
 
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
 }
@@ -631,9 +631,9 @@ TEST_F(SQLite3Update, updateConflict) {
 
     // Once we rollback the other attempt of change, we should be able to
     // start and commit the transaction using the main accessor.
-    another_accessor->rollbackUpdateZone();
+    another_accessor->rollback();
     accessor->startUpdateZone("example.com.", true);
-    accessor->commitUpdateZone();
+    accessor->commit();
 }
 
 TEST_F(SQLite3Update, duplicateUpdate) {
@@ -643,11 +643,11 @@ TEST_F(SQLite3Update, duplicateUpdate) {
 }
 
 TEST_F(SQLite3Update, commitWithoutTransaction) {
-    EXPECT_THROW(accessor->commitUpdateZone(), DataSourceError);
+    EXPECT_THROW(accessor->commit(), DataSourceError);
 }
 
 TEST_F(SQLite3Update, rollbackWithoutTransaction) {
-    EXPECT_THROW(accessor->rollbackUpdateZone(), DataSourceError);
+    EXPECT_THROW(accessor->rollback(), DataSourceError);
 }
 
 TEST_F(SQLite3Update, addRecord) {
@@ -664,7 +664,7 @@ TEST_F(SQLite3Update, addRecord) {
     checkRecords(*accessor, zone_id, "newdata.example.com.", expected_stored);
 
     // Commit the change, and confirm the new data is still there.
-    accessor->commitUpdateZone();
+    accessor->commit();
     checkRecords(*accessor, zone_id, "newdata.example.com.", expected_stored);
 }
 
@@ -678,7 +678,7 @@ TEST_F(SQLite3Update, addThenRollback) {
     expected_stored.push_back(new_data);
     checkRecords(*accessor, zone_id, "newdata.example.com.", expected_stored);
 
-    accessor->rollbackUpdateZone();
+    accessor->rollback();
     checkRecords(*accessor, zone_id, "newdata.example.com.", empty_stored);
 }
 
@@ -717,7 +717,7 @@ TEST_F(SQLite3Update, deleteRecord) {
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
 
     // Commit the change, and confirm the deleted data still isn't there.
-    accessor->commitUpdateZone();
+    accessor->commit();
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
 }
 
@@ -730,7 +730,7 @@ TEST_F(SQLite3Update, deleteThenRollback) {
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", empty_stored);
 
     // Rollback the change, and confirm the data still exists.
-    accessor->rollbackUpdateZone();
+    accessor->rollback();
     checkRecords(*accessor, zone_id, "foo.bar.example.com.", expected_stored);
 }