Code

Opened 4 years ago

Closed 16 months ago

#12789 closed Bug (duplicate)

ConditionalGetMiddleware behavior improvement.

Reported by: penzoil Owned by: nobody
Component: Core (Cache system) Version: master
Severity: Normal Keywords: ETag, ConditionalGetMiddleware, patch_response_headers
Cc: forest Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This patch is intended to improve the behavior of the django.middleware.http.ConditionalGetMiddleware? middleware. When a particular django view has at least two decorators that work with redirecting behavior, Firefox will send a ETag that seems to be valid since the response is nothing more than a Location redirect, thus the content-length is 0 and the ETag is valid for the two steps created by the two decorators that redirect the user to different pages (Example decorator : @login_required and another that acts very similarly). The bug is that the browser will receive a Not Modified header once the users passes the first decorator when he gets redirected by to the original view.

The FIX: The fix changes the behavior of the django.middleware.http.ConditionalGetMiddleware? to not send a 304 response code if the content-length is 0 or undefined.

I'm not sure if it's proper, but it seems good of a fix and it actually fixes my issue. So, I just wanted to shared it, hopefully it can make it in the code-base.

This fix will require a little bit of documentation change to explain the extra condition.

Attachments (2)

fix_middleware_http.diff (1.0 KB) - added by penzoil 4 years ago.
django-12789.patch (612 bytes) - added by forest 4 years ago.
Patch fixing patch_response_headers.

Download all attachments as: .zip

Change History (15)

Changed 4 years ago by penzoil

comment:1 Changed 4 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

I don't deny that this fixes the problem you describe, but it feels like there is something else going on. Firefox should only be sending an etag if the server provided one. Before I start making exceptions in ConditionalGetMiddleware, I'd like to rule out the possiblity that the ETags are getting set incorrectly in the first place.

Can you provide a specific test case where this happens i.e., an example view, decorated in a way that exhibits the problem? You should also provide your Firefox version, in case it is version dependent.

comment:2 Changed 4 years ago by penzoil

You probably have a point there. Maybe we could return no ETag when a Location header is sent with the response. I'll have to test this when I have time.

For what is of the test case views, I'll get on that as soon as I can. For what is of the Firefox version it was 3.5.7 at the time of implementation (On Windows).

comment:3 Changed 4 years ago by forest

Hi,

The problem is that patch_response_headers sets an ETag header regardless of the response status_code. This differs from the behavior implemented by CommonMiddleware, which only sets ETags when 200 <= response.status_code < 300. Making it consistent with CommonMiddleware behavior fixes things for me.

I'll attach a new patch.

Thanks,
Forest

Changed 4 years ago by forest

Patch fixing patch_response_headers.

comment:4 Changed 4 years ago by forest

  • Keywords ConditionalGetMiddleware, patch_response_headers added; ConditionalGetMiddleware removed
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Design decision needed to Unreviewed

Resetting some flags on the ticket given new patch.

comment:5 Changed 4 years ago by forest

Hi,

I'm sorry, it looks like CommonMiddleware actually does set an ETag for non-2XX responses, but it never returns HTTP 304 unless the response generated by the view has a 2XX status code.

Now I'm unsure about what the proper fix should be. I think we should do one or both of the following:

  1. Don't set ETag on non-2XX responses (CommonMiddleware currently does *not* do this, my patch implements it for patch_response_headers).
  2. Don't replace non-2XX responses with HTTP 304 (CommonMiddleware currently does this, ConditionalGetMiddleware currently does *not* do this).

I suspect that CommonMiddleware should be fixed to not set an ETag on non-2XX responses.

Thoughts?

Thanks,
Forest

comment:6 Changed 4 years ago by forest

  • Cc forest added

comment:7 Changed 4 years ago by Honza_Kral

  • Needs tests set

comment:8 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Design decision needed

Reverting to DDN. The problem seems real, but we need to work out the right solution. Some programatic test cases would help considerably.

comment:9 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:10 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 16 months ago by aaugustin

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

As said by Russell early in the thread, this is actually a problem with ETag generation, which is tracked in #17834.

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.