Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove HeadResponder #3147

Merged
merged 2 commits into from Apr 23, 2024
Merged

Remove HeadResponder #3147

merged 2 commits into from Apr 23, 2024

Conversation

baarde
Copy link
Sponsor Contributor

@baarde baarde commented Jan 30, 2024

These changes are now available in 4.93.1

The HEAD method is identical to GET except that the server must not send content in the response (RFC 9110, section 9.3.2).

The previous default behaviour of returning 200 OK to every HEAD request to a constant route is not standard-compliant.

The new behaviour is to always forward the request to the GET route, unless the developer explicitely configured a custom HEAD route.

This PR fixes #2680 and #2749.

@gwynne gwynne added bug Something isn't working semver-patch Internal changes only labels Jan 30, 2024
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @0xTim?

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in this. I think this is good, but I'd like to see a test for the explicit override of a HEAD request

@baarde
Copy link
Sponsor Contributor Author

baarde commented Apr 9, 2024

@0xTim I think this is already covered by testExplicitHeadRouteOverridesGeneratedHandler testExplicitHeadRouteOverridesForwardingToGet (I've renamed the method to reflect the implementation change).

@baarde baarde requested a review from 0xTim April 17, 2024 13:09
The HEAD method is identical to GET except that the server must not send content in the response (RFC 9110, section 9.3.2).

The previous default behaviour of returning 200 OK to every HEAD request to a constant route is not standard-compliant.

The new behaviour is to always forward the request to the GET route, unless the developer explicitly configured a custom HEAD route.
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.54%. Comparing base (11cdb29) to head (42658b5).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3147      +/-   ##
==========================================
- Coverage   76.86%   76.54%   -0.33%     
==========================================
  Files         211      211              
  Lines        8119     7677     -442     
==========================================
- Hits         6241     5876     -365     
+ Misses       1878     1801      -77     
Files Coverage Δ
Sources/Vapor/Responder/DefaultResponder.swift 98.91% <ø> (-0.25%) ⬇️

... and 89 files with indirect coverage changes

@0xTim 0xTim linked an issue Apr 19, 2024 that may be closed by this pull request
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I missed that. Ok this looks fine to me

@owainhunt since you did the original PR, does the overriding as described work for you?

@owainhunt
Copy link
Contributor

LGTM 👍

@0xTim 0xTim enabled auto-merge (squash) April 23, 2024 11:13
@0xTim 0xTim merged commit 0311f9a into vapor:main Apr 23, 2024
16 checks passed
keniwhat pushed a commit to keniwhat/vapor that referenced this pull request Apr 27, 2024
* main:
  Patch configuration and log actual port on startup (vapor#3160)
  Update provider tests to 5.10 (vapor#3178)
  Migrate to Async NIOFileIO APIs (vapor#3167)
  Removed streamFile deprecation + deactivated advancedETagComparison by default (vapor#3177)
  Remove HeadResponder (vapor#3147)
  Advanced ETag Comparison now supported (vapor#3015)
  Enabled Request Decompression By Default (vapor#3175)
  HTTP2 Response Compression/Request Decompression (vapor#3126)
  Don't set ignore status for SIGTERM and SIGINT on Linux (vapor#3174)
  Fix typos across the codebase (vapor#3162)
  Fix some Sendable warnings on 5.10 (vapor#3158)
  Allow `HTTPServer`'s configuration to be dynamically updatable (vapor#3132)
  Fix issue when client disconnects midway through a stream (vapor#3102)
  Fix handling of "flag" URL query params (vapor#3151)
  Bump the dependencies group with 1 update (vapor#3148)
  Merge Async Tests (vapor#3141)
  Fix URI handling with multiple slashes and variable components. (vapor#3143)
  Fix broken URI behaviors (vapor#3140)

# Conflicts:
#	Package.swift
@baarde baarde deleted the remove-head-responder branch April 29, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reverse Pull #2492 HEAD route ignores Middlewares
4 participants