[1535] address review comments

- removed superfluous toText() calls in out-of-zone exception creation
- moved in-memory out-of-zone check to be part of findNode instead of separate check in find()
- added out-of-zone check to addAdditional lookup (and don't lookup unless in zone)
- removed FIND_DEFAULT parameters in unit tests (they are default values)
- change notify_out sqlite test data to have out-of-zone-but-known NS target

src/lib/datasrc/database.cc

@@ -865,8 +865,7 @@ DatabaseClient::Finder::findInternal(const Name& name, const RRType& type,
         name.compare(getOrigin()).getRelation();
     if (reln != NameComparisonResult::SUBDOMAIN &&
         reln != NameComparisonResult::EQUAL) {
-        isc_throw(OutOfZoneFind, name.toText() << " not in " <<
-                                 getOrigin().toText());
+        isc_throw(OutOfZoneFind, name.toText() << " not in " << getOrigin());
     }
 
     // First, go through all superdomains from the origin down, searching for

src/lib/datasrc/memory_datasrc.cc

@@ -362,8 +362,7 @@ ZoneData::findNode(const Name& name, ZoneFinder::FindOptions options) const {
     if (result == DomainTree::EXACTMATCH) {
         return (ResultType(ZoneFinder::SUCCESS, node, state.rrset_,
                            zonecut_flag));
-    }
-    if (result == DomainTree::PARTIALMATCH) {
+    } else if (result == DomainTree::PARTIALMATCH) {
         assert(node != NULL);
         if (state.dname_node_ != NULL) { // DNAME
             LOG_DEBUG(logger, DBG_TRACE_DATA, DATASRC_MEM_DNAME_FOUND).
@@ -408,10 +407,15 @@ ZoneData::findNode(const Name& name, ZoneFinder::FindOptions options) const {
                                FindNodeResult::FIND_WILDCARD |
                                zonecut_flag));
         }
+        // Nothing really matched.
+        LOG_DEBUG(logger, DBG_TRACE_DATA, DATASRC_MEM_NOT_FOUND).arg(name);
+        return (ResultType(ZoneFinder::NXDOMAIN, node, state.rrset_));
+    } else {
+        // If the name is neither an exact or partial match, it is
+        // out of bailiwick, which is considered an error.
+        isc_throw(OutOfZoneFind, name.toText() << " not in " <<
+                                 origin_data_->getName());
     }
-    // Nothing really matched.  The name may even be out-of-bailiwick.
-    LOG_DEBUG(logger, DBG_TRACE_DATA, DATASRC_MEM_NOT_FOUND).arg(name);
-    return (ResultType(ZoneFinder::NXDOMAIN, node, state.rrset_));
 }
 } // unnamed namespace
 
@@ -1202,14 +1206,6 @@ struct InMemoryZoneFinder::InMemoryZoneFinderImpl {
         LOG_DEBUG(logger, DBG_TRACE_BASIC, DATASRC_MEM_FIND).arg(name).
             arg(type);
 
-        const NameComparisonResult::NameRelation reln =
-            name.compare(origin_).getRelation();
-        if (reln != NameComparisonResult::SUBDOMAIN &&
-            reln != NameComparisonResult::EQUAL) {
-            isc_throw(OutOfZoneFind, name.toText() <<
-                                     " not in " << origin_.toText());
-        }
-
         // Get the node.  All other cases than an exact match are handled
         // in findNode().  We simply construct a result structure and return.
         const ZoneData::FindNodeResult node_result =
@@ -1459,6 +1455,13 @@ addAdditional(RBNodeRRset* rrset, ZoneData* zone_data,
 
         const Name& name = getAdditionalName(rrset->getType(),
                                              rdata_iterator->getCurrent());
+        // if the name is not in or below this zone, skip it
+        const NameComparisonResult::NameRelation reln =
+            name.compare(zone_data->origin_data_->getName()).getRelation();
+         if (reln != NameComparisonResult::SUBDOMAIN &&
+             reln != NameComparisonResult::EQUAL) {
+            continue;
+        }
         const ZoneData::FindMutableNodeResult result =
             zone_data->findNode<ZoneData::FindMutableNodeResult>(
                 name, ZoneFinder::FIND_GLUE_OK);

src/lib/datasrc/tests/database_unittest.cc

@@ -1872,34 +1872,32 @@ TYPED_TEST(DatabaseClientTest, find) {
 }
 
 TYPED_TEST(DatabaseClientTest, findOutOfZone) {
-    // If the query name is out-of-zone it should result in NXDOMAIN
+    // If the query name is out-of-zone it should result in an exception
     boost::shared_ptr<DatabaseClient::Finder> finder(this->getFinder());
     vector<ConstRRsetPtr> target;
 
     // Superdomain
-    EXPECT_THROW(finder->find(Name("org"), this->qtype_,
-                 ZoneFinder::FIND_DEFAULT), OutOfZoneFind);
+    EXPECT_THROW(finder->find(Name("org"), this->qtype_), OutOfZoneFind);
     EXPECT_THROW(finder->findAll(Name("org"), target), OutOfZoneFind);
 
     // sharing a common ancestor
-    EXPECT_THROW(finder->find(Name("noexample.org"), this->qtype_,
-                 ZoneFinder::FIND_DEFAULT), OutOfZoneFind);
+    EXPECT_THROW(finder->find(Name("noexample.org"), this->qtype_),
+                 OutOfZoneFind);
     EXPECT_THROW(finder->findAll(Name("noexample.org"), target),
                  OutOfZoneFind);
 
     // totally unrelated domain, smaller number of labels
-    EXPECT_THROW(finder->find(Name("com"), this->qtype_,
-                 ZoneFinder::FIND_DEFAULT), OutOfZoneFind);
+    EXPECT_THROW(finder->find(Name("com"), this->qtype_), OutOfZoneFind);
     EXPECT_THROW(finder->findAll(Name("com"), target), OutOfZoneFind);
 
     // totally unrelated domain, same number of labels
-    EXPECT_THROW(finder->find(Name("example.com"), this->qtype_,
-                 ZoneFinder::FIND_DEFAULT), OutOfZoneFind);
+    EXPECT_THROW(finder->find(Name("example.com"), this->qtype_),
+                 OutOfZoneFind);
     EXPECT_THROW(finder->findAll(Name("example.com"), target), OutOfZoneFind);
 
     // totally unrelated domain, larger number of labels
-    EXPECT_THROW(finder->find(Name("more.example.com"), this->qtype_,
-                 ZoneFinder::FIND_DEFAULT), OutOfZoneFind);
+    EXPECT_THROW(finder->find(Name("more.example.com"), this->qtype_),
+                 OutOfZoneFind);
     EXPECT_THROW(finder->findAll(Name("more.example.com"), target),
                  OutOfZoneFind);
 }
@@ -2829,13 +2827,12 @@ TYPED_TEST(DatabaseClientTest, addDeviantRR) {
     this->expected_rdatas_.clear();
     this->expected_rdatas_.push_back("192.0.2.100");
     {
-        // Note: find() rejects out-of-zone query name with NXDOMAIN
+        // Note: find() rejects out-of-zone query name with an exception
         // regardless of whether adding the RR succeeded, so this check
         // actually doesn't confirm it.
         SCOPED_TRACE("add out-of-zone RR");
         EXPECT_THROW(this->updater_->getFinder().find(Name("example.com"),
-                                                      this->qtype_,
-                                                      ZoneFinder::FIND_DEFAULT),
+                                                      this->qtype_),
                      OutOfZoneFind);
     }
 }

src/lib/datasrc/tests/memory_datasrc_unittest.cc

@@ -998,8 +998,8 @@ InMemoryZoneFinderTest::findCheck(ZoneFinder::FindResultFlags expected_flags) {
     // These domains don't exist (and one is out of the zone)
     findTest(Name("nothere.example.org"), RRType::A(), ZoneFinder::NXDOMAIN,
              true, ConstRRsetPtr(), expected_flags);
-    EXPECT_THROW(zone_finder_.find(Name("example.net"), RRType::A(),
-                                   ZoneFinder::FIND_DEFAULT), OutOfZoneFind);
+    EXPECT_THROW(zone_finder_.find(Name("example.net"), RRType::A()),
+                 OutOfZoneFind);
 }
 
 TEST_F(InMemoryZoneFinderTest, find) {
@@ -1054,8 +1054,7 @@ InMemoryZoneFinderTest::emptyNodeCheck(
     // Note: basically we don't expect such a query to be performed (the common
     // operation is to identify the best matching zone first then perform
     // search it), but we shouldn't be confused even in the unexpected case.
-    EXPECT_THROW(zone_finder_.find(Name("org"), RRType::A(),
-                                   ZoneFinder::FIND_DEFAULT),
+    EXPECT_THROW(zone_finder_.find(Name("org"), RRType::A()),
                  OutOfZoneFind);
 }
 
@@ -1514,16 +1513,12 @@ TEST_F(InMemoryZoneFinderTest, swap) {
     EXPECT_EQ(RRClass::CH(), finder1.getClass());
     EXPECT_EQ(RRClass::IN(), finder2.getClass());
     // make sure the zone data is swapped, too
-    EXPECT_THROW(finder1.find(origin_, RRType::NS(),
-                              ZoneFinder::FIND_DEFAULT),
-                 OutOfZoneFind);
+    EXPECT_THROW(finder1.find(origin_, RRType::NS()), OutOfZoneFind);
     findTest(other_origin, RRType::TXT(), ZoneFinder::SUCCESS, false,
              ConstRRsetPtr(), ZoneFinder::RESULT_DEFAULT, &finder1);
     findTest(origin_, RRType::NS(), ZoneFinder::SUCCESS, false,
              ConstRRsetPtr(), ZoneFinder::RESULT_DEFAULT, &finder2);
-    EXPECT_THROW(finder2.find(other_origin, RRType::TXT(),
-                              ZoneFinder::FIND_DEFAULT),
-                 OutOfZoneFind);
+    EXPECT_THROW(finder2.find(other_origin, RRType::TXT()), OutOfZoneFind);
 }
 
 TEST_F(InMemoryZoneFinderTest, getFileName) {

src/lib/python/isc/datasrc/tests/datasrc_test.py

@@ -382,7 +382,7 @@ class DataSrcClient(unittest.TestCase):
 
         self.assertRaises(isc.datasrc.OutOfZoneFind, finder.find,
                           isc.dns.Name("www.some.other.domain"),
-                          isc.dns.RRType.A(), finder.FIND_DEFAULT)
+                          isc.dns.RRType.A())
 
         result, rrset, _ = finder.find(isc.dns.Name("www.example.com"),
                                        isc.dns.RRType.TXT(),