[1330] Updated with tests for valid sequences

src/lib/datasrc/sqlite3_accessor.cc

@@ -141,10 +141,8 @@ struct SQLite3Parameters {
     void
     finalizeStatements() {
         for (int i = 0; i < NUM_STATEMENTS; ++i) {
-            if (statements_[i] != NULL) {
-                sqlite3_finalize(statements_[i]);
-                statements_[i] = NULL;
-            }
+            sqlite3_finalize(statements_[i]);
+            statements_[i] = NULL;
         }
     }
 
@@ -626,15 +624,13 @@ public:
             int high_id = findIndex(HIGH_DIFF_ID, zone_id, end);
 
             // Prepare the statement that will return data values
-            clearBindings(DIFF_RECS);
+            reset(DIFF_RECS);
             bindInt(DIFF_RECS, 1, zone_id);
             bindInt(DIFF_RECS, 2, low_id);
             bindInt(DIFF_RECS, 3, high_id);
-
-            // last_status_ has been initialized to pretend that the last
-            // getNext() returned a record.
-
         } catch (...) {
+
+            // Something wrong, clear up everything.
             accessor_->dbparameters_->finalizeStatements();
             throw;
         }
@@ -656,12 +652,14 @@ public:
     /// \exceptions any Varied
     bool getNext(std::string (&data)[COLUMN_COUNT]) {
 
-        // Get a pointer to the statement for brevity (does not transfer
-        // resources)
-        sqlite3_stmt* stmt = accessor_->dbparameters_->getStatement(DIFF_RECS);
-
-        // If there is a row to get, get it.
         if (last_status_ != SQLITE_DONE) {
+            // Last call (if any) didn't reach end of result set, so we
+            // can read another row from it.
+            //
+            // Get a pointer to the statement for brevity (does not transfer
+            // resources)
+            sqlite3_stmt* stmt = accessor_->dbparameters_->getStatement(DIFF_RECS);
+
             const int rc(sqlite3_step(stmt));
             if (rc == SQLITE_ROW) {
                 // Copy the data across to the output array
@@ -669,8 +667,8 @@ public:
                 copyColumn(DIFF_RECS, data, TTL_COLUMN);
                 copyColumn(DIFF_RECS, data, NAME_COLUMN);
                 copyColumn(DIFF_RECS, data, RDATA_COLUMN);
+
             } else if (rc != SQLITE_DONE) {
-                accessor_->dbparameters_->finalizeStatements();
                 isc_throw(DataSourceError,
                           "Unexpected failure in sqlite3_step: " <<
                           sqlite3_errmsg(accessor_->dbparameters_->db_));
@@ -684,13 +682,12 @@ private:
 
     /// \brief Clear Statement Bindings
     ///
-    /// Clears the bindings of variables in a prepared statement and resets
-    /// them to null.
+    /// Resets the statement and clears any bindings attached to it.
     ///
     /// \param stindex Index of prepared statement to which to bind
-    void clearBindings(int stindex) {
-        if (sqlite3_clear_bindings(
-            accessor_->dbparameters_->getStatement(stindex)) != SQLITE_OK) {
+    void reset(int stindex) {
+        sqlite3_stmt* stmt = accessor_->dbparameters_->getStatement(stindex);
+        if ((sqlite3_reset(stmt) != SQLITE_OK) || (sqlite3_clear_bindings(stmt) != SQLITE_OK)) {
             isc_throw(SQLite3Error, "Could not clear statement bindings in '" <<
                       text_statements[stindex] << "': " << 
                       sqlite3_errmsg(accessor_->dbparameters_->db_));
@@ -785,7 +782,7 @@ private:
     int findIndex(StatementID stindex, int zone_id, uint32_t serial) {
 
         // Set up the statement
-        clearBindings(stindex);
+        reset(stindex);
         bindInt(stindex, 1, zone_id);
         bindInt(stindex, 2, serial);
 
@@ -828,7 +825,8 @@ private:
     // Attributes
 
     boost::shared_ptr<const SQLite3Accessor> accessor_; // Accessor object
-    int last_status_;   // Last status received from sqlite3_step
+    sqlite3_stmt*   stmt_;      // Prepared statement for this iterator
+    int last_status_;           // Last status received from sqlite3_step
 };
 
 // ... and return the iterator

src/lib/datasrc/sqlite3_accessor.h

@@ -82,6 +82,7 @@ public:
         isc::Exception(file, line, what) {}
 };
 
+
 struct SQLite3Parameters;
 
 /**

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -236,28 +236,106 @@ TEST_F(SQLite3AccessorTest, diffIteratorNoRecords) {
 
 }
 
+// Simple check to test that the sequence is valid.  It gets the next record
+// from the iterator, checks that it is not null, then checks the data.
+void checkRR(DatabaseAccessor::IteratorContextPtr& context,
+     std::string name, std::string ttl, std::string type, std::string rdata) {
+
+    // Mark where we are in the text
+    SCOPED_TRACE(name + " " + ttl + " " + type + " " + rdata);
+
+    std::string data[DatabaseAccessor::COLUMN_COUNT];
+
+    // Get next record
+    EXPECT_TRUE(context->getNext(data));
+
+    // ... and check expected values
+    EXPECT_EQ(name, data[DatabaseAccessor::NAME_COLUMN]);
+    EXPECT_EQ(ttl, data[DatabaseAccessor::TTL_COLUMN]);
+    EXPECT_EQ(type, data[DatabaseAccessor::TYPE_COLUMN]);
+    EXPECT_EQ(rdata, data[DatabaseAccessor::RDATA_COLUMN]);
+}
+
+
 // Try to iterate through a valid set of differences
-TEST_F(SQLite3AccessorTest, validSequence) {
+TEST_F(SQLite3AccessorTest, diffIteratorSequences) {
+    std::string data[DatabaseAccessor::COLUMN_COUNT];
 
     // Our test zone is conveniently small, but not empty
     initAccessor(SQLITE_DBFILE_DIFFS, "IN");
 
     const std::pair<bool, int> zone_info(accessor->getZone("example.org."));
     ASSERT_TRUE(zone_info.first);
+
+    // Check the difference sequence 1230-1231 (two adjacent differences)
     // Get the iterator context
     DatabaseAccessor::IteratorContextPtr
-        context(accessor->getDiffs(zone_info.second, 1230, 1232));
-    ASSERT_NE(DatabaseAccessor::IteratorContextPtr(), context);
-
-    std::string data[DatabaseAccessor::COLUMN_COUNT];
+        context1(accessor->getDiffs(zone_info.second, 1230, 1231));
+    ASSERT_NE(DatabaseAccessor::IteratorContextPtr(), context1);
+
+    // Change: 1230-1231
+    checkRR(context1, "example.org.", "1800", "SOA",
+            "ns1.example.org. admin.example.org. 1230 3600 1800 2419200 7200");
+    checkRR(context1, "example.org.", "3600", "SOA",
+            "ns1.example.org. admin.example.org. 1231 3600 1800 2419200 7200");
+
+    // Check there's no other and that calling it again after no records doesn't
+    // cause problems.
+    EXPECT_FALSE(context1->getNext(data));
+    EXPECT_FALSE(context1->getNext(data));
+
+    // Check that the difference sequence 1231-1233 (two separate difference
+    // sequences) is OK.
+    DatabaseAccessor::IteratorContextPtr
+        context2(accessor->getDiffs(zone_info.second, 1231, 1233));
+    ASSERT_NE(DatabaseAccessor::IteratorContextPtr(), context2);
+
+    // Change 1231-1232
+    checkRR(context2, "example.org.", "3600", "SOA",
+            "ns1.example.org. admin.example.org. 1231 3600 1800 2419200 7200");
+    checkRR(context2, "unused.example.org.", "3600", "A", "192.0.2.102");
+    checkRR(context2, "example.org.", "3600", "SOA",
+            "ns1.example.org. admin.example.org. 1232 3600 1800 2419200 7200");
+
+    // Change: 1232-1233
+    checkRR(context2, "example.org.", "3600", "SOA",
+            "ns1.example.org. admin.example.org. 1232 3600 1800 2419200 7200");
+    checkRR(context2, "example.org.", "3600", "SOA",
+            "ns1.example.org. admin.example.org. 1233 3600 1800 2419200 7200");
+    checkRR(context2, "sub.example.org.", "3600", "NS", "ns.sub.example.org.");
+    checkRR(context2, "ns.sub.example.org.", "3600", "A", "192.0.2.101");
+
+
+    // Check there's no other and that calling it again after no records doesn't
+    // cause problems.
+    EXPECT_FALSE(context2->getNext(data));
+    EXPECT_FALSE(context2->getNext(data));
+
+    // Check that the difference sequence 4294967280 to 1230 (serial number
+    // rollover) is OK
+    DatabaseAccessor::IteratorContextPtr
+        context3(accessor->getDiffs(zone_info.second, 4294967280U, 1230));
+    ASSERT_NE(DatabaseAccessor::IteratorContextPtr(), context3);
+
+    // Change 4294967280 to 1230.
+    checkRR(context3, "example.org.", "3600", "SOA",
+            "ns1.example.org. admin.example.org. 4294967280 3600 1800 2419200 7200");
+    checkRR(context3, "www.example.org.", "3600", "A", "192.0.2.31");
+    checkRR(context3, "example.org.", "1800", "SOA",
+            "ns1.example.org. admin.example.org. 1230 3600 1800 2419200 7200");
+    checkRR(context3, "www.example.org.", "3600", "A", "192.0.2.21");
+
+    EXPECT_FALSE(context3->getNext(data));
+    EXPECT_FALSE(context3->getNext(data));
+
+    // Finally, check that if the start and end versions are present, but that
+    // they are given in the wrong order, the result is an empty difference set.
+    DatabaseAccessor::IteratorContextPtr
+        context4(accessor->getDiffs(zone_info.second, 1233, 1231));
+    ASSERT_NE(DatabaseAccessor::IteratorContextPtr(), context2);
 
-    // Check the records
-    EXPECT_TRUE(context->getNext(data));
-    EXPECT_EQ("SOA", data[DatabaseAccessor::TYPE_COLUMN]);
-    EXPECT_EQ("1800", data[DatabaseAccessor::TTL_COLUMN]);
-    EXPECT_EQ("ns1.example.org. admin.example.org. 1230 3600 1800 2419200 7200",
-        data[DatabaseAccessor::RDATA_COLUMN]);
-    EXPECT_EQ("example.org.", data[DatabaseAccessor::NAME_COLUMN]);
+    EXPECT_FALSE(context4->getNext(data));
+    EXPECT_FALSE(context4->getNext(data));
 }
 
 TEST(SQLite3Open, getDBNameExample2) {

src/lib/datasrc/tests/testdata/diffs_table.sql

@@ -29,7 +29,8 @@
 -- the ".quit" on the command line then  getting executed to exit SQLite3.
 
 -- Create the diffs table
-CREATE TABLE diffs (id INTEGER PRIMARY KEY AUTOINCREMENT,
+DROP TABLE diffs;
+CREATE TABLE diffs (id INTEGER PRIMARY KEY,
                     zone_id INTEGER NOT NULL,
                     version INTEGER NOT NULL,
                     operation INTEGER NOT NULL,
@@ -51,7 +52,7 @@ INSERT INTO diffs(zone_id, version, operation, name, rrtype, ttl, rdata)
 
 -- Records added in version 1230 of the zone
 INSERT INTO diffs(zone_id, version, operation, name, rrtype, ttl, rdata)
-    VALUES(1, 1230, 1, "example.org.", "SOA", 3600,
+    VALUES(1, 1230, 1, "example.org.", "SOA", 1800,
            "ns1.example.org. admin.example.org. 1230 3600 1800 2419200 7200");
 INSERT INTO diffs(zone_id, version, operation, name, rrtype, ttl, rdata)
     VALUES(1, 1230, 1, "www.example.org.", "A", 3600, "192.0.2.21");