Skip to content

libutil: Make MemorySourceAccessor throw more precise errors #14862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xokdvium merged 1 commit into from
Dec 24, 2025
+182 −17
Merged

libutil: Make MemorySourceAccessor throw more precise errors #14862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 163 additions & 2 deletions src/libutil-tests/memory-source-accessor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
#include <string_view>

#include "nix/util/memory-source-accessor.hh"
#include "nix/util/tests/json-characterization.hh"
#include "nix/util/tests/gmock-matchers.hh"

#include <gmock/gmock.h>
#include <gtest/gtest.h>

#include <string_view>

namespace nix {

Expand Down Expand Up @@ -59,6 +63,163 @@ ref<MemorySourceAccessor> exampleComplex()

} // namespace memory_source_accessor

/* ----------------------------------------------------------------------------
* MemorySourceAccessor
* --------------------------------------------------------------------------*/

using ::nix::testing::HasSubstrIgnoreANSIMatcher;

class MemorySourceAccessorTestErrors : public ::testing::Test
{
protected:
ref<MemorySourceAccessor> accessor = make_ref<MemorySourceAccessor>();
MemorySink sink{*accessor};

void SetUp() override
{
accessor->setPathDisplay("somepath");
sink.createDirectory(CanonPath::root);
}
};

TEST_F(MemorySourceAccessorTestErrors, readFileNotFound)
{
EXPECT_THAT(
[&] { accessor->readFile(CanonPath("nonexistent")); },
ThrowsMessage<FileNotFound>(
AllOf(HasSubstrIgnoreANSIMatcher("somepath/nonexistent"), HasSubstrIgnoreANSIMatcher("does not exist"))));
}

TEST_F(MemorySourceAccessorTestErrors, readFileNotARegularFile)
{
sink.createDirectory(CanonPath("subdir"));

EXPECT_THAT(
[&] { accessor->readFile(CanonPath("subdir")); },
ThrowsMessage<NotARegularFile>(
AllOf(HasSubstrIgnoreANSIMatcher("somepath/subdir"), HasSubstrIgnoreANSIMatcher("is not a regular file"))));
}

TEST_F(MemorySourceAccessorTestErrors, readDirectoryNotFound)
{
EXPECT_THAT(
[&] { accessor->readDirectory(CanonPath("nonexistent")); },
ThrowsMessage<FileNotFound>(
AllOf(HasSubstrIgnoreANSIMatcher("somepath/nonexistent"), HasSubstrIgnoreANSIMatcher("does not exist"))));
}

TEST_F(MemorySourceAccessorTestErrors, readDirectoryNotADirectory)
{
sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {});

EXPECT_THAT(
[&] { accessor->readDirectory(CanonPath("file")); },
ThrowsMessage<NotADirectory>(
AllOf(HasSubstrIgnoreANSIMatcher("somepath/file"), HasSubstrIgnoreANSIMatcher("is not a directory"))));
}

TEST_F(MemorySourceAccessorTestErrors, readLinkNotFound)
{
EXPECT_THAT(
[&] { accessor->readLink(CanonPath("nonexistent")); },
ThrowsMessage<FileNotFound>(
AllOf(HasSubstrIgnoreANSIMatcher("somepath/nonexistent"), HasSubstrIgnoreANSIMatcher("does not exist"))));
}

TEST_F(MemorySourceAccessorTestErrors, readLinkNotASymlink)
{
sink.createRegularFile(CanonPath("file"), [](CreateRegularFileSink &) {});

EXPECT_THAT(
[&] { accessor->readLink(CanonPath("file")); },
ThrowsMessage<NotASymlink>(
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<Error>(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<NotARegularFile>(
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<Error>(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<NotADirectory>(
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<Error>(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<NotARegularFile>(
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<Error>(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<NotASymlink>(
AllOf(HasSubstrIgnoreANSIMatcher("somepath/file"), HasSubstrIgnoreANSIMatcher("is not a symbolic link"))));
}

/* ----------------------------------------------------------------------------
* JSON
* --------------------------------------------------------------------------*/
Expand Down
6 changes: 5 additions & 1 deletion src/libutil/include/nix/util/source-accessor.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 14 additions & 14 deletions src/libutil/memory-source-accessor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<File::Regular>(&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)
Expand Down Expand Up @@ -105,26 +105,26 @@ 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<File::Directory>(&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 {};
}

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<File::Symlink>(&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)
Expand All @@ -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<File::Regular>(&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));
Copy link
Contributor

@Radvendii Radvendii Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be "cannot be created because it already exists and is not a regular file" or something like that?

Copy link
Contributor Author

@xokdvium xokdvium Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, ideally. I didn't want to make the scope too large for now, since MemorySourceAccessor isn't very user-facing.


return SourcePath{ref(shared_from_this()), path};
}
Expand All @@ -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<File::Directory>(f->raw))
throw Error("file '%s' is not a directory", path);
Copy link
Contributor

@Radvendii Radvendii Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar transformation here "cannot be created because it already exists and is not a directory"

throw NotADirectory("file '%s' is not a directory", dst.showPath(path));
};

struct CreateMemoryRegularFile : CreateRegularFileSink
Expand All @@ -174,12 +174,12 @@ void MemorySink::createRegularFile(const CanonPath & path, std::function<void(Cr
{
auto * f = dst.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", path);
if (auto * rp = std::get_if<File::Regular>(&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()
Expand All @@ -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<File::Symlink>(&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<SourceAccessor> makeEmptySourceAccessor()
Expand Down
Loading