Opened 7 years ago

Closed 4 years 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 Bond 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 7 years ago.
django-12789.patch (612 bytes) - added by Forest Bond 6 years ago.
Patch fixing patch_response_headers.

Download all attachments as: .zip

Change History (15)

Changed 7 years ago by penzoil

Attachment: fix_middleware_http.diff added

comment:1 Changed 7 years ago by Russell Keith-Magee

Patch needs improvement: set
Triage Stage: UnreviewedDesign 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 7 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 6 years ago by Forest Bond

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 6 years ago by Forest Bond

Attachment: django-12789.patch added

Patch fixing patch_response_headers.

comment:4 Changed 6 years ago by Forest Bond

Keywords: patch_response_headers added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: Design decision neededUnreviewed

Resetting some flags on the ticket given new patch.

comment:5 Changed 6 years ago by Forest Bond

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 6 years ago by Forest Bond

Cc: Forest Bond added

comment:7 Changed 6 years ago by Honza Král

Needs tests: set

comment:8 Changed 6 years ago by Russell Keith-Magee

Triage Stage: UnreviewedDesign 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 6 years ago by Luke Plant

Type: Bug

comment:10 Changed 6 years ago by Luke Plant

Severity: Normal

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 Changed 4 years ago by Aymeric Augustin

Resolution: duplicate
Status: newclosed

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

Note: See TracTickets for help on using tickets.
Back to Top