[1177] Don't wrap around in previous

src/lib/datasrc/database.h

@@ -487,13 +487,18 @@ public:
      *     and the DNSSEC order correspond (eg. org.example.a is followed
      *     by org.example.a.b which is followed by org.example.b, etc).
      * \param zone_id The zone to look through.
-     * \return The previous name, or the last name in the zone, if there's
-     *     no previous one (including out-of-zone cases).
-     * \note This function must return previous/last name even in case
+     * \return The previous name.
+     * \note This function must return previous name even in case
      *     the queried rname does not exist in the zone.
+     * \note This method must skip under-the-zone-cut data (glue data).
+     *     This might be implemented by looking for NSEC records (as glue
+     *     data don't have them) in the zone or in some other way.
      *
      * \throw DataSourceError if there's a problem with the database.
-     * \throw NotImplemented if this database doesn't support DNSSEC.
+     * \throw NotImplemented if this database doesn't support DNSSEC
+     *     or there's no previous name for the queried one (the NSECs
+     *     might be missing or the queried name is less or equal the
+     *     apex of the zone).
      */
     virtual std::string findPreviousName(int zone_id,
                                          const std::string& rname) const = 0;

src/lib/datasrc/sqlite3_accessor.cc

@@ -48,8 +48,7 @@ enum StatementID {
     DEL_RECORD = 8,
     ITERATE = 9,
     FIND_PREVIOUS = 10,
-    FIND_PREVIOUS_WRAP = 11,
-    NUM_STATEMENTS = 12
+    NUM_STATEMENTS = 11
 };
 
 const char* const text_statements[NUM_STATEMENTS] = {
@@ -72,17 +71,13 @@ const char* const text_statements[NUM_STATEMENTS] = {
     "SELECT rdtype, ttl, sigtype, rdata, name FROM records " // ITERATE
     "WHERE zone_id = ?1 ORDER BY name, rdtype",
     /*
-     * The ones for finding previous name. The first of them takes
-     * biggest smaller than something (therefore previous to the something),
-     * the second takes biggest (used in case when there's no previous,
-     * to "wrap around").
+     * This one looks for previous name with NSEC record. It is done by
+     * using the reversed name. The NSEC is checked because we need to
+     * skip glue data, which don't have the NSEC.
      */
     "SELECT name FROM records " // FIND_PREVIOUS
     "WHERE zone_id=?1 AND rdtype = 'NSEC' AND "
-    "rname < $2 ORDER BY rname DESC LIMIT 1",
-    "SELECT name FROM records " // FIND_PREVIOUS_WRAP
-    "WHERE zone_id = ?1 AND rdtype = 'NSEC' "
-    "ORDER BY rname DESC LIMIT 1"
+    "rname < $2 ORDER BY rname DESC LIMIT 1"
 };
 
 struct SQLite3Parameters {
@@ -702,31 +697,10 @@ SQLite3Accessor::findPreviousName(int zone_id, const std::string& rname)
     sqlite3_reset(dbparameters_->statements_[FIND_PREVIOUS]);
 
     if (rc == SQLITE_DONE) {
-        // Nothing previous, wrap around (is it needed for anything?
-        // Well, just for completeness)
-        sqlite3_reset(dbparameters_->statements_[FIND_PREVIOUS_WRAP]);
-        sqlite3_clear_bindings(dbparameters_->statements_[FIND_PREVIOUS_WRAP]);
-
-        if (sqlite3_bind_int(
-            dbparameters_->statements_[FIND_PREVIOUS_WRAP], 1, zone_id) !=
-            SQLITE_OK) {
-            isc_throw(SQLite3Error, "Could not bind zone ID " << zone_id <<
-                      " to SQL statement (find previous wrap): " <<
-                      sqlite3_errmsg(dbparameters_->db_));
-        }
-
-        rc = sqlite3_step(dbparameters_->statements_[FIND_PREVIOUS_WRAP]);
-        if (rc == SQLITE_ROW) {
-            // We found it
-            result =
-                convertToPlainChar(sqlite3_column_text(dbparameters_->
-                    statements_[FIND_PREVIOUS_WRAP], 0), dbparameters_->db_);
-        }
-        sqlite3_reset(dbparameters_->statements_[FIND_PREVIOUS_WRAP]);
-        if (rc == SQLITE_DONE) {
-            // No NSEC records, this DB doesn't support DNSSEC
-            isc_throw(isc::NotImplemented, "The zone doesn't support DNSSEC");
-        }
+        // No NSEC records here, this DB doesn't support DNSSEC or
+        // we asked before the apex
+        isc_throw(isc::NotImplemented, "The zone doesn't support DNSSEC or "
+                  "query before apex");
     }
 
     if (rc != SQLITE_ROW && rc != SQLITE_DONE) {

src/lib/datasrc/tests/database_unittest.cc

@@ -175,8 +175,6 @@ const char* const TEST_RECORDS[][5] = {
     {"bao.example.org.", "NSEC", "3600", "", "wild.*.foo.*.bar.example.org. NSEC"},
     {"*.cnamewild.example.org.", "CNAME", "3600", "", "www.example.org."},
     {"*.nswild.example.org.", "NS", "3600", "", "ns.example.com."},
-    // For finding previous, this one is the last one in the zone
-    {"zzz.example.org.", "NSEC", "3600", "", "example.org NSEC"},
     // For NSEC empty non-terminal
     {"l.example.org.", "NSEC", "3600", "", "empty.nonterminal.example.org. NSEC"},
     {"empty.nonterminal.example.org.", "A", "3600", "", "192.0.2.1"},
@@ -558,9 +556,7 @@ public:
         if (id == -1) {
             isc_throw(isc::NotImplemented, "Test not implemented behaviour");
         } else if (id == 42) {
-            if (rname == "org.example.") {
-                return ("zzz.example.org.");
-            } else if (rname == "org.example.nonterminal.") {
+            if (rname == "org.example.nonterminal.") {
                 return ("l.example.org.");
             } else if (rname == "org.example.www2." ||
                        rname == "org.example.www1.") {
@@ -2331,9 +2327,6 @@ TYPED_TEST(DatabaseClientTest, previous) {
 
     EXPECT_EQ(Name("www.example.org."),
               finder->findPreviousName(Name("www2.example.org.")));
-    // Check wrap around
-    EXPECT_EQ(Name("zzz.example.org."),
-              finder->findPreviousName(Name("example.org.")));
     // Check a name that doesn't exist there
     EXPECT_EQ(Name("www.example.org."),
               finder->findPreviousName(Name("www1.example.org.")));

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -360,12 +360,7 @@ TEST_F(SQLite3AccessorTest, findPrevious) {
     // Largest name
     EXPECT_EQ("www.example.com.",
               accessor->findPreviousName(1, "com.example.wwww"));
-    // Wrap around
-    EXPECT_EQ("www.example.com.",
-              accessor->findPreviousName(1, "com.example."));
-    // Out of zone before and after
-    EXPECT_EQ("www.example.com.",
-              accessor->findPreviousName(1, "bb.example."));
+    // Out of zone after the last name
     EXPECT_EQ("www.example.com.",
               accessor->findPreviousName(1, "org.example."));
     // Case insensitive?

src/lib/datasrc/zone.h

@@ -219,12 +219,15 @@ public:
     /// however it is recommended to stick to the ones listed here. The user
     /// of this method should be able to handle any exceptions.
     ///
+    /// This method does not include under-zone-cut data (glue data).
+    ///
     /// \param query The name for which one we look for a previous one. The
     ///     queried name doesn't have to exist in the zone.
     /// \return The preceding name
     ///
     /// \throw NotImplemented in case the data source backend doesn't support
-    ///     DNSSEC.
+    ///     DNSSEC or there is no previous in the zone (NSEC records might be
+    ///     missing in the DB, the queried name is less or equal to the apex).
     /// \throw DataSourceError for low-level or internal datasource errors
     ///     (like broken connection to database, wrong data living there).
     /// \throw std::bad_alloc For allocation errors.