fix: Another approach to parse tag string with spaces to avoid long running time on regex matching. #7625

dkphm · 2024-10-29T19:09:04Z

Which issue(s) does this change fix?

#6657

Why is this change necessary?

Previous fix (fix: improve tag parsing performance for list input #7049) only handle the case when tags are provided through samconfig.toml, which resulted into a list to CfnTags, but it doesn't fix the case when customer specify --tags parameter. In the later case, the provided value will be in a string, and we will face the issue with re.findall again, mainly due to the complex regex provided to parse {tag}={tag} pattern in tags.
The solution is instead of parsing a full complex regex with {tag}={tag}, we will look for any quote strings with space in the value (e.g. "Test App"), replace the space with some replacement, and restart the parsing process.

How does it address the issue?

Run sam deploy with --tags tag1="value 1" tag2="value 2"

What side effects does this change have?

Customer might not be able to set tags properly during sam deploy.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

Add input/output type hints to new functions/methods
Write design document if needed (Do I need to write a design document?)
Write/update unit tests
Write/update integration tests
Write/update functional tests if needed
make pr passes
make update-reproducible-reqs if dependencies were changed
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…unning time on regex matching.

lucashuy · 2024-11-04T23:29:28Z

samcli/cli/types.py

+            parsed, parsed_tag = CfnTags._standard_key_value_parser(value)
+            if not parsed:
+                return False, None
+            tags_dict = {**tags_dict, **parsed_tag}


nit (if parsed_tag is of type dict)

Suggested change

tags_dict = {**tags_dict, **parsed_tag}

tags_dict.update(parsed_tag)

lucashuy · 2024-11-04T23:31:25Z

samcli/cli/types.py

+        for value in tag_value.split():
+            parsed, parsed_tag = CfnTags._standard_key_value_parser(value)
+            if not parsed:
+                return False, None


How come we're also returning a boolean here? Can we just return None, or an empty dict instead of returning a tuple?

lucashuy · 2024-11-04T23:36:47Z

samcli/cli/types.py

+                # first, remove all the spaces and start over again.
+


Is there missing code here, or maybe duplicate comment? This comment and the one under say "first"

lucashuy · 2024-11-04T23:40:26Z

samcli/cli/types.py

+
+                # Next, looking for a quote strings that contain spaces and proceed to replace them
+                quoted_strings_with_spaces = re.findall(self._quoted_pattern, modified_val)
+                quoted_strings_without_spaces = [self._replace_spaces(s) for s in quoted_strings_with_spaces]


nit: no single character variable names

lucashuy · 2024-11-04T23:42:52Z

samcli/cli/types.py

-                for group in groups:
-                    key, v = group
-                    self._add_value(result, _unquote_wrapped_quotes(key), _unquote_wrapped_quotes(v))
+                # Instead of parsing a full {tag}={tag} pattern, we will try to look for quoted string with spaces


Can we refactor this new logic out to a separate method to unit test? There's a lot of moving parts here

This is tricky because the block below

groups = re.findall(self._pattern, val)

take longer time to return

lucashuy · 2024-11-04T23:46:34Z

samcli/cli/types.py

@@ -194,6 +219,7 @@ def __init__(self, multiple_values_per_key=False):
    TAG_REGEX = '[A-Za-z0-9\\"_:\\.\\/\\+-\\@=]'

    _pattern = r"{tag}={tag}".format(tag=_generate_match_regex(match_pattern=TAG_REGEX, delim=" "))
+    _quoted_pattern = _generate_quoted_match_regex(match_pattern=TAG_REGEX)


Can we reuse or refactor the old _generate_match_regex? They share similar return values.

They have different regex, hence creating a new function.

lucashuy · 2024-11-04T23:51:50Z

samcli/cli/types.py

+                # Finally, restart the parsing with key=value separated by (multiple) spaces.
+                parsed, tags = self._multiple_space_separated_key_value_parser(modified_val)
+                if parsed:
+                    for k in tags:


nit:

Suggested change

for k in tags:

for key, value in tags.items():

lucashuy · 2024-11-04T23:56:08Z

samcli/cli/types.py

+        space_positions = [i for i, char in enumerate(text) if char == " "]
+        modified = text.replace(" ", replacement)
+
+        return modified, space_positions


Thoughts on using dataclasses for return objects, instead of returning tuples and performing array indexes to get the modified strings/replacement index array?

I imagine that changing things to dataclasses will make it easier for future readers to understand what is happening quickly.

lucashuy · 2024-11-05T00:00:10Z

What side effects does this change have?

Customer might not be able to set tags properly during sam deploy.

What does this mean? Does this change how tags works for customers?

dkphm requested a review from a team as a code owner October 29, 2024 19:09

dkphm requested review from lucashuy and mildaniel October 29, 2024 19:09

github-actions bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Oct 29, 2024

dkphm added pr/internal and removed pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Oct 29, 2024

dkphm force-pushed the tags_with_space branch from a7e99d7 to e45df9e Compare October 29, 2024 19:58

dkphm added 3 commits October 29, 2024 13:14

fix: Another approach to parse tag string with spaces to avoid long r…

c1b29e4

…unning time on regex matching.

Update format

93209d4

fix format

6366d33

dkphm force-pushed the tags_with_space branch from e45df9e to 6366d33 Compare October 29, 2024 20:23

dkphm requested review from hawflau and removed request for mildaniel October 29, 2024 21:18

lucashuy reviewed Nov 4, 2024

View reviewed changes

dkphm and others added 5 commits November 12, 2024 09:54

Address comments

85feda9

Merge branch 'develop' into tags_with_space

c21c3b0

Update format

6840c4d

Update format

a30d6fa

Update format

55e7b85

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Another approach to parse tag string with spaces to avoid long running time on regex matching. #7625

fix: Another approach to parse tag string with spaces to avoid long running time on regex matching. #7625

dkphm commented Oct 29, 2024

lucashuy Nov 4, 2024

lucashuy Nov 4, 2024

lucashuy Nov 4, 2024

lucashuy Nov 4, 2024

lucashuy Nov 4, 2024

dkphm Nov 12, 2024

lucashuy Nov 4, 2024

dkphm Nov 12, 2024

lucashuy Nov 4, 2024

lucashuy Nov 4, 2024

lucashuy commented Nov 5, 2024

	tags_dict = {tags_dict, parsed_tag}
	tags_dict.update(parsed_tag)

fix: Another approach to parse tag string with spaces to avoid long running time on regex matching. #7625

Are you sure you want to change the base?

fix: Another approach to parse tag string with spaces to avoid long running time on regex matching. #7625

Conversation

dkphm commented Oct 29, 2024

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Mandatory Checklist

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

lucashuy commented Nov 5, 2024