Merge pull request #7639 from obsidiansystems/output-names

Test store paths, with property tests, fix bug

configure.ac

@@ -274,6 +274,12 @@ fi
 PKG_CHECK_MODULES([GTEST], [gtest_main])
 
 
+# Look for rapidcheck.
+# No pkg-config yet, https://github.com/emil-e/rapidcheck/issues/302
+AC_CHECK_HEADERS([rapidcheck/gtest.h], [], [], [#include <gtest/gtest.h>])
+AC_CHECK_LIB([rapidcheck], [])
+
+
 # Look for nlohmann/json.
 PKG_CHECK_MODULES([NLOHMANN_JSON], [nlohmann_json >= 3.9])
 

doc/manual/src/contributing/hacking.md

@@ -92,7 +92,8 @@ $ nix develop
 
 The unit-tests for each Nix library (`libexpr`, `libstore`, etc..) are defined
 under `src/{library_name}/tests` using the
-[googletest](https://google.github.io/googletest/) framework.
+[googletest](https://google.github.io/googletest/) and
+[rapidcheck](https://github.com/emil-e/rapidcheck) frameworks.
 
 You can run the whole testsuite with `make check`, or the tests for a specific component with `make libfoo-tests_RUN`. Finer-grained filtering is also possible using the [--gtest_filter](https://google.github.io/googletest/advanced.html#running-a-subset-of-the-tests) command-line option.
 

flake.nix

@@ -82,7 +82,9 @@
         });
 
         configureFlags =
-          lib.optionals stdenv.isLinux [
+          [
+            "CXXFLAGS=-I${lib.getDev rapidcheck}/extras/gtest/include"
+          ] ++ lib.optionals stdenv.isLinux [
             "--with-boost=${boost}/lib"
             "--with-sandbox-shell=${sh}/bin/busybox"
           ]
@@ -116,6 +118,7 @@
             boost
             lowdown-nix
             gtest
+            rapidcheck
           ]
           ++ lib.optionals stdenv.isLinux [libseccomp]
           ++ lib.optional (stdenv.isLinux || stdenv.isDarwin) libsodium

src/libstore/derivations.hh

@@ -13,6 +13,7 @@
 
 namespace nix {
 
+class Store;
 
 /* Abstract syntax of derivations. */
 

src/libstore/outputs-spec.cc

@@ -1,8 +1,10 @@
-#include "util.hh"
-#include "outputs-spec.hh"
-#include "nlohmann/json.hpp"
-
 #include <regex>
+#include <nlohmann/json.hpp>
+
+#include "util.hh"
+#include "regex-combinators.hh"
+#include "outputs-spec.hh"
+#include "path-regex.hh"
 
 namespace nix {
 
@@ -18,11 +20,14 @@ bool OutputsSpec::contains(const std::string & outputName) const
     }, raw());
 }
 
+static std::string outputSpecRegexStr =
+    regex::either(
+        regex::group(R"(\*)"),
+        regex::group(regex::list(nameRegexStr)));
 
 std::optional<OutputsSpec> OutputsSpec::parseOpt(std::string_view s)
 {
-    // See checkName() for valid output name characters.
-    static std::regex regex(R"((\*)|([a-zA-Z\+\-\._\?=]+(,[a-zA-Z\+\-\._\?=]+)*))");
+    static std::regex regex(std::string { outputSpecRegexStr });
 
     std::smatch match;
     std::string s2 { s }; // until some improves std::regex

src/libstore/path-regex.hh

@@ -0,0 +1,7 @@
+#pragma once
+
+namespace nix {
+
+static constexpr std::string_view nameRegexStr = R"([0-9a-zA-Z\+\-\._\?=]+)";
+
+}

src/libstore/path.cc

@@ -8,8 +8,10 @@ static void checkName(std::string_view path, std::string_view name)
 {
     if (name.empty())
         throw BadStorePath("store path '%s' has an empty name", path);
-    if (name.size() > 211)
-        throw BadStorePath("store path '%s' has a name longer than 211 characters", path);
+    if (name.size() > StorePath::MaxPathLen)
+        throw BadStorePath("store path '%s' has a name longer than '%d characters",
+            StorePath::MaxPathLen, path);
+    // See nameRegexStr for the definition
     for (auto c : name)
         if (!((c >= '0' && c <= '9')
                 || (c >= 'a' && c <= 'z')

src/libstore/path.hh

@@ -5,7 +5,6 @@
 
 namespace nix {
 
-class Store;
 struct Hash;
 
 class StorePath
@@ -17,6 +16,8 @@ public:
     /* Size of the hash part of store paths, in base-32 characters. */
     constexpr static size_t HashLen = 32; // i.e. 160 bits
 
+    constexpr static size_t MaxPathLen = 211;
+
     StorePath() = delete;
 
     StorePath(std::string_view baseName);

src/libstore/realisation.hh

@@ -7,6 +7,8 @@
 
 namespace nix {
 
+class Store;
+
 struct DrvOutput {
     // The hash modulo of the derivation
     Hash drvHash;

src/libstore/tests/libstoretests.hh

@@ -0,0 +1,23 @@
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include "store-api.hh"
+
+namespace nix {
+
+class LibStoreTest : public ::testing::Test {
+    public:
+        static void SetUpTestSuite() {
+            initLibStore();
+        }
+
+    protected:
+        LibStoreTest()
+            : store(openStore("dummy://"))
+        { }
+
+        ref<Store> store;
+};
+
+
+} /* namespace nix */

src/libstore/tests/local.mk

@@ -12,4 +12,4 @@ libstore-tests_CXXFLAGS += -I src/libstore -I src/libutil
 
 libstore-tests_LIBS = libstore libutil
 
-libstore-tests_LDFLAGS := $(GTEST_LIBS)
+libstore-tests_LDFLAGS := -lrapidcheck $(GTEST_LIBS)

src/libstore/tests/outputs-spec.cc

@@ -47,6 +47,13 @@ TEST(OutputsSpec, names_underscore) {
     ASSERT_EQ(expected.to_string(), str);
 }
 
+TEST(OutputsSpec, names_numberic) {
+    std::string_view str = "01";
+    OutputsSpec expected = OutputsSpec::Names { "01" };
+    ASSERT_EQ(OutputsSpec::parse(str), expected);
+    ASSERT_EQ(expected.to_string(), str);
+}
+
 TEST(OutputsSpec, names_out_bin) {
     OutputsSpec expected = OutputsSpec::Names { "out", "bin" };
     ASSERT_EQ(OutputsSpec::parse("out,bin"), expected);

src/libstore/tests/path.cc

@@ -0,0 +1,144 @@
+#include <regex>
+
+#include <nlohmann/json.hpp>
+#include <gtest/gtest.h>
+#include <rapidcheck/gtest.h>
+
+#include "path-regex.hh"
+#include "store-api.hh"
+
+#include "libstoretests.hh"
+
+namespace nix {
+
+#define STORE_DIR "/nix/store/"
+#define HASH_PART "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q"
+
+class StorePathTest : public LibStoreTest
+{
+};
+
+static std::regex nameRegex { std::string { nameRegexStr } };
+
+#define TEST_DONT_PARSE(NAME, STR)                           \
+    TEST_F(StorePathTest, bad_ ## NAME) {                    \
+        std::string_view str =                               \
+            STORE_DIR HASH_PART "-" STR;                     \
+        ASSERT_THROW(                                        \
+            store->parseStorePath(str),                      \
+            BadStorePath);                                   \
+        std::string name { STR };                            \
+        EXPECT_FALSE(std::regex_match(name, nameRegex));     \
+    }
+
+TEST_DONT_PARSE(empty, "")
+TEST_DONT_PARSE(garbage, "&*()")
+TEST_DONT_PARSE(double_star, "**")
+TEST_DONT_PARSE(star_first, "*,foo")
+TEST_DONT_PARSE(star_second, "foo,*")
+TEST_DONT_PARSE(bang, "foo!o")
+
+#undef TEST_DONT_PARSE
+
+#define TEST_DO_PARSE(NAME, STR)                             \
+    TEST_F(StorePathTest, good_ ## NAME) {                   \
+        std::string_view str =                               \
+            STORE_DIR HASH_PART "-" STR;                     \
+        auto p = store->parseStorePath(str);                 \
+        std::string name { p.name() };                       \
+        EXPECT_TRUE(std::regex_match(name, nameRegex));      \
+    }
+
+// 0-9 a-z A-Z + - . _ ? =
+
+TEST_DO_PARSE(numbers, "02345")
+TEST_DO_PARSE(lower_case, "foo")
+TEST_DO_PARSE(upper_case, "FOO")
+TEST_DO_PARSE(plus, "foo+bar")
+TEST_DO_PARSE(dash, "foo-dev")
+TEST_DO_PARSE(underscore, "foo_bar")
+TEST_DO_PARSE(period, "foo.txt")
+TEST_DO_PARSE(question_mark, "foo?why")
+TEST_DO_PARSE(equals_sign, "foo=foo")
+
+#undef TEST_DO_PARSE
+
+// For rapidcheck
+void showValue(const StorePath & p, std::ostream & os) {
+    os << p.to_string();
+}
+
+}
+
+namespace rc {
+using namespace nix;
+
+template<>
+struct Arbitrary<StorePath> {
+    static Gen<StorePath> arbitrary();
+};
+
+Gen<StorePath> Arbitrary<StorePath>::arbitrary()
+{
+    auto len = *gen::inRange<size_t>(1, StorePath::MaxPathLen);
+
+    std::string pre { HASH_PART "-" };
+    pre.reserve(pre.size() + len);
+
+    for (size_t c = 0; c < len; ++c) {
+        switch (auto i = *gen::inRange<uint8_t>(0, 10 + 2 * 26 + 6)) {
+            case 0 ... 9:
+                pre += '0' + i;
+            case 10 ... 35:
+                pre += 'A' + (i - 10);
+                break;
+            case 36 ... 61:
+                pre += 'a' + (i - 36);
+                break;
+            case 62:
+                pre += '+';
+                break;
+            case 63:
+                pre += '-';
+                break;
+            case 64:
+                pre += '.';
+                break;
+            case 65:
+                pre += '_';
+                break;
+            case 66:
+                pre += '?';
+                break;
+            case 67:
+                pre += '=';
+                break;
+            default:
+                assert(false);
+        }
+    }
+
+    return gen::just(StorePath { pre });
+}
+
+} // namespace rc
+
+namespace nix {
+
+RC_GTEST_FIXTURE_PROP(
+    StorePathTest,
+    prop_regex_accept,
+    (const StorePath & p))
+{
+    RC_ASSERT(std::regex_match(std::string { p.name() }, nameRegex));
+}
+
+RC_GTEST_FIXTURE_PROP(
+    StorePathTest,
+    prop_round_rip,
+    (const StorePath & p))
+{
+    RC_ASSERT(p == store->parseStorePath(store->printStorePath(p)));
+}
+
+}

src/libutil/regex-combinators.hh

@@ -0,0 +1,30 @@
+#pragma once
+
+#include <string_view>
+
+namespace nix::regex {
+
+// TODO use constexpr string building like
+// https://github.com/akrzemi1/static_string/blob/master/include/ak_toolkit/static_string.hpp
+
+static inline std::string either(std::string_view a, std::string_view b)
+{
+    return std::string { a } + "|" + b;
+}
+
+static inline std::string group(std::string_view a)
+{
+    return std::string { "(" } + a + ")";
+}
+
+static inline std::string many(std::string_view a)
+{
+    return std::string { "(?:" } + a + ")*";
+}
+
+static inline std::string list(std::string_view a)
+{
+    return std::string { a } + many(group("," + a));
+}
+
+}