Merge pull request #73 from seddonym/error-on-invalid-containers

Error on invalid containers

src/layer_linter/cmdline.py

@@ -83,18 +83,18 @@ def _main(package_name, config_filename=None, is_debug=False,
     if is_debug:
         logging.basicConfig(level=logging.DEBUG)
 
-    try:
-        contracts = _get_contracts(config_filename)
-    except Exception as e:
-        ConsolePrinter.print_error(str(e))
-        return EXIT_STATUS_ERROR
-
     try:
         package = _get_package(package_name)
     except ValueError as e:
         _print_package_name_error_and_help(str(e))
         return EXIT_STATUS_ERROR
 
+    try:
+        contracts = _get_contracts(config_filename, package_name)
+    except Exception as e:
+        ConsolePrinter.print_error(str(e))
+        return EXIT_STATUS_ERROR
+
     graph = DependencyGraph(package=package)
 
     try:
@@ -171,12 +171,12 @@ def _get_package(package_name: str) -> SafeFilenameModule:
     return SafeFilenameModule(name=package_name, filename=package_filename.origin)
 
 
-def _get_contracts(config_filename: str) -> List[Contract]:
+def _get_contracts(config_filename: str, package_name: str) -> List[Contract]:
     # Parse contracts file.
     if config_filename is None:
         config_filename = os.path.join(os.getcwd(), 'layers.yml')
     try:
-        return get_contracts(filename=config_filename)
+        return get_contracts(filename=config_filename, package_name=package_name)
     except FileNotFoundError as e:
         raise RuntimeError("{}: {}".format(e.strerror, e.filename))
     except ContractParseError as e:

src/layer_linter/contract.py

@@ -1,6 +1,6 @@
 from typing import List, Dict, Iterable, Optional
 import yaml
-import os
+import importlib
 import logging
 from copy import copy
 
@@ -155,7 +155,7 @@ def __repr__(self) -> str:
         return '<{}: {}>'.format(self.__class__.__name__, self)
 
 
-def contract_from_yaml(key: str, data: Dict) -> Contract:
+def contract_from_yaml(key: str, data: Dict, package_name: str) -> Contract:
     layers: List[Layer] = []
     if 'layers' not in data:
         raise ContractParseError(f"'{key}' is missing a list of layers.")
@@ -169,8 +169,10 @@ def contract_from_yaml(key: str, data: Dict) -> Contract:
         if 'packages' in data:
             error_message += " (Tip: try renaming 'packages' to 'containers'.)"
         raise ContractParseError(error_message)
-    for package_name in data['containers']:
-        containers.append(Module(package_name))
+
+    for container_name in data['containers']:
+        _validate_container_name(container_name, package_name)
+        containers.append(Module(container_name))
 
     whitelisted_paths: List[ImportPath] = []
     for whitelist_data in data.get('whitelisted_paths', []):
@@ -190,7 +192,7 @@ def contract_from_yaml(key: str, data: Dict) -> Contract:
     )
 
 
-def get_contracts(filename: str) -> List[Contract]:
+def get_contracts(filename: str, package_name: str) -> List[Contract]:
     """Read in any contracts from the given filename.
     """
     contracts = []
@@ -202,6 +204,23 @@ def get_contracts(filename: str) -> List[Contract]:
             logger.debug(e)
             raise ContractParseError('Could not parse {}.'.format(filename))
         for key, data in data_from_yaml.items():
-            contracts.append(contract_from_yaml(key, data))
+            contracts.append(contract_from_yaml(key, data, package_name))
 
     return contracts
+
+
+def _validate_container_name(container_name, package_name):
+    """Raise a ValueError if the suppled container name is not a valid container for the supplied
+    package name.
+    """
+    if not container_name.startswith(package_name):
+        raise ValueError(
+            f"Invalid container '{container_name}': containers must be either a "
+            f"subpackage of '{package_name}', or '{package_name}' itself."
+        )
+
+    # Check that the container actually exists.
+    if importlib.util.find_spec(container_name) is None:
+        raise ValueError(
+            f"Invalid container '{container_name}': no such package."
+        )

tests/assets/singlecontractfile/layers.yml

@@ -1,14 +1,14 @@
 Contract A:
   containers:
-    - foo
-    - bar
+    - singlecontractfile.foo
+    - singlecontractfile.bar
   layers:
     - one
     - two
 
 Contract B:
   containers:
-    - baz/*
+    - singlecontractfile
   layers:
     - one
     - two

tests/assets/singlecontractfile/layers_with_missing_container.yml

@@ -0,0 +1,8 @@
+Contract A:
+  containers:
+    - singlecontractfile.foo
+    - singlecontractfile.missing
+    - singlecontractfile.bar
+  layers:
+    - one
+    - two

tests/assets/successpackage/layers_with_missing_container.yml

@@ -0,0 +1,10 @@
+Modular:
+  containers:
+    - successpackage.one
+    - successpackage.two
+    - successpackage.missing
+    - successpackage.three
+  layers:
+    - gamma
+    - beta
+    - alpha

tests/functional/test_get_contracts.py

@@ -1,50 +1,70 @@
 import os
+import sys
+
+import pytest
 
 from layer_linter.contract import get_contracts, Layer
 from layer_linter.dependencies import ImportPath
 from layer_linter.module import Module
 
 
-def test_get_contracts():
-    dirname = os.path.dirname(__file__)
-    filename = os.path.join(dirname, '..', 'assets', 'singlecontractfile', 'layers.yml')
-
-    contracts = get_contracts(filename)
-
-    assert len(contracts) == 2
-    expected_contracts = [
-        {
-            'name': 'Contract A',
-            'packages': ['foo', 'bar'],
-            'layers': ['one', 'two'],
-        },
-        {
-            'name': 'Contract B',
-            'packages': ['baz/*'],
-            'layers': ['one', 'two', 'three'],
-            'whitelisted_paths': [
-                ('baz.utils', 'baz.three.green'),
-                ('baz.three.blue', 'baz.two'),
-            ],
-        },
-    ]
-    sorted_contracts = sorted(contracts, key=lambda i: i.name)
-    for contract_index, contract in enumerate(sorted_contracts):
-        expected_data = expected_contracts[contract_index]
-        assert contract.name == expected_data['name']
-
-        for package_index, package in enumerate(contract.containers):
-            expected_package_name = expected_data['packages'][package_index]
-            assert package == Module(expected_package_name)
-
-        for layer_index, layer in enumerate(contract.layers):
-            expected_layer_data = expected_data['layers'][layer_index]
-            assert isinstance(layer, Layer)
-            assert layer.name == expected_layer_data
-
-        for whitelisted_index, whitelisted_path in enumerate(contract.whitelisted_paths):
-            expected_importer, expected_imported = expected_data['whitelisted_paths'][
-                whitelisted_index]
-            assert isinstance(whitelisted_path, ImportPath)
-            assert whitelisted_path.importer == Module(expected_importer)
-            assert whitelisted_path.imported == Module(expected_imported)
+class TestGetContracts:
+    def test_happy_path(self):
+        self._initialize_test()
+
+        contracts = get_contracts(self.filename_and_path, package_name='singlecontractfile')
+
+        assert len(contracts) == 2
+        expected_contracts = [
+            {
+                'name': 'Contract A',
+                'packages': ['singlecontractfile.foo', 'singlecontractfile.bar'],
+                'layers': ['one', 'two'],
+            },
+            {
+                'name': 'Contract B',
+                'packages': ['singlecontractfile'],
+                'layers': ['one', 'two', 'three'],
+                'whitelisted_paths': [
+                    ('baz.utils', 'baz.three.green'),
+                    ('baz.three.blue', 'baz.two'),
+                ],
+            },
+        ]
+        sorted_contracts = sorted(contracts, key=lambda i: i.name)
+        for contract_index, contract in enumerate(sorted_contracts):
+            expected_data = expected_contracts[contract_index]
+            assert contract.name == expected_data['name']
+
+            for package_index, package in enumerate(contract.containers):
+                expected_package_name = expected_data['packages'][package_index]
+                assert package == Module(expected_package_name)
+
+            for layer_index, layer in enumerate(contract.layers):
+                expected_layer_data = expected_data['layers'][layer_index]
+                assert isinstance(layer, Layer)
+                assert layer.name == expected_layer_data
+
+            for whitelisted_index, whitelisted_path in enumerate(contract.whitelisted_paths):
+                expected_importer, expected_imported = expected_data['whitelisted_paths'][
+                    whitelisted_index]
+                assert isinstance(whitelisted_path, ImportPath)
+                assert whitelisted_path.importer == Module(expected_importer)
+                assert whitelisted_path.imported == Module(expected_imported)
+
+    def test_container_does_not_exist(self):
+        self._initialize_test('layers_with_missing_container.yml')
+
+        with pytest.raises(ValueError) as e:
+            get_contracts(self.filename_and_path, package_name='singlecontractfile')
+
+        assert str(e.value) == "Invalid container 'singlecontractfile.missing': no such package."
+
+    def _initialize_test(self, config_filename='layers.yml'):
+        # Append the package directory to the path.
+        dirname = os.path.dirname(__file__)
+        package_dirname = os.path.join(dirname, '..', 'assets', 'singlecontractfile')
+        sys.path.append(package_dirname)
+
+        # Set the full config filename and path as an instance attribute.
+        self.filename_and_path = os.path.join(package_dirname, config_filename)

tests/functional/test_main.py

@@ -38,12 +38,12 @@ def test_failure(self, verbosity_count, is_quiet, should_always_fail):
         assert result == EXIT_STATUS_ERROR
 
     def test_specify_config_file(self, verbosity_count, is_quiet, should_always_fail):
-        package_name = 'successpackage'
+        package_name = 'dependenciespackage'
         self._chdir_and_add_to_system_path(package_name)
 
         result = _main(
             package_name,
-            config_filename='layers_alternative.yml',
+            config_filename='../successpackage/layers_alternative.yml',
             verbosity_count=verbosity_count,
             is_quiet=is_quiet)
 
@@ -52,6 +52,16 @@ def test_specify_config_file(self, verbosity_count, is_quiet, should_always_fail
         else:
             assert result == EXIT_STATUS_SUCCESS
 
+    def test_missing_container(self, verbosity_count, is_quiet, should_always_fail):
+        package_name = 'successpackage'
+        self._chdir_and_add_to_system_path(package_name)
+
+        result = _main(package_name,
+                       config_filename='layers_with_missing_container.yml',
+                       verbosity_count=verbosity_count, is_quiet=is_quiet)
+
+        assert result == EXIT_STATUS_ERROR
+
     def _chdir_and_add_to_system_path(self, package_name):
         path = os.path.join(assets_path, package_name)
         sys.path.append(path)

tests/unit/test_contract.py

@@ -1,4 +1,5 @@
 from unittest import mock
+import importlib
 import pytest
 from layer_linter import contract
 from layer_linter.module import Module
@@ -286,19 +287,34 @@ def test_only_shortest_violation_is_reported(self, longer_first):
             ]
 
 
+@mock.patch.object(importlib.util, 'find_spec')
 class TestContractFromYAML:
-    def test_incorrect_whitelisted_path_format(self):
+
+    def test_incorrect_whitelisted_path_format(self, mock_find_spec):
         data = {
-            'containers': ['foo', 'bar'],
+            'containers': ['mypackage.foo', 'mypackage.bar'],
             'layers': ['one', 'two'],
             'whitelisted_paths': [
                 'not the right format',
             ]
         }
 
         with pytest.raises(ValueError) as exception:
-            contract.contract_from_yaml('Contract Foo', data)
+            contract.contract_from_yaml('Contract Foo', data, 'mypackage')
         assert str(exception.value) == (
             'Whitelisted paths must be in the format '
             '"importer.module <- imported.module".'
         )
+
+    def test_container_not_in_package(self, mock_find_spec):
+        data = {
+            'containers': ['mypackage.foo', 'anotherpackage.foo'],
+            'layers': ['one', 'two'],
+        }
+
+        with pytest.raises(ValueError) as exception:
+            contract.contract_from_yaml('Contract Foo', data, 'mypackage')
+        assert str(exception.value) == (
+            "Invalid container 'anotherpackage.foo': containers must be either a subpackage of "
+            "'mypackage', or 'mypackage' itself."
+        )