Fix admin_register to avoid rewriting duplicate ModelAdmin definitions (

#472)

Fixes #471.

CHANGELOG.rst

@@ -2,6 +2,10 @@
 Changelog
 =========
 
+* Fix the ``admin_register`` fixer to avoid rewriting when there are duplicate ``ModelAdmin`` classes in the file.
+
+  `Issue #471 <https://github.com/adamchainz/django-upgrade/issues/471>`__.
+
 1.19.0 (2024-06-27)
 -------------------
 

src/django_upgrade/fixers/admin_register.py

@@ -45,7 +45,9 @@ def __init__(self, parent: ast.AST, lineno: int) -> None:
         self.model_names_per_site: dict[str, set[str]] = {}
 
 
-decorable_admins: MutableMapping[State, dict[str, AdminDetails]] = WeakKeyDictionary()
+decorable_admins: MutableMapping[State, dict[str, AdminDetails | None]] = (
+    WeakKeyDictionary()
+)
 # Name of site to set of unregistered model names, or True if potentially all
 # models have been unregistered
 unregistered_site_models: MutableMapping[State, dict[str, set[str] | Literal[True]]] = (
@@ -67,9 +69,14 @@ def visit_ClassDef(
     parents: tuple[ast.AST, ...],
 ) -> Iterable[tuple[Offset, TokenFunc]]:
     if _is_django_admin_imported(state) and not uses_full_super_in_init_or_new(node):
-        decorable_admins.setdefault(state, {})[node.name] = AdminDetails(
-            parents[-1], node.lineno
-        )
+        admin_detailses = decorable_admins.setdefault(state, {})
+        if node.name in admin_detailses:
+            # Duplicate name, ignore
+            admin_detailses[node.name] = None
+            return
+        else:
+            admin_detailses[node.name] = AdminDetails(parents[-1], node.lineno)
+
         if not node.decorator_list:
             offset = ast_start_offset(node)
             decorated = False
@@ -125,7 +132,7 @@ def update_class_def(
     tokens: list[Token], i: int, *, name: str, state: State, decorated: bool
 ) -> None:
     admin_details = decorable_admins.get(state, {})[name]
-    if not admin_details.model_names_per_site:
+    if admin_details is None or not admin_details.model_names_per_site:
         return
 
     if decorated:
@@ -221,7 +228,12 @@ def visit_Call(
                 admin_details.model_names_per_site.setdefault(site_name, set()).update(
                     model_names
                 )
-                yield ast_start_offset(node), partial(erase_node, node=parents[-1])
+                yield ast_start_offset(parents[-1]), partial(
+                    remove_register,
+                    name=admin_name,
+                    state=state,
+                    node=parents[-1],
+                )
         elif node.func.attr == "unregister" and (
             (  # admin.site.unregister(...)
                 isinstance(node.func.value, ast.Attribute)
@@ -266,6 +278,16 @@ def visit_Call(
                     existing_names.update(unregistered_names)
 
 
+def remove_register(
+    tokens: list[Token], i: int, *, name: str, state: State, node: ast.AST
+) -> None:
+    admin_details = decorable_admins.get(state, {})[name]
+    if admin_details is None:
+        return
+
+    erase_node(tokens, i, node=node)
+
+
 site_definitions: MutableMapping[ast.Module, dict[str, int | None]] = (
     WeakKeyDictionary()
 )

tests/fixers/test_admin_register.py

@@ -65,6 +65,20 @@ class AuthorAdmin(admin.ModelAdmin):
     )
 
 
+def test_register_assigned():
+    check_noop(
+        """\
+        from django.contrib import admin
+        from myapp.models import Author
+
+        class AuthorAdmin(admin.ModelAdmin):
+            pass
+
+        value = admin.site.register(Author, AuthorAdmin)
+        """,
+    )
+
+
 def test_py2_style_init_super():
     check_noop(
         """\
@@ -669,6 +683,75 @@ class MyCustomAdmin:
     )
 
 
+def test_duplicate_model_admins_register_one():
+    check_noop(
+        """\
+        from django.contrib import admin
+
+        class BookAdmin(admin.ModelAdmin):
+            pass
+
+        admin.site.register(Book, BookAdmin)
+
+        class BookAdmin(admin.ModelAdmin):
+            pass
+        """,
+    )
+
+
+def test_duplicate_model_admins_register_two():
+    check_noop(
+        """\
+        from django.contrib import admin
+
+        class BookAdmin(admin.ModelAdmin):
+            pass
+
+        class BookAdmin(admin.ModelAdmin):
+            pass
+
+        admin.site.register(Book, BookAdmin)
+        """,
+    )
+
+
+def test_duplicate_model_admins_with_intermediate():
+    check_transformed(
+        """\
+        from django.contrib import admin
+
+        class BookAdmin(admin.ModelAdmin):
+            pass
+
+        class DuckAdmin(admin.ModelAdmin):
+            pass
+
+        admin.site.register(Duck, DuckAdmin)
+
+        class BookAdmin(admin.ModelAdmin):
+            pass
+
+        admin.site.register(Book, BookAdmin)
+        """,
+        """\
+        from django.contrib import admin
+
+        class BookAdmin(admin.ModelAdmin):
+            pass
+
+        @admin.register(Duck)
+        class DuckAdmin(admin.ModelAdmin):
+            pass
+
+
+        class BookAdmin(admin.ModelAdmin):
+            pass
+
+        admin.site.register(Book, BookAdmin)
+        """,
+    )
+
+
 def test_complete():
     check_transformed(
         """\