[2851] update getCachedZoneWriter so it only checks config and real datasrc.

- it now still create the zone writer even if the zone is currently not
  loaded (due to initial failure).  the original behavior is mostly preserved,
  but in some minor cases the resulting error code (or whether to throw)
  differ.  test cases were adjusted accordingly.
- some description of CacheStatus now does not perfectly match the behavior,
  so are updated.

src/bin/auth/tests/datasrc_clients_builder_unittest.cc

@@ -406,6 +406,22 @@ TEST_F(DataSrcClientsBuilderTest,
     EXPECT_EQ(orig_lock_count + 1, map_mutex.lock_count);
     EXPECT_EQ(orig_unlock_count + 1, map_mutex.unlock_count);
 
+    // zone doesn't exist in the data source
+    const ConstElementPtr config_nozone(Element::fromJSON("{"
+        "\"IN\": [{"
+        "    \"type\": \"sqlite3\","
+        "    \"params\": {\"database_file\": \"" + test_db + "\"},"
+        "    \"cache-enable\": true,"
+        "    \"cache-zones\": [\"nosuchzone.example\"]"
+        "}]}"));
+    clients_map = configureDataSource(config_nozone);
+    EXPECT_THROW(
+        builder.handleCommand(
+            Command(LOADZONE, Element::fromJSON(
+                        "{\"class\": \"IN\","
+                        " \"origin\": \"nosuchzone.example\"}"))),
+        TestDataSrcClientsBuilder::InternalCommandError);
+
     // basically impossible case: in-memory cache is completely disabled.
     // In this implementation of manager-builder, this should never happen,
     // but it catches it like other configuration error and keeps going.
@@ -503,14 +519,6 @@ TEST_F(DataSrcClientsBuilderTest, loadZoneInvalidParams) {
             }, "");
     }
 
-    // zone doesn't exist in the data source
-    EXPECT_THROW(
-        builder.handleCommand(
-            Command(LOADZONE,
-                    Element::fromJSON(
-                        "{\"class\": \"IN\", \"origin\": \"xx\"}"))),
-        TestDataSrcClientsBuilder::InternalCommandError);
-
     // origin is bogus
     EXPECT_THROW(builder.handleCommand(
                      Command(LOADZONE,

src/lib/datasrc/client_list.cc

@@ -314,26 +314,31 @@ ConfigurableClientList::getCachedZoneWriter(const Name& name) {
     if (!allow_cache_) {
         return (ZoneWriterPair(CACHE_DISABLED, ZoneWriterPtr()));
     }
-    // Try to find the correct zone.
-    MutableResult result;
-    findInternal(result, name, true, true);
-    if (!result.finder) {
-        return (ZoneWriterPair(ZONE_NOT_FOUND, ZoneWriterPtr()));
-    }
 
+    // Find the data source from which the zone to be loaded into memory.
     // Then get the appropriate load action and create a zone writer.
-    // Note that getCacheConfig() must return non NULL in this module (only
-    // tests could set it to a bogus value).
-    const memory::LoadAction load_action =
-        result.info->getCacheConfig()->getLoadAction(rrclass_, name);
-    if (!load_action) {
-        return (ZoneWriterPair(ZONE_NOT_CACHED, ZoneWriterPtr()));
+    BOOST_FOREACH(const DataSourceInfo& info, data_sources_) {
+        // If there's a underlying "real" data source and it doesn't contain
+        // the given name, obviously we cannot load it.
+        if (info.data_src_client_ &&
+            info.data_src_client_->findZone(name).code != result::SUCCESS) {
+            continue;
+        }
+
+        // Note that getCacheConfig() must return non NULL in this module
+        // (only tests could set it to a bogus value).
+        const memory::LoadAction load_action =
+            info.getCacheConfig()->getLoadAction(rrclass_, name);
+        if (!load_action) {
+            return (ZoneWriterPair(ZONE_NOT_CACHED, ZoneWriterPtr()));
+        }
+        return (ZoneWriterPair(ZONE_SUCCESS,
+                               ZoneWriterPtr(
+                                   new memory::ZoneWriter(
+                                       *info.ztable_segment_,
+                                       load_action, name, rrclass_))));
     }
-    return (ZoneWriterPair(ZONE_SUCCESS,
-                           ZoneWriterPtr(
-                               new memory::ZoneWriter(
-                                   *result.info->ztable_segment_,
-                                   load_action, name, rrclass_))));
+    return (ZoneWriterPair(ZONE_NOT_FOUND, ZoneWriterPtr()));
 }
 
 // NOTE: This function is not tested, it would be complicated. However, the

src/lib/datasrc/client_list.h

@@ -348,12 +348,12 @@ public:
     /// This is used as a result of the getCachedZoneWriter() method.
     enum CacheStatus {
         CACHE_DISABLED,     ///< The cache is not enabled in this list.
-        ZONE_NOT_CACHED,    ///< Zone is served directly, not from cache
-                            ///  (including the case cache is disabled for
-                            ///  the specific data source).
+        ZONE_NOT_CACHED,    ///< Zone is not to be cached (including the case
+                            ///  where caching is disabled for the specific
+                            ///  data source).
         ZONE_NOT_FOUND,     ///< Zone does not exist in this list.
-        ZONE_SUCCESS        ///< The zone was successfully reloaded or
-                            ///  the writer provided.
+        ZONE_SUCCESS        ///< Zone to be cached is successfully found and
+                            ///  is ready to be loaded
     };
 
     /// \brief Return value of getCachedZoneWriter()
@@ -408,12 +408,12 @@ public:
         boost::shared_ptr<memory::ZoneTableSegment> ztable_segment_;
         std::string name_;
 
+        // cache_conf_ can be accessed only from this read-only getter,
+	// to protect its integrity as much as possible.
         const internal::CacheConfig* getCacheConfig() const {
             return (cache_conf_.get());
         }
     private:
-        // this is kept private for now.  When it needs to be accessed,
-        // we'll add a read-only getter method.
         boost::shared_ptr<internal::CacheConfig> cache_conf_;
     };
 

src/lib/datasrc/tests/client_list_unittest.cc

@@ -859,7 +859,7 @@ ListTest::doReload(const Name& origin) {
 TEST_F(ListTest, reloadSuccess) {
     list_->configure(config_elem_zones_, true);
     const Name name("example.org");
-    this->prepareCache(0, name);
+    prepareCache(0, name);
     // The cache currently contains a tweaked version of zone, which
     // doesn't have "tstzonedata" A record.  So the lookup should result
     // in NXDOMAIN.
@@ -917,8 +917,8 @@ TEST_F(ListTest, reloadNoSuchZone) {
     // Not in the data sources
     EXPECT_EQ(ConfigurableClientList::ZONE_NOT_FOUND,
               doReload(Name("exmaple.cz")));
-    // Not cached
-    EXPECT_EQ(ConfigurableClientList::ZONE_NOT_FOUND, doReload(name));
+    // If it's not configured to be cached, it won't be reloaded.
+    EXPECT_EQ(ConfigurableClientList::ZONE_NOT_CACHED, doReload(name));
     // Partial match
     EXPECT_EQ(ConfigurableClientList::ZONE_NOT_FOUND,
               doReload(Name("sub.example.com")));
@@ -952,8 +952,8 @@ TEST_F(ListTest, reloadZoneGone) {
     static_cast<MockDataSourceClient*>(
         list_->getDataSources()[0].data_src_client_)->eraseZone(name);
 
-    // The zone is not there, so abort the reload.
-    EXPECT_THROW(doReload(name), DataSourceError);
+    // The zone is not there, so reload doesn't take place.
+    EXPECT_EQ(ConfigurableClientList::ZONE_NOT_FOUND, doReload(name));
     // The (cached) zone is not hurt.
     EXPECT_EQ(ZoneFinder::SUCCESS,
               list_->find(name).finder_->find(name, RRType::SOA())->code);