express icon indicating copy to clipboard operation
express copied to clipboard

Do not auto-remove content headers for body-less status codes

Open LJ1102 opened this issue 2 years ago • 5 comments

According to RFC 2616 (HTTP/1.1 spec) a HEAD request is supposed to return exactly the same entity-headers as a GET request but no body, ~~imho responding to such a request with a 204 status code seems reasonable, thus we should not remove any headers but only ensure that no body is sent.~~

LJ1102 avatar Nov 16 '23 18:11 LJ1102

I did some more digging and in fact RFC 7230 specifies the following:

Transfer-Encoding MAY be sent in a response to a HEAD request or in a 304 (Not Modified) response

A server MAY send a Content-Length header field in a 304 (Not Modified) response to a conditional GET request

A server MUST NOT send a Content-Length header field in any response with a status code of 1xx (Informational) or 204 (No Content).

So I altered my changes (and adjusted the unit tests) to be compliant to that, so that with a 304 we leave the content-headers untouched, but on 204 we remove them.

LJ1102 avatar Nov 17 '23 03:11 LJ1102

write clean code

TahsinAyman avatar Dec 08 '23 15:12 TahsinAyman

@gooroodev please explain

admsev avatar May 26 '24 09:05 admsev

@admsev, thanks for the invite!

1. Summary of Changes

  • Code Changes in lib/response.js:
    • Modified logic to handle response headers for specific status codes.
    • Removed the stripping of content headers for status code 304.
    • Consolidated the logic to skip the response body for status codes 204, 205, 304, and for the HEAD method.
  • Test Changes in test/res.send.js:
    • Updated the test for status code 304 to check that the body is ignored without verifying the removal of headers.

2. Issues, Bugs, or Typos

  • Code Issues:
    • The chunk variable is no longer being set to an empty string for status codes 204 and 205. This could potentially affect the response if the chunk variable is used later in the function.
  • Proposed Improvements:
    1. Setting the chunk Variable:
      diff --git a/lib/response.js b/lib/response.js
      index 17ddf021d1..newindex 100644
      --- a/lib/response.js
      +++ b/lib/response.js
      @@ -214,6 +214,7 @@ res.send = function send(body) {
           this.removeHeader('Content-Length');
           this.removeHeader('Transfer-Encoding');
           chunk = '';
      +    body = '';
        }
      
        // alter headers for 205
        if (this.statusCode === 205) {
           this.set('Content-Length', '0');
           this.removeHeader('Transfer-Encoding');
           chunk = '';
      +    body = '';
        }
      

3. General Review of Code Quality and Style

  • Code Quality:

    • The changes are clear and improve the handling of response headers and bodies for specific status codes.
    • Removing the content headers only for status code 204 aligns with the HTTP specification.
    • The consolidation of the logic to skip the body for certain status codes and the HEAD method enhances readability and maintainability.
  • Code Style:

    • The code follows a consistent style, making it easy to read and understand.
    • The use of comments to explain the purpose of each code block is helpful.

Overall, the pull request improves the handling of response headers and bodies for specific status codes, aligning better with HTTP specifications. The proposed improvements ensure that the chunk variable is appropriately handled. The code quality and style are consistent and maintainable.

Yours, Gooroo.dev. Feel free to react or reply to this review!

gooroodev avatar May 26 '24 09:05 gooroodev