[services] Tunnelbroker - Check for an empty string in ConfigManager
ClosedPublic
Actions

Authored by • max on May 26 2022, 8:25 AM.

Details

Reviewers

• karol
tomek

Commits

rCOMM1d64bb76e110: [services] Tunnelbroker - Check for an empty string in ConfigManager

Summary

This diff fixes a bug when the option in the config file provided, but has an empty value the app throws the following error:
what(): basic_string::_M_construct null not valid.
To fix this we will check the option value not to be empty or throw an appropriate error.

Linear task: ENG-1059

Test Plan

In the config file located at services/tunnelbroker/tunnelbroker.ini change the option value to an empty.
Run service using the command yarn run-tunnelbroker-service and expect the 'Error: config parameter X can not be empty' error.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.May 26 2022, 8:25 AM

• max held this revision as a draft.

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptMay 26 2022, 8:25 AM

• max edited the summary of this revision. (Show Details)May 26 2022, 8:26 AM

• max added reviewers: • karol, tomek.

• max published this revision for review.May 26 2022, 8:28 AM

Harbormaster completed remote builds in B9493: Diff 13147.May 26 2022, 8:32 AM

Are we sure that empty parameter value is always invalid? Maybe this check should be made in a place where getParameter is called - and only in the places where empty string is invalid.

services/tunnelbroker/src/Tools/ConfigManager.cpp
97–102 ↗	(On Diff #13147)	We can simplify the code

This revision now requires changes to proceed.May 27 2022, 2:30 AM

• max planned changes to this revision.May 29 2022, 11:52 PM

Reduce double variables map calling to one.

• max planned changes to this revision.May 30 2022, 7:24 AM

Harbormaster completed remote builds in B9526: Diff 13208.May 30 2022, 7:32 AM

In D4137#117046, @palys-swm wrote:

Are we sure that empty parameter value is always invalid?

I think yes because there are only two options when the config parameter can be empty here:

When the value is empty in the config ini file, for example:

[section]
value =

We don't want to allow that in ini file.

When the default value constant is empty (in Constants.h).

There is a possibility that we will need an empty value in the future, but for now, we don't use any empty values in our constants and in default values in the configuration as well.
Also I don't imagine how we can use it in a such way.

services/tunnelbroker/src/Tools/ConfigManager.cpp
97–102 ↗	(On Diff #13147)	We can simplify the code Good optimization suggestion! We don't need to call to `variablesMap[param]` twice here. I've updated on this.