diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9fe3c959..177d45da 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,11 @@ Changelog ========= +Unreleased +---------- + +* Add closed layers to layer contract. + 3.9 (2025-05-05) ---------------- diff --git a/docs/usage.rst b/docs/usage.rst index c342f06d..54710f2b 100644 --- a/docs/usage.rst +++ b/docs/usage.rst @@ -304,7 +304,8 @@ Higher level analysis passing ``independent=False`` when instantiating the :class:`.Layer`. For convenience, if a layer consists only of one module name then a string may be passed in place of the :class:`.Layer` object. Additionally, if the layer consists of multiple *independent* modules, that can be passed as a set of strings instead of a - :class:`.Layer` object. + :class:`.Layer` object. A closed layer may be created by passing ``closed=True`` to prevent higher layers + from importing directly from layers below the closed layer (see `Closed layers`_ section below). *Any modules specified that don't exist in the graph will be silently ignored.* :param set[str] containers: The parent modules of the layers, as absolute names that you could import, such as ``mypackage.foo``. (Optional.) @@ -409,6 +410,18 @@ Higher level analysis ), ) + Closed layers + ^^^^^^^^^^^^^ + + A closed layer may be created by passing ``closed=True``. Closed layers provide an additional + constraint in your architecture that prevents higher layers from "reaching through" to access + lower layers directly. Imports from higher to lower layers cannot bypass closed layers - the + closed layer must be included in the import chain. + + This is particularly useful for enforcing architectural boundaries where you want to hide + implementation details of lower layers and ensure that higher layers only interact with + the public interface provided by the closed layer. + Return value ^^^^^^^^^^^^ @@ -575,4 +588,4 @@ Module expressions - ``mypackage.foo*``: is not a valid expression. (The wildcard must replace a whole module name.) .. _namespace packages: https://docs.python.org/3/glossary.html#term-namespace-package -.. _namespace portion: https://docs.python.org/3/glossary.html#term-portion \ No newline at end of file +.. _namespace portion: https://docs.python.org/3/glossary.html#term-portion diff --git a/rust/src/graph/higher_order_queries.rs b/rust/src/graph/higher_order_queries.rs index ebb1ba71..dad84825 100644 --- a/rust/src/graph/higher_order_queries.rs +++ b/rust/src/graph/higher_order_queries.rs @@ -16,6 +16,9 @@ pub struct Level { #[getset(get_copy = "pub")] independent: bool, + + #[getset(get_copy = "pub")] + closed: bool, } #[derive(Debug, Clone, PartialEq, Eq, new, Getters)] @@ -57,7 +60,7 @@ impl Graph { .flat_map(|m| m.conv::>().with_descendants(self)) .collect::>(); - self.generate_module_permutations(levels) + self.generate_illegal_import_permutations_for_layers(levels) .into_par_iter() .try_fold( Vec::new, @@ -81,15 +84,20 @@ impl Graph { ) } - fn generate_module_permutations(&self, levels: &[Level]) -> Vec<(ModuleToken, ModuleToken)> { - let mut permutations = vec![]; + /// Returns a set of tuples (importer, imported) describing the illegal + /// import permutations for the given layers. + fn generate_illegal_import_permutations_for_layers( + &self, + levels: &[Level], + ) -> FxHashSet<(ModuleToken, ModuleToken)> { + let mut permutations = FxHashSet::default(); for (index, level) in levels.iter().enumerate() { for module in &level.layers { // Should not be imported by lower layers. for lower_level in &levels[index + 1..] { for lower_module in &lower_level.layers { - permutations.push((*lower_module, *module)); + permutations.insert((*lower_module, *module)); } } @@ -99,8 +107,19 @@ impl Graph { if sibling_module == module { continue; } - permutations.push((*module, *sibling_module)); + permutations.insert((*module, *sibling_module)); + } + } + + // Should not be imported by higher layers if there is a closed layer inbetween. + let mut closed = false; + for higher_level in levels[..index].iter().rev() { + if closed { + for higher_module in &higher_level.layers { + permutations.insert((*higher_module, *module)); + } } + closed |= higher_level.closed; } } } @@ -208,3 +227,106 @@ impl Graph { ) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::graph::Graph; + use rustc_hash::FxHashSet; + + #[test] + fn test_generate_module_permutations_simple_layers() { + let mut graph = Graph::default(); + + let top_module = graph.get_or_add_module("app.top").token; + let middle_module = graph.get_or_add_module("app.middle").token; + let bottom_module = graph.get_or_add_module("app.bottom").token; + + let mut top_layer = FxHashSet::default(); + top_layer.insert(top_module); + + let mut middle_layer = FxHashSet::default(); + middle_layer.insert(middle_module); + + let mut bottom_layer = FxHashSet::default(); + bottom_layer.insert(bottom_module); + + let top_level = Level::new(top_layer, false, false); + let middle_level = Level::new(middle_layer, false, false); + let bottom_level = Level::new(bottom_layer, false, false); + + let levels = vec![top_level, middle_level, bottom_level]; + + let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); + + assert_eq!( + permutations, + FxHashSet::from_iter([ + (bottom_module, middle_module), + (bottom_module, top_module), + (middle_module, top_module), + ]) + ); + } + + #[test] + fn test_generate_module_permutations_independent_layer() { + let mut graph = Graph::default(); + + let module_a = graph.get_or_add_module("app.independent.a").token; + let module_b = graph.get_or_add_module("app.independent.b").token; + + let mut independent_layer = FxHashSet::default(); + independent_layer.insert(module_a); + independent_layer.insert(module_b); + + let independent_level = Level::new(independent_layer, true, false); + + let levels = vec![independent_level]; + + let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); + + assert_eq!( + permutations, + FxHashSet::from_iter([(module_a, module_b), (module_b, module_a),]) + ); + } + + #[test] + fn test_generate_module_permutations_closed_layer() { + let mut graph = Graph::default(); + + // Create three layers with the middle one closed + let top_module = graph.get_or_add_module("app.top").token; + let middle_module = graph.get_or_add_module("app.middle").token; + let bottom_module = graph.get_or_add_module("app.bottom").token; + + let mut top_layer = FxHashSet::default(); + top_layer.insert(top_module); + + let mut middle_layer = FxHashSet::default(); + middle_layer.insert(middle_module); + + let mut bottom_layer = FxHashSet::default(); + bottom_layer.insert(bottom_module); + + let top_level = Level::new(top_layer, false, false); + let middle_level = Level::new(middle_layer, false, true); // Closed layer + let bottom_level = Level::new(bottom_layer, false, false); + + let levels = vec![top_level, middle_level, bottom_level]; + + let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); + + assert_eq!( + permutations, + FxHashSet::from_iter([ + (bottom_module, middle_module), + (bottom_module, top_module), + (middle_module, top_module), + // Top should not import Bottom due to closed middle layer + (top_module, bottom_module), + ]) + ); + } +} diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 18327cd2..4be399ee 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -543,7 +543,7 @@ impl GraphWrapper { .unwrap() .into_iter() .map(|name| match container.clone() { - Some(container) => format!("{}.{}", container, name), + Some(container) => format!("{container}.{name}"), None => name, }) .filter_map(|name| match self.get_visible_module_by_name(&name) { @@ -561,7 +561,14 @@ impl GraphWrapper { .extract::() .unwrap(); - levels.push(Level::new(layers, independent)); + let closed = level_dict + .get_item("closed") + .unwrap() + .unwrap() + .extract::() + .unwrap(); + + levels.push(Level::new(layers, independent, closed)); } levels_by_container.push(levels); } diff --git a/rust/tests/large.rs b/rust/tests/large.rs index 7e7da36f..d72312c2 100644 --- a/rust/tests/large.rs +++ b/rust/tests/large.rs @@ -42,6 +42,7 @@ fn test_large_graph_deep_layers() { .token() .conv::>(), true, + false, ) }) .collect(); diff --git a/src/grimp/adaptors/graph.py b/src/grimp/adaptors/graph.py index ac8bfe7a..5ede492d 100644 --- a/src/grimp/adaptors/graph.py +++ b/src/grimp/adaptors/graph.py @@ -161,7 +161,11 @@ def find_illegal_dependencies_for_layers( try: result = self._rustgraph.find_illegal_dependencies_for_layers( layers=tuple( - {"layers": layer.module_tails, "independent": layer.independent} + { + "layers": layer.module_tails, + "independent": layer.independent, + "closed": layer.closed, + } for layer in layers ), containers=set(containers) if containers else set(), @@ -201,9 +205,9 @@ def _parse_layers(layers: Sequence[Layer | str | set[str]]) -> tuple[Layer, ...] if isinstance(layer, Layer): out_layers.append(layer) elif isinstance(layer, str): - out_layers.append(Layer(layer, independent=True)) + out_layers.append(Layer(layer)) else: - out_layers.append(Layer(*tuple(layer), independent=True)) + out_layers.append(Layer(*tuple(layer))) return tuple(out_layers) diff --git a/src/grimp/application/ports/graph.py b/src/grimp/application/ports/graph.py index ec589b07..e5530fde 100644 --- a/src/grimp/application/ports/graph.py +++ b/src/grimp/application/ports/graph.py @@ -373,6 +373,11 @@ def find_illegal_dependencies_for_layers( compatibility it is also possible to pass a simple `set[str]` to describe a layer. In this case the sibling modules within the layer will be considered independent. + By default layers are open. `Layer.closed` can be set to True to create a closed layer. + Imports from higher to lower layers cannot bypass closed layers - the closed layer must be + included in the import chain. For example, given the layers high -> mid (closed) -> low then + all import chains from high -> low must go via mid. + Arguments: - layers: A sequence, each element of which consists either of a `Layer`, the name diff --git a/src/grimp/domain/valueobjects.py b/src/grimp/domain/valueobjects.py index 63145f17..35ef338a 100644 --- a/src/grimp/domain/valueobjects.py +++ b/src/grimp/domain/valueobjects.py @@ -69,12 +69,15 @@ class Layer: module_tails: Set[str] independent: bool + closed: bool # A custom `__init__` is needed since `module_tails` is a variadic argument. - def __init__(self, *module_tails: str, independent: bool = True) -> None: + def __init__(self, *module_tails: str, independent: bool = True, closed: bool = False) -> None: # `object.__setattr__` is needed since the dataclass is frozen. object.__setattr__(self, "module_tails", set(module_tails)) object.__setattr__(self, "independent", independent) + object.__setattr__(self, "closed", closed) def __str__(self) -> str: - return f"{self.module_tails}, independent={self.independent}" + module_tails = sorted(self.module_tails) + return f"{module_tails}, independent={self.independent}, closed={self.closed}" diff --git a/tests/unit/adaptors/graph/test_layers.py b/tests/unit/adaptors/graph/test_layers.py index e85d2b25..e9019624 100644 --- a/tests/unit/adaptors/graph/test_layers.py +++ b/tests/unit/adaptors/graph/test_layers.py @@ -3,6 +3,7 @@ import itertools import logging import re +from collections.abc import Iterable import pytest # type: ignore @@ -958,3 +959,157 @@ def _pairwise(iterable): a, b = itertools.tee(iterable) next(b, None) return zip(a, b) + + +class TestClosedLayers: + @pytest.mark.parametrize( + "importer, imported, expect_ok", + [ + # Legal imports (sample) + ("highest", "high", True), + ("highest", "mid", True), + ("mid", "lowest", True), + # Illegal imports (exhaustive) + ("highest", "low", False), + ("highest", "lowest", False), + ("high", "low", False), + ("high", "lowest", False), + ], + ) + def test_one_closed_layer(self, importer, imported, expect_ok): + layers = [ + Layer("highest"), + Layer("high"), + Layer("mid", closed=True), + Layer("low"), + Layer("lowest"), + ] + graph = self._build_layers(layers) + + graph.add_import(importer=importer, imported=imported) + + result = graph.find_illegal_dependencies_for_layers(layers) + + if expect_ok: + assert result == set() + else: + assert result == { + PackageDependency.new( + importer=importer, + imported=imported, + routes={Route.single_chained(importer, imported)}, + ), + } + + @pytest.mark.parametrize( + "importer, imported, expect_ok", + [ + ("highest", "high", True), + ("highest", "mid", False), + ("highest", "low", False), + ("highest", "lowest", False), + ("high", "mid", True), + ("high", "low", True), + ("high", "lowest", False), + ("mid", "low", True), + ("mid", "lowest", False), + ("low", "lowest", True), + ], + ) + def test_two_closed_layers(self, importer, imported, expect_ok): + layers = [ + Layer("highest"), + Layer("high", closed=True), + Layer("mid"), + Layer("low", closed=True), + Layer("lowest"), + ] + graph = self._build_layers(layers) + + graph.add_import(importer=importer, imported=imported) + + result = graph.find_illegal_dependencies_for_layers(layers) + + if expect_ok: + assert result == set() + else: + assert result == { + PackageDependency.new( + importer=importer, + imported=imported, + routes={Route.single_chained(importer, imported)}, + ), + } + + @pytest.mark.parametrize( + "importer, imported, expect_ok", + [ + ("high", "mid_high", True), + ("high", "mid_low", False), + ("high", "low", False), + ("mid_high", "mid_low", True), + ("mid_high", "low", False), + ("mid_low", "low", True), + ], + ) + def test_two_consecutive_closed_layers(self, importer, imported, expect_ok): + layers = [ + Layer("high"), + Layer("mid_high", closed=True), + Layer("mid_low", closed=True), + Layer("low"), + ] + graph = self._build_layers(layers) + + graph.add_import(importer=importer, imported=imported) + + result = graph.find_illegal_dependencies_for_layers(layers) + + if expect_ok: + assert result == set() + else: + assert result == { + PackageDependency.new( + importer=importer, + imported=imported, + routes={Route.single_chained(importer, imported)}, + ), + } + + def test_indirect_import_cannot_bypass_closed_layer(self): + layers = [ + Layer("high"), + Layer("mid", closed=True), + Layer("low"), + ] + graph = self._build_layers(layers) + + graph.add_module("other") + graph.add_import(importer="high", imported="other") + graph.add_import(importer="other", imported="low") + + result = graph.find_illegal_dependencies_for_layers(layers) + + assert result == { + PackageDependency.new( + importer="high", + imported="low", + routes={Route.single_chained("high", "other", "low")}, + ), + } + + def _build_layers(self, layers: Iterable[Layer]): + graph = ImportGraph() + + modules = [] + for layer in layers: + assert len(layer.module_tails) == 1 + module = list(layer.module_tails)[0] + graph.add_module(module) + modules.append(module) + + # Legal imports, from higher layer to immediate lower layer + for higher_module, lower_module in zip(modules[:-1], modules[1:]): + graph.add_import(importer=higher_module, imported=lower_module) + + return graph diff --git a/tests/unit/domain/test_valueobjects.py b/tests/unit/domain/test_valueobjects.py index 39448b4c..cc883397 100644 --- a/tests/unit/domain/test_valueobjects.py +++ b/tests/unit/domain/test_valueobjects.py @@ -1,6 +1,6 @@ import pytest # type: ignore -from grimp.domain.valueobjects import DirectImport, Module +from grimp.domain.valueobjects import DirectImport, Module, Layer class TestModule: @@ -33,3 +33,9 @@ def test_str(self): line_contents="import bar", ) assert str(import_path) == "foo -> bar (l. 10)" + + +class TestLayer: + def test_str(self): + layer = Layer("foo", "bar", independent=True, closed=False) + assert str(layer) == "['bar', 'foo'], independent=True, closed=False"