[1062] more review updates (comment:14)

src/lib/datasrc/database.cc

@@ -83,12 +83,18 @@ namespace {
 // Raises a DataSourceError if the type does not
 // match, or if the given rdata string does not
 // parse correctly for the given type and class
+//
+// The DatabaseConnection is passed to print the
+// database name in the log message if the TTL is
+// modified
 void addOrCreate(isc::dns::RRsetPtr& rrset,
                     const isc::dns::Name& name,
                     const isc::dns::RRClass& cls,
                     const isc::dns::RRType& type,
                     const isc::dns::RRTTL& ttl,
-                    const std::string& rdata_str)
+                    const std::string& rdata_str,
+                    const DatabaseConnection& conn
+                )
 {
     if (!rrset) {
         rrset.reset(new isc::dns::RRset(name, cls, type, ttl));
@@ -100,7 +106,8 @@ void addOrCreate(isc::dns::RRsetPtr& rrset,
                 rrset->setTTL(ttl);
             }
             logger.info(DATASRC_DATABASE_FIND_TTL_MISMATCH)
-                .arg(name).arg(cls).arg(type).arg(rrset->getTTL());
+                .arg(conn.getDBName()).arg(name).arg(cls)
+                .arg(type).arg(rrset->getTTL());
         }
     }
     try {
@@ -174,9 +181,9 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
     try {
         connection_->searchForRecords(zone_id_, name.toText());
 
-        std::string columns[DatabaseConnection::RECORDCOLUMNCOUNT];
+        std::string columns[DatabaseConnection::COLUMN_COUNT];
         while (connection_->getNextRecord(columns,
-                                        DatabaseConnection::RECORDCOLUMNCOUNT)) {
+                                        DatabaseConnection::COLUMN_COUNT)) {
             if (!records_found) {
                 records_found = true;
             }
@@ -203,7 +210,8 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
                                   "the only record for " + name.toText());
                     }
                     addOrCreate(result_rrset, name, getClass(), cur_type,
-                                cur_ttl, columns[DatabaseConnection::RDATA_COLUMN]);
+                                cur_ttl, columns[DatabaseConnection::RDATA_COLUMN],
+                                *connection_);
                 } else if (cur_type == isc::dns::RRType::CNAME()) {
                     // There should be no other data, so result_rrset should
                     // be empty.
@@ -212,7 +220,8 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
                                   "the only record for " + name.toText());
                     }
                     addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl,
-                                columns[DatabaseConnection::RDATA_COLUMN]);
+                                columns[DatabaseConnection::RDATA_COLUMN],
+                                *connection_);
                     result_status = CNAME;
                 } else if (cur_type == isc::dns::RRType::RRSIG()) {
                     // If we get signatures before we get the actual data, we

src/lib/datasrc/database.h

@@ -93,7 +93,7 @@ public:
      * In the case of a database error, a DatasourceError is thrown.
      *
      * The columns passed is an array of std::strings consisting of
-     * DatabaseConnection::RECORDCOLUMNCOUNT elements, the elements of which
+     * DatabaseConnection::COLUMN_COUNT elements, the elements of which
      * are defined in DatabaseConnection::RecordColumns, in their basic
      * string representation.
      *
@@ -146,7 +146,7 @@ public:
     };
 
     /// The number of fields the columns array passed to getNextRecord should have
-    static const size_t RECORDCOLUMNCOUNT = 4;
+    static const size_t COLUMN_COUNT = 4;
 
     /**
         * \brief Returns a string identifying this dabase backend
@@ -237,6 +237,20 @@ public:
          *       different. It is left in for compatibility at the moment.
          * \note options are ignored at this moment
          *
+         * \note Maybe counter intuitively, this method is not a const member
+         * function.  This is intentional; some of the underlying implementations
+         * are expected to use a database backend, and would internally contain
+         * some abstraction of "database connection".  In the most strict sense
+         * any (even read only) operation might change the internal state of
+         * such a connection, and in that sense the operation cannot be considered
+         * "const".  In order to avoid giving a false sense of safety to the
+         * caller, we indicate a call to this method may have a surprising
+         * side effect.  That said, this view may be too strict and it may
+         * make sense to say the internal database connection doesn't affect
+         * external behavior in terms of the interface of this method.  As
+         * we gain more experiences with various kinds of backends we may
+         * revisit the constness.
+         *
          * \exception DataSourceError when there is a problem reading
          *                            the data from the dabase backend.
          *                            This can be a connection, code, or

src/lib/datasrc/datasrc_messages.mes

@@ -73,7 +73,7 @@ The error message contains specific information about the error.
 Debug information. The database data source is looking up records with the given
 name and type in the database.
 
-% DATASRC_DATABASE_FIND_TTL_MISMATCH TTL values differ for elements of %1/%2/%3, setting to %4
+% DATASRC_DATABASE_FIND_TTL_MISMATCH TTL values differ in %1 for elements of %2/%3/%4, setting to %5
 The datasource backend provided resource records for the given RRset with
 different TTL values. The TTL of the RRSET is set to the lowest value, which
 is printed in the log message.

src/lib/datasrc/sqlite3_connection.cc

@@ -370,11 +370,10 @@ convertToPlainChar(const unsigned char* ucp,
 
 bool
 SQLite3Connection::getNextRecord(std::string columns[], size_t column_count) {
-    if (column_count != RECORDCOLUMNCOUNT) {
+    if (column_count != COLUMN_COUNT) {
             isc_throw(DataSourceError,
                     "Datasource backend caller did not pass a column array "
-                    "of size " << RECORDCOLUMNCOUNT <<
+                    "of size " << COLUMN_COUNT << " to getNextRecord()");
-                    " to getNextRecord()");
     }
 
     sqlite3_stmt* current_stmt = dbparameters_->q_any_;

src/lib/datasrc/tests/database_unittest.cc

@@ -92,7 +92,7 @@ public:
             throw std::exception();
         }
 
-        if (column_count != DatabaseConnection::RECORDCOLUMNCOUNT) {
+        if (column_count != DatabaseConnection::COLUMN_COUNT) {
             isc_throw(DataSourceError, "Wrong column count in getNextRecord");
         }
         if (cur_record < cur_name.size()) {
@@ -324,6 +324,25 @@ TEST_F(DatabaseClientTest, noConnException) {
 }
 
 namespace {
+// checks if the given rrset matches the
+// given name, class, type and rdatas
+void
+checkRRset(isc::dns::ConstRRsetPtr rrset,
+           const isc::dns::Name& name,
+           const isc::dns::RRClass& rrclass,
+           const isc::dns::RRType& rrtype,
+           const isc::dns::RRTTL& rrttl,
+           const std::vector<std::string> rdatas) {
+    isc::dns::RRsetPtr expected_rrset(
+        new isc::dns::RRset(name, rrclass, rrtype, rrttl));
+    for (unsigned int i = 0; i < rdatas.size(); ++i) {
+        expected_rrset->addRdata(
+            isc::dns::rdata::createRdata(rrtype, rrclass,
+                                         rdatas[i]));
+    }
+    isc::testutils::rrsetCheck(expected_rrset, rrset);
+}
+
 void
 doFindTest(shared_ptr<DatabaseClient::Finder> finder,
            const isc::dns::Name& name,
@@ -338,25 +357,13 @@ doFindTest(shared_ptr<DatabaseClient::Finder> finder,
         finder->find(name, type, NULL, ZoneFinder::FIND_DEFAULT);
     ASSERT_EQ(expected_result, result.code) << name << " " << type;
     if (expected_rdatas.size() > 0) {
-        EXPECT_EQ(expected_rdatas.size(), result.rrset->getRdataCount());
+        checkRRset(result.rrset, name, finder->getClass(),
-        EXPECT_EQ(expected_ttl, result.rrset->getTTL());
+                   expected_type, expected_ttl, expected_rdatas);
-        EXPECT_EQ(expected_type, result.rrset->getType());
-
-        isc::dns::RRsetPtr expected_rrset(
-            new isc::dns::RRset(name, finder->getClass(),
-                                expected_type, expected_ttl));
-        for (unsigned int i = 0; i < expected_rdatas.size(); ++i) {
-            expected_rrset->addRdata(
-                isc::dns::rdata::createRdata(expected_type,
-                                             finder->getClass(),
-                                             expected_rdatas[i]));
-        }
-        isc::testutils::rrsetCheck(expected_rrset, result.rrset);
 
         if (expected_sig_rdatas.size() > 0) {
-            // TODO same for sigrrset
+            checkRRset(result.rrset->getRRsig(), name,
-            EXPECT_EQ(expected_sig_rdatas.size(),
+                       finder->getClass(), isc::dns::RRType::RRSIG(),
-                      result.rrset->getRRsig()->getRdataCount());
+                       expected_ttl, expected_sig_rdatas);
         } else {
             EXPECT_EQ(isc::dns::RRsetPtr(), result.rrset->getRRsig());
         }

src/lib/datasrc/tests/sqlite3_connection_unittest.cc

@@ -134,7 +134,7 @@ TEST_F(SQLite3Conn, getRecords) {
     const int zone_id = zone_info.second;
     ASSERT_EQ(1, zone_id);
 
-    const size_t column_count = DatabaseConnection::RECORDCOLUMNCOUNT;
+    const size_t column_count = DatabaseConnection::COLUMN_COUNT;
     std::string columns[column_count];
 
     // without search, getNext() should return false