-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[20274] Validate YAML tags on parsing #85
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
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. |
7749e79
to
7cc6a18
Compare
7cc6a18
to
989aeb8
Compare
989aeb8
to
a247258
Compare
a247258
to
91c7487
Compare
6314776
to
9df8c98
Compare
0a7ee4b
to
fa147dc
Compare
Signed-off-by: tempate <danieldiaz@eprosima.com> Uncrustify Signed-off-by: tempate <danieldiaz@eprosima.com> Include YamlValidator.cpp in tests' CMakeLists Signed-off-by: tempate <danieldiaz@eprosima.com> Make validate_tags public Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
Signed-off-by: tempate <danieldiaz@eprosima.com>
8129764
to
5086a11
Compare
Signed-off-by: Lucia Echevarria <luciaechevarria@eprosima.com>
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not directly return false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's preferable to keep it this way, as it allows both warning logs to be printed in case both errors occur.
|
||
if (tags_count[tag_name] > 1) | ||
{ | ||
EPROSIMA_LOG_WARNING(DDSPIPE_YAML, "Tag <" << tag_name << "> is repeated (" << get_position_(tag) << ")."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Maybe use DDSPIPE_YAML_VALIDATOR
for more granularity.
template <> | ||
DDSPIPE_YAML_DllAPI | ||
TlsConfiguration YamlReader::get( | ||
const Yaml& yml, | ||
const YamlReaderVersion version) | ||
{ | ||
YamlValidator::validate<TlsConfiguration>(yml, version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
{ | ||
throw eprosima::utils::ConfigurationException( | ||
utils::Formatter() << | ||
"Source participant required under tag " << ROUTES_SRC_TAG << " in route definition."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this? Doesn't this offer valuable information to the user?
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this zombie code.
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not anymore: I've created a LogFilter
struct that initializes its attributes (info
, warning
, and error
) to empty strings (""
).
// 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], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
version); | ||
YamlValidator::validate_tags(yml[MONITOR_TOPICS_TAG], tags); | ||
|
||
// NOTE: Use fill instead of get to avoid throwing exceptions if tags are not present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this @Tempate , I wish you were here...
{ | ||
throw eprosima::utils::ConfigurationException( | ||
utils::Formatter() << | ||
"No routes found under tag " << ROUTES_TAG << " for topic " << topic << " ."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some useful information would be missed without this message.
"Multiple routes defined for topic " << topic << " : only one allowed."); | ||
} | ||
} | ||
fill<eprosima::ddspipe::core::types::DdsTopic>(topic.get_reference(), topic_routes_yml, version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By changing get for fill, wouldn't validation of the topic be missed?
Signed-off-by: Lucia Echevarria <luciaechevarria@eprosima.com>
Signed-off-by: Lucia Echevarria <luciaechevarria@eprosima.com>
In this version, the DDS Pipe throw a warning when a YAML tag is ignored to prevent typos, misplacements, and wrong configurations.