feat: Set HEAD routes automatically for each GET route #2700

metcoder95 · 2020-11-20T17:19:18Z

Closes #2619

Note

The feature affects other tests as collateral. Tests need to be adjusted.
All feedback welcome 👍
Transformed into a minor feature

Open questions

~~Should be able to pass a global flag to deactivate the feature?~~ Global and Route specific flags will be supported
~~Should be able to ignore specific routes (by RegExps or Strings)?~~ Discarded

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

lib/route.js

Eomm · 2020-11-21T21:01:12Z

Should be able to pass a global flag to deactivate the feature?

Reading the issue it seems not necessary: the logic: if the user defines a HEAD endpoint, the sibling GET will not process the request cover it I think

Should be able to ignore specific routes (by RegExps or Strings)?

I would avoid this feature at this first step

zekth · 2020-11-22T10:09:13Z

Thinking out loud:

Flag to activate / deactivate the feature is mandatory to me. eg: on some context you simply don't want to expose HEAD
Flag to do it on a granular way on routes can be a plus
Should be able to ignore specific routes (by RegExps or Strings)? The points enumerated before resolves this issue

metcoder95 · 2020-11-23T21:19:29Z

Thinking out loud:

Flag to activate / deactivate the feature is mandatory to me. eg: on some context you simply don't want to expose HEAD

Flag to do it on a granular way on routes can be a plus

Should be able to ignore specific routes (by RegExps or Strings)? The points enumerated before resolves this issue

Actually I was thinking in the same way, maybe deactivate it by default and activate it on demand, in that way maybe lesser changes will be needed. But not sure if it's the best choice.
Sounds good to me

jsumners · 2020-11-24T13:14:07Z

maybe deactivate it by default and activate it on demand

In that case, it wouldn't be a semver major change.

SimenB · 2020-11-24T13:51:18Z

I strongly believe it should be the default, but introducing it in a semver minor disabled by default while allowing people to opt in and then flipping the default for the next major is probably a good path forward 👍

lib/headRoute.js

lib/route.js

lib/headRoute.js

mcollina

Good work!

mcollina · 2020-11-26T09:26:32Z

fastify.js

@@ -96,6 +96,7 @@ function fastify (options) {
  const requestIdLogLabel = options.requestIdLogLabel || 'reqId'
  const bodyLimit = options.bodyLimit || defaultInitOptions.bodyLimit
  const disableRequestLogging = options.disableRequestLogging || false
+  const exposeHeadRoutes = options.exposeHeadRoutes != null ? options.exposeHeadRoutes : true


This needs docs

If the default is false, we can land this as minor against master.

I'm fine with both approaches, setting it as false by default will require way fewer changes to the tests as most of them are broken because of the feature. Should we move forward with a minor version?

+1 for default to false.

Done in 124fa9e
I'll just need to adjust my PR to point to master

mcollina · 2020-11-29T10:48:44Z

lib/route.js

@@ -231,6 +234,15 @@ function buildRouting (options) {
        return
      }

+      // TODO: @MetCoder95 Implement hability for ignoring paths


This is not on the principles of Fastify. There is no ignoring (and regexps, etc). We can support it in an ecapsulation-friendly way if we want to.

Sure, my bad! Actually, it's a leftover as I followed the suggestion of using a single-route/global flag to enable/disable the feature

lib/headRoute.js

…registered

SimenB

this is missing a change to the TypeScript types.

It probably makes sense to update the change in #2685 as well to mention it is built in from version 3.x (whenever it lands), but needs to be enabled.

docs/Routes.md

docs/Server.md

mcollina

lgtm

jsumners · 2020-12-01T14:38:28Z

this is missing a change to the TypeScript types.

We have a policy that these are updated by those who know them.

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

mcollina

You'd need to add a test for the types changes.

mcollina · 2020-12-03T11:16:13Z

lib/headRoute.js

+
+  if (typeof payload.resume === 'function') {
+    payload.on('error', (err) => {
+      reply.log.error({ err, path: req.url }, 'Error on Stream found for HEAD route')


Suggested change

reply.log.error({ err, path: req.url }, 'Error on Stream found for HEAD route')

reply.log.error({ err }, 'Error on Stream found for HEAD route')

Path is not needed

Sure, done in 130646d

… set-head-for-get

Ethan-Arrowood

LGTM

mcollina

lgtm

mcollina · 2020-12-14T15:31:02Z

@Eomm @SimenB anything to add?

Eomm · 2020-12-14T21:07:25Z

lib/route.js

+      const { exposeHeadRoute } = opts
+      const hasRouteExposeHeadRouteFlag = exposeHeadRoute != null
+      const shouldExposeHead = hasRouteExposeHeadRouteFlag ? exposeHeadRoute : globalExposeHeadRoutes
+      const headRouteExists = router.find('HEAD', path) != null


Just a thought: if there is a HEAD route and the user sets the route level option hasRouteExposeHeadRouteFlag too, would it be too harsh logging a warning?

Don't think so, would be just adding an else if to see if the headRouteExists and hasRouteExposedHeadRouteFlag it's set to true for printing the warning. What do you think?

I think it could let users say "oh gosh, it saves me a bad headache"
If you have the chance to add it, I think it will be really appreciated by someone!

haha, sure! No issue at all, I think will save someone's day, let me add it 😄

Done in 976b080
Please have a look 🙂

metcoder95 · 2020-12-15T16:46:58Z

Any clue why for Windows the tests are failing? Same issue on #2731 because the same issue

metcoder95 · 2020-12-28T16:18:11Z

Hi guys! Is there anything left I should take care of?

mcollina · 2020-12-28T18:06:45Z

Nothing, CI got stuck.

mattbishop · 2021-01-24T01:33:45Z

I'm glad to see this addition. FWIW, HEAD and GET are required according to RFC-7231:

All general-purpose servers MUST support the methods GET and HEAD.
All other methods are OPTIONAL.

HosseinAgha · 2021-04-18T13:07:51Z

@metcoder95 I have a small question about this PR:
I cannot find this option on the fastify instance passed to fastify plugins.
Is this intended?

fastifyApp.resgiter((fastify, opt) => {
  fastify.route({
     url: ...
     exposeHeadRoutes: true // I get a TypeScript error here and when I bypass it using "as any" nothing happens
  })
})

metcoder95 · 2021-04-19T19:33:12Z

Hmm, the type for the route options has the exposeHeadRoute defined on route.d.ts:L25 which used for extending RouteOptions interface, as stated in route.d.ts:L112. Is also giving you troubles by using the singular form? exposeHeadRoute, without the s?

HosseinAgha · 2021-05-04T12:13:16Z

The exposeHeadRoute (singular) is added to fastify options after updating to 3.14.2. Thanks.

github-actions · 2022-05-05T00:21:00Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

metcoder95 mentioned this pull request Nov 20, 2020

Automatically handle HEAD requests in GET handler unless explicit HEAD handler exists #2619

Closed

Eomm reviewed Nov 21, 2020

View reviewed changes

lib/route.js Show resolved Hide resolved

Eomm added semver-major Issue or PR that should land as semver major v4.x Issue or pr related to Fastify v4 labels Nov 21, 2020

mcollina requested changes Nov 24, 2020

View reviewed changes

lib/headRoute.js Show resolved Hide resolved

lib/route.js Show resolved Hide resolved

lib/headRoute.js Show resolved Hide resolved

lib/headRoute.js Outdated Show resolved Hide resolved

metcoder95 requested review from mcollina and Eomm November 25, 2020 22:08

mcollina reviewed Nov 26, 2020

View reviewed changes

mcollina requested changes Nov 29, 2020

View reviewed changes

metcoder95 added 6 commits November 30, 2020 23:03

route: implement automatic creation of HEAD routes on each GET route …

4ea65d7

…registered

fix: add use strict

1d3f791

feat: implement global/route-specific flags

02b6675

docs: add documentation for exposeHeadRoute & exposeHeadRoutes

32d9d1c

refactor: set the exposeHeadRoute to false by default

cabd46f

fix: handle stream.onError properly

38f80fe

metcoder95 changed the base branch from next to master November 30, 2020 22:04

metcoder95 marked this pull request as ready for review November 30, 2020 22:04

metcoder95 requested a review from mcollina November 30, 2020 22:07

SimenB reviewed Dec 1, 2020

View reviewed changes

docs/Routes.md Outdated Show resolved Hide resolved

docs/Server.md Outdated Show resolved Hide resolved

mcollina approved these changes Dec 1, 2020

View reviewed changes

metcoder95 and others added 4 commits December 2, 2020 22:05

docs: update docs/Server.md

495936a

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>

types: add types for exposeHeadRoute/exposeHeadRoutes

26db0d2

refactor: log error on Error event

1cc5d00

feat: support HEAD routes previously registered

f636732

mcollina added semver-minor Issue or PR that should land as semver minor and removed semver-major Issue or PR that should land as semver major v4.x Issue or pr related to Fastify v4 labels Dec 3, 2020

mcollina requested changes Dec 3, 2020

View reviewed changes

metcoder95 added 2 commits December 3, 2020 22:53

refactor: do not print path on error

130646d

Merge branch 'set-head-for-get' of github.com:MetCoder95/fastify into…

7aba2ce

… set-head-for-get

metcoder95 requested a review from mcollina December 3, 2020 21:56

Ethan-Arrowood approved these changes Dec 14, 2020

View reviewed changes

mcollina approved these changes Dec 14, 2020

View reviewed changes

Eomm approved these changes Dec 14, 2020

View reviewed changes

SimenB approved these changes Dec 15, 2020

View reviewed changes

RafaelGSS approved these changes Dec 15, 2020

View reviewed changes

metcoder95 added 2 commits December 15, 2020 17:36

feat: add warning message when duplicated HEAD routes

976b080

Merge branch 'master' into set-head-for-get

12a5faf

metcoder95 requested a review from Eomm December 15, 2020 16:41

mcollina merged commit 834a4e5 into fastify:master Dec 28, 2020

fralonra mentioned this pull request Jan 4, 2021

Update: 6928211...834a4e5 fastify/docs-chinese#238

Merged

mcollina mentioned this pull request Jan 31, 2021

Create shallow copy of the route context config #2827

Merged

3 tasks

zekth mentioned this pull request Feb 2, 2021

Set exposeHeadRoutes: true by default #2826

Merged

4 tasks

acro5piano mentioned this pull request May 8, 2021

404 Not Found on OPTIONS request fastify/fastify-cors#20

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 5, 2022

	reply.log.error({ err, path: req.url }, 'Error on Stream found for HEAD route')
	reply.log.error({ err }, 'Error on Stream found for HEAD route')

feat: Set HEAD routes automatically for each GET route #2700

feat: Set HEAD routes automatically for each GET route #2700

Conversation

metcoder95 commented Nov 20, 2020 • edited

Checklist

Eomm commented Nov 21, 2020

zekth commented Nov 22, 2020

metcoder95 commented Nov 23, 2020 • edited

jsumners commented Nov 24, 2020

SimenB commented Nov 24, 2020

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

jsumners commented Dec 1, 2020

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Dec 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eomm Dec 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metcoder95 commented Dec 15, 2020

metcoder95 commented Dec 28, 2020

mcollina commented Dec 28, 2020

mattbishop commented Jan 24, 2021

HosseinAgha commented Apr 18, 2021 • edited

metcoder95 commented Apr 19, 2021

HosseinAgha commented May 4, 2021

github-actions bot commented May 5, 2022

metcoder95 commented Nov 20, 2020 •

edited

metcoder95 commented Nov 23, 2020 •

edited

Eomm Dec 14, 2020 •

edited

HosseinAgha commented Apr 18, 2021 •

edited