fix formats/yaml: check for key uniqueness in mappings

Anton3 · Anton3 · commit 185112e108bb · 2025-12-18T15:02:28.000+03:00
commit_hash:18d56cb82d6bdccbc3e28c26aa528208a31a733e
diff --git a/core/src/components/common_server_component_list_test.cpp b/core/src/components/common_server_component_list_test.cpp
@@ -60,8 +60,6 @@ constexpr std::string_view kStaticConfig = R"(
           overflow_behavior: $overflow_behavior
     tracer:
         service-name: config-service
-    statistics-storage:
-      # Nothing
     dynamic-config:
       updates-enabled: true
       fs-cache-path: $dynamic-config-cache-path
@@ -109,8 +107,6 @@ constexpr std::string_view kStaticConfig = R"(
       config-settings: false
       additional-cleanup-interval: 5m
       first-update-fail-ok: true
-    tracer:
-        service-name: config-service
     statistics-storage:
       # Nothing
     http-client-statistics-core:
diff --git a/redis/functional_tests/pytest_redis_cluster_topology_plugin/pytest_plugin.py b/redis/functional_tests/pytest_redis_cluster_topology_plugin/pytest_plugin.py
@@ -189,7 +189,9 @@ def stop(self):
 
     def assert_running(self):
         pid = self._read_pid()
-        assert _is_pid_running(pid), f'Redis server process is dead, please look at the logfile {self.logpath}'
+        assert pid is not None and _is_pid_running(pid), (
+            f'Redis server process is dead, please look at the logfile {self.log_path}'
+        )
 
     def _check_instance(self):
         with self.get_client() as client:
diff --git a/universal/include/userver/formats/yaml/serialize.hpp b/universal/include/userver/formats/yaml/serialize.hpp
@@ -30,6 +30,10 @@ namespace blocking {
 formats::yaml::Value FromFile(const std::string& path);
 }  // namespace blocking
 
+namespace impl {
+formats::yaml::Value FromStringAllowRepeatedKeys(const std::string& doc);
+}  // namespace impl
+
 }  // namespace formats::yaml
 
 USERVER_NAMESPACE_END
diff --git a/universal/include/userver/formats/yaml/value.hpp b/universal/include/userver/formats/yaml/value.hpp
@@ -19,6 +19,10 @@ namespace formats::yaml {
 
 class ValueBuilder;
 
+namespace impl {
+formats::yaml::Value FromStringAllowRepeatedKeys(const std::string& doc);
+}  // namespace impl
+
 /// @ingroup userver_universal userver_containers userver_formats
 ///
 /// @brief Non-mutable YAML value representation.
@@ -291,6 +295,7 @@ class Value final {
     friend formats::yaml::Value FromString(const std::string&);
     friend formats::yaml::Value FromStream(std::istream&);
     friend void Serialize(const formats::yaml::Value&, std::ostream&);
+    friend formats::yaml::Value impl::FromStringAllowRepeatedKeys(const std::string& doc);
 };
 
 template <typename T>
diff --git a/universal/src/formats/yaml/serialize.cpp b/universal/src/formats/yaml/serialize.cpp
@@ -1,27 +1,187 @@
 #include <userver/formats/yaml/serialize.hpp>
 
+#include <algorithm>
 #include <array>
 #include <fstream>
+#include <functional>
 #include <memory>
 #include <sstream>
+#include <string_view>
+#include <type_traits>
 
 #include <fmt/format.h>
+#include <yaml-cpp/yaml.h>
+#include <boost/container/small_vector.hpp>
 
+#include <userver/formats/common/path.hpp>
 #include <userver/formats/yaml/exception.hpp>
-
-#include <yaml-cpp/yaml.h>
+#include <userver/formats/yaml/value.hpp>
+#include <userver/utils/enumerate.hpp>
+#include <userver/utils/not_null.hpp>
 
 USERVER_NAMESPACE_BEGIN
 
 namespace formats::yaml {
 
+namespace {
+
+constexpr std::size_t kInitialStackDepth = 16;
+constexpr std::size_t kInitialSortedKeysSize = 32;
+
+struct PathElement {
+    explicit PathElement(YAML::Node& parent)
+        : current(parent.begin()),
+          end(parent.end())
+    {}
+
+    // Cannot store Node[&], because it has broken copy/move semantics for iteration.
+    // https://github.com/jbeder/yaml-cpp/issues/928
+    //
+    // Cannot store const_iterator, because incurs extra copies.
+    YAML::Node::iterator current;
+    YAML::Node::iterator end;
+};
+
+class RawYamlVisitor final {
+public:
+    explicit RawYamlVisitor(YAML::Node& root)
+        : root_(root)
+    {}
+
+    template <typename Visitor, typename = std::enable_if_t<std::is_invocable_v<Visitor&, YAML::Node&>>>
+    void VisitPreOrder(Visitor visitor) {
+        if (!root_.IsSequence() && !root_.IsMap()) {
+            return;
+        }
+
+        visitor(root_);
+        path_stack_.emplace_back(root_);
+
+        while (true) {
+            if (path_stack_.back().current == path_stack_.back().end) {
+                path_stack_.pop_back();
+                if (path_stack_.empty()) {
+                    break;
+                }
+                ++path_stack_.back().current;
+                continue;
+            }
+
+            auto item = *path_stack_.back().current;
+            auto& node = item.IsDefined() ? item : item.second;
+
+            visitor(node);
+            if (node.IsSequence() || node.IsMap()) {
+                path_stack_.emplace_back(node);
+            } else {
+                ++path_stack_.back().current;
+            }
+        }
+    }
+
+    // Complexity: O(whole-yaml-size)
+    std::string ComputeCurrentPath() const {
+        if (path_stack_.empty()) {
+            return common::kPathRoot;
+        }
+
+        std::string path;
+        for (const auto [idx, element] : utils::enumerate(path_stack_)) {
+            if (element.current->IsDefined()) {
+                // Sequence iterator.
+                const auto begin = idx == 0 ? root_.begin() : path_stack_[idx - 1].current->begin();
+                common::AppendPath(path, std::distance(begin, element.current));
+            } else {
+                // Map iterator.
+                const auto map_item = *element.current;
+                const auto& key = map_item.first;
+                switch (key.Type()) {
+                    case YAML::NodeType::Null:
+                        common::AppendPath(path, "null");
+                        break;
+                    case YAML::NodeType::Scalar:
+                        common::AppendPath(path, key.Scalar());
+                        break;
+                    default: {
+                        // YAML can have non-scalar keys:
+                        // https://yaml.org/spec/1.2.2/#mapping
+                        std::ostringstream stream;
+                        stream << key;
+                        common::AppendPath(path, stream.str());
+                        break;
+                    }
+                }
+            }
+        }
+        return path;
+    }
+
+private:
+    YAML::Node& root_;
+    boost::container::small_vector<PathElement, kInitialStackDepth> path_stack_;
+};
+
+// Non-unique keys in mappings are not allowed, but yaml-cpp does not check for them:
+// https://github.com/jbeder/yaml-cpp/issues/60
+class UniquenessChecker final {
+public:
+    explicit UniquenessChecker(YAML::Node& root)
+        : visitor_(root)
+    {}
+
+    void CheckKeyUniquenessIterative() {
+        visitor_.VisitPreOrder([this](const YAML::Node& node) {
+            if (node.IsMap()) {
+                CheckSingleMapKeyUniqueness(node);
+            }
+        });
+    }
+
+private:
+    void CheckSingleMapKeyUniqueness(const YAML::Node& node) {
+        UASSERT(node.IsMap());
+        sorted_keys_.clear();
+        sorted_keys_.reserve(node.size());
+        for (const auto item : node) {
+            const auto& key = item.first;
+            switch (key.Type()) {
+                case YAML::NodeType::Null:
+                    sorted_keys_.push_back("null");
+                    break;
+                case YAML::NodeType::Scalar:
+                    sorted_keys_.push_back(key.Scalar());
+                    break;
+                default:
+                    // This is a valid key, but too complex to check. YAML nodes may be recursive and infinitely deep.
+                    break;
+            }
+        }
+
+        std::sort(sorted_keys_.begin(), sorted_keys_.end());
+        if (const auto duplicate = std::adjacent_find(sorted_keys_.begin(), sorted_keys_.end());
+            duplicate != sorted_keys_.end())
+        {
+            throw ParseException(
+                fmt::format("Duplicate mapping key '{}' at path '{}'", *duplicate, visitor_.ComputeCurrentPath())
+            );
+        }
+    }
+
+    RawYamlVisitor visitor_;
+    boost::container::small_vector<std::string_view, kInitialSortedKeysSize> sorted_keys_;
+};
+
+}  // namespace
+
 formats::yaml::Value FromString(const std::string& doc) {
     if (doc.empty()) {
         throw ParseException("YAML document is empty");
     }
 
     try {
-        return YAML::Load(doc);
+        auto node = YAML::Load(doc);
+        UniquenessChecker{node}.CheckKeyUniquenessIterative();
+        return formats::yaml::Value(node);
     } catch (const YAML::ParserException& e) {
         throw ParseException(e.what());
     }
@@ -33,7 +193,9 @@ formats::yaml::Value FromStream(std::istream& is) {
     }
 
     try {
-        return YAML::Load(is);
+        auto node = YAML::Load(is);
+        UniquenessChecker{node}.CheckKeyUniquenessIterative();
+        return formats::yaml::Value(node);
     } catch (const YAML::ParserException& e) {
         throw ParseException(e.what());
     }
@@ -63,6 +225,22 @@ formats::yaml::Value FromFile(const std::string& path) {
 }
 }  // namespace blocking
 
+namespace impl {
+
+formats::yaml::Value FromStringAllowRepeatedKeys(const std::string& doc) {
+    if (doc.empty()) {
+        throw ParseException("YAML document is empty");
+    }
+
+    try {
+        return formats::yaml::Value(YAML::Load(doc));
+    } catch (const YAML::ParserException& e) {
+        throw ParseException(e.what());
+    }
+}
+
+}  // namespace impl
+
 }  // namespace formats::yaml
 
 USERVER_NAMESPACE_END
diff --git a/universal/src/formats/yaml/serialize_test.cpp b/universal/src/formats/yaml/serialize_test.cpp
@@ -2,6 +2,7 @@
 
 #include <userver/formats/yaml/exception.hpp>
 #include <userver/formats/yaml/serialize.hpp>
+#include <userver/utest/assert_macros.hpp>
 
 #include <formats/common/serialize_test.hpp>
 
@@ -27,4 +28,103 @@ struct Serialization<formats::yaml::Value> : public ::testing::Test {
 
 INSTANTIATE_TYPED_TEST_SUITE_P(FormatsYaml, Serialization, formats::yaml::Value);
 
+TEST(FormatsYamlDuplicateKeys, AtRoot) {
+    const std::string yaml_string = R"(
+        Key1: 1
+        Key2: 2
+        Key3: 3
+        Key1: 1
+    )";
+    UEXPECT_THROW_MSG(
+        formats::yaml::FromString(yaml_string),
+        formats::yaml::ParseException,
+        "Duplicate mapping key 'Key1' at path '/'"
+    );
+}
+
+TEST(FormatsYamlDuplicateKeys, InObject) {
+    const std::string yaml_string = R"(
+        Key1:
+            Key2: 2
+            Key3:
+                Key4: 4
+                Key5: 5
+                Key4: 4
+        Key6: 6
+        Key7: 7)";
+    UEXPECT_THROW_MSG(
+        formats::yaml::FromString(yaml_string),
+        formats::yaml::ParseException,
+        "Duplicate mapping key 'Key4' at path 'Key1.Key3'"
+    );
+}
+
+TEST(FormatsYamlDuplicateKeys, InArray) {
+    const std::string yaml_string = R"(
+        - 1
+        - 2
+        - - 3
+          - Key4: 1
+            Key4: 1
+    )";
+    UEXPECT_THROW_MSG(
+        formats::yaml::FromString(yaml_string),
+        formats::yaml::ParseException,
+        "Duplicate mapping key 'Key4' at path '[2][1]'"
+    );
+}
+
+TEST(FormatsYamlDuplicateKeys, VariousScalarsInMapKeys) {
+    {
+        const std::string yaml_string = R"(
+            1: foo
+            1: foo
+        )";
+        UEXPECT_THROW_MSG(
+            formats::yaml::FromString(yaml_string),
+            formats::yaml::ParseException,
+            "Duplicate mapping key '1' at path '/'"
+        );
+    }
+
+    {
+        const std::string yaml_string = R"(
+            true: foo
+            true: foo
+        )";
+        UEXPECT_THROW_MSG(
+            formats::yaml::FromString(yaml_string),
+            formats::yaml::ParseException,
+            "Duplicate mapping key 'true' at path '/'"
+        );
+    }
+
+    {
+        const std::string yaml_string = R"(
+            null: foo
+            null: foo
+        )";
+        UEXPECT_THROW_MSG(
+            formats::yaml::FromString(yaml_string),
+            formats::yaml::ParseException,
+            "Duplicate mapping key 'null' at path '/'"
+        );
+    }
+}
+
+TEST(FormatsYamlDuplicateKeys, NonScalarsInMapKeys) {
+    {
+        const std::string yaml_string = R"(
+            [1, 2, 3]: foo
+            [1, 2, 3]: bar
+        )";
+        formats::yaml::Value value;
+        // This check relies on implementation of FromString. Currently, we do not throw an exception in this case,
+        // but we could do so in the future. In that case the test should be changed.
+        // Anyway, we should not hang or crash here.
+        UEXPECT_NO_THROW(value = formats::yaml::FromString(yaml_string));
+        EXPECT_EQ(value.GetSize(), 2);
+    }
+}
+
 USERVER_NAMESPACE_END
