Fix InlineModelAdmin parent-object method signatures#3308
Conversation
| @property | ||
| @override | ||
| def view_on_site(self) -> Callable[[_ParentModelT], str | None] | bool: ... |
There was a problem hiding this comment.
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
| _ParentModelT = TypeVar("_ParentModelT", bound=Model) | ||
|
|
||
| class InlineModelAdmin(BaseModelAdmin[_ChildModelT], Generic[_ChildModelT, _ParentModelT]): | ||
| class InlineModelAdmin(BaseModelAdmin[Any], Generic[_ChildModelT, _ParentModelT]): |
There was a problem hiding this comment.
Why the change from _ChildModelT to Any ?
Can it cause issue with get_sortable_by ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @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: ... |
There was a problem hiding this comment.
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 view_on_site(self, obj: "A") -> str: | ||
| return "http://example.org" | ||
| - case: test_inline_get_readonly_fields_parent_obj |
There was a problem hiding this comment.
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 !
2 participants
PR Summary
I fixed the InlineModelAdmin.get_readonly_fields typing issue from #3224 and added regression tests for it. The new test covers StackedInline[Child, Parent] with obj: Parent | None, and I also added a negative case to make sure wrong obj types are still rejected.
I ended up using BaseModelAdmin[Any] in InlineModelAdmin because I couldn’t find a cleaner way to satisfy mypy override checks while keeping compatibility and preserving Django’s inline parent-object semantics. I’m not fully happy with this approach, and I’m still not 100% sure Any can’t leak into some edge paths and affect typing somewhere unexpected. I tried to reduce that risk by explicitly typing the important inline methods and adding tests, but I’d definitely welcome a better alternative.