Fix multiple ignored import bug (#109)

Fix multiple ignored import bug

#108

CHANGELOG.rst

@@ -1,6 +1,11 @@
 Changelog
 =========
 
+latest
+------
+
+* Fix bug with ignoring external imports that occur multiple times in the same module.
+
 1.2.5 (2021-09-21)
 ------------------
 

src/importlinter/domain/helpers.py

@@ -23,16 +23,22 @@ def pop_imports(
         MissingImport if an import is not present in the graph.
     """
     removed_imports: List[Dict[str, Union[str, int]]] = []
-    for import_to_remove in imports:
+
+    imports_to_remove = _dedupe_imports(imports)
+
+    for import_to_remove in imports_to_remove:
         import_details = graph.get_import_details(
             importer=import_to_remove.importer.name, imported=import_to_remove.imported.name
         )
         if not import_details:
             raise MissingImport(f"Ignored import {import_to_remove} not present in the graph.")
-        removed_imports.extend(import_details)
+
         graph.remove_import(
             importer=import_to_remove.importer.name, imported=import_to_remove.imported.name
         )
+
+        removed_imports.extend(import_details)
+
     return removed_imports
 
 
@@ -110,6 +116,48 @@ def add_imports(graph: ImportGraph, import_details: List[Dict[str, Union[str, in
         )
 
 
+def _dedupe_imports(imports: Iterable[DirectImport]) -> Iterable[DirectImport]:
+    """
+    Return the imports with the metadata and any duplicates removed.
+
+    For example:
+
+        _dedupe_imports([
+            DirectImport(
+                importer="blue",
+                imported="green",
+                line_number=1,
+                line_contents="from blue import green.one",
+            ),
+            DirectImport(
+                importer="blue",
+                imported="green",
+                line_number=3,
+                line_contents="from blue import green.two",
+            ),
+        ]) == {
+            DirectImport(
+                importer="blue",
+                imported="green",
+            ),
+        }
+
+    This is to make it easy for the calling function to remove the set of imports from a graph
+    without attempting to remove certain imports twice.
+    """
+    deduped_imports: List[DirectImport] = []
+
+    # Why don't we use a set here? Because we want to preserve the order (mainly for testability).
+    imports_without_metadata = [
+        DirectImport(imported=i.imported, importer=i.importer) for i in imports
+    ]
+    for import_without_metadata in imports_without_metadata:
+        if import_without_metadata not in deduped_imports:
+            deduped_imports.append(import_without_metadata)
+
+    return deduped_imports
+
+
 def _to_pattern(expression: str) -> Pattern:
     """
     Function which translates an import expression into a regex pattern.

tests/unit/domain/test_helpers.py

@@ -65,10 +65,52 @@ def test_raises_missing_import_if_module_not_found(self):
         )
         with pytest.raises(
             MissingImport,
-            match=re.escape(f"Ignored import {non_existent_import} not present in the graph."),
+            match=re.escape(
+                "Ignored import mypackage.nonexistent -> mypackage.yellow "
+                "not present in the graph."
+            ),
         ):
             pop_imports(graph, [non_existent_import])
 
+    def test_works_with_multiple_external_imports_from_same_module(self):
+        imports_to_pop = [
+            dict(
+                importer="mypackage.green",
+                imported="someexternalpackage",
+                line_number=2,
+                line_contents="from someexternalpackage import one",
+            ),
+            dict(
+                importer="mypackage.green",
+                imported="someexternalpackage",
+                line_number=2,
+                line_contents="from someexternalpackage import two",
+            ),
+        ]
+        imports = self.IMPORTS + imports_to_pop
+        graph = self._build_graph(imports=imports)
+
+        result = pop_imports(
+            graph,
+            [
+                DirectImport(
+                    importer=Module(i["importer"]),
+                    imported=Module(i["imported"]),
+                    line_number=i["line_number"],
+                    line_contents=i["line_contents"],
+                )
+                for i in imports_to_pop
+            ],
+        )
+
+        assert result == imports_to_pop
+        one_of_the_popped_imports = imports_to_pop[0]
+        assert not graph.direct_import_exists(
+            importer=one_of_the_popped_imports["importer"],
+            imported=one_of_the_popped_imports["imported"],
+        )
+        assert graph.count_imports() == len(self.IMPORTS)
+
     def _build_graph(self, imports):
         graph = ImportGraph()
         for import_ in imports:
@@ -114,6 +156,20 @@ class TestImportExpressionsToImports:
             line_number=1,
             line_contents="-",
         ),
+        # Direct imports of external packages can appear more than once, as the external package
+        # is squashed.
+        DirectImport(
+            importer=Module("mypackage.brown"),
+            imported=Module("someotherpackage"),
+            line_number=1,
+            line_contents="from someotherpackage import one",
+        ),
+        DirectImport(
+            importer=Module("mypackage.brown"),
+            imported=Module("someotherpackage"),
+            line_number=2,
+            line_contents="from someotherpackage import two",
+        ),
     ]
 
     @pytest.mark.parametrize(
@@ -175,6 +231,13 @@ class TestImportExpressionsToImports:
                 ],
                 [DIRECT_IMPORTS[1]],
             ),
+            (
+                "Multiple imports of external package with same importer",
+                [
+                    ImportExpression(importer="mypackage.brown", imported="someotherpackage"),
+                ],
+                DIRECT_IMPORTS[6:8],
+            ),
         ],
     )
     def test_succeeds(self, description, expressions, expected):