[#95] Additional review comments

src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc
    Commentary clean up

src/lib/pgsql/pgsql_connection.cc
src/lib/pgsql/tests/pgsql_connection_unittest.cc
    PgSqlConnection::createSavepoint()
    PgSqlConnection::rollbackToSavepoint() - now throw
    InvalidOperation instead Unexpected

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
index 023067be11eaca0c4ac9ab53554c23d107cc3cb3..d0d282053a7c27e0b11dcdfdd85aae955b378bcd 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
@@ -3654,6 +3654,8 @@ MySqlConfigBackendDHCPv4Impl::MySqlConfigBackendDHCPv4Impl(const DatabaseConnect
     // database is read only for the current user.
     conn_.prepareStatements(tagged_statements.begin(),
                             tagged_statements.end());
+// @todo As part of enabling read-only CB access, statements need to
+// be limited:
 //                            tagged_statements.begin() + WRITE_STMTS_BEGIN);
 
     // Create unique timer name per instance.

diff --git a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc
index d5c6c2c3799690651de817a4ac1e91cab6f87fbe..26805eee2a2cee32be8b4bfa68fdf76a35f5a904 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc
+++ b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2018-2021 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2018-2022 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -67,15 +67,6 @@ public:
 };
 
 /// @brief Test fixture class for @c MySqlConfigBackendDHCPv4.
-///
-/// @todo The tests we're providing here only test cases when the
-/// server selector is set to 'ALL' (configuration elements belong to
-/// all servers). Currently we have no API to insert servers into
-/// the database, and therefore we can't test the case when
-/// configuration elements are assigned to particular servers by
-/// server tags. We will have to expand existing tests when
-/// the API is extended allowing for inserting servers to the
-/// database.
 class MySqlConfigBackendDHCPv4Test : public MySqlGenericBackendTest {
 public:
 
@@ -4779,6 +4770,8 @@ TEST_F(MySqlConfigBackendDHCPv4Test, multipleAuditEntries) {
     }
 }
 
+/// @brief Test fixture for verifying database connection loss-recovery
+/// behavior.
 class MySqlConfigBackendDHCPv4DbLostCallbackTest : public ::testing::Test {
 public:
     MySqlConfigBackendDHCPv4DbLostCallbackTest()

diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc
index a80b5d3e5f794ddb586c3fa0167be218cfcd16b8..c958e78d10fd38cafdf03010b9b413be817e1578 100644 (file)
--- a/src/lib/pgsql/pgsql_connection.cc
+++ b/src/lib/pgsql/pgsql_connection.cc
@@ -438,7 +438,7 @@ PgSqlConnection::rollback() {
 void
 PgSqlConnection::createSavepoint(const std::string& name) {
     if (transaction_ref_count_ <= 0) {
-        isc_throw(Unexpected, "no transaction, cannot create savepoint: " << name);
+        isc_throw(InvalidOperation, "no transaction, cannot create savepoint: " << name);
     }
 
     DB_LOG_DEBUG(DB_DBG_TRACE_DETAIL, PGSQL_CREATE_SAVEPOINT).arg(name);
@@ -449,7 +449,7 @@ PgSqlConnection::createSavepoint(const std::string& name) {
 void
 PgSqlConnection::rollbackToSavepoint(const std::string& name) {
     if (transaction_ref_count_ <= 0) {
-        isc_throw(Unexpected, "no transaction, cannot rollback to savepoint: " << name);
+        isc_throw(InvalidOperation, "no transaction, cannot rollback to savepoint: " << name);
     }
 
     std::string sql("ROLLBACK TO SAVEPOINT " + name);

diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h
index 6facc1954b502df107a0b38d722591af97efb007..2f11474ea0f74e7288ce705ff18a7580f4308ff7 100644 (file)
--- a/src/lib/pgsql/pgsql_connection.h
+++ b/src/lib/pgsql/pgsql_connection.h
@@ -327,10 +327,9 @@ public:
     ///
     /// Creates a named savepoint within the current transaction.
     ///
-    /// @param name name of the savepoint to create. This name
-    /// must be unique within the transaction?
+    /// @param name name of the savepoint to create.
     ///
-    /// @throw Unexpected if called outside a transaction.
+    /// @throw InvalidOperation if called outside a transaction.
     /// @throw DbOperationError If the savepoint cannot be created.
     void createSavepoint(const std::string& name);
 
@@ -341,7 +340,7 @@ public:
     ///
     /// @param name name of the savepoint to which to rollback.
     ///
-    /// @throw Unexpected if called outside a transaction.
+    /// @throw InvalidOperation if called outside a transaction.
     /// @throw DbOperationError if the rollback failed.
     void rollbackToSavepoint(const std::string& name);
 

diff --git a/src/lib/pgsql/tests/pgsql_connection_unittest.cc b/src/lib/pgsql/tests/pgsql_connection_unittest.cc
index c129baa4c793ba29325cc6a093ad8cef07b71a28..64be3234db1532b633e5890ab21df281f6a4862a 100644 (file)
--- a/src/lib/pgsql/tests/pgsql_connection_unittest.cc
+++ b/src/lib/pgsql/tests/pgsql_connection_unittest.cc
@@ -441,9 +441,9 @@ TEST_F(PgSqlConnectionTest, savepoints) {
 
     // Creating or rollback to savepoints outside of transactions
     // should throw.
-    ASSERT_THROW_MSG(conn_->createSavepoint("rubbish"), Unexpected,
+    ASSERT_THROW_MSG(conn_->createSavepoint("rubbish"), InvalidOperation,
                      "no transaction, cannot create savepoint: rubbish");
-    ASSERT_THROW_MSG(conn_->rollbackToSavepoint("rubbish"), Unexpected,
+    ASSERT_THROW_MSG(conn_->rollbackToSavepoint("rubbish"), InvalidOperation,
                      "no transaction, cannot rollback to savepoint: rubbish");
 
     // Test that we can create and rollback to a savepoint, then