Restrict yaml loading in evaluate_parameters by ivanpauno · Pull Request #33 · ros2/launch_ros

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

        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 sloretz and wjwwood 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 9 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

-                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

Conversation

ivanpauno commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz commented Jun 3, 2019

Uh oh!

ivanpauno commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hidmic commented Jun 3, 2019

Uh oh!

wjwwood commented Jun 3, 2019

Uh oh!

ivanpauno commented Jun 4, 2019

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivanpauno 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!

sloretz Jun 7, 2019 • 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!

ivanpauno commented Jul 1, 2019

Uh oh!

ivanpauno commented Jul 2, 2019

Uh oh!

hidmic 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!

ivanpauno commented Jul 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivanpauno commented Jun 3, 2019 •

edited

Loading

ivanpauno commented Jun 3, 2019 •

edited

Loading

sloretz Jun 7, 2019 •

edited

Loading