Skip to content
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.

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

Update admin.sites._ViewType bound to allow StreamingHttpResponse #2256

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Update admin.sites._ViewType bound to allow StreamingHttpResponse #2256

merged 2 commits into from
Jul 9, 2024

Conversation

savanto
Copy link
Contributor

@savanto savanto commented Jul 8, 2024

Issue

From the Django documentation:

The StreamingHttpResponse is not a subclass of HttpResponse ...

Because of this, creating an admin view that returned a StreamingHttpResponse was causing the following Mypy error:

testing/admin.py:14: error: Value of type variable "_ViewType" of "admin_view" of "AdminSite" cannot be "Callable[[HttpRequest], StreamingHttpResponse]"  [type-var]

Changes

This updates the admin _ViewType type-var bound to allow a Callable that can return a union of HttpResponse | StreamingHttpResponse HttpResponseBase.

After this update to the stubs file, the Mypy error is no longer reproducible.


I was not able to find any open issues related to this. This change can be tested using the following code snippets:

# testing/models.py
from django.db import models


class Model(models.Model):
    pass
# testing/admin.py
from __future__ import annotations

from django.contrib import admin
from django.http import HttpRequest, StreamingHttpResponse
from django.urls import URLPattern, path

from testing.models import Model


@admin.register(Model)
class Admin(admin.ModelAdmin[Model]):
    def get_urls(self) -> list[URLPattern]:
        return [
            path("_stream/", self.admin_site.admin_view(self.get_stream), name="get_stream"),
            *super().get_urls(),
        ]

    def get_stream(self, _request: HttpRequest) -> StreamingHttpResponse:
        return StreamingHttpResponse(("a", "b", "c"))

Testing

I tested this manually by running Mypy before and after the change.

I was not able to run pytest successfully before or after the change (388 failures with ModuleNotFoundError: No module named 'scripts').

I was not able to run stubtest.sh successfully before or after the change (1928 failures with unused allowlist entry ...).

All pre-commit run --all-files checks passed.

From the [Django documentation](https://docs.djangoproject.com/en/5.0/ref/request-response/#streaminghttpresponse-objects):
> The `StreamingHttpResponse` is not a subclass of `HttpResponse` ...

Because of this, creating an admin view that returned a
`StreamingHttpResponse` was causing the following Mypy error:

```
testing/admin.py:14: error: Value of type variable "_ViewType" of "admin_view" of "AdminSite" cannot be "Callable[[HttpRequest], StreamingHttpResponse]"  [type-var]
```

This updates the admin `_ViewType` type-var bound to allow a Callable
that can return a union of `HttpResponse | StreamingHttpResponse`.

After this update to the stubs file, the Mypy error is no longer
reproducible.
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

@sobolevn sobolevn merged commit 7573e2e into typeddjango:master Jul 9, 2024
36 checks passed
@savanto savanto deleted the admin-viewtype-callable-return-streaming branch July 9, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants