blb-ventures / strawberry-django-plus

Enhanced Strawberry GraphQL integration with Django
MIT License
179 stars 47 forks source link

Use a type's get_queryset for Relay connections #215

Closed moritz89 closed 1 year ago

moritz89 commented 1 year ago

Why:

This change addresses the need by:

Closes #214

codecov-commenter commented 1 year ago

Codecov Report

Merging #215 (810d3fd) into main (19ab9cf) will decrease coverage by 0.03%. The diff coverage is 81.81%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #215      +/-   ##
==========================================
- Coverage   90.31%   90.28%   -0.03%     
==========================================
  Files          45       45              
  Lines        4337     4345       +8     
==========================================
+ Hits         3917     3923       +6     
- Misses        420      422       +2     
Impacted Files Coverage Δ
strawberry_django_plus/type.py 89.15% <81.81%> (-0.56%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

moritz89 commented 1 year ago
  1. I updated the coverage report to exclude tests from being counted towards test coverage as this is a bit disingenuous.
  2. I added the debugpy dependency to the dev group as it is really useful for debugging in VS Code. Might be an idea to add integration with a cli flag in another MR
moritz89 commented 1 year ago

One open point is having to warm up the user object in the middleware to avoid async / sync collisions. Don't know if this is the best approach. In my ASGI project I've added a middleware to warm it up before entering the GQL handlers

bellini666 commented 1 year ago
  1. I updated the coverage report to exclude tests from being counted towards test coverage as this is a bit disingenuous.

Awesome! :)

  1. I added the debugpy dependency to the dev group as it is really useful for debugging in VS Code. Might be an idea to add integration with a cli flag in another MR

I'm not a vscode user (I use neovim), but AFAIK it should actually use it from an internal venv. But I have nothing against addiing debugpy as a dev dependency :)

One open point is having to warm up the user object in the middleware to avoid async / sync collisions. Don't know if this is the best approach. In my ASGI project I've added a middleware to warm it up before entering the GQL handlers

I saw that and I was going to actually mention that in the code review =P

I think a better solution would be to also write a middleware for the demo project, which is also the ones that will be used by tests. Otherwise you would be forcing everyone using this lib's debug-toolbar implementation to also "warm up" the user object even if they are not going to use it.

Probably something like this, and add this after the authentication middleware.

@sync_and_async_middleware
def user_warmup_middleware(
    get_response: Callable[[HttpRequest], HttpResponse | Coroutine[Any, Any, HttpResponse]],
):
    if inspect.iscoroutinefunction(get_response):

        async def middleware(request):  # type: ignore
            # Warm up user object in sync context
            await sync_to_async(getattr)(request, "user")
            return await cast(Awaitable, get_response(request))
    else:

        def middleware(request):
            # Warm up user object in sync context
            request.user.is_anonymous  # noqa: B018
            return get_response(request)

    return middleware

Btw, the main issue here is that the get_queryset method currently doesn't support being called asynchronously. Otherwise you could do await sync_to_async(getattr)(info.context.request, "user")

On my projects I actually define a custom context (inherited from StrawberryDjangoContext) to provide a get_user and an async aget_user to retrieve it inside resolvers, and also to improve the typing for my custom user instead of AbstractBaseUser.

Commenting as a suggestion for your projects if that makes sense:

class HttpRequest(_HttpRequest):
    user: "User | AnonymousUser"

class HttpResponse(_HttpResponse):
    ...

@dataclasses.dataclass
class Context(StrawberryDjangoContext):
    request: HttpRequest
    response: HttpResponse | None

    @overload
    def get_user(self, *, required: Literal[True]) -> "User":
        ...

    @overload
    def get_user(self, *, required: None = ...) -> "User | None":
        ...

    def get_user(self, *, required: Literal[True] | None = None) -> "User | None":
        user = self.request.user

        if not user or not user.is_authenticated or not user.is_active:
            if required:
                raise PermissionDenied(_("No user logged in"))

            return None

        return cast("User", user)

    @overload
    async def aget_user(self, *, required: Literal[True]) -> "User":
        ...

    @overload
    async def aget_user(self, *, required: None = ...) -> "User | None":
        ...

    async def aget_user(self, *, required: Literal[True] | None = None) -> "User | None":
        return await sync_to_async(self.get_user)(required=required)

But as I mentioned above, that unfortunately won't work for get_queryset right now

moritz89 commented 1 year ago

Added the changes. Hopefully I dont't stumble upon anything else :smile: