From: Jelte Jansen <jelte@isc.org>
Date: Wed, 12 Dec 2012 14:15:01 +0000 (+0100)
Subject: [2541] Address more comments; add tests and update docs
X-Git-Tag: bind10-1.0.0-beta-release~26^2~7
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=bc8aae68ec152b7dce35c158758537bd9912688f;p=thirdparty%2Fkea.git

[2541] Address more comments; add tests and update docs
---

diff --git a/src/lib/datasrc/client.cc b/src/lib/datasrc/client.cc
index e9db6bb072..919e9abb4c 100644
--- a/src/lib/datasrc/client.cc
+++ b/src/lib/datasrc/client.cc
@@ -44,6 +44,5 @@ DataSourceClient::createZone(const dns::Name&) {
               "Data source doesn't support addZone");
 }
 
-
 } // end namespace datasrc
 } // end namespace isc
diff --git a/src/lib/datasrc/sqlite3_accessor.cc b/src/lib/datasrc/sqlite3_accessor.cc
index 516cad998b..4ef49d054b 100644
--- a/src/lib/datasrc/sqlite3_accessor.cc
+++ b/src/lib/datasrc/sqlite3_accessor.cc
@@ -625,6 +625,9 @@ SQLite3Accessor::addZone(const std::string& name) {
     }
 
     StatementProcessor proc(*dbparameters_, ADD_ZONE, "add zone");
+    // note: using TRANSIENT here, STATIC would be slightly more
+    // efficient, but TRANSIENT is safer and performance is not
+    // as important in this specific code path.
     proc.bindText(1, name.c_str(), SQLITE_TRANSIENT);
     proc.bindText(2, class_.c_str(), SQLITE_TRANSIENT);
     proc.exec();
diff --git a/src/lib/datasrc/sqlite3_accessor.h b/src/lib/datasrc/sqlite3_accessor.h
index 2117d9f0a5..e931573be1 100644
--- a/src/lib/datasrc/sqlite3_accessor.h
+++ b/src/lib/datasrc/sqlite3_accessor.h
@@ -119,9 +119,9 @@ public:
 
     /// \brief Add a zone
     ///
-    /// This implements the addZone from DatabaseAccessor and adds an (empty)
-    /// zone into the zones table. If the zone exists already, it is still
-    /// added, so the caller should make sure this does not happen (by making
+    /// This implements the addZone from DatabaseAccessor and adds a zone
+    /// into the zones table. If the zone exists already, it is still added,
+    /// so the caller should make sure this does not happen (by making
     /// sure the zone does not exist). In the case of duplicate addition,
     /// it is undefined which zone id is returned.
     ///
diff --git a/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc b/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
index c4903642d1..d45d672d3f 100644
--- a/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
+++ b/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
@@ -675,11 +675,10 @@ TEST_F(SQLite3Create, addZone) {
         new SQLite3Accessor(SQLITE_NEW_DBFILE, "IN"));
 
     const std::string zone_name("example.com");
-    const std::pair<bool, int> zone_info(accessor->getZone(zone_name));
-    ASSERT_FALSE(zone_info.first);
+    EXPECT_FALSE(accessor->getZone(zone_name).first);
 
     // Calling addZone without transaction should fail
-    ASSERT_THROW(accessor->addZone(zone_name), DataSourceError);
+    EXPECT_THROW(accessor->addZone(zone_name), DataSourceError);
 
     // Add the zone. Returns the new zone id
     accessor->startTransaction();
@@ -687,17 +686,68 @@ TEST_F(SQLite3Create, addZone) {
     accessor->commit();
 
     // Check that it exists now, but has no records at this point
-    const std::pair<bool, int> zone_info2(accessor->getZone(zone_name));
-    ASSERT_TRUE(zone_info2.first);
-    ASSERT_EQ(new_zone_id, zone_info2.second);
+    const std::pair<bool, int> zone_info(accessor->getZone(zone_name));
+    ASSERT_TRUE(zone_info.first);
+    EXPECT_EQ(new_zone_id, zone_info.second);
 
     DatabaseAccessor::IteratorContextPtr context =
-        accessor->getAllRecords(zone_info2.second);
+        accessor->getAllRecords(zone_info.second);
     string data[DatabaseAccessor::COLUMN_COUNT];
-    ASSERT_NE(DatabaseAccessor::IteratorContextPtr(), context);
+    EXPECT_NE(DatabaseAccessor::IteratorContextPtr(), context);
     EXPECT_FALSE(context->getNext(data));
 }
 
+// Test addZone does not get confused with different classes
+TEST_F(SQLite3Create, addZoneDifferentClass) {
+    const std::string zone_name("example.com.");
+
+    // Add IN zone
+    boost::shared_ptr<SQLite3Accessor> accessor(
+        new SQLite3Accessor(SQLITE_NEW_DBFILE, "IN"));
+    accessor->startTransaction();
+    const int new_zone_id_IN = accessor->addZone(zone_name);
+    accessor->commit();
+
+    // Add CH zone
+    accessor.reset(new SQLite3Accessor(SQLITE_NEW_DBFILE, "CH"));
+    accessor->startTransaction();
+    const int new_zone_id_CH = accessor->addZone(zone_name);
+    accessor->commit();
+
+    // id's should differ
+    ASSERT_NE(new_zone_id_IN, new_zone_id_CH);
+
+    // Reopen the database for both classes, and make sure
+    // we get the right zone id on searches
+    accessor.reset(new SQLite3Accessor(SQLITE_NEW_DBFILE, "IN"));
+    EXPECT_EQ(new_zone_id_IN, accessor->getZone(zone_name).second);
+    accessor.reset(new SQLite3Accessor(SQLITE_NEW_DBFILE, "CH"));
+    EXPECT_EQ(new_zone_id_CH, accessor->getZone(zone_name).second);
+}
+
+// Test addZone fails correctly if the entire db is locked
+TEST_F(SQLite3Create, addZoneWhileLocked) {
+    const std::string zone_name("example.com.");
+
+    boost::shared_ptr<SQLite3Accessor> accessor(
+        new SQLite3Accessor(SQLITE_NEW_DBFILE, "IN"));
+    accessor->startTransaction();
+
+    // Manually make an exclusive lock
+    sqlite3* db;
+    ASSERT_EQ(SQLITE_OK, sqlite3_open(SQLITE_NEW_DBFILE, &db));
+    sqlite3_exec(db, "BEGIN EXCLUSIVE TRANSACTION", NULL, NULL, NULL);
+
+    // addZone should throw exception that it is locket
+    ASSERT_THROW(accessor->addZone(zone_name), DataSourceError);
+
+    sqlite3_exec(db, "ROLLBACK TRANSACTION", NULL, NULL, NULL);
+
+    // Despite commit, zone should not exist
+    accessor->commit();
+    EXPECT_FALSE(accessor->getZone(zone_name).first);
+}
+
 TEST_F(SQLite3Create, emptytest) {
     ASSERT_FALSE(isReadable(SQLITE_NEW_DBFILE));