Code

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#17305 closed Cleanup/optimization (invalid)

Cache middleware keys too difficult to customize

Reported by: subsume Owned by:
Component: Core (Cache system) Version:
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Attached is a patch which brings many otherwise-unused "utils.cache" utils under the middleware method as classmethods. This is to provide easy hooks so that people can make their own cache key structures.

In case anyone cares, the problem I ran into was that nginx couldn't digest the default cache keys which that middlware creates as they were needlessly complex.

Attachments (2)

diff (2) (23.1 KB) - added by subsume 2 years ago.
17305.diff (23.1 KB) - added by ptone 2 years ago.
adding extension to initial diff for proper formatting

Download all attachments as: .zip

Change History (9)

Changed 2 years ago by subsume

Changed 2 years ago by ptone

adding extension to initial diff for proper formatting

comment:1 Changed 2 years ago by ptone

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

unused in Django itself doesn't mean that other people aren't making use of those util functions in other code in the form of custom cache middleware - this is a rather drastic and backwards incompatible refactor

What problems would you have had in solving your problem if you had simply chosen to subclass the existing middleware and modify it to suit your needs?

comment:2 Changed 2 years ago by subsume

I had thought of that and I'm open to a patch that makes things backwards compatible--I didn't know how to approach that given the drasticness as you say. Still, I hope that drastic changes aren't a blocker to giving people useful hooks. Also, I had to update the tests and found them to not be difficult to adapt as the classmethods can still be used separately.

Subclassing the existing middleware wouldn't have been fruitful at all. If you take a look at them, the internals are entrenched by these "utils". It is the organization of these utils that makes these middlewares un-subclassable (usefully anyway).

comment:3 Changed 2 years ago by subsume

Also I think there's an oversight in the Fetch middleware:

https://code.djangoproject.com/browser/django/trunk/django/middleware/cache.py#L133

Perhaps someone can inform me if I am being ignorant, but the Fetch middleware seems to make no reference to settings.CACHE_MIDDLEWARE_UNAUTHENTICATED_ONLY before creating a connection to cache and checking for the value. The only reason it happens to work in the current setup is because the Update happens to skip creation of a key. So even though I have explicitly set UNUAUTH=True, when I use a key that isn't generated by the above utils, it will result in my auth users getting a cached version.

Began ticket about this #17313

comment:4 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Design decision needed

comment:5 Changed 2 years ago by subsume

  • Owner subsume deleted

FWIW I abandoned this. Nobody besides me minds how its setup now, my related ticket #17313 was closed with a nonsense explanation, and the more I dig into the CACHE_MIDDLEWARE_ANONYMOUS_ONLY the more I realize that setting is totally misleading to begin with and has absolutely nothing to do with anonymous users.

comment:6 Changed 2 years ago by aaugustin

  • Resolution set to invalid
  • Status changed from new to closed

Closing ticket since the reporter has lost interest and I'm skeptical about the idea. I'd need a more detailed proposal, and analysis of the consequence wrt. backwards compatibility, to validate it.

comment:7 Changed 2 years ago by subsume

The idea was making the cache keys which django creates digestible to other services but django is such a mess in this regard its just easier to duplicate my cache entirely for the sake of letting something else access it.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.