Config validation extraction#615
Conversation
Thomas-Mikhael
requested review from
LittleHuba,
bemerybmw,
castler,
crimson11,
hoe-jo and
limdor
as code owners
…s and methods to pass without failing
…allowed empty lists of events, fields and methods to pass without failing)
…allowed empty lists of events, fields and methods to pass without failing)
castler
left a comment
castler
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
I think there are two main points that contradict our current style:
1st)
We try to keep functions as local as possible - as the config checks are just used in the config parser, we should have them there as anonymous functions.
2nd)
We strictly want to allow empty services.
On a side-note it would be nice trying to avoid merge commits in Pull-Requests.
Thank you!
| { | ||
|
|
||
| template <typename Map, typename Key, typename Value> | ||
| void EmplaceOrFatal(Map& map, Key&& key, Value&& value, std::string_view element_description) |
There was a problem hiding this comment.
This function is local to config_parser.cpp thus, it should be within an anonymous namespace within the config parser.
This statement is also true for all other extractions that are done in later commits.
There was a problem hiding this comment.
This function will mostly be also used for the flatbuffer parser, that's why I put it in the common config_validate.h
If you suggest adding these files directly to the 'configuration_json_parsing_strategy' lib directly instead of making it as a separate common lib 'config_validate' that to be used later with 'configuration_flatbuffer_parsing_strategy' I would see it okay.
Or did I missunderstood something?
| ServiceElementInstanceDeploymentParser deployment_parser{field_object}; | ||
|
|
||
| const auto& field_name_it = field_object.find(kFieldNameKey); | ||
| deployment_parser.CheckContainsField(field_name_it, service); |
There was a problem hiding this comment.
I do not fully understand why we remove this check
There was a problem hiding this comment.
Because it's redundant check.
CheckContainsField() and CheckContainsEvent() are using misleading naming, it is just ensuring that there's no duplicates, which would never happen using .emplace()
| ServiceElementInstanceDeploymentParser deployment_parser{event_object}; | ||
|
|
||
| const auto& event_name_it = event_object.find(kEventNameKey.data()); | ||
| deployment_parser.CheckContainsEvent(event_name_it, service); |
There was a problem hiding this comment.
I also do not understand fully, why we remove this check?
There was a problem hiding this comment.
Because it's redundant check.
CheckContainsField() and CheckContainsEvent() are using misleading naming, it is just ensuring that there's no duplicates, which would never happen using .emplace()
| // That the application will terminate | ||
| SCORE_LANGUAGE_FUTURECPP_EXPECT_CONTRACT_VIOLATED(score::mw::com::impl::configuration::Parse(std::move(j2))); |
There was a problem hiding this comment.
Its not a bug - its a feature :)
There have been use cases in the past, where an empty service was used. We see no reason why an empty service (e.g. as marker via the service discovery) shall not be allowed.
Can you maybe elaborate a bit, why you think this is a bug?
There was a problem hiding this comment.
In the original code you can see the in config_parser.cpp the error message description as following:
score::mw::log::LogFatal("lola") << "Configuration should contain at least one event, field, or method.";
That's why I considered it as an unintended bug.
There was a problem hiding this comment.
If we consider it as a feature then we shall remove the check at all. What do you suggest?
2 participants
Uh oh!
There was an error while loading. Please reload this page.