adamchainz / django-cors-headers

Django app for handling the server headers required for Cross-Origin Resource Sharing (CORS)
MIT License
5.38k stars 536 forks source link

check_request_enabled.send() should not be called from an async context #915

Open Ariki opened 10 months ago

Ariki commented 10 months ago

Understanding CORS

Python Version

3.11

Django Version

4.2

Package Version

4.3.1

Description

The current CorsMiddleware implementation can work both in sync and async mode. The issue is that even in async mode, it uses Django signal's send method which isn't intended to be used from an async context.

This kind of works in simple cases but you cannot use Django ORM functions from a signal handler called in an asynchronous context.

Starting from version 5.0, Django supports asynchronous signal handlers and introduces the asend method which can be used to send signals from asynchronous code.

So I believe the correct approach would be as follows:

Some refactoring is needed for this to work, as await can be used only in async methods.

adamchainz commented 10 months ago

There’s no edict that the synchronous send() function should not be called from an asynchronous context. It’s legitimate but it does prevent the receivers from being asynchronous.

I don’t think we can remove the call as that is backwards incompatible. Maybe we can add an asend() call, though that would require significant restructuring.

the signal is my least favourite feature. It was added before I took over maintenance and I haven’t removed it due to backwards compatibility concerns. In most cases I think subclassjng the middleware makes more sense, though it’s not documented or well structured. If you have a specific use case, perhaps we can find a way to support that through subclassing.

Ariki commented 10 months ago

It’s legitimate but it does prevent the receivers from being asynchronous.

It also prevents the receivers from accessing the database (which is my use case; my code was broken after upgrading from an older version of django-cors-headers).

But I'm ok with subclassing. My only concern is that I had to copy&paste the whole add_response_headers method into my subclass just to make its async version and be able to use await in the middle of it.

adamchainz commented 1 week ago

I'd be open to proposed restructuring to make subclassing easier, but I don't have time to look at it.