From e0c298548ca1cc4289a0c92f77d235c3d3d46ad6 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sat, 12 Jul 2025 21:57:03 +0200 Subject: [PATCH 01/11] Make clippy happy --- rust/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 18327cd2..8c603133 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) { From b5be3ff21a4073b1e1fe174fd0d1f39b0b1417b5 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 18 Jun 2025 20:19:17 +0200 Subject: [PATCH 02/11] Add unit tests for generate_module_permutations --- rust/src/graph/higher_order_queries.rs | 72 ++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/rust/src/graph/higher_order_queries.rs b/rust/src/graph/higher_order_queries.rs index ebb1ba71..3fdd8e0e 100644 --- a/rust/src/graph/higher_order_queries.rs +++ b/rust/src/graph/higher_order_queries.rs @@ -208,3 +208,75 @@ 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); + let middle_level = Level::new(middle_layer, false); + let bottom_level = Level::new(bottom_layer, false); + + let levels = vec![top_level, middle_level, bottom_level]; + + let permutations = graph.generate_module_permutations(&levels); + + assert_eq!( + permutations, + vec![ + (middle_module, top_module), + (bottom_module, top_module), + (bottom_module, middle_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); + + let levels = vec![independent_level]; + + let permutations = graph.generate_module_permutations(&levels); + + assert_eq!( + permutations + .into_iter() + // Sorted for stable comparision. + .sorted_by_key(|(importer, imported)| ( + graph.get_module(*importer).unwrap().name(), + graph.get_module(*imported).unwrap().name() + )) + .collect_vec(), + vec![(module_a, module_b), (module_b, module_a),] + ); + } +} From 026fc006d60418b568fff4c5109cd730e3f5b04d Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Thu, 19 Jun 2025 17:55:17 +0200 Subject: [PATCH 03/11] Add closed to Layer class --- src/grimp/domain/valueobjects.py | 7 +++++-- tests/unit/domain/test_valueobjects.py | 8 +++++++- 2 files changed, 12 insertions(+), 3 deletions(-) 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/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" From ac5b19708f8e6f604ebd2135d95dc797d0036bf1 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Thu, 19 Jun 2025 17:58:49 +0200 Subject: [PATCH 04/11] Add closed to Level struct --- rust/src/graph/higher_order_queries.rs | 11 +++++++---- rust/src/lib.rs | 9 ++++++++- rust/tests/large.rs | 1 + src/grimp/adaptors/graph.py | 10 +++++++--- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/rust/src/graph/higher_order_queries.rs b/rust/src/graph/higher_order_queries.rs index 3fdd8e0e..727dc1e3 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)] @@ -232,9 +235,9 @@ mod tests { let mut bottom_layer = FxHashSet::default(); bottom_layer.insert(bottom_module); - let top_level = Level::new(top_layer, false); - let middle_level = Level::new(middle_layer, false); - let bottom_level = Level::new(bottom_layer, false); + 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]; @@ -261,7 +264,7 @@ mod tests { independent_layer.insert(module_a); independent_layer.insert(module_b); - let independent_level = Level::new(independent_layer, true); + let independent_level = Level::new(independent_layer, true, false); let levels = vec![independent_level]; diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 8c603133..4be399ee 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -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) From d096153cfaf257fb306faafddc7d02ff3e7d8686 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Thu, 19 Jun 2025 18:01:11 +0200 Subject: [PATCH 05/11] Adapt generate_module_permutations --- rust/src/graph/higher_order_queries.rs | 56 ++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/rust/src/graph/higher_order_queries.rs b/rust/src/graph/higher_order_queries.rs index 727dc1e3..6246b3d3 100644 --- a/rust/src/graph/higher_order_queries.rs +++ b/rust/src/graph/higher_order_queries.rs @@ -105,6 +105,17 @@ impl Graph { permutations.push((*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.push((*higher_module, *module)); + } + } + closed |= higher_level.closed; + } } } @@ -282,4 +293,49 @@ mod tests { vec![(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_module_permutations(&levels); + + assert_eq!( + permutations + .into_iter() + // Sorted for stable comparision. + .sorted_by_key(|(importer, imported)| ( + graph.get_module(*importer).unwrap().name(), + graph.get_module(*imported).unwrap().name() + )) + .collect_vec(), + vec![ + (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), + ] + ); + } } From 252fe62036443a157ca3e462a9d1a87c16c245aa Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 18 Jun 2025 20:45:01 +0200 Subject: [PATCH 06/11] Add TestClosedLayers --- tests/unit/adaptors/graph/test_layers.py | 76 ++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/tests/unit/adaptors/graph/test_layers.py b/tests/unit/adaptors/graph/test_layers.py index e85d2b25..4e9d62bc 100644 --- a/tests/unit/adaptors/graph/test_layers.py +++ b/tests/unit/adaptors/graph/test_layers.py @@ -958,3 +958,79 @@ def _pairwise(iterable): a, b = itertools.tee(iterable) next(b, None) return zip(a, b) + + +class TestClosedLayers: + @pytest.mark.parametrize( + "importer, imported", + [ + ("mypackage.highest", "mypackage.low"), + ("mypackage.highest", "mypackage.lowest"), + ("mypackage.high", "mypackage.low"), + ("mypackage.high", "mypackage.lowest"), + ], + ) + def test_cannot_import_through_closed_mid(self, importer, imported): + graph = self._build_legal_graph() + graph.add_import(importer=importer, imported=imported) + + result = self._analyze(graph) + + assert result == { + PackageDependency.new( + importer=importer, + imported=imported, + routes={Route.single_chained(importer, imported)}, + ), + } + + def test_cannot_import_through_closed_mid_indirect(self): + graph = self._build_legal_graph() + graph.add_import(importer="mypackage.high", imported="mypackage.other") + graph.add_import(importer="mypackage.other", imported="mypackage.low") + + result = self._analyze(graph) + + assert result == { + PackageDependency.new( + importer="mypackage.high", + imported="mypackage.low", + routes={ + Route.single_chained("mypackage.high", "mypackage.other", "mypackage.low") + }, + ), + } + + def _build_legal_graph(self): + graph = ImportGraph() + for module in ( + "mypackage", + "mypackage.highest", + "mypackage.high", + "mypackage.mid", + "mypackage.low", + "mypackage.lowest", + "mypackage.other", + ): + graph.add_module(module) + + # Add some 'legal' imports that respect the layering + graph.add_import(importer="mypackage.highest", imported="mypackage.high") + graph.add_import(importer="mypackage.high", imported="mypackage.mid") + graph.add_import(importer="mypackage.mid", imported="mypackage.low") + graph.add_import(importer="mypackage.low", imported="mypackage.lowest") + graph.add_import(importer="mypackage.highest", imported="mypackage.mid") + graph.add_import(importer="mypackage.mid", imported="mypackage.lowest") + + return graph + + def _analyze(self, graph: ImportGraph) -> set[PackageDependency]: + return graph.find_illegal_dependencies_for_layers( + layers=( + Layer("mypackage.highest", closed=False), + Layer("mypackage.high", closed=False), + Layer("mypackage.mid", closed=True), # Closed layer + Layer("mypackage.low", closed=False), + Layer("mypackage.lowest", closed=False), + ), + ) From 7362cf90303128c8b087e5b797d4c80f5ce505b8 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 18 Jun 2025 21:00:40 +0200 Subject: [PATCH 07/11] Update documentation --- docs/usage.rst | 17 +++++++++++++++-- src/grimp/application/ports/graph.py | 5 +++++ 2 files changed, 20 insertions(+), 2 deletions(-) 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/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 From 2f33fee15fa57ec1f124169ae14a4cdaad2c5b12 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 18 Jun 2025 21:01:08 +0200 Subject: [PATCH 08/11] Update changelog --- CHANGELOG.rst | 5 +++++ 1 file changed, 5 insertions(+) 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) ---------------- From 5b669fb09c0e2516bf31611035a431d17f986c8c Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 13 Jul 2025 07:26:41 +0200 Subject: [PATCH 09/11] Return hashset from generate_module_permutations The returned order isn't stable anyway, and order doesn't matter. --- rust/src/graph/higher_order_queries.rs | 45 ++++++++++---------------- 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/rust/src/graph/higher_order_queries.rs b/rust/src/graph/higher_order_queries.rs index 6246b3d3..c5813215 100644 --- a/rust/src/graph/higher_order_queries.rs +++ b/rust/src/graph/higher_order_queries.rs @@ -84,15 +84,18 @@ impl Graph { ) } - fn generate_module_permutations(&self, levels: &[Level]) -> Vec<(ModuleToken, ModuleToken)> { - let mut permutations = vec![]; + fn generate_module_permutations( + &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)); } } @@ -102,7 +105,7 @@ impl Graph { if sibling_module == module { continue; } - permutations.push((*module, *sibling_module)); + permutations.insert((*module, *sibling_module)); } } @@ -111,7 +114,7 @@ impl Graph { for higher_level in levels[..index].iter().rev() { if closed { for higher_module in &higher_level.layers { - permutations.push((*higher_module, *module)); + permutations.insert((*higher_module, *module)); } } closed |= higher_level.closed; @@ -256,11 +259,11 @@ mod tests { assert_eq!( permutations, - vec![ - (middle_module, top_module), - (bottom_module, top_module), + FxHashSet::from_iter([ (bottom_module, middle_module), - ] + (bottom_module, top_module), + (middle_module, top_module), + ]) ); } @@ -282,15 +285,8 @@ mod tests { let permutations = graph.generate_module_permutations(&levels); assert_eq!( - permutations - .into_iter() - // Sorted for stable comparision. - .sorted_by_key(|(importer, imported)| ( - graph.get_module(*importer).unwrap().name(), - graph.get_module(*imported).unwrap().name() - )) - .collect_vec(), - vec![(module_a, module_b), (module_b, module_a),] + permutations, + FxHashSet::from_iter([(module_a, module_b), (module_b, module_a),]) ); } @@ -321,21 +317,14 @@ mod tests { let permutations = graph.generate_module_permutations(&levels); assert_eq!( - permutations - .into_iter() - // Sorted for stable comparision. - .sorted_by_key(|(importer, imported)| ( - graph.get_module(*importer).unwrap().name(), - graph.get_module(*imported).unwrap().name() - )) - .collect_vec(), - vec![ + 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), - ] + ]) ); } } From 10d41fbe1aadd66b717272ebcb8918167eb32c28 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Sun, 13 Jul 2025 07:45:44 +0200 Subject: [PATCH 10/11] Rename generate_module_permutations --- rust/src/graph/higher_order_queries.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/rust/src/graph/higher_order_queries.rs b/rust/src/graph/higher_order_queries.rs index c5813215..dad84825 100644 --- a/rust/src/graph/higher_order_queries.rs +++ b/rust/src/graph/higher_order_queries.rs @@ -60,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, @@ -84,7 +84,9 @@ impl Graph { ) } - fn generate_module_permutations( + /// 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)> { @@ -255,7 +257,7 @@ mod tests { let levels = vec![top_level, middle_level, bottom_level]; - let permutations = graph.generate_module_permutations(&levels); + let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); assert_eq!( permutations, @@ -282,7 +284,7 @@ mod tests { let levels = vec![independent_level]; - let permutations = graph.generate_module_permutations(&levels); + let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); assert_eq!( permutations, @@ -314,7 +316,7 @@ mod tests { let levels = vec![top_level, middle_level, bottom_level]; - let permutations = graph.generate_module_permutations(&levels); + let permutations = graph.generate_illegal_import_permutations_for_layers(&levels); assert_eq!( permutations, From 5c6a9cf6747d13141ef91c45166f43b6ad707302 Mon Sep 17 00:00:00 2001 From: Peter Byfield Date: Wed, 16 Jul 2025 07:13:23 +0200 Subject: [PATCH 11/11] Refactor and extend TestClosedLayers --- tests/unit/adaptors/graph/test_layers.py | 185 ++++++++++++++++------- 1 file changed, 132 insertions(+), 53 deletions(-) diff --git a/tests/unit/adaptors/graph/test_layers.py b/tests/unit/adaptors/graph/test_layers.py index 4e9d62bc..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 @@ -962,75 +963,153 @@ def _pairwise(iterable): class TestClosedLayers: @pytest.mark.parametrize( - "importer, imported", + "importer, imported, expect_ok", [ - ("mypackage.highest", "mypackage.low"), - ("mypackage.highest", "mypackage.lowest"), - ("mypackage.high", "mypackage.low"), - ("mypackage.high", "mypackage.lowest"), + # 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_cannot_import_through_closed_mid(self, importer, imported): - graph = self._build_legal_graph() + 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 = self._analyze(graph) + result = graph.find_illegal_dependencies_for_layers(layers) - assert result == { - PackageDependency.new( - importer=importer, - imported=imported, - routes={Route.single_chained(importer, imported)}, - ), - } + if expect_ok: + assert result == set() + else: + assert result == { + PackageDependency.new( + importer=importer, + imported=imported, + routes={Route.single_chained(importer, imported)}, + ), + } - def test_cannot_import_through_closed_mid_indirect(self): - graph = self._build_legal_graph() - graph.add_import(importer="mypackage.high", imported="mypackage.other") - graph.add_import(importer="mypackage.other", imported="mypackage.low") + @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) - result = self._analyze(graph) + 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="mypackage.high", - imported="mypackage.low", - routes={ - Route.single_chained("mypackage.high", "mypackage.other", "mypackage.low") - }, + importer="high", + imported="low", + routes={Route.single_chained("high", "other", "low")}, ), } - def _build_legal_graph(self): + def _build_layers(self, layers: Iterable[Layer]): graph = ImportGraph() - for module in ( - "mypackage", - "mypackage.highest", - "mypackage.high", - "mypackage.mid", - "mypackage.low", - "mypackage.lowest", - "mypackage.other", - ): + + modules = [] + for layer in layers: + assert len(layer.module_tails) == 1 + module = list(layer.module_tails)[0] graph.add_module(module) + modules.append(module) - # Add some 'legal' imports that respect the layering - graph.add_import(importer="mypackage.highest", imported="mypackage.high") - graph.add_import(importer="mypackage.high", imported="mypackage.mid") - graph.add_import(importer="mypackage.mid", imported="mypackage.low") - graph.add_import(importer="mypackage.low", imported="mypackage.lowest") - graph.add_import(importer="mypackage.highest", imported="mypackage.mid") - graph.add_import(importer="mypackage.mid", imported="mypackage.lowest") + # 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 - - def _analyze(self, graph: ImportGraph) -> set[PackageDependency]: - return graph.find_illegal_dependencies_for_layers( - layers=( - Layer("mypackage.highest", closed=False), - Layer("mypackage.high", closed=False), - Layer("mypackage.mid", closed=True), # Closed layer - Layer("mypackage.low", closed=False), - Layer("mypackage.lowest", closed=False), - ), - )