Config validation extraction by Thomas-Mikhael · Pull Request #615 · eclipse-score/communication

Thomas-Mikhael · 2026-06-30T06:15:08Z

Extract the common config validation checks into a common file/lib config_validate (preparing for use in both parsing strategies, JSON & Flatbuffer).
Unify the checking of duplicates while emplacing, EmplaceOrFatal()
Remove redundant checks of duplicate Events and Fields (CheckContainsEvent, CheckContainsField)
Change ParseLolaxxx functions to return void instead of ignoring the return

castler

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!

castler · 2026-07-02T14:19:01Z

+{
+
+template <typename Map, typename Key, typename Value>
+void EmplaceOrFatal(Map& map, Key&& key, Value&& value, std::string_view element_description)


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.

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?

No you can keep it then in a separate library. Maybe a explanation comment would be good, that explains why this is in a custom file and what the purpose if it would be.

I have that already in the description of the PR:
Extract the common config validation checks into a common file/lib config_validate (preparing for use in both parsing strategies, JSON & Flatbuffer).
Or you mean to comment it in the Bazel BUILD file?

I added some comments

castler · 2026-07-02T14:21:34Z

        ServiceElementInstanceDeploymentParser deployment_parser{field_object};

        const auto& field_name_it = field_object.find(kFieldNameKey);
-        deployment_parser.CheckContainsField(field_name_it, service);


I do not fully understand why we remove this check

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()

castler · 2026-07-02T14:22:20Z

        ServiceElementInstanceDeploymentParser deployment_parser{event_object};

        const auto& event_name_it = event_object.find(kEventNameKey.data());
-        deployment_parser.CheckContainsEvent(event_name_it, service);


I also do not understand fully, why we remove this check?

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()

castler · 2026-07-03T07:59:32Z

+    // That the application will terminate
+    SCORE_LANGUAGE_FUTURECPP_EXPECT_CONTRACT_VIOLATED(score::mw::com::impl::configuration::Parse(std::move(j2)));


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?

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.

If we consider it as a feature then we shall remove the check at all. What do you suggest?

If we consider it as a feature then we shall remove the check at all. What do you suggest?

That would be great! Then things should be more consistent!

Removed and reverted the Tests changes

castler

Thanks the changes look good.

The only thing I would ask you to clean up the git history, right now its quite some mess. I am fine with having few proper commits (with proper messages).

In addition - but this could be done in a follow up if you want - we have the best practice that for each library we have our custom tests.

Since you now extracted these functions into config_validate they are most certainly covered via the existing test. But we would now normally rather create a custom config_validate_test.cpp to ensure that only this unit is correctly covered.

+ Remove redundant checks of duplicate Events and Fields (CheckContainsEvent, CheckContainsField) + Change ParseLolaxxx functions to return void instead of ignoring the return

Thomas-Mikhael · 2026-07-14T10:00:47Z

@castler I agree.
Added config_validate_test.cpp
Fixed up the commits into 2 commits

Thomas-Mikhael requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners June 30, 2026 06:15

github-project-automation Bot added this to COM - Communication FT Jun 30, 2026

github-project-automation Bot moved this to Backlog in COM - Communication FT Jun 30, 2026

castler requested changes Jul 3, 2026

View reviewed changes

Thomas-Mikhael requested a review from castler July 3, 2026 09:12

castler reviewed Jul 13, 2026

View reviewed changes

+ Extract common config validation checks (config_validate lib)

2d9f140

+ Remove redundant checks of duplicate Events and Fields (CheckContainsEvent, CheckContainsField) + Change ParseLolaxxx functions to return void instead of ignoring the return

Thomas-Mikhael force-pushed the feature/config_validation_extraction branch from 634fdd3 to c5ba3e2 Compare July 14, 2026 09:46

Add config_validate_test.cpp

58c4671

Thomas-Mikhael force-pushed the feature/config_validation_extraction branch from f88e3d7 to 58c4671 Compare July 14, 2026 09:59

Thomas-Mikhael requested a review from castler July 14, 2026 09:59

Merge branch 'main' into feature/config_validation_extraction

d5bf693

castler approved these changes Jul 14, 2026

View reviewed changes

castler enabled auto-merge July 14, 2026 11:46

castler added this pull request to the merge queue Jul 14, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 14, 2026

castler added this pull request to the merge queue Jul 14, 2026

Merged via the queue into eclipse-score:main with commit 35841ed Jul 14, 2026
8 checks passed

github-project-automation Bot moved this from Backlog to Done in COM - Communication FT Jul 14, 2026

Thomas-Mikhael deleted the feature/config_validation_extraction branch July 14, 2026 13:17

		// That the application will terminate
		SCORE_LANGUAGE_FUTURECPP_EXPECT_CONTRACT_VIOLATED(score::mw::com::impl::configuration::Parse(std::move(j2)));

Uh oh!

Conversation

Thomas-Mikhael commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

castler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thomas-Mikhael Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

castler left a comment

Choose a reason for hiding this comment

Uh oh!

Thomas-Mikhael commented Jul 14, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Thomas-Mikhael commented Jun 30, 2026 •

edited

Loading

Thomas-Mikhael Jul 3, 2026 •

edited

Loading