Harmonise the Store::queryPathInfoUncached interface

This:
 - Consistently returns `nullptr` for a non-existent
   store path, instead of a mix of `nullptr` and
   throwing exceptions.

 - If a store returns "bad" store paths in response
   to a request (e.g. incorrect hash or name), don't
   cache this result. This removes some duplication
   of code at the cache-access layer of queryPathInfo()
   checking this, and ­allows us to provide more
   specific errors.

Part of #270.

Change-Id: I86612c6499b1a37ab872c712c2304d6a3ff19edb

5 files changed, 32 insertions, 16 deletions

diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc
index ea643fd31..3600eca60 100644
--- a/src/libstore/binary-cache-store.cc
+++ b/src/libstore/binary-cache-store.cc
@@ -361,7 +361,7 @@ std::shared_ptr<const ValidPathInfo> BinaryCacheStore::queryPathInfoUncached(con
 
     auto data = getFile(narInfoFile);
 
-    if (!data) return {};
+    if (!data) return nullptr;
 
     stats.narInfoRead++;
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index f5eaa9f5f..bae5fad7b 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -899,7 +899,7 @@ std::shared_ptr<const ValidPathInfo> LocalStore::queryPathInfoInternal(State & s
     auto useQueryPathInfo(state.stmts->QueryPathInfo.use()(printStorePath(path)));
 
     if (!useQueryPathInfo.next())
-        return std::shared_ptr<ValidPathInfo>();
+        return nullptr;
 
     auto id = useQueryPathInfo.getInt(0);
 
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 93b1afabd..966dd3fe0 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -309,14 +309,14 @@ std::shared_ptr<const ValidPathInfo> RemoteStore::queryPathInfoUncached(const St
     try {
         conn.processStderr();
     } catch (Error & e) {
-        // Ugly backwards compatibility hack.
+        // Ugly backwards compatibility hack. TODO(fj#325): remove.
         if (e.msg().find("is not valid") != std::string::npos)
-            throw InvalidPath(std::move(e.info()));
+            return nullptr;
         throw;
     }
     if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 17) {
         bool valid; conn->from >> valid;
-        if (!valid) throw InvalidPath("path '%s' is not valid", printStorePath(path));
+        if (!valid) return nullptr;
     }
     return std::make_shared<ValidPathInfo>(
         StorePath{path},
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index 952958d51..e0e842060 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -666,11 +666,19 @@ bool Store::isValidPathUncached(const StorePath & path)
 }
 
 
-static bool goodStorePath(const StorePath & expected, const StorePath & actual)
+static void ensureGoodStorePath(Store * store, const StorePath & expected, const StorePath & actual)
 {
-    return
-        expected.hashPart() == actual.hashPart()
-        && (expected.name() == Store::MissingName || expected.name() == actual.name());
+    if (expected.hashPart() != actual.hashPart()) {
+        throw Error(
+            "the queried store path hash '%s' did not match expected '%s' while querying the store path '%s'",
+            expected.hashPart(), actual.hashPart(), store->printStorePath(expected)
+        );
+    } else if (expected.name() != Store::MissingName && expected.name() != actual.name()) {
+        throw Error(
+            "the queried store path name '%s' did not match expected '%s' while querying the store path '%s'",
+            expected.name(), actual.name(), store->printStorePath(expected)
+        );
+    }
 }
 
 
@@ -683,7 +691,7 @@ ref<const ValidPathInfo> Store::queryPathInfo(const StorePath & storePath)
         if (res && res->isKnownNow()) {
             stats.narInfoReadAverted++;
             if (!res->didExist())
-                throw InvalidPath("path '%s' is not valid", printStorePath(storePath));
+                throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath));
             return ref<const ValidPathInfo>(res->value);
         }
     }
@@ -696,27 +704,31 @@ ref<const ValidPathInfo> Store::queryPathInfo(const StorePath & storePath)
                 auto state_(state.lock());
                 state_->pathInfoCache.upsert(std::string(storePath.to_string()),
                     res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second });
-                if (res.first == NarInfoDiskCache::oInvalid ||
-                    !goodStorePath(storePath, res.second->path))
-                    throw InvalidPath("path '%s' is not valid", printStorePath(storePath));
+                if (res.first == NarInfoDiskCache::oInvalid)
+                    throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath));
             }
             return ref<const ValidPathInfo>(res.second);
         }
     }
 
     auto info = queryPathInfoUncached(storePath);
+    if (info) {
+        // first, before we cache anything, check that the store gave us valid data.
+        ensureGoodStorePath(this, storePath, info->path);
+    }
 
-    if (diskCache)
+    if (diskCache) {
         diskCache->upsertNarInfo(getUri(), hashPart, info);
+    }
 
     {
         auto state_(state.lock());
         state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info });
     }
 
-    if (!info || !goodStorePath(storePath, info->path)) {
+    if (!info) {
         stats.narInfoMissing++;
-        throw InvalidPath("path '%s' is not valid", printStorePath(storePath));
+        throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath));
     }
 
     return ref<const ValidPathInfo>(info);
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index 397ebe759..de4dc3f03 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -399,6 +399,10 @@ public:
 
 protected:
 
+    /**
+     * Queries the path info without caching.
+     * Note to implementors: should return `nullptr` when the path is not found.
+     */
     virtual std::shared_ptr<const ValidPathInfo> queryPathInfoUncached(const StorePath & path) = 0;
     virtual std::shared_ptr<const Realisation> queryRealisationUncached(const DrvOutput &) = 0;