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
This commit is contained in:
julia 2024-05-18 15:38:33 +10:00
parent 4734ce7831
commit 0fa289f559
5 changed files with 32 additions and 16 deletions

View file

@ -361,7 +361,7 @@ std::shared_ptr<const ValidPathInfo> BinaryCacheStore::queryPathInfoUncached(con
auto data = getFile(narInfoFile); auto data = getFile(narInfoFile);
if (!data) return {}; if (!data) return nullptr;
stats.narInfoRead++; stats.narInfoRead++;

View file

@ -899,7 +899,7 @@ std::shared_ptr<const ValidPathInfo> LocalStore::queryPathInfoInternal(State & s
auto useQueryPathInfo(state.stmts->QueryPathInfo.use()(printStorePath(path))); auto useQueryPathInfo(state.stmts->QueryPathInfo.use()(printStorePath(path)));
if (!useQueryPathInfo.next()) if (!useQueryPathInfo.next())
return std::shared_ptr<ValidPathInfo>(); return nullptr;
auto id = useQueryPathInfo.getInt(0); auto id = useQueryPathInfo.getInt(0);

View file

@ -309,14 +309,14 @@ std::shared_ptr<const ValidPathInfo> RemoteStore::queryPathInfoUncached(const St
try { try {
conn.processStderr(); conn.processStderr();
} catch (Error & e) { } 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) if (e.msg().find("is not valid") != std::string::npos)
throw InvalidPath(std::move(e.info())); return nullptr;
throw; throw;
} }
if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 17) { if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 17) {
bool valid; conn->from >> valid; 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>( return std::make_shared<ValidPathInfo>(
StorePath{path}, StorePath{path},

View file

@ -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 if (expected.hashPart() != actual.hashPart()) {
expected.hashPart() == actual.hashPart() throw Error(
&& (expected.name() == Store::MissingName || expected.name() == actual.name()); "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()) { if (res && res->isKnownNow()) {
stats.narInfoReadAverted++; stats.narInfoReadAverted++;
if (!res->didExist()) 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); return ref<const ValidPathInfo>(res->value);
} }
} }
@ -696,27 +704,31 @@ ref<const ValidPathInfo> Store::queryPathInfo(const StorePath & storePath)
auto state_(state.lock()); auto state_(state.lock());
state_->pathInfoCache.upsert(std::string(storePath.to_string()), state_->pathInfoCache.upsert(std::string(storePath.to_string()),
res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second }); res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second });
if (res.first == NarInfoDiskCache::oInvalid || if (res.first == NarInfoDiskCache::oInvalid)
!goodStorePath(storePath, res.second->path)) throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath));
throw InvalidPath("path '%s' is not valid", printStorePath(storePath));
} }
return ref<const ValidPathInfo>(res.second); return ref<const ValidPathInfo>(res.second);
} }
} }
auto info = queryPathInfoUncached(storePath); 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); diskCache->upsertNarInfo(getUri(), hashPart, info);
}
{ {
auto state_(state.lock()); auto state_(state.lock());
state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info }); state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info });
} }
if (!info || !goodStorePath(storePath, info->path)) { if (!info) {
stats.narInfoMissing++; 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); return ref<const ValidPathInfo>(info);

View file

@ -399,6 +399,10 @@ public:
protected: 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 ValidPathInfo> queryPathInfoUncached(const StorePath & path) = 0;
virtual std::shared_ptr<const Realisation> queryRealisationUncached(const DrvOutput &) = 0; virtual std::shared_ptr<const Realisation> queryRealisationUncached(const DrvOutput &) = 0;