Restrict yaml loading in evaluate_parameters #33

ivanpauno · 2019-06-03T21:05:39Z

Follow up of #31.

The idea of this PR is to restrict what conversions from yaml are allowed, and what conversions are precluded. Also, substitutions that are later loaded as a list (from yaml), are converted to tuples (as it was done in the other cases).

I didn't want to raise an error in the conversions that aren't allowed, but I think it would be a good idea.

sloretz · 2019-06-03T21:08:22Z

Mind adding a test here to show what this is correcting?

https://github.com/ros2/launch_ros/blob/master/test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

ivanpauno · 2019-06-03T21:10:22Z

Mind adding a test here to show what this is correcting?

https://github.com/ros2/launch_ros/blob/master/test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

I will add tests tomorrow.

hidmic · 2019-06-03T21:30:14Z

I second @sloretz request. Just for the record though, code seems to be precluding nested parameter arrays and verifying that arrays evaluated from substitutions have a single element type.

wjwwood · 2019-06-03T21:37:18Z

Same, a test would be nice, it would also help convey what's being changed here.

ivanpauno · 2019-06-04T13:45:39Z

Sorry for the unclear PR yesterday. I updated the PR with a clearer description.
Tests were added in 1456aca.

wjwwood

Thanks for adding the tests, it makes the expected change in behavior clearer. I find the logic difficult to follow, but that's not uncommon when parsing schema-less data like this, so in general I think this is looking good. I had only a few comments.

launch_ros/launch_ros/utilities/evaluate_parameters.py

test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

ivanpauno

Addressed review comments in 3f328d7.
Started raising exceptions in 4efbfe5.

ivanpauno · 2019-06-05T16:25:44Z

launch_ros/launch_ros/utilities/evaluate_parameters.py

+                yaml_evaluated_value: Union[List[str], List[int], List[float], List[bool]] = [
+                    yaml.safe_load(item) for item in evaluated_value
+                ]
+                if check_sequence_type_is_allowed(yaml_evaluated_value):


Suggested change

if check_sequence_type_is_allowed(yaml_evaluated_value):

if not check_sequence_type_is_allowed(yaml_evaluated_value):

raise TypeError(

'Expected a non-empty sequence, with items of uniform type. '

'Allowed sequence item types are bool, int, float, str.'

)

evaluated_value = tuple(yaml_evaluated_value)

That change will also raise an error in:

launch_ros/test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

Lines 214 to 239 in 4efbfe5

def test_dictionary_with_dissimilar_array():

orig = [{'foo': 1, 'fiz': [True, 2.0, 3]}]

norm = normalize_parameters(orig)

expected = ({'foo': 1, 'fiz': ('True', '2.0', '3')},)

# assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{'foo': 1, 'fiz': [True, 1, TextSubstitution(text='foo')]}]

norm = normalize_parameters(orig)

print(norm)

expected = ({'foo': 1, 'fiz': ('True', '1', 'foo')},)

assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{'foo': 1, 'fiz': [TextSubstitution(text='foo'), True, 1]}]

norm = normalize_parameters(orig)

expected = ({'foo': 1, 'fiz': ('foo', 'True', '1')},)

assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{'foo': 1, 'fiz': [True, 1, [TextSubstitution(text='foo')]]}]

norm = normalize_parameters(orig)

expected = ({'foo': 1, 'fiz': ('True', '1', 'foo')},)

assert evaluate_parameters(LaunchContext(), norm) == expected

orig = [{'foo': 1, 'fiz': [[TextSubstitution(text='foo')], True, 1]}]

norm = normalize_parameters(orig)

expected = ({'foo': 1, 'fiz': ('foo', 'True', '1')},)

assert evaluate_parameters(LaunchContext(), norm) == expected

launch_ros/test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

Lines 283 to 293 in 4efbfe5

def test_list_of_substitutions_with_yaml_lists():

orig = [{

'foo': 1,

'fiz': [

[TextSubstitution(text="['asd', 'bsd']")],

[TextSubstitution(text="['asd', 'csd']")]

]

}]

norm = normalize_parameters(orig)

expected = ({'foo': 1, 'fiz': ("['asd', 'bsd']", "['asd', 'csd']")},)

assert evaluate_parameters(LaunchContext(), norm) == expected

I can add complicated logic for allowing interpreting some dissimilar arrays as arrays of strings, and some others not.
But, what's that useful for?

I think that in all the cases in test_dictionary_with_dissimilar_array, the user could write easily the parameters in other wat for getting an array of strings (if they want that). I wouldn't be so flexible as we are being now.

If we want that change, I could later simplify normalize_parameters logic in a follow-up PR.

That sounds fine to me. It's better to be strict and relax it later if someone makes a case for it, than to make it stricter later.

To be clear, +1 to ratcheting up the exception more if all the cases don't throw yet, whether or not that's just this change or something additional.

sloretz · 2019-06-07T16:14:08Z

test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

-    expected = ({'foo': 1, 'fiz': ('True', '2.0', '3')},)
-    # assert evaluate_parameters(LaunchContext(), norm) == expected
+    with pytest.raises(TypeError) as exc:
+        orig = [{'foo': 1, 'fiz': [True, 2.0, 3]}]


What happens if orig is [{'foo': 1, 'fiz': ['True', '2.0', '3']}] ? Does that get ~~correcly~~ interpreted as a list of strings, or does it raise TypeError?

It raises TypeError. For avoiding it, the user should write ["'True'", "'2.0'", "'3'"].
I added test cases in: 23421c9.

I don't know if this is the best way to go or not. But I think that behavior is consistent with the other test cases I have added (in 23421c9).

How so? Are plain dictionary values evaluated as YAML too? It doesn't look like it.

Strings are converted to TextSubstitution in normalize_parameters before.
I don't have a better solution in mind, for avoiding the extra quotes.

How so? Are plain dictionary values evaluated as YAML too? It doesn't look like it.

Thinking about this again, I stopped converting raw strings (str) and TextSubstitution like yaml.
The second one is needed, as str are converted to TextSubstitution in normalize_parameters.

I'm happy with how the test looks now, but I have to recognize that both normalize_parameters and evaluate_parameters need a refactor. But I think that's ok for a follow-up.

ivanpauno · 2019-07-01T19:37:46Z

@sloretz do you think this is ready? (after having green CI)

ivanpauno · 2019-07-02T19:19:20Z

Refactored in db6a00f after being lightened by @hidmic.
In 821395a, I have corrected an old error I found in normalize_parameters while reading the code. Added test case for it.

hidmic

LGTM now! @sloretz ?

hidmic · 2019-07-03T14:32:55Z

launch_ros/launch_ros/utilities/normalize_parameters.py

@@ -74,7 +76,7 @@ def _normalize_parameter_array_value(value: SomeParameterValue) -> ParameterValu
        # all were floats or ints, so return floats
        make_mypy_happy_float = cast(List[Union[int, float]], value)
        return tuple(float(e) for e in make_mypy_happy_float)
-    elif Substitution in has_types and has_types.issubset({str, Substitution, tuple}):
+    elif Substitution in has_types and has_types.issubset({str, Substitution}):


@ivanpauno why was tuple removed?

It doesn't have sense, normalize_to_list_of_substitutions can't handle that and fails:

launch_ros/launch_ros/launch_ros/utilities/normalize_parameters.py

Lines 77 to 79 in 78469a9

elif Substitution in has_types and has_types.issubset({str, Substitution, tuple}):

# make a list of substitutions forming a single string

return tuple(normalize_to_list_of_substitutions(cast(SomeSubstitutionsType, value)))

I added a test case for that:

launch_ros/test_launch_ros/test/test_launch_ros/test_normalize_parameters.py

Lines 95 to 101 in 821395a

orig = [{'foo': [

[TextSubstitution(text='fiz'), TextSubstitution(text='buz')],

TextSubstitution(text='fiz')

]}]

norm = normalize_parameters(orig)

expected = ({'foo': ('fizbuz', 'fiz')},)

assert evaluate_parameters(LaunchContext(), norm) == expected

.
Without removing tuple, it was failing.
The old test cases are passing with the removal.

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

ivanpauno · 2019-07-03T14:52:50Z

Rebased with master before running CI.

CI (up to test_launch_ros, launch_ros):

Linux
Linux-aarch64
macOS
Windows

ivanpauno added the in review label Jun 3, 2019

ivanpauno requested review from wjwwood and sloretz June 3, 2019 21:05

ivanpauno self-assigned this Jun 3, 2019

ivanpauno mentioned this pull request Jun 3, 2019

Add frontend parsing methods for Node, ExecutableInPackage and FindPackage substitution #23

Merged

ivanpauno changed the title ~~Correct how evaluate_parameters handles a list loaded from yaml~~ Restrict yaml loading in evaluate_parameters Jun 4, 2019

wjwwood reviewed Jun 4, 2019

View reviewed changes

ivanpauno commented Jun 5, 2019

View reviewed changes

ivanpauno requested a review from wjwwood June 6, 2019 19:32

sloretz reviewed Jun 7, 2019

View reviewed changes

ivanpauno requested a review from sloretz July 1, 2019 19:38

ivanpauno requested a review from hidmic July 2, 2019 19:19

hidmic approved these changes Jul 3, 2019

View reviewed changes

ivanpauno added 8 commits July 3, 2019 11:45

Correct how evaluate_parameters handles a list loaded as yaml

74a5928

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Add test cases. Corrected code after testing

c2c66e8

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Address PR comments. Clean the code a little bit.

c38e11f

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Raise exception in some not allowed yaml conversions

8a45353

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Stop allowing dissimilar arrays in parameters

2b5e3a7

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Add more test cases

1e26569

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Do not convert like yaml raw strings

ff07123

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Clean up code

076df87

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

Correct error in normalize_parameters

2672750

Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>

ivanpauno force-pushed the ivanpauno/correct-evaluate-parameters branch from 821395a to 2672750 Compare July 3, 2019 14:45

ivanpauno merged commit 098c084 into master Jul 3, 2019

delete-merged-branch bot deleted the ivanpauno/correct-evaluate-parameters branch July 3, 2019 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict yaml loading in evaluate_parameters #33

Restrict yaml loading in evaluate_parameters #33

-                if check_sequence_type_is_allowed(yaml_evaluated_value):
+                if not check_sequence_type_is_allowed(yaml_evaluated_value):
+                    raise TypeError(
+                        'Expected a non-empty sequence, with items of uniform type. '
+                        'Allowed sequence item types are bool, int, float, str.'
+                    )
+                evaluated_value = tuple(yaml_evaluated_value)

	def test_dictionary_with_dissimilar_array():
	orig = [{'foo': 1, 'fiz': [True, 2.0, 3]}]
	norm = normalize_parameters(orig)
	expected = ({'foo': 1, 'fiz': ('True', '2.0', '3')},)
	# assert evaluate_parameters(LaunchContext(), norm) == expected

	orig = [{'foo': 1, 'fiz': [True, 1, TextSubstitution(text='foo')]}]
	norm = normalize_parameters(orig)
	print(norm)
	expected = ({'foo': 1, 'fiz': ('True', '1', 'foo')},)
	assert evaluate_parameters(LaunchContext(), norm) == expected

	orig = [{'foo': 1, 'fiz': [TextSubstitution(text='foo'), True, 1]}]
	norm = normalize_parameters(orig)
	expected = ({'foo': 1, 'fiz': ('foo', 'True', '1')},)
	assert evaluate_parameters(LaunchContext(), norm) == expected

	orig = [{'foo': 1, 'fiz': [True, 1, [TextSubstitution(text='foo')]]}]
	norm = normalize_parameters(orig)
	expected = ({'foo': 1, 'fiz': ('True', '1', 'foo')},)
	assert evaluate_parameters(LaunchContext(), norm) == expected

	orig = [{'foo': 1, 'fiz': [[TextSubstitution(text='foo')], True, 1]}]
	norm = normalize_parameters(orig)
	expected = ({'foo': 1, 'fiz': ('foo', 'True', '1')},)
	assert evaluate_parameters(LaunchContext(), norm) == expected

	def test_list_of_substitutions_with_yaml_lists():
	orig = [{
	'foo': 1,
	'fiz': [
	[TextSubstitution(text="['asd', 'bsd']")],
	[TextSubstitution(text="['asd', 'csd']")]
	]
	}]
	norm = normalize_parameters(orig)
	expected = ({'foo': 1, 'fiz': ("['asd', 'bsd']", "['asd', 'csd']")},)
	assert evaluate_parameters(LaunchContext(), norm) == expected

	elif Substitution in has_types and has_types.issubset({str, Substitution, tuple}):
	# make a list of substitutions forming a single string
	return tuple(normalize_to_list_of_substitutions(cast(SomeSubstitutionsType, value)))

	orig = [{'foo': [
	[TextSubstitution(text='fiz'), TextSubstitution(text='buz')],
	TextSubstitution(text='fiz')
	]}]
	norm = normalize_parameters(orig)
	expected = ({'foo': ('fizbuz', 'fiz')},)
	assert evaluate_parameters(LaunchContext(), norm) == expected

Restrict yaml loading in evaluate_parameters #33

Restrict yaml loading in evaluate_parameters #33

Conversation

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

Choose a reason for hiding this comment