Merge pull request #97 from zyskarch/fix_module_resolution

Bugfix: Internal modules now recognised as such when root and module …

Author: zyskarch

docs/changelog.md

@@ -2,6 +2,10 @@
 
 This project uses semantic versioning and follows [keep a changelog](https://keepachangelog.com).
 
+## [2.0.2] -- 2024-03-16
+### Fixed
+- Module resolution for absolute imports when root and module path differ.
+
 ## [2.0.1] -- 2024-03-14
 ### Fixed
 - Readme description on module name references.

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "PyTestArch"
-version = "2.0.1"
+version = "2.0.2"
 description = "Test framework for software architecture based on imports between modules"
 authors = ["zyskarch <zyskarch@gmail.com>"]
 maintainers = ["zyskarch <zyskarch@gmail.com>"]

src/pytestarch/eval_structure_generation/file_import/converter.py

@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 import ast
-from typing import List, Optional, Sequence
+from typing import List, Optional, Sequence, Set
 
 from pytestarch.eval_structure.types import Import
 from pytestarch.eval_structure_generation.file_import.import_types import (
@@ -14,7 +14,13 @@
 class ImportConverter:
     """Converts all ast imports to custom import types."""
 
-    def convert(self, asts: List[NamedModule]) -> Sequence[Import]:
+    def convert(
+        self,
+        asts: List[NamedModule],
+        root_prefix_relevant: bool,
+        root_prefix: str,
+        all_modules: Set[str],
+    ) -> Sequence[Import]:
         """Converts ast modules to custom import modules. Filters out all modules
         that are not imports.
 
@@ -36,15 +42,26 @@ def convert(self, asts: List[NamedModule]) -> Sequence[Import]:
                     [NamedModule(m, module_name) for m in ast_module.body]  # type: ignore
                 )
             else:
-                new_imports = self._convert(module.module, module.name)
+                new_imports = self._convert(
+                    module.module,
+                    module.name,
+                    root_prefix_relevant,
+                    root_prefix,
+                    all_modules,
+                )
 
                 if new_imports:
                     imports.extend(new_imports)
 
         return imports
 
     def _convert(
-        self, module: ast.Module, module_name: str
+        self,
+        module: ast.Module,
+        module_name: str,
+        root_prefix_relevant: bool,
+        root_prefix: str,
+        all_internal_modules: Set[str],
     ) -> Optional[Sequence[Import]]:
         """Calculates all imports of the given ast module.
 
@@ -60,10 +77,10 @@ def _convert(
         if isinstance(module, ast.Import):
             new_imports = []
             for alias in module.names:
-                new_imports.append(AbsoluteImport(module_name, alias.name))  # type: ignore
+                new_imports.append(AbsoluteImport(module_name, self._adjust_with_root_prefix(alias.name, root_prefix_relevant, root_prefix, all_internal_modules)))  # type: ignore
         elif isinstance(module, ast.ImportFrom):
             if module.level == 0:
-                new_imports = [AbsoluteImport(module_name, module.module)]  # type: ignore
+                new_imports = [AbsoluteImport(module_name, self._adjust_with_root_prefix(module.module, root_prefix_relevant, root_prefix, all_internal_modules))]  # type: ignore
             else:
                 new_imports = []
                 for alias in module.names:
@@ -74,3 +91,28 @@ def _convert(
                     )
 
         return new_imports
+
+    @classmethod
+    def _adjust_with_root_prefix(
+        cls,
+        module_name: str,
+        root_prefix_relevant: bool,
+        root_prefix: str,
+        all_internal_modules: Set[str],
+    ) -> str:
+        """If the user specifies a module import start point that is not the root path, the ast module names will not contain
+        the root path. Hence, they will later be flagged as external modules.
+        To avoid this, we prepend the root path and check whether this gives a module that we have detected as an internal module earlier.
+        If so, we optimistically assume that this prepended module name is the correct module name.
+
+        Only absolute imports are affected by this, as the relative imports are resolved internally anyway.
+        """
+        if not root_prefix_relevant:
+            return module_name
+
+        potential_full_name = f"{root_prefix}.{module_name}"
+
+        if potential_full_name in all_internal_modules:
+            return potential_full_name
+
+        return module_name

src/pytestarch/eval_structure_generation/graph_generation/graph_generator.py

@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 from pathlib import Path
-from typing import List, Optional, Sequence, Tuple
+from typing import List, Optional, Sequence, Set, Tuple
 
 from pytestarch.eval_structure.evaluable_graph import EvaluableArchitectureGraph
 from pytestarch.eval_structure.networkxgraph import NetworkxGraph, Node
@@ -31,12 +31,18 @@ def generate_graph(
     )
 
     all_modules, ast = _get_all_ast_modules(module_path, root_path, exclusions)
-    imports = _get_imports_from_ast(ast)
 
     internal_module_prefix = _get_internal_module_prefix(
         path_diff_between_root_and_module, root_path
     )
 
+    imports = _get_imports_from_ast(
+        ast,
+        _actual_difference_between_root_and_module(path_diff_between_root_and_module),
+        root_path.name,
+        _get_all_internal_modules(all_modules, internal_module_prefix),
+    )
+
     imports = _remove_excluded_imports(
         exclude_external_libraries, imports, internal_module_prefix
     )
@@ -78,16 +84,22 @@ def _get_internal_module_prefix(
 ) -> str:
     """In general, all internal modules should start with the root module name to be able to differentiate between
     internal and external modules. If the root and base module differ, the modules between them also need to be taken
-    into account, as not root.a.b modules are external, but root.a.b.base are internal.
+    into account, as not-root.a.b-modules are external, but root.a.b.base-modules are internal.
     """
     internal_module_prefix = root_path.name + "."
 
-    if path_diff_between_root_and_module != ".":
+    if _actual_difference_between_root_and_module(path_diff_between_root_and_module):
         internal_module_prefix += path_diff_between_root_and_module
 
     return internal_module_prefix
 
 
+def _actual_difference_between_root_and_module(
+    path_diff_between_root_and_module: str,
+) -> bool:
+    return path_diff_between_root_and_module != "."
+
+
 def _add_extra_levels_to_limit_if_root_and_module_path_differ(
     level_limit: Optional[int], path_diff_between_root_and_module: str
 ) -> Optional[int]:
@@ -97,16 +109,23 @@ def _add_extra_levels_to_limit_if_root_and_module_path_differ(
     if level_limit is None:
         return None
 
-    if path_diff_between_root_and_module != ".":
+    if _actual_difference_between_root_and_module(path_diff_between_root_and_module):
         levels = len(path_diff_between_root_and_module.split("."))
         level_limit += levels
 
     return level_limit
 
 
-def _get_imports_from_ast(ast: List[NamedModule]) -> Sequence[Import]:
+def _get_imports_from_ast(
+    ast: List[NamedModule],
+    root_prefix_relevant: bool,
+    root_prefix: str,
+    all_internal_modules: Set[str],
+) -> Sequence[Import]:
     converter = ImportConverter()
-    return converter.convert(ast)
+    return converter.convert(
+        ast, root_prefix_relevant, root_prefix, all_internal_modules
+    )
 
 
 def _get_all_ast_modules(
@@ -118,3 +137,9 @@ def _get_all_ast_modules(
     parser = Parser(file_filter, root_path)
     all_modules, ast = parser.parse(module_path)
     return all_modules, ast
+
+
+def _get_all_internal_modules(
+    modules: List[str], internal_module_prefix: str
+) -> Set[str]:
+    return {m for m in modules if m.startswith(internal_module_prefix)}

tests/eval_structure_generation/test_converter.py

@@ -16,7 +16,7 @@ def test_converter_retains_all_non_empty_modules() -> None:
     all_modules, asts = parser.parse(RESOURCES_DIR)
 
     converter = ImportConverter()
-    imports = converter.convert(asts)
+    imports = converter.convert(asts, False, "", set())
 
     expected_imports = {
         "pytestarch.pytestarch",

tests/eval_structure_generation/test_module_graph.py

@@ -44,7 +44,7 @@ def modules_and_ast(
 
 @pytest.fixture(scope="module")
 def imports(modules_and_ast: Tuple[List[str], List[NamedModule]]) -> Sequence[Import]:
-    return ImportConverter().convert(modules_and_ast[1])
+    return ImportConverter().convert(modules_and_ast[1], False, "", set())
 
 
 @pytest.fixture(scope="module")

tests/integration/test_integration.py

@@ -1,13 +1,17 @@
 from __future__ import annotations
 
+import os
+
 import pytest
 from integration.interesting_rules_for_tests import (
     partial_name_match_test_cases,
     single_rule_subject_multiple_rule_objects_error_message_test_cases,
     single_rule_subject_single_rule_object_error_message_test_cases,
 )
+from resources import root_module_mismatch_project
+from resources.root_module_mismatch_project import app
 
-from pytestarch import EvaluableArchitecture, Rule
+from pytestarch import EvaluableArchitecture, Rule, get_evaluable_architecture
 
 
 @pytest.mark.parametrize(
@@ -48,3 +52,51 @@ def test_rule_violation_correctly_detected_for_partial_name_matches(
     else:
         rule.assert_applies(graph_based_on_string_module_names)
         # no exception
+
+
+def test_root_module_mismatch_handled_as_expected() -> None:
+    evaluable = get_evaluable_architecture(
+        os.path.dirname(root_module_mismatch_project.__file__),
+        os.path.dirname(app.__file__),
+        ("*__pycache__", "*__init__.py", "*Test.py"),
+    )
+
+    rule = (
+        Rule()
+        .modules_that()
+        .are_sub_modules_of("root_module_mismatch_project.app.red")
+        .should_not()
+        .be_imported_by_modules_that()
+        .are_sub_modules_of("root_module_mismatch_project.app.green")
+    )
+
+    error_message = (
+        '"root_module_mismatch_project.app.red.red" is imported by "root_module_mismatch_project.app.green.green".\n'
+        '"root_module_mismatch_project.app.red.red2" is imported by "root_module_mismatch_project.app.green.green".\n'
+        '"root_module_mismatch_project.app.red.red3" is imported by "root_module_mismatch_project.app.green.green".'
+    )
+    with pytest.raises(AssertionError, match=error_message):
+        rule.assert_applies(evaluable)
+
+
+def test_root_module_match_handled_as_expected() -> None:
+    evaluable = get_evaluable_architecture(
+        os.path.dirname(app.__file__),
+        os.path.dirname(app.__file__),
+        ("*__pycache__", "*__init__.py", "*Test.py"),
+    )
+
+    rule = (
+        Rule()
+        .modules_that()
+        .are_named("app.red")
+        .should_not()
+        .be_imported_by_modules_that()
+        .are_sub_modules_of("app.green")
+    )
+
+    with pytest.raises(
+        AssertionError,
+        match='"app.red.red" is imported by "app.green.green".\n"app.red.red2" is imported by "app.green.green".\n"app.red.red3" is imported by "app.green.green".',
+    ):
+        rule.assert_applies(evaluable)