PR#984: init: Validate whitespace in comps groups - pungi

pungi

#984 init: Validate whitespace in comps groups

Merged 5 years ago by lsedlar. Opened 5 years ago by lsedlar.

lsedlar/pungi check-comps into master

init: Validate whitespace in comps groups

Lubomír Sedlář • 5 years ago

470c0ab

pungi/phases/init.py

file modified

+10 -1

		`@@ -42,7 +42,8 @@`
		`def run(self):`
		`if self.compose.has_comps:`
		`# write global comps and arch comps, create comps repos`
		`- write_global_comps(self.compose)`
		`+ global_comps = write_global_comps(self.compose)`
		`+ validate_comps(global_comps)`
		`for arch in self.compose.get_arches():`
		`write_arch_comps(self.compose, arch)`
		`create_comps_repo(self.compose, arch, None)`
		`@@ -88,6 +89,8 @@`
		`shutil.copy2(os.path.join(tmp_dir, comps_name), comps_file_global)`
		`shutil.rmtree(tmp_dir)`

		`+ return comps_file_global`
		`+`

		`def write_arch_comps(compose, arch):`
		`comps_file_arch = compose.paths.work.comps(arch=arch)`
		`@@ -203,3 +206,9 @@`
		`raise RuntimeError(`
		`"There are duplicated module defaults:\n%s" % "\n".join(errors)`
		`)`
		`+`
		`+`
		`+ def validate_comps(path):`
		`+ """Check that there are whitespace issues in comps."""`
		`+ wrapper = CompsWrapper(path)`
		`+ wrapper.validate()`

pungi/wrappers/comps.py

file modified

+23

		`@@ -49,6 +49,10 @@`
		`])`


		`+ class CompsValidationError(ValueError):`
		`+ pass`
		`+`
		`+`
		`class CompsFilter(object):`
		`"""`
		`Processor for extended comps file. This class treats the input as just an`
		`@@ -230,6 +234,25 @@`
		`return [pkg.name for pkg in grp.packages]`
		`raise KeyError('No such group %r' % group)`

		`+ def validate(self):`
		`+ """Check that no package name contains whitespace, and raise a`
		`+ RuntimeError if there is a problem.`
		`+ """`
		`+ errors = []`
		`+ for group in self.get_comps_groups():`
		`+ for pkg in self.get_packages(group):`
		`+ stripped_pkg = pkg.strip()`
		`+ if pkg != stripped_pkg:`
		`+ errors.append(`
		`+ "Package name %s in group '%s' contains leading or trailing whitespace"`
		`+ % (stripped_pkg, group)`
		`+ )`
		`+`
		`+ if errors:`
		`+ raise CompsValidationError(`
		`+ "Comps file contains errors:\n%s" % "\n".join(errors)`
		`+ )`
		`+`
		`def write_comps(self, comps_obj=None, target_file=None):`
		`if not comps_obj:`
		`comps_obj = self.generate_comps()`

tests/fixtures/comps-ws.xml

file added

+16

		`@@ -0,0 +1,16 @@`
		`+ <?xml version="1.0" encoding="UTF-8"?>`
		`+ <!DOCTYPE comps PUBLIC "-//Red Hat, Inc.//DTD Comps info//EN" "comps.dtd">`
		`+ <comps>`
		`+ <group>`
		`+ <id>core</id>`
		`+ <name>Core</name>`
		`+ <description>Smallest possible installation</description>`
		`+ <default>true</default>`
		`+ <uservisible>false</uservisible>`
		`+ <packagelist>`
		`+ <packagereq type="mandatory"> foo </packagereq>`
		`+ <packagereq type="mandatory">bar </packagereq>`
		`+ <packagereq type="mandatory"> baz</packagereq>`
		`+ </packagelist>`
		`+ </group>`
		`+ </comps>`

tests/test_comps_wrapper.py

file modified

+24 -1

		`@@ -12,7 +12,7 @@`

		`sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))`

		`- from pungi.wrappers.comps import CompsWrapper, CompsFilter`
		`+ from pungi.wrappers.comps import CompsWrapper, CompsFilter, CompsValidationError`
		`from tests.helpers import FIXTURE_DIR`

		`COMPS_FILE = os.path.join(FIXTURE_DIR, 'comps.xml')`
		`@@ -20,6 +20,7 @@`
		`COMPS_GROUP_FILE = os.path.join(FIXTURE_DIR, 'comps-group.xml')`
		`COMPS_ENVIRONMENT_FILE = os.path.join(FIXTURE_DIR, 'comps-env.xml')`
		`COMPS_FILE_WITH_TYPO = os.path.join(FIXTURE_DIR, 'comps-typo.xml')`
		`+ COMPS_FILE_WITH_WHITESPACE = os.path.join(FIXTURE_DIR, 'comps-ws.xml')`


		`class CompsWrapperTest(unittest.TestCase):`
		`@@ -98,6 +99,28 @@`
		`'Package dummy-bash in group core has unknown type',`
		`str(ctx.exception))`

		`+ def test_validate_correct(self):`
		`+ comps = CompsWrapper(COMPS_FILE)`
		`+ comps.validate()`
		`+`
		`+ def test_validate_with_whitespace(self):`
		`+ comps = CompsWrapper(COMPS_FILE_WITH_WHITESPACE)`
		`+ with self.assertRaises(CompsValidationError) as ctx:`
		`+ comps.validate()`
		`+`
		`+ self.assertIn(`
		`+ "Package name foo in group 'core' contains leading or trailing whitespace",`
		`+ str(ctx.exception),`
		`+ )`
		`+ self.assertIn(`
		`+ "Package name bar in group 'core' contains leading or trailing whitespace",`
		`+ str(ctx.exception),`
		`+ )`
		`+ self.assertIn(`
		`+ "Package name baz in group 'core' contains leading or trailing whitespace",`
		`+ str(ctx.exception),`
		`+ )`
		`+`

		`COMPS_IN_FILE = os.path.join(FIXTURE_DIR, 'comps.xml.in')`

tests/test_initphase.py

file modified

+31 -1

		`@@ -12,9 +12,10 @@`

		`from pungi import Modulemd`
		`from pungi.phases import init`
		`- from tests.helpers import DummyCompose, PungiTestCase, touch`
		`+ from tests.helpers import DummyCompose, PungiTestCase, touch, mk_boom`


		`+ @mock.patch("pungi.phases.init.validate_comps")`
		`@mock.patch("pungi.phases.init.validate_module_defaults")`
		`@mock.patch("pungi.phases.init.write_module_defaults")`
		`@mock.patch("pungi.phases.init.write_global_comps")`
		`@@ -33,6 +34,7 @@`
		`write_global,`
		`write_defaults,`
		`validate_defaults,`
		`+ validate_comps,`
		`):`
		`compose = DummyCompose(self.topdir, {})`
		`compose.has_comps = True`
		`@@ -72,6 +74,7 @@`
		`write_global,`
		`write_defaults,`
		`validate_defaults,`
		`+ validate_comps,`
		`):`
		`compose = DummyCompose(self.topdir, {})`
		`compose.has_comps = True`
		`@@ -82,6 +85,9 @@`
		`phase.run()`

		`self.assertEqual(write_global.mock_calls, [mock.call(compose)])`
		`+ self.assertEqual(`
		`+ validate_comps.call_args_list, [mock.call(write_global.return_value)]`
		`+ )`
		`self.assertEqual(write_prepopulate.mock_calls, [mock.call(compose)])`
		`self.assertItemsEqual(write_arch.mock_calls,`
		`[mock.call(compose, 'x86_64'), mock.call(compose, 'amd64')])`
		`@@ -110,6 +116,7 @@`
		`write_global,`
		`write_defaults,`
		`validate_defaults,`
		`+ validate_comps,`
		`):`
		`compose = DummyCompose(self.topdir, {})`
		`compose.has_comps = False`
		`@@ -118,6 +125,7 @@`
		`phase.run()`

		`self.assertItemsEqual(write_global.mock_calls, [])`
		`+ self.assertEqual(validate_comps.call_args_list, [])`
		`self.assertItemsEqual(write_prepopulate.mock_calls, [mock.call(compose)])`
		`self.assertItemsEqual(write_arch.mock_calls, [])`
		`self.assertItemsEqual(create_comps.mock_calls, [])`
		`@@ -134,6 +142,7 @@`
		`write_global,`
		`write_defaults,`
		`validate_defaults,`
		`+ validate_comps,`
		`):`
		`compose = DummyCompose(self.topdir, {})`
		`compose.has_comps = False`
		`@@ -142,6 +151,7 @@`
		`phase.run()`

		`self.assertItemsEqual(write_global.mock_calls, [])`
		`+ self.assertEqual(validate_comps.call_args_list, [])`
		`self.assertItemsEqual(write_prepopulate.mock_calls, [mock.call(compose)])`
		`self.assertItemsEqual(write_arch.mock_calls, [])`
		`self.assertItemsEqual(create_comps.mock_calls, [])`
		`@@ -517,5 +527,25 @@`
		`init.validate_module_defaults(self.topdir)`


		`+ @mock.patch("pungi.phases.init.CompsWrapper")`
		`+ class TestValidateComps(unittest.TestCase):`
		`+ def test_ok(self, CompsWrapper):`
		`+ init.validate_comps("/path")`
		`+`
		`+ self.assertEqual(`
		`+ CompsWrapper.mock_calls, [mock.call("/path"), mock.call().validate()]`
		`+ )`
		`+`
		`+ def test_fail(self, CompsWrapper):`
		`+ CompsWrapper.return_value.validate.side_effect = mk_boom()`
		`+`
		`+ with self.assertRaises(Exception):`
		`+ init.validate_comps("/path")`
		`+`
		`+ self.assertEqual(`
		`+ CompsWrapper.mock_calls, [mock.call("/path"), mock.call().validate()]`
		`+ )`
		`+`
		`+`
		`if __name__ == "__main__":`
		`unittest.main()`

lsedlar commented 5 years ago

If a package name contains leading or trailing whitespace, it will eventually lead to issues: pungi will try to include that group, but since it does not exist, the packages will not make it in.

The root cause is hard to find. Better report an error immediately.

rebased onto 4003fae98426c71f461d878ac0755a2a36e2fb6e

5 years ago

rebased onto 7be75df7f770f3a5850118fd5a8115256d2f5899

5 years ago