[#642,!373] Addressed review comments.

- Don't allow for deleting logical server 'all'.
- Additional tests to make sure that other servers aren't affected by deletion.
- Added note that getAll() doesn't return logical server all.

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
index b318ff6495dcae3e91a7ee7baf57a7ee3e23cf7e..37ba0d069b956d5d0210ad416586cd78cd2438ba 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.cc
@@ -2846,7 +2846,7 @@ MySqlConfigBackendDHCPv4::deleteServer4(const ServerTag& server_tag) {
         .arg(server_tag.get());
     uint64_t result = impl_->deleteServer(MySqlConfigBackendDHCPv4Impl::CREATE_AUDIT_REVISION,
                                           MySqlConfigBackendDHCPv4Impl::DELETE_SERVER4,
-                                          server_tag.get());
+                                          server_tag);
     LOG_DEBUG(mysql_cb_logger, DBGLVL_TRACE_BASIC, MYSQL_CB_DELETE_SERVER4_RESULT)
         .arg(result);
     return (result);

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h
index 1262cc24fea93b1d25c27aac0e327a730f9020e1..afda51a52b8d81110422723c175ea3e76e761518 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp4.h
@@ -227,6 +227,9 @@ public:
 
     /// @brief Retrieves all servers.
     ///
+    /// This method returns the list of servers excluding the logical server
+    /// 'all'.
+    ///
     /// @return Collection of servers from the backend.
     virtual db::ServerCollection
     getAllServers4() const;
@@ -480,6 +483,8 @@ public:
     ///
     /// @param server_tag Tag of the server to be deleted.
     /// @return Number of deleted servers.
+    /// @throw isc::InvalidOperation when trying to delete the logical
+    /// server 'all'.
     virtual uint64_t
     deleteServer4(const data::ServerTag& server_tag);
 

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc
index efd7ed18ccbea83224220e2b779cfc8846bf7832..67ffe7006fd5f68aa1e90792acab9965178acc90 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc
@@ -3233,7 +3233,7 @@ MySqlConfigBackendDHCPv6::deleteServer6(const ServerTag& server_tag) {
         .arg(server_tag.get());
     uint64_t result = impl_->deleteServer(MySqlConfigBackendDHCPv6Impl::CREATE_AUDIT_REVISION,
                                           MySqlConfigBackendDHCPv6Impl::DELETE_SERVER6,
-                                          server_tag.get());
+                                          server_tag);
     LOG_DEBUG(mysql_cb_logger, DBGLVL_TRACE_BASIC, MYSQL_CB_DELETE_SERVER6_RESULT)
         .arg(result);
     return (result);

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h
index 4b246f549de073ebc5d7076f78dc08e4f06fc821..1e8983f6e3e108c0191fa62c5472bdec103a8342 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.h
@@ -227,6 +227,9 @@ public:
 
     /// @brief Retrieves all servers.
     ///
+    /// This method returns the list of servers excluding the logical server
+    /// 'all'.
+    ///
     /// @return Collection of servers from the backend.
     virtual db::ServerCollection
     getAllServers6() const;
@@ -513,6 +516,8 @@ public:
     ///
     /// @param server_tag Tag of the server to be deleted.
     /// @return Number of deleted servers.
+    /// @throw isc::InvalidOperation when trying to delete the logical
+    /// server 'all'.
     virtual uint64_t
     deleteServer6(const data::ServerTag& server_tag);
 

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc
index b5a9e5d86333b55d98fb1e41753446d9c5a92326..30301fc0dfd24f51b9bd6dbfeca647c6a617c0be 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.cc
@@ -964,7 +964,14 @@ MySqlConfigBackendImpl::createUpdateServer(const int& create_audit_revision,
 uint64_t
 MySqlConfigBackendImpl::deleteServer(const int& create_audit_revision,
                                      const int& delete_index,
-                                     const std::string& server_tag) {
+                                     const ServerTag& server_tag) {
+
+    // It is not allowed to delete 'all' logical server.
+    if (server_tag.amAll()) {
+        isc_throw(InvalidOperation, "'all' is a name reserved for the server tag which"
+                  " associates the configuration elements with all servers connecting"
+                  " to the database and can't be deleted");
+    }
 
     MySqlTransaction transaction(conn_);
 
@@ -976,7 +983,7 @@ MySqlConfigBackendImpl::deleteServer(const int& create_audit_revision,
 
     // Specify which server should be deleted.
     MySqlBindingCollection in_bindings = {
-        MySqlBinding::createString(server_tag)
+        MySqlBinding::createString(server_tag.get())
     };
 
     // Attempt to delete the server.

diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h
index 4bbf2cb383cface202bd2a1cf7dff1c6f2481911..f910af865ba2b1a396e29cb153b7933921c1d92a 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h
+++ b/src/hooks/dhcp/mysql_cb/mysql_cb_impl.h
@@ -655,8 +655,10 @@ public:
     /// @param create_index Index of the DELETE query to be executed.
     /// @param server_tag Tag of the server to be deleted.
     /// @return Number of deleted servers.
+    /// @throw isc::InvalidOperation when trying to delete the logical
+    /// server 'all'.
     uint64_t deleteServer(const int& create_audit_revision, const int& index,
-                          const std::string& server_tag);
+                          const data::ServerTag& server_tag);
 
     /// @brief Attempts to delete all servers.
     ///

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 a4ce59f41b55bdf2f6d08f8b6fd15e929240712c..5c2d874664b3ad34c3ed09ae20bc96a4b3e58c9f 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
@@ -506,6 +506,12 @@ TEST_F(MySqlConfigBackendDHCPv4Test, createUpdateDeleteServer) {
     EXPECT_NO_THROW(servers_deleted = cbptr_->deleteServer4(ServerTag("server2")));
     EXPECT_EQ(0, servers_deleted);
 
+    // Make sure that the server1 wasn't deleted.
+    EXPECT_NO_THROW(returned_server = cbptr_->getServer4(ServerTag("server1")));
+    EXPECT_TRUE(returned_server);
+
+    // Deleting logical server 'all' is not allowed.
+    EXPECT_THROW(cbptr_->deleteServer4(ServerTag()), isc::InvalidOperation);
 
     // Delete the existing server.
     EXPECT_NO_THROW(servers_deleted = cbptr_->deleteServer4(ServerTag("server1")));

diff --git a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc
index 2003190e806c13bfc72e82d325c2daebea714f68..f6636112c2709e486dbfd8b297bb4f495dbbf32f 100644 (file)
--- a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc
+++ b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc
@@ -547,6 +547,12 @@ TEST_F(MySqlConfigBackendDHCPv6Test, createUpdateDeleteServer) {
     EXPECT_NO_THROW(servers_deleted = cbptr_->deleteServer6(ServerTag("server2")));
     EXPECT_EQ(0, servers_deleted);
 
+    // Make sure that the server1 wasn't deleted.
+    EXPECT_NO_THROW(returned_server = cbptr_->getServer6(ServerTag("server1")));
+    EXPECT_TRUE(returned_server);
+
+    // Deleting logical server 'all' is not allowed.
+    EXPECT_THROW(cbptr_->deleteServer6(ServerTag()), isc::InvalidOperation);
 
     // Delete the existing server.
     EXPECT_NO_THROW(servers_deleted = cbptr_->deleteServer6(ServerTag("server1")));

diff --git a/src/lib/dhcpsrv/config_backend_dhcp4.h b/src/lib/dhcpsrv/config_backend_dhcp4.h
index dcf96c6dd364261fd9a93eeea9941b95fad4ddfc..c155398868316d3434c253fcf5c4f57323ca559f 100644 (file)
--- a/src/lib/dhcpsrv/config_backend_dhcp4.h
+++ b/src/lib/dhcpsrv/config_backend_dhcp4.h
@@ -231,6 +231,9 @@ public:
 
     /// @brief Retrieves all servers.
     ///
+    /// This method returns the list of servers excluding the logical server
+    /// 'all'.
+    ///
     /// @return Collection of servers from the backend.
     virtual db::ServerCollection
     getAllServers4() const = 0;

diff --git a/src/lib/dhcpsrv/config_backend_dhcp6.h b/src/lib/dhcpsrv/config_backend_dhcp6.h
index 72f1c63041da309cc7bc3469da5be0f9a34e537c..3a0e3c411a2ba3607a2d22e3689ffebdbf6c8415 100644 (file)
--- a/src/lib/dhcpsrv/config_backend_dhcp6.h
+++ b/src/lib/dhcpsrv/config_backend_dhcp6.h
@@ -232,6 +232,9 @@ public:
 
     /// @brief Retrieves all servers.
     ///
+    /// This method returns the list of servers excluding the logical server
+    /// 'all'.
+    ///
     /// @return Collection of servers from the backend.
     virtual db::ServerCollection
     getAllServers6() const = 0;

diff --git a/src/lib/dhcpsrv/config_backend_pool_dhcp4.h b/src/lib/dhcpsrv/config_backend_pool_dhcp4.h
index 973cee42b9766f404fc0283f9682540b457fdbcf..56497be43d2570f8449062590028eac4be54a398 100644 (file)
--- a/src/lib/dhcpsrv/config_backend_pool_dhcp4.h
+++ b/src/lib/dhcpsrv/config_backend_pool_dhcp4.h
@@ -235,6 +235,9 @@ public:
 
     /// @brief Retrieves all servers from the particular backend.
     ///
+    /// This method returns the list of servers excluding the logical server
+    /// 'all'.
+    ///
     /// @param backend_selector Backend selector.
     /// @return Collection of servers from the backend.
     virtual db::ServerCollection

diff --git a/src/lib/dhcpsrv/config_backend_pool_dhcp6.h b/src/lib/dhcpsrv/config_backend_pool_dhcp6.h
index 3cf5b6bb40de0b4a06d800c326a6589f4a88fbef..c79bb44abd099fd2046abbb97b447bd29a1b1f19 100644 (file)
--- a/src/lib/dhcpsrv/config_backend_pool_dhcp6.h
+++ b/src/lib/dhcpsrv/config_backend_pool_dhcp6.h
@@ -234,6 +234,9 @@ public:
 
     /// @brief Retrieves all servers from the particular backend.
     ///
+    /// This method returns the list of servers excluding the logical server
+    /// 'all'.
+    ///
     /// @param backend_selector Backend selector.
     /// @return Collection of servers from the backend.
     virtual db::ServerCollection