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

Refs #31949 -- Made @never_cache and @cache_control() decorators to work with async functions. #16436

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

LomaxOnTheRun
Copy link
Contributor

@LomaxOnTheRun LomaxOnTheRun commented Jan 8, 2023

Reference to #31949.

This PR makes the cache_control and never_cache view decorators able to handle both sync and async views.

django/views/decorators/cache.py Outdated Show resolved Hide resolved
tests/decorators/tests.py Outdated Show resolved Hide resolved
django/views/decorators/cache.py Outdated Show resolved Hide resolved
django/views/decorators/cache.py Outdated Show resolved Hide resolved
django/views/decorators/cache.py Outdated Show resolved Hide resolved
@LomaxOnTheRun LomaxOnTheRun force-pushed the async-cache-view-decorators branch 2 times, most recently from 3095e0d to f247582 Compare March 26, 2023 15:23
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hey @LomaxOnTheRun.

Thanks for the updates here.

Apart from the @sync_and_async_middleware topic, I like this now.

I'm going to need to ask @felixxm to carry it over the line. (I'll still be around but I'm stepping down as Fellow.)

  • Can you squash and rebase on latest main?
  • The moved tests: can that be done in a separate first commit, so it's easy to see and verify?
  • Docs changes/release notes needed.

I'm going to check the patch needs improvement and needs documentation flags on the ticket. (Or make sure they're checked.) Once you've addressed the bullets, can you make sure to uncheck those flags, so the ticket will appear again on the review queue?

Thanks again! Good hustle! 🏅

@LomaxOnTheRun LomaxOnTheRun force-pushed the async-cache-view-decorators branch 5 times, most recently from 601d832 to 5bc9c9f Compare April 2, 2023 17:56
@LomaxOnTheRun
Copy link
Contributor Author

LomaxOnTheRun commented Apr 2, 2023

@carltongibson thank you for the review, and for all of the reviews and help you've given all the way to this point. I genuinely appreciate your time and effort 😊

@felixxm hiya 👋 I've updated the PR so that:

  • It's rebased on the latest main.
  • All of the moved tests are in a single commit at the start of the PR.
  • Release notes and docs have been updated.

It's worth noting that this commit has also been a large change. It alters make_middleware_decorator so that it can also handle sync and async views, using the same pattern as was used for the other (much simpler) cache decorators in this PR. This is required for the cache_page decorator to handle sync and async views.

I would suggest that it's worth splitting out the make_middleware_decorator commit out into its own PR, since it's a more complex update which also impacts a number of other decorators (csrf_protect, requires_csrf_token, ensure_csrf_cookie, gzip_page and conditional_page).

@ntachukwu
Copy link
Contributor

Hello @LomaxOnTheRun , Carlton mentioned this issue to me and I think this is a really cool feature and would love an opportunity to contribute to it. I am still studying your progress and I see you have cache covered with this PR. I want to take up one of the other decorators like http. I can totally handle a different decorator if you already have this covered also.

You've made great progress with this issue and it's lovely to see. Awesome 🤩

@LomaxOnTheRun
Copy link
Contributor Author

Hiya @Th3nn3ss 👋 I'm very happy to share out the decorators, though it would probably be useful for me to give a summary of the journey so far:

  • I originally wrote this PR to convert all builtin decorators in one go.
  • This required a lot of work every time a change was requested, as it had to be applied to ~20 commits.
  • We decided it would be better to use a single PR (this one) to figure out "the best pattern", and then create other PRs that would follow that pattern for the other decorators.

Practically, this means that I would recommend waiting until this PR is approved before diving in with other decorators, to avoid having multiple PRs being reviewed / changed for the same things. Obviously this is ultimately at @felixxm's discretion.

Once you do pick up another set of decorators, the http ones aren't a bad shout, as they don't touch on the decorator_from_middleware logic which I think is going to be more complex. You might want to use the tests from the require_http_methods commit and the other http decorators commit to save yourself some time and effort 🙂

I've slimmed this PR down to just the cache control and never_cache decorators to allow us to get it merged faster, and to allow other people to pick up decorators to work on. I'll push up a separate PR for a proposed solution to the decorator_from_middleware decorators soon, as I think that one will require a little more discussion.

@LomaxOnTheRun
Copy link
Contributor Author

I've updated the docs to say this change is for version 5.0 (which I believe is the next planned version), not 4.2.

docs/releases/4.2.txt Outdated Show resolved Hide resolved
tests/decorators/test_cache_decorators.py Outdated Show resolved Hide resolved
@felixxm felixxm self-assigned this Apr 17, 2023
@felixxm felixxm changed the title Refs #31949 -- Async cache view decorators Refs #31949 -- Made @never_cache and @cache_control() decorators to work with async functions. Apr 21, 2023
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@LomaxOnTheRun Thanks for all your efforts 👍 I squashed and reorganized commits, and pushed final edits.

django/views/decorators/cache.py Outdated Show resolved Hide resolved
tests/decorators/test_cache.py Outdated Show resolved Hide resolved
django/views/decorators/cache.py Outdated Show resolved Hide resolved
django/views/decorators/cache.py Outdated Show resolved Hide resolved
@felixxm felixxm force-pushed the async-cache-view-decorators branch 3 times, most recently from ce4cfca to 6e443f8 Compare April 21, 2023 11:16
@felixxm felixxm force-pushed the async-cache-view-decorators branch from 6e443f8 to 573e2b7 Compare April 21, 2023 11:27
@carltongibson
Copy link
Member

Thanks both 🎁. I didn't check everything in depth but this looks roughly how I was hoping/would expect, so 👍

@felixxm
Copy link
Member

felixxm commented Apr 22, 2023

@LomaxOnTheRun Do you want to take a look at edits?

@felixxm felixxm force-pushed the async-cache-view-decorators branch from 573e2b7 to 4dfc6ff Compare April 25, 2023 08:08
@felixxm felixxm merged commit 4dfc6ff into django:main Apr 25, 2023
26 checks passed
@LomaxOnTheRun
Copy link
Contributor Author

@felixxm @carltongibson apologies for my radio silence recently, I've been absolutely slammed over the last week or so. More importantly, a huge thank you to you both for pushing this over the line 😄

Now we've established an acceptable pattern, I'll push up another few PRs to cover the other decorators. I think the function based ones should be relatively easy to go through now, it's primarily the make_middleware_decorator ones that are quite different and I suspect will require more thought 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants