From a1d559fb936cac105c99afd6ca6464648f28888c Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Fri, 23 Feb 2018 14:35:07 -0800 Subject: [PATCH] Correct fix for volume ID substition sorting by length The previous attempt - caed78e - is not really correct. It sorts the dict item tuples according to the alphabetical sort order of the first item of each tuple (reversed). This will always work when both substitutions *start* with the same characters, as in the case of two strings that start with the same characters but have a different length, the shorter one sorts alphabetically first, and we reverse that. But it is not safe if the shorter substitution doesn't start with the same characters, as in the case I put in the tests: we should sort 'zzzaaaaaazzz' before 'aaaaaa' (and hence apply the 'zzzaaaaaazzz' substitution to a volume ID that contains that string and not the 'aaaaaa' one), but the previous commit did not. Signed-off-by: Adam Williamson --- pungi/util.py | 2 +- tests/test_util.py | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/pungi/util.py b/pungi/util.py index dc2cd62a..8ea945da 100644 --- a/pungi/util.py +++ b/pungi/util.py @@ -325,7 +325,7 @@ def _apply_substitutions(compose, volid): substitutions = compose.conf['volume_id_substitutions'].items() # processing should start with the longest pattern, otherwise, we could # unexpectedly replace a substring of that longest pattern - for k, v in sorted(substitutions, reverse=True): + for k, v in sorted(substitutions, key=lambda x: len(x[0]), reverse=True): volid = volid.replace(k, v) return volid diff --git a/tests/test_util.py b/tests/test_util.py index ba60c79a..2b4c2d2c 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -229,6 +229,11 @@ class TestVolumeIdGenerator(unittest.TestCase): ('Fedora-WorkstationOstree-ostree-x86_64-Rawhide', 'Fedora-WS-ostree-x86_64-rawh'), ('x86_64-compose_id-20160107', 'x86_64-compose_id-20160107'), ('x86_64-compose_id-20160107-Alpha', 'x86_64-compose_id-20160107-A'), + # These test the case where one substitution is a subset + # of the other, but sorts alphabetically ahead of it, to + # make sure we're correctly sorting by length + ('Fedora-zzzaaaaaazzz-Rawhide', 'Fedora-zzz-rawh'), + ('Fedora-aaaaaa-Rawhide', 'Fedora-aaa-rawh'), ] for volid, expected in all_keys: conf = { @@ -237,6 +242,8 @@ class TestVolumeIdGenerator(unittest.TestCase): 'WorkstationOstree': 'WS', 'Workstation': 'WS', 'Alpha': 'A', + 'zzzaaaaaazzz': 'zzz', + 'aaaaaa': 'aaa', } } c = compose.Compose(conf, self.tmp_dir)