From 76524b92eeda0ad1c6f77fcb07862406e2960215 Mon Sep 17 00:00:00 2001 From: Tom Hubrecht Date: Fri, 25 Apr 2025 20:52:27 +0200 Subject: [PATCH] libfetchers: factorize inputFromAttrs Each `inputFromAttrs` is roughly the same function in each class, we check that the attributes given are correct (in term of keys and other types) then we coppy the attributes. Instead of having the same code copied in 10 places, set it in the parent class and specify what is specific per child class. Change-Id: If9aecb76cff1e28a1ef6668d83d825686cce8353 --- lix/libfetchers/fetchers.cc | 18 +++++++++++++++ lix/libfetchers/fetchers.hh | 14 ++++++++++-- lix/libfetchers/git.cc | 33 ++++++++++++++++++--------- lix/libfetchers/github.cc | 43 ++++++++++++++++++------------------ lix/libfetchers/indirect.cc | 35 ++++++++++++++++++++--------- lix/libfetchers/mercurial.cc | 26 +++++++++++++--------- lix/libfetchers/path.cc | 36 ++++++++++++++++-------------- lix/libfetchers/tarball.cc | 43 ++++++++++++++++++++---------------- tests/functional/fetchers.sh | 6 ++--- 9 files changed, 159 insertions(+), 95 deletions(-) diff --git a/lix/libfetchers/fetchers.cc b/lix/libfetchers/fetchers.cc index 0dc9f5e0c..dd8ec9436 100644 --- a/lix/libfetchers/fetchers.cc +++ b/lix/libfetchers/fetchers.cc @@ -304,6 +304,24 @@ std::optional Input::getLastModified() const return maybeGetIntAttr(attrs, "lastModified"); } +std::optional InputScheme::inputFromAttrs(const Attrs & attrs) const +{ + if (maybeGetStrAttr(attrs, "type") != schemeType()) return {}; + + Attrs finalAttrs = preprocessAttrs(attrs); + + for (auto & [name, value] : finalAttrs) + // All attrs need to accept a `type` and `narHash` key, the rest is scheme-specific + if (name != "type" && name != "narHash" && !allowedAttrs().contains(name)) + throw UnsupportedAttributeError("unsupported input attribute '%s' for the '%s' scheme", name, schemeType()); + + Input input; + input.attrs = finalAttrs; + return input; +} + + + ParsedURL InputScheme::toURL(const Input & input) const { throw Error("don't know how to convert input '%s' to a URL", attrsToJSON(input.attrs)); diff --git a/lix/libfetchers/fetchers.hh b/lix/libfetchers/fetchers.hh index 420256bd2..77bbc8805 100644 --- a/lix/libfetchers/fetchers.hh +++ b/lix/libfetchers/fetchers.hh @@ -2,6 +2,7 @@ ///@file #include "lix/libstore/content-address.hh" +#include "lix/libutil/error.hh" #include "lix/libutil/result.hh" #include "lix/libutil/types.hh" #include "lix/libutil/hash.hh" @@ -19,6 +20,8 @@ namespace nix { class Store; } namespace nix::fetchers { +MakeError(UnsupportedAttributeError, Error); + struct Tree { Path actualPath; @@ -125,7 +128,6 @@ public: std::optional getLastModified() const; }; - /** * The InputScheme represents a type of fetcher. Each fetcher * registers with nix at startup time. When processing an input for a @@ -142,7 +144,12 @@ struct InputScheme virtual std::optional inputFromURL(const ParsedURL & url, bool requireTree) const = 0; - virtual std::optional inputFromAttrs(const Attrs & attrs) const = 0; + virtual Attrs preprocessAttrs(const Attrs & attrs) const = 0; + + // The scheme type, which is used to match attributes to a specific scheme + virtual std::string schemeType() const = 0; + + virtual std::optional inputFromAttrs(const Attrs & attrs) const; virtual ParsedURL toURL(const Input & input) const; @@ -174,6 +181,9 @@ struct InputScheme virtual bool isLockedByRev() const { return true; } protected: + // The set of allowed attributes for this specific fetcher + virtual const std::set & allowedAttrs() const = 0; + void emplaceURLQueryIntoAttrs( const ParsedURL & parsedURL, Attrs & attrs, diff --git a/lix/libfetchers/git.cc b/lix/libfetchers/git.cc index 21fa1904d..e4b2d51cb 100644 --- a/lix/libfetchers/git.cc +++ b/lix/libfetchers/git.cc @@ -288,8 +288,28 @@ static std::optional resolveRefToCachePath( return std::nullopt; } +static const std::set allowedGitAttrs = { + "allRefs", + "dirtyRev", + "dirtyShortRev", + "lastModified", + "name", + "ref", + "rev", + "revCount", + "shallow", + "submodules", + "url", +}; + struct GitInputScheme : InputScheme { + std::string schemeType() const override { return "git"; } + + const std::set & allowedAttrs() const override { + return allowedGitAttrs; + } + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != "git" && @@ -316,14 +336,7 @@ struct GitInputScheme : InputScheme return inputFromAttrs(attrs); } - std::optional inputFromAttrs(const Attrs & attrs) const override - { - if (maybeGetStrAttr(attrs, "type") != "git") return {}; - - for (auto & [name, value] : attrs) - if (name != "type" && name != "url" && name != "ref" && name != "rev" && name != "shallow" && name != "submodules" && name != "lastModified" && name != "revCount" && name != "narHash" && name != "allRefs" && name != "name" && name != "dirtyRev" && name != "dirtyShortRev") - throw Error("unsupported Git input attribute '%s'", name); - + Attrs preprocessAttrs(const Attrs & attrs) const override { parseURL(getStrAttr(attrs, "url")); maybeGetBoolAttr(attrs, "shallow"); maybeGetBoolAttr(attrs, "submodules"); @@ -334,9 +347,7 @@ struct GitInputScheme : InputScheme throw BadURL("invalid Git branch/tag name '%s'", *ref); } - Input input; - input.attrs = attrs; - return input; + return attrs; } ParsedURL toURL(const Input & input) const override diff --git a/lix/libfetchers/github.cc b/lix/libfetchers/github.cc index dcd1d4632..c8402b80e 100644 --- a/lix/libfetchers/github.cc +++ b/lix/libfetchers/github.cc @@ -1,7 +1,5 @@ #include "lix/libfetchers/attrs.hh" #include "lix/libstore/filetransfer.hh" -#include "lix/libfetchers/cache.hh" -#include "lix/libstore/globals.hh" #include "lix/libfetchers/builtin-fetchers.hh" #include "lix/libstore/store-api.hh" #include "lix/libutil/async.hh" @@ -24,19 +22,30 @@ struct DownloadUrl Headers headers; }; +static const std::set allowedGitArchiveAttrs = { + "host", + "lastModified", + "owner", + "ref", + "repo", + "rev", +}; + // A github, gitlab, or sourcehut host const static std::string hostRegexS = "[a-zA-Z0-9.-]*"; // FIXME: check std::regex hostRegex = regex::parse(hostRegexS, std::regex::ECMAScript); struct GitArchiveInputScheme : InputScheme { - virtual std::string type() const = 0; + const std::set & allowedAttrs() const override { + return allowedGitArchiveAttrs; + } virtual std::optional> accessHeaderFromToken(const std::string & token) const = 0; std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { - if (url.scheme != type()) return {}; + if (url.scheme != schemeType()) return {}; auto path = tokenizeString>(url.path, "/"); @@ -63,7 +72,7 @@ struct GitArchiveInputScheme : InputScheme throw BadURL("URL '%s' is invalid", url.url); Attrs attrs; - attrs.emplace("type", type()); + attrs.emplace("type", schemeType()); attrs.emplace("owner", path[0]); attrs.emplace("repo", path[1]); @@ -93,20 +102,18 @@ struct GitArchiveInputScheme : InputScheme return inputFromAttrs(attrs); } - std::optional inputFromAttrs(const Attrs & attrs) const override + Attrs preprocessAttrs(const Attrs & attrs) const override { // Attributes can contain refOrRev and it needs to be figured out // which one it is (see inputFromURL for when that may happen). // The correct one (ref or rev) will be written into finalAttrs and // it needs to be mutable for that. Attrs finalAttrs(attrs); - auto type_ = maybeGetStrAttr(finalAttrs, "type"); - if (type_ != type()) return {}; auto owner = getStrAttr(finalAttrs, "owner"); auto repo = getStrAttr(finalAttrs, "repo"); - auto url = fmt("%s:%s/%s", *type_, owner, repo); + auto url = fmt("%s:%s/%s", schemeType(), owner, repo); if (auto host = maybeGetStrAttr(finalAttrs, "host")) { if (!std::regex_match(*host, hostRegex)) { throw BadURL("URL '%s' contains an invalid instance host", url); @@ -132,15 +139,7 @@ struct GitArchiveInputScheme : InputScheme } } - for (auto & [name, value] : finalAttrs) { - if (name != "type" && name != "owner" && name != "repo" && name != "ref" && name != "rev" && name != "narHash" && name != "lastModified" && name != "host") { - throw Error("unsupported input attribute '%s'", name); - } - } - - Input input; - input.attrs = finalAttrs; - return input; + return finalAttrs; } ParsedURL toURL(const Input & input) const override @@ -154,7 +153,7 @@ struct GitArchiveInputScheme : InputScheme if (ref) path += "/" + *ref; if (rev) path += "/" + rev->to_string(Base::Base16, false); return ParsedURL { - .scheme = type(), + .scheme = schemeType(), .path = path, }; } @@ -239,7 +238,7 @@ struct GitArchiveInputScheme : InputScheme struct GitHubInputScheme : GitArchiveInputScheme { - std::string type() const override { return "github"; } + std::string schemeType() const override { return "github"; } std::optional> accessHeaderFromToken(const std::string & token) const override { @@ -329,7 +328,7 @@ struct GitHubInputScheme : GitArchiveInputScheme struct GitLabInputScheme : GitArchiveInputScheme { - std::string type() const override { return "gitlab"; } + std::string schemeType() const override { return "gitlab"; } std::optional> accessHeaderFromToken(const std::string & token) const override { @@ -405,7 +404,7 @@ struct GitLabInputScheme : GitArchiveInputScheme struct SourceHutInputScheme : GitArchiveInputScheme { - std::string type() const override { return "sourcehut"; } + std::string schemeType() const override { return "sourcehut"; } std::optional> accessHeaderFromToken(const std::string & token) const override { diff --git a/lix/libfetchers/indirect.cc b/lix/libfetchers/indirect.cc index 342c8974e..c671adc5c 100644 --- a/lix/libfetchers/indirect.cc +++ b/lix/libfetchers/indirect.cc @@ -8,8 +8,20 @@ namespace nix::fetchers { std::regex flakeRegex = regex::parse("[a-zA-Z][a-zA-Z0-9_-]*", std::regex::ECMAScript); +static const std::set allowedIndirectAttrs = { + "id", + "ref", + "rev", +}; + struct IndirectInputScheme : InputScheme { + std::string schemeType() const override { return "indirect"; } + + const std::set & allowedAttrs() const override { + return allowedIndirectAttrs; + } + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != "flake") return {}; @@ -47,14 +59,7 @@ struct IndirectInputScheme : InputScheme return inputFromAttrs(attrs); } - std::optional inputFromAttrs(const Attrs & attrs) const override - { - if (maybeGetStrAttr(attrs, "type") != "indirect") return {}; - - for (auto & [name, value] : attrs) - if (name != "type" && name != "id" && name != "ref" && name != "rev" && name != "narHash") - throw Error("unsupported indirect input attribute '%s'", name); - + Attrs preprocessAttrs(const Attrs & attrs) const override { auto id = getStrAttr(attrs, "id"); if (!std::regex_match(id, flakeRegex)) throw BadURL("'%s' is not a valid flake ID", id); @@ -71,9 +76,17 @@ struct IndirectInputScheme : InputScheme } } - Input input; - input.direct = false; - input.attrs = attrs; + return attrs; + } + + std::optional inputFromAttrs(const Attrs & attrs) const override + { + std::optional input = InputScheme::inputFromAttrs(attrs); + + if (input) { + input->direct = false; + } + return input; } diff --git a/lix/libfetchers/mercurial.cc b/lix/libfetchers/mercurial.cc index 0cd007d8c..72300ca52 100644 --- a/lix/libfetchers/mercurial.cc +++ b/lix/libfetchers/mercurial.cc @@ -44,8 +44,22 @@ static std::string runHg(const Strings & args) return res.second; } +static const std::set allowedMercurialAttrs = { + "name", + "ref", + "rev", + "revCount", + "url", +}; + struct MercurialInputScheme : InputScheme { + std::string schemeType() const override { return "hg"; } + + const std::set & allowedAttrs() const override { + return allowedMercurialAttrs; + } + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != "hg+http" && @@ -67,14 +81,8 @@ struct MercurialInputScheme : InputScheme return inputFromAttrs(attrs); } - std::optional inputFromAttrs(const Attrs & attrs) const override + Attrs preprocessAttrs(const Attrs & attrs) const override { - if (maybeGetStrAttr(attrs, "type") != "hg") return {}; - - for (auto & [name, value] : attrs) - if (name != "type" && name != "url" && name != "ref" && name != "rev" && name != "revCount" && name != "narHash" && name != "name") - throw Error("unsupported Mercurial input attribute '%s'", name); - parseURL(getStrAttr(attrs, "url")); if (auto ref = maybeGetStrAttr(attrs, "ref")) { @@ -82,9 +90,7 @@ struct MercurialInputScheme : InputScheme throw BadURL("invalid Mercurial branch/tag name '%s'", *ref); } - Input input; - input.attrs = attrs; - return input; + return attrs; } ParsedURL toURL(const Input & input) const override diff --git a/lix/libfetchers/path.cc b/lix/libfetchers/path.cc index dc45db021..c2da221b5 100644 --- a/lix/libfetchers/path.cc +++ b/lix/libfetchers/path.cc @@ -6,8 +6,25 @@ namespace nix::fetchers { +/* Allow the user to pass in "fake" tree info + attributes. This is useful for making a pinned tree + work the same as the repository from which is exported + (e.g. path:/nix/store/...-source?lastModified=1585388205&rev=b0c285...). */ +static const std::set allowedPathAttrs = { + "lastModified", + "path", + "rev", + "revCount", +}; + struct PathInputScheme : InputScheme { + std::string schemeType() const override { return "path"; } + + const std::set & allowedAttrs() const override { + return allowedPathAttrs; + } + std::optional inputFromURL(const ParsedURL & url, bool requireTree) const override { if (url.scheme != "path") return {}; @@ -34,26 +51,11 @@ struct PathInputScheme : InputScheme return input; } - std::optional inputFromAttrs(const Attrs & attrs) const override + Attrs preprocessAttrs(const Attrs & attrs) const override { - if (maybeGetStrAttr(attrs, "type") != "path") return {}; - getStrAttr(attrs, "path"); - for (auto & [name, value] : attrs) - /* Allow the user to pass in "fake" tree info - attributes. This is useful for making a pinned tree - work the same as the repository from which is exported - (e.g. path:/nix/store/...-source?lastModified=1585388205&rev=b0c285...). */ - if (name == "type" || name == "rev" || name == "revCount" || name == "lastModified" || name == "narHash" || name == "path") - // checked in Input::fromAttrs - ; - else - throw Error("unsupported path input attribute '%s'", name); - - Input input; - input.attrs = attrs; - return input; + return attrs; } bool isLockedByRev() const override { return false; } diff --git a/lix/libfetchers/tarball.cc b/lix/libfetchers/tarball.cc index c6fc52161..542c2aba1 100644 --- a/lix/libfetchers/tarball.cc +++ b/lix/libfetchers/tarball.cc @@ -230,12 +230,25 @@ try { co_return result::current_exception(); } +// FIXME: some of these only apply to TarballInputScheme. +static const std::set allowedCurlAttrs = { + "lastModified", + "name", + "rev", + "revCount", + "unpack", + "url", +}; + // An input scheme corresponding to a curl-downloadable resource. struct CurlInputScheme : InputScheme { - virtual const std::string inputType() const = 0; const std::set transportUrlSchemes = {"file", "http", "https"}; + const std::set & allowedAttrs() const override { + return allowedCurlAttrs; + } + bool hasTarballExtension(std::string_view path) const { return path.ends_with(".zip") || path.ends_with(".tar") @@ -254,7 +267,7 @@ struct CurlInputScheme : InputScheme auto url = _url; Attrs attrs; - attrs.emplace("type", inputType()); + attrs.emplace("type", schemeType()); url.scheme = parseUrlScheme(url.scheme).transport; @@ -264,24 +277,16 @@ struct CurlInputScheme : InputScheme return inputFromAttrs(attrs); } - std::optional inputFromAttrs(const Attrs & attrs) const override + Attrs preprocessAttrs(const Attrs & attrs) const override { - auto type = maybeGetStrAttr(attrs, "type"); - if (type != inputType()) return {}; - - // FIXME: some of these only apply to TarballInputScheme. - std::set allowedNames = {"type", "url", "narHash", "name", "unpack", "rev", "revCount", "lastModified"}; for (auto & [name, value] : attrs) - if (!allowedNames.count(name)) - throw Error("unsupported %s input attribute '%s'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }'", *type, name); + if (name != "type" && name != "narHash" && !allowedAttrs().contains(name)) + throw UnsupportedAttributeError("unsupported tarball input attribute '%s'. If you wanted to fetch a tarball with a query parameter, please use '{ type = \"tarball\"; url = \"...\"; }'", name); - Input input; - input.attrs = attrs; - - //input.locked = (bool) maybeGetStrAttr(input.attrs, "hash"); - return input; + return attrs; } + ParsedURL toURL(const Input & input) const override { auto url = parseURL(getStrAttr(input.attrs, "url")); @@ -303,14 +308,14 @@ struct CurlInputScheme : InputScheme struct FileInputScheme : CurlInputScheme { - const std::string inputType() const override { return "file"; } + std::string schemeType() const override { return "file"; } bool isValidURL(const ParsedURL & url, bool requireTree) const override { auto parsedUrlScheme = parseUrlScheme(url.scheme); return transportUrlSchemes.count(std::string(parsedUrlScheme.transport)) && (parsedUrlScheme.application - ? parsedUrlScheme.application.value() == inputType() + ? parsedUrlScheme.application.value() == schemeType() : (!requireTree && !hasTarballExtension(url.path))); } @@ -328,7 +333,7 @@ struct FileInputScheme : CurlInputScheme struct TarballInputScheme : CurlInputScheme { - const std::string inputType() const override { return "tarball"; } + std::string schemeType() const override { return "tarball"; } bool isValidURL(const ParsedURL & url, bool requireTree) const override { @@ -336,7 +341,7 @@ struct TarballInputScheme : CurlInputScheme return transportUrlSchemes.count(std::string(parsedUrlScheme.transport)) && (parsedUrlScheme.application - ? parsedUrlScheme.application.value() == inputType() + ? parsedUrlScheme.application.value() == schemeType() : (requireTree || hasTarballExtension(url.path))); } diff --git a/tests/functional/fetchers.sh b/tests/functional/fetchers.sh index d1d6d1b63..b9c953862 100644 --- a/tests/functional/fetchers.sh +++ b/tests/functional/fetchers.sh @@ -59,7 +59,7 @@ testFetchTreeError \ # test for unsupported attributes / validation in git fetcher testFetchTreeError \ "\"git+https://github.com/owner/repo?invalid=1\"" \ - "unsupported Git input attribute 'invalid'" + "unsupported input attribute 'invalid' for the 'git' scheme" testFetchTreeError \ "\"git+https://github.com/owner/repo?url=foo\"" \ @@ -76,11 +76,11 @@ testFetchTreeError \ # same for mercurial testFetchTreeError \ "\"hg+https://forge.tld/owner/repo?invalid=1\"" \ - "unsupported Mercurial input attribute 'invalid'" + "unsupported input attribute 'invalid' for the 'hg' scheme" testFetchTreeError \ "{ type = \"hg\"; url = \"https://forge.tld/owner/repo\"; invalid = 1; }" \ - "unsupported Mercurial input attribute 'invalid'" + "unsupported input attribute 'invalid' for the 'hg' scheme" testFetchTreeError \ "\"hg+https://forge.tld/owner/repo?ref=,\"" \