Skip to content

Fix InlineModelAdmin parent-object method signatures #3308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MyGodIsHe wants to merge 2 commits into
base: master
Choose a base branch
from
+67 −4
Open

Fix InlineModelAdmin parent-object method signatures #3308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99504e1
Fix InlineModelAdmin parent-object method signatures
MyGodIsHe Apr 14, 2026
2bc3eff
fix unused type: ignore
MyGodIsHe Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions django-stubs/contrib/admin/options.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,9 @@ class ModelAdmin(BaseModelAdmin[_ModelT]):
_ChildModelT = TypeVar("_ChildModelT", bound=Model)
_ParentModelT = TypeVar("_ParentModelT", bound=Model)

class InlineModelAdmin(BaseModelAdmin[_ChildModelT], Generic[_ChildModelT, _ParentModelT]):
class InlineModelAdmin(BaseModelAdmin[Any], Generic[_ChildModelT, _ParentModelT]):
Copy link
Copy Markdown
Contributor

@UnknownPlatypus UnknownPlatypus May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change from _ChildModelT to Any ?

Can it cause issue with get_sortable_by ?

Copy link
Copy Markdown
Author

@MyGodIsHe MyGodIsHe May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because with _ChildModelT, this case is incompatible with the base class signature.

If we keep class InlineModelAdmin(BaseModelAdmin[_ChildModelT], ...) then in BaseModelAdmin, the method get_readonly_fields is typed as obj: _ChildModelT | None.

However, for a real Django inline, it needs to be obj: _ParentModelT | None.

There is another option: introduce an additional type parameter.

_ObjT = TypeVar("_ObjT", bound=Model, default=_ModelT)

class BaseModelAdmin(Generic[_ModelT, _ObjT]): ...

class InlineModelAdmin(
    BaseModelAdmin[_ChildModelT, _ParentModelT],
    Generic[_ChildModelT, _ParentModelT],
): ...

This approach is cleaner, but it changes the public API. What do you think?

Copy link
Copy Markdown
Contributor

@UnknownPlatypus UnknownPlatypus May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach is cleaner

I'm not sure it is, the notion of Child/Parent leak to BaseModelAdmin which does not have to know about it and I think it might be confusing for users to have 2 TypeVar on ModelAdmin while it make sense on InlineModelAdmin, maybe your approach with Any is correct but it does mean we have to redefine every method that would have used _ChildModelT in BaseModelAdmin right ?

Copy link
Copy Markdown
Author

@MyGodIsHe MyGodIsHe May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right.

Copy link
Copy Markdown
Author

@MyGodIsHe MyGodIsHe May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the second approach, _ObjT has a default (_ObjT = _ModelT), so existing one-parameter usages should continue to work.
The model becomes more complex, but it matches Django’s real semantics: InlineModelAdmin is child-model based, while obj in many hooks is parent-model based.

Copy link
Copy Markdown
Contributor

@UnknownPlatypus UnknownPlatypus May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still feels off to me to declare a typevar in BaseModelAdmin that we don't use in the class directly.
I think the version with Any is better, just need to make sure we redefine every method with the new typevars

model: type[_ChildModelT]
form: type[forms.ModelForm[_ChildModelT]]
fk_name: str | None
formset: type[BaseInlineFormSet[_ChildModelT, _ParentModelT, forms.ModelForm[_ChildModelT]]]
extra: int
Expand All @@ -324,15 +325,36 @@ class InlineModelAdmin(BaseModelAdmin[_ChildModelT], Generic[_ChildModelT, _Pare
self, request: HttpRequest, obj: _ParentModelT | None = ..., **kwargs: Any
) -> type[BaseInlineFormSet[_ChildModelT, _ParentModelT, forms.ModelForm[_ChildModelT]]]: ...
@override
def get_exclude(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> _ListOrTuple[str] | None: ...
@override
def get_fields(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> _FieldGroups: ...
@override
def get_fieldsets(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> _FieldsetSpec: ...
@override
def get_inlines(self, request: HttpRequest, obj: _ParentModelT | None) -> _ListOrTuple[type[InlineModelAdmin]]: ...
@override
def get_readonly_fields(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> _ListOrTuple[str]: ...
@override
def get_prepopulated_fields(
self, request: HttpRequest, obj: _ParentModelT | None = ...
) -> dict[str, Sequence[str]]: ...
@override
def has_view_or_change_permission(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> bool: ...
@override
def get_view_on_site_url(self, obj: _ParentModelT | None = ...) -> str | None: ...
@property
@override
def view_on_site(self) -> Callable[[_ParentModelT], str | None] | bool: ...
Comment on lines +345 to +347
Copy link
Copy Markdown
Contributor

@UnknownPlatypus UnknownPlatypus May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are class level view_on_site = False still accepted ?

This can be a boolena or a method https://docs.djangoproject.com/en/6.0/ref/contrib/admin/#django.contrib.admin.ModelAdmin.view_on_site

Comment on lines +343 to +347
Copy link
Copy Markdown
Contributor

@UnknownPlatypus UnknownPlatypus May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these two take the child instance, see https://github.com/django/django/blob/50bbf71bbd51616e2ce48785336ca3746fbe5f24/django/contrib/admin/helpers.py#L346-L359

@override
def get_queryset(self, request: HttpRequest) -> QuerySet[_ChildModelT]: ...
@override
def has_add_permission(self, request: HttpRequest, obj: _ParentModelT | None) -> bool: ... # type: ignore[override]
@override
def has_change_permission(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> bool: ... # type: ignore[override]
def has_change_permission(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> bool: ...
@override
def has_delete_permission(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> bool: ... # type: ignore[override]
def has_delete_permission(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> bool: ...
@override
def has_view_permission(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> bool: ... # type: ignore[override]
def has_view_permission(self, request: HttpRequest, obj: _ParentModelT | None = ...) -> bool: ...

class StackedInline(InlineModelAdmin[_ChildModelT, _ParentModelT]):
template: str
Expand Down
41 changes: 41 additions & 0 deletions tests/typecheck/contrib/admin/test_options.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,47 @@
@override
def view_on_site(self, obj: "A") -> str:
return "http://example.org"
- case: test_inline_get_readonly_fields_parent_obj
Copy link
Copy Markdown
Contributor

@UnknownPlatypus UnknownPlatypus May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just migrated the rest of this file test to the tests/assert_type folder so that it also checks ty/pyrefly/pyright.

Could you rebase on master and put these two test case in the file tests/assert_type/contrib/admin/test_options.py ?

Thanks !

main: |
from django.contrib import admin
from django.db import models
from django.http.request import HttpRequest
from typing_extensions import override

class A(models.Model):
pass

class B(models.Model):
a = models.ForeignKey(A, on_delete=models.CASCADE)

class I(admin.StackedInline[B, A]):
model = B

@override
def get_readonly_fields(
self, request: HttpRequest, obj: A | None = None
) -> list[str] | tuple[str, ...]:
return super().get_readonly_fields(request, obj)
- case: errors_for_inline_parent_obj_methods_invalid_obj_type
main: |
from django.contrib import admin
from django.db import models
from django.http.request import HttpRequest

class A(models.Model):
pass

class B(models.Model):
a = models.ForeignKey(A, on_delete=models.CASCADE)

class I(admin.StackedInline[B, A]):
model = B

req = HttpRequest()
inline = I(A, admin.AdminSite())
inline.get_fields(req, 1) # E: Argument 2 to "get_fields" of "InlineModelAdmin" has incompatible type "int"; expected "A | None" [arg-type]
inline.get_inlines(req, 1) # E: Argument 2 to "get_inlines" of "InlineModelAdmin" has incompatible type "int"; expected "A | None" [arg-type]
inline.get_readonly_fields(req, 1) # E: Argument 2 to "get_readonly_fields" of "InlineModelAdmin" has incompatible type "int"; expected "A | None" [arg-type]
- case: errors_on_omitting_fields_from_fieldset_opts
main: |
from django.contrib import admin
Expand Down
Loading