From 76c6f3cfd043f7823c4f8503f5f507a213e7fb50 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Wed, 24 Dec 2025 01:49:29 +0300 Subject: [PATCH] libutil: Make MemorySourceAccessor throw more precise errors Makes the error messages render paths correctly, also introduces a new hierarchy of error classes for SourceAccessor related errors that we might want to handle differently (e.g. like when doing a readFile on a directory and such). This should make it easier to implement better UnionSourceAccessor and AllowListSourceAccessor by catching these errors consistently. --- src/libutil-tests/memory-source-accessor.cc | 165 +++++++++++++++++- .../include/nix/util/source-accessor.hh | 6 +- src/libutil/memory-source-accessor.cc | 28 +-- 3 files changed, 182 insertions(+), 17 deletions(-) diff --git a/src/libutil-tests/memory-source-accessor.cc b/src/libutil-tests/memory-source-accessor.cc index 6c7c9ce9e81..0b262ea4359 100644 --- a/src/libutil-tests/memory-source-accessor.cc +++ b/src/libutil-tests/memory-source-accessor.cc @@ -1,7 +1,11 @@ -#include - #include "nix/util/memory-source-accessor.hh" #include "nix/util/tests/json-characterization.hh" +#include "nix/util/tests/gmock-matchers.hh" + +#include +#include + +#include namespace nix { @@ -59,6 +63,163 @@ ref exampleComplex() } // namespace memory_source_accessor +/* ---------------------------------------------------------------------------- + * MemorySourceAccessor + * --------------------------------------------------------------------------*/ + +using ::nix::testing::HasSubstrIgnoreANSIMatcher; + +class MemorySourceAccessorTestErrors : public ::testing::Test +{ +protected: + ref accessor = make_ref(); + MemorySink sink{*accessor}; + + void SetUp() override + { + accessor->setPathDisplay("somepath"); + sink.createDirectory(CanonPath::root); + } +}; + +TEST_F(MemorySourceAccessorTestErrors, readFileNotFound) +{ + EXPECT_THAT( + [&] { accessor->readFile(CanonPath("nonexistent")); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/nonexistent"), HasSubstrIgnoreANSIMatcher("does not exist")))); +} + +TEST_F(MemorySourceAccessorTestErrors, readFileNotARegularFile) +{ + sink.createDirectory(CanonPath("subdir")); + + EXPECT_THAT( + [&] { accessor->readFile(CanonPath("subdir")); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/subdir"), HasSubstrIgnoreANSIMatcher("is not a regular file")))); +} + +TEST_F(MemorySourceAccessorTestErrors, readDirectoryNotFound) +{ + EXPECT_THAT( + [&] { accessor->readDirectory(CanonPath("nonexistent")); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/nonexistent"), HasSubstrIgnoreANSIMatcher("does not exist")))); +} + +TEST_F(MemorySourceAccessorTestErrors, readDirectoryNotADirectory) +{ + sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {}); + + EXPECT_THAT( + [&] { accessor->readDirectory(CanonPath("file")); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/file"), HasSubstrIgnoreANSIMatcher("is not a directory")))); +} + +TEST_F(MemorySourceAccessorTestErrors, readLinkNotFound) +{ + EXPECT_THAT( + [&] { accessor->readLink(CanonPath("nonexistent")); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/nonexistent"), HasSubstrIgnoreANSIMatcher("does not exist")))); +} + +TEST_F(MemorySourceAccessorTestErrors, readLinkNotASymlink) +{ + sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {}); + + EXPECT_THAT( + [&] { accessor->readLink(CanonPath("file")); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/file"), HasSubstrIgnoreANSIMatcher("is not a symbolic link")))); +} + +TEST_F(MemorySourceAccessorTestErrors, addFileParentNotDirectory) +{ + sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {}); + + EXPECT_THAT( + [&] { accessor->addFile(CanonPath("file/child"), "contents"); }, + ThrowsMessage(AllOf( + HasSubstrIgnoreANSIMatcher("somepath/file/child"), + HasSubstrIgnoreANSIMatcher("cannot be created because some parent file is not a directory")))); +} + +TEST_F(MemorySourceAccessorTestErrors, addFileNotARegularFile) +{ + sink.createDirectory(CanonPath("subdir")); + + EXPECT_THAT( + [&] { accessor->addFile(CanonPath("subdir"), "contents"); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/subdir"), HasSubstrIgnoreANSIMatcher("is not a regular file")))); +} + +TEST_F(MemorySourceAccessorTestErrors, createDirectoryParentNotDirectory) +{ + sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {}); + + EXPECT_THAT( + [&] { sink.createDirectory(CanonPath("file/child")); }, + ThrowsMessage(AllOf( + HasSubstrIgnoreANSIMatcher("somepath/file/child"), + HasSubstrIgnoreANSIMatcher("cannot be created because some parent file is not a directory")))); +} + +TEST_F(MemorySourceAccessorTestErrors, createDirectoryNotADirectory) +{ + sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {}); + + EXPECT_THAT( + [&] { sink.createDirectory(CanonPath("file")); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/file"), HasSubstrIgnoreANSIMatcher("is not a directory")))); +} + +TEST_F(MemorySourceAccessorTestErrors, createRegularFileParentNotDirectory) +{ + sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {}); + + EXPECT_THAT( + [&] { sink.createRegularFile(CanonPath("file/child"), [](CreateRegularFileSink &) {}); }, + ThrowsMessage(AllOf( + HasSubstrIgnoreANSIMatcher("file/child"), + HasSubstrIgnoreANSIMatcher("cannot be created because some parent file is not a directory")))); +} + +TEST_F(MemorySourceAccessorTestErrors, createRegularFileNotARegularFile) +{ + sink.createDirectory(CanonPath("subdir")); + + EXPECT_THAT( + [&] { sink.createRegularFile(CanonPath("subdir"), [](CreateRegularFileSink &) {}); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/subdir"), HasSubstrIgnoreANSIMatcher("is not a regular file")))); +} + +TEST_F(MemorySourceAccessorTestErrors, createSymlinkParentNotDirectory) +{ + sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {}); + + EXPECT_THAT( + [&] { sink.createSymlink(CanonPath("file/child"), "target"); }, + ThrowsMessage(AllOf( + HasSubstrIgnoreANSIMatcher("somepath/file/child"), + HasSubstrIgnoreANSIMatcher("cannot be created because some parent file is not a directory")))); +} + +TEST_F(MemorySourceAccessorTestErrors, createSymlinkNotASymlink) +{ + sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {}); + + EXPECT_THAT( + [&] { sink.createSymlink(CanonPath("file"), "target"); }, + ThrowsMessage( + AllOf(HasSubstrIgnoreANSIMatcher("somepath/file"), HasSubstrIgnoreANSIMatcher("is not a symbolic link")))); +} + /* ---------------------------------------------------------------------------- * JSON * --------------------------------------------------------------------------*/ diff --git a/src/libutil/include/nix/util/source-accessor.hh b/src/libutil/include/nix/util/source-accessor.hh index 59af81569c7..808fcf622b6 100644 --- a/src/libutil/include/nix/util/source-accessor.hh +++ b/src/libutil/include/nix/util/source-accessor.hh @@ -30,7 +30,11 @@ enum class SymlinkResolution { Full, }; -MakeError(FileNotFound, Error); +MakeError(SourceAccessorError, Error); +MakeError(FileNotFound, SourceAccessorError); +MakeError(NotASymlink, SourceAccessorError); +MakeError(NotADirectory, SourceAccessorError); +MakeError(NotARegularFile, SourceAccessorError); /** * A read-only filesystem abstraction. This is used by the Nix diff --git a/src/libutil/memory-source-accessor.cc b/src/libutil/memory-source-accessor.cc index ec21c846dde..fee0454fda5 100644 --- a/src/libutil/memory-source-accessor.cc +++ b/src/libutil/memory-source-accessor.cc @@ -57,11 +57,11 @@ std::string MemorySourceAccessor::readFile(const CanonPath & path) { auto * f = open(path, std::nullopt); if (!f) - throw Error("file '%s' does not exist", path); + throw FileNotFound("file '%s' does not exist", showPath(path)); if (auto * r = std::get_if(&f->raw)) return r->contents; else - throw Error("file '%s' is not a regular file", path); + throw NotARegularFile("file '%s' is not a regular file", showPath(path)); } bool MemorySourceAccessor::pathExists(const CanonPath & path) @@ -105,14 +105,14 @@ MemorySourceAccessor::DirEntries MemorySourceAccessor::readDirectory(const Canon { auto * f = open(path, std::nullopt); if (!f) - throw Error("file '%s' does not exist", path); + throw FileNotFound("file '%s' does not exist", showPath(path)); if (auto * d = std::get_if(&f->raw)) { DirEntries res; for (auto & [name, file] : d->entries) res.insert_or_assign(name, file.lstat().type); return res; } else - throw Error("file '%s' is not a directory", path); + throw NotADirectory("file '%s' is not a directory", showPath(path)); return {}; } @@ -120,11 +120,11 @@ std::string MemorySourceAccessor::readLink(const CanonPath & path) { auto * f = open(path, std::nullopt); if (!f) - throw Error("file '%s' does not exist", path); + throw FileNotFound("file '%s' does not exist", showPath(path)); if (auto * s = std::get_if(&f->raw)) return s->target; else - throw Error("file '%s' is not a symbolic link", path); + throw NotASymlink("file '%s' is not a symbolic link", showPath(path)); } SourcePath MemorySourceAccessor::addFile(CanonPath path, std::string && contents) @@ -135,11 +135,11 @@ SourcePath MemorySourceAccessor::addFile(CanonPath path, std::string && contents auto * f = open(path, File{File::Regular{}}); if (!f) - throw Error("file '%s' cannot be made because some parent file is not a directory", path); + throw Error("file '%s' cannot be created because some parent file is not a directory", showPath(path)); if (auto * r = std::get_if(&f->raw)) r->contents = std::move(contents); else - throw Error("file '%s' is not a regular file", path); + throw NotARegularFile("file '%s' is not a regular file", showPath(path)); return SourcePath{ref(shared_from_this()), path}; } @@ -150,10 +150,10 @@ void MemorySink::createDirectory(const CanonPath & path) { auto * f = dst.open(path, File{File::Directory{}}); if (!f) - throw Error("file '%s' cannot be made because some parent file is not a directory", path); + throw Error("directory '%s' cannot be created because some parent file is not a directory", dst.showPath(path)); if (!std::holds_alternative(f->raw)) - throw Error("file '%s' is not a directory", path); + throw NotADirectory("file '%s' is not a directory", dst.showPath(path)); }; struct CreateMemoryRegularFile : CreateRegularFileSink @@ -174,12 +174,12 @@ void MemorySink::createRegularFile(const CanonPath & path, std::function(&f->raw)) { CreateMemoryRegularFile crf{*rp}; func(crf); } else - throw Error("file '%s' is not a regular file", path); + throw NotARegularFile("file '%s' is not a regular file", dst.showPath(path)); } void CreateMemoryRegularFile::isExecutable() @@ -201,11 +201,11 @@ void MemorySink::createSymlink(const CanonPath & path, const std::string & targe { auto * f = dst.open(path, File{File::Symlink{}}); if (!f) - throw Error("file '%s' cannot be made because some parent file is not a directory", path); + throw Error("symlink '%s' cannot be created because some parent file is not a directory", dst.showPath(path)); if (auto * s = std::get_if(&f->raw)) s->target = target; else - throw Error("file '%s' is not a symbolic link", path); + throw NotASymlink("file '%s' is not a symbolic link", dst.showPath(path)); } ref makeEmptySourceAccessor()