Proposal: Make ModelSerializer generic on Model

[antonagestam]

We currently end up having to add e.g. assert isinstance(instance, SomeModel) to update methods. I think it would make sense for ModelSerializer to be defined as approximately:

M = TypeVar("M", bound=Model)

class ModelSerializer(Serializer, Generic[M]):
    class Meta:
        model: ClassVar[M]

    def update(self, instance: M, validated_data: Any) -> M:
        ...

When we narrow the instance argument to update() ourselves, we violate LSP because update() is defined in BaseSerializer and instance need to accept any Model. I think it would be better if that LSP violation was moved into djangorestframework-stubs to paint over the upstream design decision and keeping it from leaking into "user space".

[sobolevn]

I absolutely agree!

@antonagestam are you willing to work on this? It would be awesome addition!

[antonagestam]

@sobolevn Awesome! 👍 I'd be willing to work on this, I'll try to find the time in the coming weeks :)

[sobolevn]

Very appreciated! 👍

[antonagestam]

Update: I started working on this but ran into a few issues. I had anticipated that mypy could bind a generic typevar in an outer class using a classvar in a nested class, but that doesn't seem to be the case. Will have to spend some more time to this to see if it can be worked around using a protocol.

[antonagestam]

As type arguments seem to be the only way to bind type vars I now think this will not be feasible without an upstream change to implement __class_getitem__. I might create a PR with django-rest-framework to pursuit that.

[Goldziher]

Done @sobolevn :)

[sobolevn]

@Goldziher let's add tests as well! This stuff is important!

[Goldziher]

Sure, but that is a different issue.

[martinlehoux]

Hi there! Is there something I can do to help with this issue? It's not critical to me, but I ran into it and I'd to see Django well typed!

[sobolevn]

@martinlehoux it is already generic: https://github.com/typeddjango/djangorestframework-stubs/blob/master/rest_framework-stubs/serializers.pyi#L214

But, if you wish to help, then you can add typetests for this feature! 👍 Thanks!

[martinlehoux]

Ok I think I narrowed down what I really wanted to achieve. I guess I expected:

class UserSerializer(ModelSerializer):
    class Meta:
        model = User

to be enough, that Meta.model would do the typing part for the whole class, but now I'm not so sure it's possible, and I guess I need to do that:

class UserSerializer(ModelSerializer["User"]):
    class Meta:
        model = User

Have you already thought about this feature? I'm not 100% sure but that's what I can read in the previous comments

[sobolevn]

I don't think that this is possible 🤔

Because type inference does not work this way in mypy. When you specify class UserSerializer(ModelSerializer): what you really write is class UserSerializer(ModelSerializer[Any]):

And it stops inference from any further steps.

[antonagestam]

That's the same conclusion I came to as well. Don't know if this would be possible with a plugin, but for me the requirement to specify the model twice is OK.

Perhaps one can use a class decorator that sets the model on Meta and is annotated like model: M -> ModelSerializer[M] to get rid of the duplication.

[w0rp]

One issue I've noticed with the type of ModelSerializer is that class Meta is already defined in the type, and you get type errors when you write class Meta: in serializer files with Pyright in strict mode without # type: ignore. It should be possible to fix that by defining Meta with a type annotation instead of with class.