Merge #2877 (the rest)

Clean-ups so one doesn't have to pass the hash twice when deleting an NSEC3
record.

src/lib/datasrc/database.cc

@@ -1707,15 +1707,16 @@ DatabaseUpdater::deleteRRset(const AbstractRRset& rrset) {
             LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_DELETEDIFF).
                 arg(cvtr.getName()).arg(cvtr.getType()).arg(rdata_txt);
         }
-        const string params[Accessor::DEL_PARAM_COUNT] =
-            { nsec3_type ? cvtr.getNSEC3Name() : cvtr.getName(),
-              cvtr.getType(), rdata_txt,
-              nsec3_type ? cvtr.getNSEC3Name() : cvtr.getRevName() };
         if (nsec3_type) {
+            const string params[Accessor::DEL_NSEC3_PARAM_COUNT] =
+                { cvtr.getNSEC3Name(), cvtr.getType(), rdata_txt };
             accessor_->deleteNSEC3RecordInZone(params);
             LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_DELETENSEC3).
                 arg(cvtr.getNSEC3Name()).arg(rdata_txt);
         } else {
+            const string params[Accessor::DEL_PARAM_COUNT] =
+                { cvtr.getName(), cvtr.getType(), rdata_txt,
+                  cvtr.getRevName() };
             accessor_->deleteRecordInZone(params);
             LOG_DEBUG(logger, DBG_TRACE_DETAILED, DATASRC_DATABASE_DELETERR).
                 arg(cvtr.getName()).arg(cvtr.getType()).arg(rdata_txt);

src/lib/datasrc/database.h

@@ -116,14 +116,13 @@ public:
         ADD_NSEC3_COLUMN_COUNT = 4 ///< Number of columns
     };
 
-    /// \brief Definitions of the fields to be passed to deleteRecordInZone()
-    /// and deleteNSEC3RecordInZone()
+    /// \brief Definitions of the fields to be passed to deleteRecordInZone().
     ///
     /// Each derived implementation of deleteRecordInZone() should expect
     /// the "params" array to be filled with the values as described in this
     /// enumeration, in this order.
     ///
-    /// DEL_RNAME is included in case the reversed from is more convenient
+    /// DEL_RNAME is included in case the reversed form is more convenient
     /// for the underlying implementation to identify the record to be
     /// deleted (reversed names are generally easier to sort, which may help
     /// perform the search faster).  It's up to the underlying implementation
@@ -132,16 +131,29 @@ public:
     /// in that sense redundant.  But both are provided so the underlying
     /// implementation doesn't have to deal with DNS level concepts.
     enum DeleteRecordParams {
-        DEL_NAME = 0, ///< The owner name of the record (a domain name)
-                      ///< or the hash label for deleteNSEC3RecordInZone()
+        DEL_NAME = 0, ///< The owner name of the record (a domain name).
         DEL_TYPE = 1, ///< The RRType of the record (A/NS/TXT etc.)
         DEL_RDATA = 2, ///< Full text representation of the record's RDATA
         DEL_RNAME = 3, ///< As DEL_NAME, but with the labels of domain name
-                       ///< in reverse order (eg. org.example.). With NSEC3,
-                       ///< it is the same as DEL_NAME.
+                       ///< in reverse order (eg. org.example.).
         DEL_PARAM_COUNT = 4 ///< Number of parameters
     };
 
+    /// \brief Definitions of the fields to be passed to
+    /// deleteNSEC3RecordInZone().
+    ///
+    /// Each derived implementation of deleteNSEC3RecordInZone() should expect
+    /// the "params" array to be filled with the values as described in this
+    /// enumeration, in this order.
+    enum DeleteNSEC3RecordParams {
+        DEL_NSEC3_HASH = 0, ///< The hash (1st) label of the owren name,
+                            ///< excluding the dot character.
+        DEL_NSEC3_TYPE = 1, ///< The type of RR. Either RRSIG or NSEC3.
+        DEL_NSEC3_RDATA = 2, ///< Full text representation of the record's
+                             ///<  RDATA. Must match the one in the database.
+        DEL_NSEC3_PARAM_COUNT = 3 ///< Number of parameters.
+    };
+
     /// \brief Operation mode when adding a record diff.
     ///
     /// This is used as the "operation" parameter value of addRecordDiff().
@@ -588,11 +600,8 @@ public:
     /// \c addRecordToZone() and \c addNSEC3RecordToZone(), and the same
     /// notes apply to this method.
     ///
-    /// This method uses the same set of parameters to specify the record
-    /// to be deleted as \c deleteRecordInZone(), but the \c DEL_NAME column
-    /// is expected to only store the hash label of the owner name.
-    /// This is the same as \c ADD_NSEC3_HASH column for
-    /// \c addNSEC3RecordToZone().
+    /// This method uses the \c DeleteNSEC3RecordParams enum to specify the
+    /// values.
     ///
     /// \exception DataSourceError Invalid call without starting a transaction,
     /// or other internal database error.
@@ -602,7 +611,7 @@ public:
     /// \param params An array of strings that defines a record to be deleted
     /// from the NSEC3 namespace of the zone.
     virtual void deleteNSEC3RecordInZone(
-        const std::string (&params)[DEL_PARAM_COUNT]) = 0;
+        const std::string (&params)[DEL_NSEC3_PARAM_COUNT]) = 0;
 
     /// \brief Start a general transaction.
     ///

src/lib/datasrc/sqlite3_accessor.cc

@@ -1311,20 +1311,14 @@ SQLite3Accessor::deleteRecordInZone(const string (&params)[DEL_PARAM_COUNT]) {
 
 void
 SQLite3Accessor::deleteNSEC3RecordInZone(
-    const string (&params)[DEL_PARAM_COUNT])
+    const string (&params)[DEL_NSEC3_PARAM_COUNT])
 {
     if (!dbparameters_->updating_zone) {
         isc_throw(DataSourceError, "deleting NSEC3-related record in SQLite3 "
                   "data source without transaction");
     }
-    const size_t SQLITE3_DEL_PARAM_COUNT = DEL_PARAM_COUNT - 1;
-    const string sqlite3_params[SQLITE3_DEL_PARAM_COUNT] = {
-        params[DEL_NAME],
-        params[DEL_TYPE],
-        params[DEL_RDATA]
-    };
-    doUpdate<const string (&)[SQLITE3_DEL_PARAM_COUNT]>(
-        *dbparameters_, DEL_NSEC3_RECORD, sqlite3_params,
+    doUpdate<const string (&)[DEL_NSEC3_PARAM_COUNT]>(
+        *dbparameters_, DEL_NSEC3_RECORD, params,
         "delete NSEC3 record from zone");
 }
 

src/lib/datasrc/sqlite3_accessor.h

@@ -230,7 +230,7 @@ public:
         const std::string (&params)[DEL_PARAM_COUNT]);
 
     virtual void deleteNSEC3RecordInZone(
-        const std::string (&params)[DEL_PARAM_COUNT]);
+        const std::string (&params)[DEL_NSEC3_PARAM_COUNT]);
 
     /// This derived version of the method prepares an SQLite3 statement
     /// for adding the diff first time it's called, and if it fails throws

src/lib/datasrc/tests/database_unittest.cc

@@ -167,7 +167,8 @@ public:
     virtual void addNSEC3RecordToZone(const string (&)[ADD_NSEC3_COLUMN_COUNT])
     {}
     virtual void deleteRecordInZone(const string (&)[DEL_PARAM_COUNT]) {}
-    virtual void deleteNSEC3RecordInZone(const string (&)[DEL_PARAM_COUNT]) {}
+    virtual void deleteNSEC3RecordInZone(const string
+                                         (&)[DEL_NSEC3_PARAM_COUNT]) {}
     virtual void addRecordDiff(int, uint32_t, DiffOperation,
                                const std::string (&)[DIFF_PARAM_COUNT]) {}
 
@@ -634,9 +635,8 @@ private:
     };
 
     // Common subroutine for deleteRecordinZone and deleteNSEC3RecordInZone.
-    void deleteRecord(Domains& domains,
-                      const string (&params)[DEL_PARAM_COUNT])
-    {
+    template<size_t param_count>
+    void deleteRecord(Domains& domains, const string (&params)[param_count]) {
         vector<vector<string> >& records =
             domains[params[DatabaseAccessor::DEL_NAME]];
         records.erase(remove_if(records.begin(), records.end(),
@@ -655,7 +655,7 @@ public:
     }
 
     virtual void deleteNSEC3RecordInZone(
-        const string (&params)[DEL_PARAM_COUNT])
+        const string (&params)[DEL_NSEC3_PARAM_COUNT])
     {
         deleteRecord(*update_nsec3_namespace_, params);
     }

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -823,8 +823,7 @@ const char* const nsec3_sig_data[DatabaseAccessor::ADD_NSEC3_COLUMN_COUNT] = {
 const char* const nsec3_deleted_data[] = {
     // Delete parameters for nsec3_data
     apex_hash, nsec3_data[DatabaseAccessor::ADD_NSEC3_TYPE],
-    nsec3_data[DatabaseAccessor::ADD_NSEC3_RDATA],
-    apex_hash
+    nsec3_data[DatabaseAccessor::ADD_NSEC3_RDATA]
 };
 
 class SQLite3Update : public SQLite3AccessorTest {
@@ -854,6 +853,7 @@ protected:
     std::string add_columns[DatabaseAccessor::ADD_COLUMN_COUNT];
     std::string add_nsec3_columns[DatabaseAccessor::ADD_NSEC3_COLUMN_COUNT];
     std::string del_params[DatabaseAccessor::DEL_PARAM_COUNT];
+    std::string del_nsec3_params[DatabaseAccessor::DEL_NSEC3_PARAM_COUNT];
     std::string diff_params[DatabaseAccessor::DIFF_PARAM_COUNT];
 
     vector<const char* const*> expected_stored; // placeholder for checkRecords
@@ -1193,8 +1193,9 @@ TEST_F(SQLite3Update, deleteNSEC3Record) {
 
     // Delete it, and confirm that.
     copy(nsec3_deleted_data,
-         nsec3_deleted_data + DatabaseAccessor::DEL_PARAM_COUNT, del_params);
-    accessor->deleteNSEC3RecordInZone(del_params);
+         nsec3_deleted_data + DatabaseAccessor::DEL_NSEC3_PARAM_COUNT,
+         del_nsec3_params);
+    accessor->deleteNSEC3RecordInZone(del_nsec3_params);
     checkNSEC3Records(*accessor, zone_id, apex_hash, empty_stored);
 
     // Commit the change, and confirm the deleted data still isn't there.
@@ -1251,7 +1252,7 @@ TEST_F(SQLite3Update, invalidDelete) {
     EXPECT_THROW(accessor->deleteRecordInZone(del_params), DataSourceError);
 
     // Same for NSEC3.
-    EXPECT_THROW(accessor->deleteNSEC3RecordInZone(del_params),
+    EXPECT_THROW(accessor->deleteNSEC3RecordInZone(del_nsec3_params),
                  DataSourceError);
 }