[20274] Validate YAML tags on parsing #85

Tempate · 2024-01-19T08:09:39Z

In this version, the DDS Pipe throw a warning when a YAML tag is ignored to prevent typos, misplacements, and wrong configurations.

codecov-commenter · 2024-01-19T08:49:48Z

Codecov Report

Attention: 81 lines in your changes are missing coverage. Please review.

Comparison is base (16b1acb) 36.74% compared to head (4672579) 36.56%.

Files	Patch %	Lines
ddspipe_yaml/src/cpp/YamlReader_types.cpp	32.69%	21 Missing and 14 partials ⚠️
ddspipe_yaml/src/cpp/YamlReader_participants.cpp	0.00%	25 Missing ⚠️
ddspipe_yaml/src/cpp/YamlReader_features.cpp	23.80%	4 Missing and 12 partials ⚠️
ddspipe_yaml/src/cpp/YamlValidator.cpp	54.54%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #85      +/-   ##
==========================================
- Coverage   36.74%   36.56%   -0.19%     
==========================================
  Files         116      118       +2     
  Lines        4975     5060      +85     
  Branches     1909     1949      +40     
==========================================
+ Hits         1828     1850      +22     
- Misses       2333     2383      +50     
- Partials      814      827      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ddspipe_yaml/src/cpp/YamlReader_features.cpp

ddspipe_yaml/include/ddspipe_yaml/YamlValidator.hpp

ddspipe_yaml/src/cpp/YamlValidator.cpp

ddspipe_yaml/include/ddspipe_yaml/YamlReader.hpp

ddspipe_yaml/include/ddspipe_yaml/YamlValidator.hpp

ddspipe_yaml/src/cpp/YamlReader_generic.cpp

ddspipe_yaml/src/cpp/YamlReader_participants.cpp

Signed-off-by: tempate <[email protected]> Uncrustify Signed-off-by: tempate <[email protected]> Include YamlValidator.cpp in tests' CMakeLists Signed-off-by: tempate <[email protected]> Make validate_tags public Signed-off-by: tempate <[email protected]>

Signed-off-by: tempate <[email protected]>

Signed-off-by: Lucia Echevarria <[email protected]>

juanlofer-eprosima · 2024-09-19T09:50:43Z

ddspipe_yaml/src/cpp/YamlValidator.cpp

+        if (!valid_tags.count(tag_name))
+        {
+            EPROSIMA_LOG_WARNING(DDSPIPE_YAML, "Tag <" << tag_name << "> is not a valid tag (" << get_position_(tag) << ").");
+            is_valid = false;


Why not directly return false?

I believe it's preferable to keep it this way, as it allows both warning logs to be printed in case both errors occur.

juanlofer-eprosima · 2024-09-19T09:51:31Z

ddspipe_yaml/src/cpp/YamlValidator.cpp

+
+        if (tags_count[tag_name] > 1)
+        {
+            EPROSIMA_LOG_WARNING(DDSPIPE_YAML, "Tag <" << tag_name << "> is repeated (" << get_position_(tag) << ").");


NIT: Maybe use DDSPIPE_YAML_VALIDATOR for more granularity.

juanlofer-eprosima · 2024-09-19T11:00:43Z

ddspipe_yaml/src/cpp/YamlReader_types.cpp

 template <>
 DDSPIPE_YAML_DllAPI
 TlsConfiguration YamlReader::get(
        const Yaml& yml,
        const YamlReaderVersion version)
 {
+    YamlValidator::validate<TlsConfiguration>(yml, version);


NIT: I think it would be more correct to check the value returned, and throw an exception if false (so basically you would be ignoring instead the ret value of validate_tags in each validate implementation). Some of the verifications done in fill implementations might be moved there, and in the future other checks could be added there.

juanlofer-eprosima · 2024-09-19T12:39:22Z

ddspipe_yaml/src/cpp/YamlReader_features.cpp

        {
            throw eprosima::utils::ConfigurationException(
                      utils::Formatter() <<
-                          "Source participant required under tag " << ROUTES_SRC_TAG << " in route definition.");


Why remove this? Doesn't this offer valuable information to the user?

juanlofer-eprosima · 2024-09-19T12:41:09Z

ddspipe_yaml/src/cpp/YamlReader_features.cpp

@@ -163,44 +204,24 @@ void YamlReader::fill(

    for (const auto& topic_routes_yml : yml)
    {
+        YamlValidator::validate<core::TopicRoutesConfiguration>(topic_routes_yml, version);
+
        // utils::Heritable<ddspipe::core::types::DistributedTopic> topic;


Please remove this zombie code.

juanlofer-eprosima · 2024-09-19T13:07:42Z

ddspipe_yaml/src/cpp/YamlReader_types.cpp

@@ -497,16 +599,33 @@ void YamlReader::fill(
    // Filter optional
    if (is_tag_present(yml, LOG_FILTER_TAG))
    {
-        fill<utils::LogFilter>(object.filter, get_value_in_tag(yml, LOG_FILTER_TAG), version);
+        object.filter = get<utils::LogFilter>(yml, LOG_FILTER_TAG, version);


This changes behaviour.

Not anymore: I've created a LogFilter struct that initializes its attributes (info, warning, and error) to empty strings ("").

juanlofer-eprosima · 2024-09-19T13:45:32Z

ddspipe_yaml/src/cpp/YamlReader_features.cpp

+        // NOTE: Use fill instead of get to avoid throwing exceptions if tags are not present
+        fill<core::MonitorProducerConfiguration>(object.producers[core::STATUS_MONITOR_PRODUCER_ID],


GENERAL NOTE: I find this very confusing and delicate. Wouldn't it be better to call validate from within fill methods? This way you would make sure the method is always called (since get calls fill), and only once.

juanlofer-eprosima · 2024-09-19T13:49:08Z

ddspipe_yaml/src/cpp/YamlReader_features.cpp

-                        version);
+        YamlValidator::validate_tags(yml[MONITOR_TOPICS_TAG], tags);
+
+        // NOTE: Use fill instead of get to avoid throwing exceptions if tags are not present


I don't get this @Tempate , I wish you were here...

juanlofer-eprosima · 2024-09-19T14:10:24Z

ddspipe_yaml/src/cpp/YamlReader_features.cpp

        {
            throw eprosima::utils::ConfigurationException(
                      utils::Formatter() <<
-                          "No routes found under tag " << ROUTES_TAG << " for topic " << topic << " .");


I think some useful information would be missed without this message.

juanlofer-eprosima · 2024-09-19T14:11:38Z

ddspipe_yaml/src/cpp/YamlReader_features.cpp

-                              "Multiple routes defined for topic " << topic  << " : only one allowed.");
-            }
-        }
+        fill<eprosima::ddspipe::core::types::DdsTopic>(topic.get_reference(), topic_routes_yml, version);


By changing get for fill, wouldn't validation of the topic be missed?

Signed-off-by: Lucia Echevarria <[email protected]>

Tempate had a problem deploying to codecov January 19, 2024 08:09 — with GitHub Actions Failure

Tempate temporarily deployed to codecov January 19, 2024 08:41 — with GitHub Actions Inactive

Tempate temporarily deployed to codecov January 19, 2024 12:34 — with GitHub Actions Inactive

Tempate temporarily deployed to codecov January 22, 2024 14:12 — with GitHub Actions Inactive

Tempate marked this pull request as ready for review January 22, 2024 14:18

Tempate changed the title ~~Validate the YAML configuration file on parsing~~ [20274] Validate YAML tags on parsing Jan 22, 2024

LuciaEchevarria99 requested changes Apr 8, 2024

View reviewed changes

Tempate force-pushed the feature/yaml_validator branch from 7749e79 to 7cc6a18 Compare April 11, 2024 07:19

Tempate had a problem deploying to codecov April 11, 2024 07:19 — with GitHub Actions Failure

Tempate force-pushed the feature/yaml_validator branch from 7cc6a18 to 989aeb8 Compare May 20, 2024 06:25

Tempate had a problem deploying to codecov May 20, 2024 06:25 — with GitHub Actions Failure

Tempate force-pushed the feature/yaml_validator branch from 989aeb8 to a247258 Compare July 3, 2024 09:59

Tempate had a problem deploying to codecov July 3, 2024 10:00 — with GitHub Actions Failure

Tempate force-pushed the feature/yaml_validator branch from a247258 to 91c7487 Compare July 17, 2024 13:32

LuciaEchevarria99 requested changes Jul 18, 2024

View reviewed changes

Tempate changed the base branch from main to integration/bump-3.0.0 July 18, 2024 12:32

Tempate had a problem deploying to codecov July 18, 2024 12:49 — with GitHub Actions Failure

LuciaEchevarria99 force-pushed the integration/bump-3.0.0 branch from 6314776 to 9df8c98 Compare August 16, 2024 09:53

rsanchez15 mentioned this pull request Sep 3, 2024

YAML check tags [18356] #33

Closed

5 tasks

LuciaEchevarria99 force-pushed the integration/bump-3.0.0 branch from 0a7ee4b to fa147dc Compare September 10, 2024 06:29

Base automatically changed from integration/bump-3.0.0 to main September 10, 2024 14:00

Tempate added 5 commits September 19, 2024 08:27

Validate YAML tags on parsing

2c0f46a

Signed-off-by: tempate <[email protected]> Uncrustify Signed-off-by: tempate <[email protected]> Include YamlValidator.cpp in tests' CMakeLists Signed-off-by: tempate <[email protected]> Make validate_tags public Signed-off-by: tempate <[email protected]>

Throw as many warnings as incorrect tags

820a7ea

Signed-off-by: tempate <[email protected]>

Validate the Timestamp's YAML tags on parsing

ee2b96f

Signed-off-by: tempate <[email protected]>

Validate new YAML options

46fd335

Signed-off-by: tempate <[email protected]>

Make tag-sets static

604ba45

Signed-off-by: tempate <[email protected]>

Tempate added 5 commits September 19, 2024 08:27

Remove unnecessary includes

89af6ef

Signed-off-by: tempate <[email protected]>

Check if tags are repeated

97add3d

Signed-off-by: tempate <[email protected]>

Log the line and column when a tag is faulty

3a7eb77

Signed-off-by: tempate <[email protected]>

Fixes after tests

d45a6f9

Signed-off-by: tempate <[email protected]>

Apply suggestions

5086a11

Signed-off-by: tempate <[email protected]>

LuciaEchevarria99 force-pushed the feature/yaml_validator branch from 8129764 to 5086a11 Compare September 19, 2024 09:03

LuciaEchevarria99 had a problem deploying to codecov September 19, 2024 09:03 — with GitHub Actions Failure

Fix changes after rebase

101604b

Signed-off-by: Lucia Echevarria <[email protected]>

LuciaEchevarria99 had a problem deploying to codecov September 19, 2024 09:05 — with GitHub Actions Failure

juanlofer-eprosima requested changes Sep 19, 2024

View reviewed changes

Adapt code to new LogFilter struct

ebbccf3

Signed-off-by: Lucia Echevarria <[email protected]>

LuciaEchevarria99 had a problem deploying to codecov September 24, 2024 10:06 — with GitHub Actions Failure

Apply suggested changes

2ad717f

Signed-off-by: Lucia Echevarria <[email protected]>

LuciaEchevarria99 had a problem deploying to codecov September 24, 2024 10:08 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[20274] Validate YAML tags on parsing #85

[20274] Validate YAML tags on parsing #85

Tempate commented Jan 19, 2024

codecov-commenter commented Jan 19, 2024 •

edited

Loading

juanlofer-eprosima Sep 19, 2024

LuciaEchevarria99 Sep 23, 2024

juanlofer-eprosima Sep 19, 2024

juanlofer-eprosima Sep 19, 2024

juanlofer-eprosima Sep 19, 2024

juanlofer-eprosima Sep 19, 2024

juanlofer-eprosima Sep 19, 2024

LuciaEchevarria99 Sep 24, 2024

juanlofer-eprosima Sep 19, 2024

juanlofer-eprosima Sep 19, 2024

juanlofer-eprosima Sep 19, 2024

juanlofer-eprosima Sep 19, 2024

		// NOTE: Use fill instead of get to avoid throwing exceptions if tags are not present
		fill<core::MonitorProducerConfiguration>(object.producers[core::STATUS_MONITOR_PRODUCER_ID],

[20274] Validate YAML tags on parsing #85

Are you sure you want to change the base?

[20274] Validate YAML tags on parsing #85

Conversation

Tempate commented Jan 19, 2024

codecov-commenter commented Jan 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 19, 2024 •

edited

Loading