diff --git a/SilKit/source/config/ParticipantConfigurationFromXImpl.cpp b/SilKit/source/config/ParticipantConfigurationFromXImpl.cpp index 47f9e9f5d..4676db25a 100644 --- a/SilKit/source/config/ParticipantConfigurationFromXImpl.cpp +++ b/SilKit/source/config/ParticipantConfigurationFromXImpl.cpp @@ -654,6 +654,9 @@ void ProcessIncludes(const ParticipantConfiguration& config, ConfigIncludeData& auto PaticipantConfigurationWithIncludes(const std::string& text, struct ConfigIncludeData& configData) -> SilKit::Config::ParticipantConfiguration { + // Validate the root configuration against the schema (included files are validated in ProcessIncludes) + SilKit::Config::Validate(text); + auto configuration = SilKit::Config::Deserialize(text); if (configuration.schemaVersion.empty()) { diff --git a/SilKit/source/config/Test_ParticipantConfiguration.cpp b/SilKit/source/config/Test_ParticipantConfiguration.cpp index 98b33fbdb..203f15874 100644 --- a/SilKit/source/config/Test_ParticipantConfiguration.cpp +++ b/SilKit/source/config/Test_ParticipantConfiguration.cpp @@ -140,6 +140,33 @@ Description: Example include configuration for CAN Controllers ASSERT_EQ(participantConfig, participantConfigRef); ASSERT_TRUE(participantConfig.logging.sinks.at(0).enabledTopics.at(0) == SilKit::Services::Logging::Topic::Can); } + +TEST_F(Test_ParticipantConfiguration, participant_config_from_string_root_schema_validated) +{ + // Regression: the root configuration (not just included files) must be schema-validated. + // A misplaced known keyword at the top level must be rejected. + const auto configString = R"raw( +--- +ParticipantName: P1 +# FlushLevel is a Logging sub-element, not valid at the document root +FlushLevel: Info +)raw"; + + EXPECT_THROW(SilKit::Config::ParticipantConfigurationFromStringImpl(configString), SilKit::ConfigurationError); +} + +TEST_F(Test_ParticipantConfiguration, participant_config_from_string_unknown_field_warns_but_loads) +{ + // Unknown, non-reserved fields are warned about but must not prevent loading the config. + const auto configString = R"raw( +--- +ParticipantName: P1 +Foobar: true +)raw"; + + EXPECT_NO_THROW(SilKit::Config::ParticipantConfigurationFromStringImpl(configString)); +} + TEST_F(Test_ParticipantConfiguration, participant_config_from_string_includes) { const auto configString = R"raw( diff --git a/SilKit/source/config/Test_YamlParser.cpp b/SilKit/source/config/Test_YamlParser.cpp index 8007fe3b2..5be69f932 100644 --- a/SilKit/source/config/Test_YamlParser.cpp +++ b/SilKit/source/config/Test_YamlParser.cpp @@ -352,6 +352,31 @@ TEST_F(Test_YamlParser, yaml_native_type_conversions) } } +TEST_F(Test_YamlParser, yaml_metrics_update_interval) +{ + // UpdateInterval is read from YAML as an integer count of seconds + auto config = Deserialize(R"( +Experimental: + Metrics: + CollectFromRemote: true + UpdateInterval: 42 +)"); + EXPECT_EQ(config.experimental.metrics.updateInterval, 42s); + + // ... and round-trips through serialization + auto txt = Serialize(config); + auto config2 = Deserialize(txt); + EXPECT_EQ(config2.experimental.metrics.updateInterval, 42s); + + // An absent UpdateInterval keeps the default + auto configDefault = Deserialize(R"( +Experimental: + Metrics: + CollectFromRemote: true +)"); + EXPECT_EQ(configDefault.experimental.metrics.updateInterval, 1s); +} + TEST_F(Test_YamlParser, middleware_convert) { auto config = Deserialize(R"( diff --git a/SilKit/source/config/Test_YamlValidator.cpp b/SilKit/source/config/Test_YamlValidator.cpp index f3e68c24d..bac23c1e7 100644 --- a/SilKit/source/config/Test_YamlValidator.cpp +++ b/SilKit/source/config/Test_YamlValidator.cpp @@ -55,7 +55,93 @@ Description: Sample configuration for CAN std::cout << "Yaml Validator warnings: " << warnings.str() << std::endl; EXPECT_TRUE(yamlValid) << "We ignore non-keyword errors and typos, but generate warnings!"; EXPECT_GT(warnings.str().size(), 0u) << "Yaml Validator warnings: '" << warnings.str() << "'"; - ; + // The warning must be complete: it names the field and the schema path it is ignored under + EXPECT_THAT(warnings.str(), testing::HasSubstr("CanControllerss")); + EXPECT_THAT(warnings.str(), testing::HasSubstr("is being ignored")); + EXPECT_THAT(warnings.str(), testing::HasSubstr("schema path \"/\"")); +} + +TEST_F(Test_YamlValidator, validate_unknown_toplevel_arbitrary) +{ + auto yamlString = R"yaml( +schemaVersion: 1 +ParticipantName: P1 +# a completely unknown field, not a near-miss of any keyword +Foobar: true +)yaml"; + + std::stringstream warnings; + bool yamlValid = ValidateWithSchema(yamlString, warnings); + std::cout << "Yaml Validator warnings: " << warnings.str() << std::endl; + EXPECT_TRUE(yamlValid) << "Unknown fields are warned about, not rejected"; + EXPECT_THAT(warnings.str(), testing::HasSubstr("Foobar")); + EXPECT_THAT(warnings.str(), testing::HasSubstr("is being ignored")); + EXPECT_THAT(warnings.str(), testing::HasSubstr("schema path \"/\"")); +} + +TEST_F(Test_YamlValidator, validate_unknown_nested) +{ + auto yamlString = R"yaml( +schemaVersion: 1 +ParticipantName: P1 +Middleware: + Foobar: true +CanControllers: + - Name: CAN1 + Foobaz: 42 +)yaml"; + + std::stringstream warnings; + bool yamlValid = ValidateWithSchema(yamlString, warnings); + std::cout << "Yaml Validator warnings: " << warnings.str() << std::endl; + EXPECT_TRUE(yamlValid) << "Unknown nested fields are warned about, not rejected"; + EXPECT_THAT(warnings.str(), testing::HasSubstr("Foobar")); + EXPECT_THAT(warnings.str(), testing::HasSubstr("schema path \"/Middleware\"")); + EXPECT_THAT(warnings.str(), testing::HasSubstr("Foobaz")); + EXPECT_THAT(warnings.str(), testing::HasSubstr("schema path \"/CanControllers\"")); +} + +TEST_F(Test_YamlValidator, validate_logging_sink_experimental) +{ + // Regression: the reader reads EnabledTopics/DisabledTopics under a nested Experimental + // node, so the schema must accept that nesting without warnings. + auto yamlString = R"yaml( +schemaVersion: 1 +ParticipantName: P1 +Logging: + Sinks: + - Type: Remote + Experimental: + EnabledTopics: + - TopicA + DisabledTopics: + - TopicB +)yaml"; + + std::stringstream warnings; + bool yamlValid = ValidateWithSchema(yamlString, warnings); + EXPECT_TRUE(yamlValid) << "YamlValidator warnings: " << warnings.str(); + EXPECT_EQ(warnings.str(), "") << "Nested Sink Experimental topics must validate cleanly"; +} + +TEST_F(Test_YamlValidator, validate_rpc_deprecated_channel_alias) +{ + // Regression: deprecated but still-supported RPC keys Channel/RpcChannel must not warn. + auto yamlString = R"yaml( +schemaVersion: 1 +ParticipantName: P1 +RpcServers: + - Name: Server1 + Channel: FuncA +RpcClients: + - Name: Client1 + RpcChannel: FuncA +)yaml"; + + std::stringstream warnings; + bool yamlValid = ValidateWithSchema(yamlString, warnings); + EXPECT_TRUE(yamlValid) << "YamlValidator warnings: " << warnings.str(); + EXPECT_EQ(warnings.str(), "") << "Deprecated RPC Channel aliases must validate cleanly"; } TEST_F(Test_YamlValidator, validate_duplicate_element) @@ -74,6 +160,32 @@ TEST_F(Test_YamlValidator, validate_duplicate_element) EXPECT_GT(warnings.str().size(), 0u); } +TEST_F(Test_YamlValidator, validate_repeated_container_keys_in_sequence) +{ + // Regression: the same container key (e.g. Replay) appearing in different elements of a + // sequence must NOT be reported as a duplicate. Duplicate detection is per-map only. + auto yamlString = R"yaml( +schemaVersion: 1 +ParticipantName: P1 +CanControllers: + - Name: CAN1 + Replay: + UseTraceSource: Source1 + MdfChannel: + ChannelName: Ch1 + - Name: CAN2 + Replay: + UseTraceSource: Source2 + MdfChannel: + ChannelName: Ch2 +)yaml"; + + std::stringstream warnings; + bool yamlValid = ValidateWithSchema(yamlString, warnings); + EXPECT_TRUE(yamlValid) << "YamlValidator warnings: " << warnings.str(); + EXPECT_EQ(warnings.str(), "") << "Repeated keys across sequence elements are not duplicates"; +} + TEST_F(Test_YamlValidator, validate_unnamed_children) { auto yamlString = R"yaml( diff --git a/SilKit/source/config/YamlReader.cpp b/SilKit/source/config/YamlReader.cpp index d85639a11..e6c24cbe5 100644 --- a/SilKit/source/config/YamlReader.cpp +++ b/SilKit/source/config/YamlReader.cpp @@ -272,6 +272,11 @@ void YamlReader::Read(SilKit::Config::Metrics& obj) { OptionalRead(obj.sinks, "Sinks"); OptionalRead(obj.collectFromRemote, "CollectFromRemote"); + + // UpdateInterval is an integer count of seconds; keep the default if the key is absent + std::chrono::seconds::rep updateIntervalSeconds{obj.updateInterval.count()}; + OptionalRead(updateIntervalSeconds, "UpdateInterval"); + obj.updateInterval = std::chrono::seconds{updateIntervalSeconds}; } void YamlReader::Read(SilKit::Config::MdfChannel& obj) diff --git a/SilKit/source/config/YamlValidator.cpp b/SilKit/source/config/YamlValidator.cpp index 86417d62e..5453a40bb 100644 --- a/SilKit/source/config/YamlValidator.cpp +++ b/SilKit/source/config/YamlValidator.cpp @@ -143,6 +143,7 @@ const std::set schemaPaths_v1 = { "/Experimental/Metrics/Sinks", "/Experimental/Metrics/Sinks/Name", "/Experimental/Metrics/Sinks/Type", + "/Experimental/Metrics/UpdateInterval", "/Experimental/TimeSynchronization", "/Experimental/TimeSynchronization/AnimationFactor", "/Experimental/TimeSynchronization/EnableMessageAggregation", @@ -314,8 +315,9 @@ const std::set schemaPaths_v1 = { "/Logging/Sinks/Level", "/Logging/Sinks/LogName", "/Logging/Sinks/Type", - "/Logging/Sinks/EnabledTopics", - "/Logging/Sinks/DisabledTopics", + "/Logging/Sinks/Experimental", + "/Logging/Sinks/Experimental/EnabledTopics", + "/Logging/Sinks/Experimental/DisabledTopics", "/Middleware", "/Middleware/AcceptorUris", "/Middleware/ConnectAttempts", @@ -330,7 +332,9 @@ const std::set schemaPaths_v1 = { "/Middleware/TcpSendBufferSize", "/ParticipantName", "/RpcClients", + "/RpcClients/Channel", "/RpcClients/FunctionName", + "/RpcClients/RpcChannel", "/RpcClients/Labels", "/RpcClients/Labels/Key", "/RpcClients/Labels/Kind", @@ -348,7 +352,9 @@ const std::set schemaPaths_v1 = { "/RpcClients/Replay/UseTraceSource", "/RpcClients/UseTraceSinks", "/RpcServers", + "/RpcServers/Channel", "/RpcServers/FunctionName", + "/RpcServers/RpcChannel", "/RpcServers/Labels", "/RpcServers/Labels/Key", "/RpcServers/Labels/Kind", @@ -460,7 +466,6 @@ struct ValidatingVisitor std::ostream& warnings; std::string currentNodePath; std::deque nodePaths; - std::set userDefinedPaths; ryml::Parser& parser; bool ok{true}; @@ -477,12 +482,6 @@ struct ValidatingVisitor ValidatingVisitor(const ValidatingVisitor&) = delete; ValidatingVisitor& operator=(const ValidatingVisitor&) = delete; - bool PathIsAlreadyDefined(const std::string& path) - { - auto it = userDefinedPaths.insert(path); - return !std::get<1>(it); - } - auto GetCurrentLocation(ryml::ConstNodeRef node) -> std::string { auto&& location = parser.location(node); @@ -503,6 +502,28 @@ struct ValidatingVisitor void push(ryml::ConstNodeRef node, ryml::id_type /* level */) { + // Detect keys defined more than once within the same map (real YAML duplicate keys). + // This is scoped to a single map's direct children; the same key appearing in + // different elements of a sequence is not a duplicate. + if (node.is_map()) + { + std::set seenKeys; + for (auto child : node.children()) + { + if (!child.has_key()) + { + continue; + } + auto childKey = to_string(child.key()); + if (!seenKeys.insert(childKey).second) + { + warnings << "At " << GetCurrentLocation(child) << ": Element \"" << childKey << "\"" + << " is already defined in path \"" << currentNodePath << "\"\n"; + ok &= false; + } + } + } + if (!node.has_key()) { return; @@ -510,13 +531,6 @@ struct ValidatingVisitor auto nodeKey = to_string(node.key()); auto nodePath = MakePath(currentNodePath, nodeKey); - if (PathIsAlreadyDefined(nodePath)) - { - warnings << "At " << GetCurrentLocation(node) << ": Element \"" << nodePath << "\"" - << " is already defined in path \"" << currentNodePath << "\"\n"; - ok &= false; - } - nodePaths.push_back(nodePath); currentNodePath = nodePaths.back(); return; @@ -546,9 +560,10 @@ struct ValidatingVisitor } else { - // We only report error if the element is a reserved keyword + // Unknown, non-reserved elements are only warned about, not treated as errors warnings << "At " << GetCurrentLocation(node) << ": Element \"" << nodeName << "\"" - << " is being ignored. It is not a sub-element of schema path \""; + << " is being ignored. It is not a sub-element of schema path \"" << currentNodePath + << "\"\n"; } } } @@ -627,6 +642,12 @@ bool ValidateWithSchema(const std::string& yamlString, std::ostream& warnings) } } + if (!root.is_map() && !root.is_seq()) + { + // Empty or scalar documents have no fields to validate against the schema + return true; + } + ValidatingVisitor visitor{parser, warnings}; visit_stacked(root, visitor); return visitor.IsValid(); diff --git a/SilKit/source/config/YamlWriter.cpp b/SilKit/source/config/YamlWriter.cpp index e7db490bc..d74742747 100644 --- a/SilKit/source/config/YamlWriter.cpp +++ b/SilKit/source/config/YamlWriter.cpp @@ -283,12 +283,15 @@ void YamlWriter::Write(const SilKit::Config::MetricsSink& obj) void YamlWriter::Write(const SilKit::Config::Metrics& obj) { + static const SilKit::Config::Metrics defaultObj{}; MakeMap(); OptionalWrite(obj.sinks, "Sinks"); if (obj.collectFromRemote.has_value()) { WriteKeyValue("CollectFromRemote", obj.collectFromRemote.value()); } + // UpdateInterval is serialized as an integer count of seconds + NonDefaultWrite(obj.updateInterval.count(), "UpdateInterval", defaultObj.updateInterval.count()); }