Opened 6 years ago

Closed 3 years ago

#10762 closed Cleanup/optimization (fixed)

Gzip Middleware Should Compress 201 (Created) Responses

Reported by: rwagner@… Owned by: aaugustin
Component: HTTP handling Version: master
Severity: Normal Keywords: middleware gzip
Cc: madjar, cannona Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Hi,

RESTful web services that use POST for creation of resources often respond with a representation of the newly created resource, which can be lengthy. However, the appropriate status code is 201, which the Gzip middleware ignores. There's a trivial patch attached that contains these lines:

-        if response.status_code != 200 or len(response.content) < 200:
+        if response.status_code not in (200, 201) or len(response.content) < 200:

This is not a particularly big deal, just a convenience.

Thanks,
Rick

Attachments (3)

gzip.patch (536 bytes) - added by rwagner@… 6 years ago.
10762.patch (646 bytes) - added by aaugustin 4 years ago.
10762_with_tests.patch (2.3 KB) - added by madjar 3 years ago.
The previous patch, with added tests

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by rwagner@…

comment:1 follow-up: Changed 6 years ago by julianb

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

While we are at it: What about 404 or 403 responses?

comment:2 in reply to: ↑ 1 Changed 6 years ago by rpwagner

Replying to julianb:

While we are at it: What about 404 or 403 responses?

I don't believe that was what was intended by the original condition. The 2XX status codes indicate successful operations, whereas 4XX are client errors. In practice, I think it is a good idea to render errors in the simplest formats possible, to make interpreting them easier on the client side. But, I can see the argument for compressing a custom 404 response that is rendered from a template, and could get large.

--Rick

comment:3 Changed 6 years ago by Alex

  • Triage Stage changed from Unreviewed to Design decision needed

comment:4 Changed 4 years ago by aaugustin

The comment in gzip.py say: "It's not worth compressing non-OK or really short responses."

If non-OK responses are not worth compressing because they are expected to be short, testing their length is enough, which means the check on response.status_code should be removed.

If there is another reason why non-OK responses should not be compressed, the proper check is 200 <= response.status_code < 300, and the reason should be explained in a comment.

comment:5 Changed 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Cleanup/optimization

Changed 4 years ago by aaugustin

comment:6 Changed 4 years ago by aaugustin

  • Easy pickings unset

Regarding Rick's comment above: the content will only be gzipped if the user agent sent an Accept-Encoding: gzip header. It seems reasonable to assume it will be able to decode gzip, even if the server returns an error page.

comment:7 Changed 4 years ago by aaugustin

  • Easy pickings set
  • Needs tests set
  • Triage Stage changed from Design decision needed to Accepted
  • UI/UX unset

Changed 3 years ago by madjar

The previous patch, with added tests

comment:8 Changed 3 years ago by madjar

  • Cc madjar added
  • Needs tests unset

Here are simple tests to check that long responses are compressed and short responses are not, regardless of the return type. Are they ok ?

comment:9 Changed 3 years ago by cannona

  • Cc cannona added

comment:10 Changed 3 years ago by aaugustin

  • Owner changed from nobody to aaugustin

See also #17514.

comment:11 Changed 3 years ago by aaugustin

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

In [17365]:

Fixed #10762, #17514 -- Prevented the GZip middleware from returning a response longer than the original content, allowed compression of non-200 responses, and added tests (there were none). Thanks cannona for the initial patch.

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