Code

Opened 3 years ago

Closed 3 years ago

#16003 closed Bug (fixed)

Admin incompatible with USE_ETAGS = True

Reported by: lamby Owned by: nobody
Component: contrib.admin Version: master
Severity: Release blocker Keywords: regression
Cc: pterk@…, real.human@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by kmtracey)

In SVN, the admin does not work with USE_ETAGS = True:

Environment:

Traceback:
File "/srv/www.example.com/example/packages/django/core/handlers/base.py" in get_response
  111.                         response = callback(request, *callback_args, **callback_kwargs)
File "/srv/www.example.com/example/packages/django/contrib/admin/sites.py" in wrapper
  213.                 return self.admin_view(view, cacheable)(*args, **kwargs)
File "/srv/www.example.com/example/packages/django/utils/decorators.py" in _wrapped_view
  91.                     response = view_func(request, *args, **kwargs)
File "/srv/www.example.com/example/packages/django/views/decorators/cache.py" in _wrapped_view_func
  77.         response = view_func(request, *args, **kwargs)
File "/srv/www.example.com/example/packages/django/contrib/admin/sites.py" in inner
  196.             return view(request, *args, **kwargs)
File "/srv/www.example.com/example/packages/django/views/decorators/cache.py" in _wrapped_view_func
  78.         add_never_cache_headers(response)
File "/srv/www.example.com/example/packages/django/utils/cache.py" in add_never_cache_headers
  116.     patch_response_headers(response, cache_timeout=-1)
File "/srv/www.example.com/example/packages/django/utils/cache.py" in patch_response_headers
  105.         response['ETag'] = '"%s"' % hashlib.md5(response.content).hexdigest()
File "/srv/www.example.com/example/packages/django/template/response.py" in _get_content
  110.             raise ContentNotRenderedError('The response content must be rendered before it can be accessed.')

Exception Type: ContentNotRenderedError at /a/admin/
Exception Value: The response content must be rendered before it can be accessed.

Attachments (2)

16003.patch (873 bytes) - added by pterk 3 years ago.
A preliminary patch that fixes the issue in the admin. Needs more work for an alround solution. Also regression tests
16003.2.patch (7.6 KB) - added by pterk 3 years ago.
An updated patch /w tests

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by kmtracey

  • Description modified (diff)
  • Keywords regression added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

Fixed formatting, please use preview. I suspect this is due to r16087. It does not happen with 1.3 release. There's also a likely-related thread on django-developers: http://groups.google.com/group/django-developers/browse_thread/thread/f96e982254fbe5c3

comment:2 Changed 3 years ago by jezdez

  • Component changed from Uncategorized to contrib.admin

Yeah, this seems like a rather major issue with the TemplateResponse feature, which I think can only be fixed by actually rendering the content if needed instead of raising a ContentNotRenderedError exception.

With proper documentation of which middlewares/decorator modify the content we should be able to keep the usefulness of TemplateResponse up.

comment:3 Changed 3 years ago by pterk

  • Needs tests set
  • Patch needs improvement set
  • UI/UX unset

I've got a preliminary fix for this. I'll be at the djangocon sprint in A'dam tomorrow from ~10.00 on to continue work on this.

Changed 3 years ago by pterk

A preliminary patch that fixes the issue in the admin. Needs more work for an alround solution. Also regression tests

comment:4 Changed 3 years ago by pterk

  • Cc pterk added

Changed 3 years ago by pterk

An updated patch /w tests

comment:5 Changed 3 years ago by pterk

  • Cc pterk removed
  • Has patch set
  • Needs tests unset
  • Patch needs improvement unset

I think I fixed this for all places patch_response_header is called. I've added a test that fails when USE_ETAGS is set to True and an admin site is called. I've added a few more tests to check various headers with TemplateResponse but I think only the etag is relevant because it requires the content is complete when the etag value is calculated.

The solution, IMO, is using the add_post_render_callback if/when the response is a TemplateResponse (using ducktype checking now rather than using isinstance).

comment:6 Changed 3 years ago by pterk

  • Cc pterk@… added

comment:7 Changed 3 years ago by mrmachine

Nevermind. I didn't fully understand the issue.

Last edited 3 years ago by mrmachine (previous) (diff)

comment:8 Changed 3 years ago by mrmachine

  • Cc real.human@… added

comment:9 Changed 3 years ago by jezdez

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

In [16729]:

Fixed #16003 -- Restored compatibility of the admin when using USE_ETAGS. Thanks for the initial patch, pterk.

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.