Expose router config via getters on Request #2506

kibertoad · 2020-08-21T08:27:05Z

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

Benchmarks:
After change: 148k requests in 5.12s, 27.6 MB read
Before change: 146k requests in 5.12s, 26.8 MB read

kibertoad · 2020-08-21T08:28:34Z

lib/route.js

@@ -320,7 +320,7 @@ function buildRouting (options) {

    var queryPrefix = req.url.indexOf('?')
    var query = querystringParser(queryPrefix > -1 ? req.url.slice(queryPrefix + 1) : '')
-    var request = new context.Request(id, params, req, query, childLogger)
+    var request = new context.Request(id, params, req, query, childLogger, context)


@Eomm Is it the only place where we instantiate Request in runtime, or I missed some?

In the 404 handler we create a new one:

fastify/lib/fourOhFour.js

Line 147 in e70fd41

function fourOhFourFallBack (req, res) {

kibertoad · 2020-08-21T08:32:08Z

types/request.d.ts

@@ -30,6 +30,8 @@ export interface FastifyRequest<
  hostname: string;
  url: string;
  method: string;
+  routerUrl: string;
+  routerMethod: string;


@Eomm You suggested exposing router method; are there any cases when you expect router method to differ from request method?

I can't remember it, could you link me the issue?

The only use case is when we register one route with multiple methods or some strange edge-cases

It's here: #2435
We could create a getter in the request for a limited set of data: URL, method.. nothing more

I'll try to remember why I say that but right now it seems odd 😅
Because if you are in a handler, the routing succeed and the request method will be equals to the context as you said.

The only utility seems to be a shortcut

@Eomm Should I keep it just in case or remove?

mcollina

lgtm

fox1t

LGTM

I think we need to add these methods to docs.

delvedor

LGTM

This would nicely solve #2143 as well.

RafaelGSS

LGTM with the addition to 404 handler

delvedor

Almost forgot, we are adding the context object to reply as well, which also contains .request.
Can you make reply.context a getter? This should save us some memory allocation.

test/router-options.test.js

kibertoad · 2020-08-21T11:49:12Z

@delvedor Replaced reply instance with getter, even though that particular change ended up being more invasive than I would like it to be; would appreciate if you could review that whole batch.

docs/Request.md

jsumners

LGTM

kibertoad · 2020-08-21T11:52:43Z

fastify.js

@@ -484,8 +491,8 @@ function fastify (options) {

      childLogger.info({ req }, 'incoming request')

-      const request = new Request(id, req, null, childLogger)


I think parameters were wrong here to begin with, second arg is params, not req, but please double-check if this change makes sense.

I think you are right and we should improve this test because it is not checking it:

fastify/test/router-options.test.js

Line 63 in e70fd41

test('Should honor frameworkErrors option', t => {

(in this PR or opening a new issue)

@Eomm I would prefer not to increase scope of this PR further, and handle it in a separate PR.

delvedor

LGTM

kibertoad · 2020-08-23T10:29:57Z

@mcollina When do we plan to cut the next telease?

mcollina · 2020-08-23T10:59:15Z

We tend to ship Mondays or Tuesdays if there is something to ship.

kibertoad · 2020-08-25T08:10:14Z

@mcollina Are we shipping today?

mcollina · 2020-08-25T08:54:01Z

Shipped v3.3.0

lance · 2020-09-03T10:52:00Z

lib/request.js

    this.id = id
+    this.context = context


Ooofff. It took me a while to figure out that the public API had changed when all of a sudden my decorator started failing nodeshift/faas-js-runtime@81a94e1. I guess since the change here was additive it wouldn’t normally qualify as a major version bump, but something to consider in the future when changing the public interface.

Good spot, sorry for the incovenience.

No worries - my stuff here is all R&D still, so things break ALL THE TIME. 👍

same here. Please can we get the doc updated with this new prop in request?

same here. Please can we get the doc updated with this new prop in request?

PRs are welcome.

PRs are welcome.

@jsumners I took a brief look at the code and it seems that Request#context is an instance of Context.

fastify/lib/route.js

Lines 233 to 247 in 9d703b0

const context = new Context(

opts.schema,

opts.handler.bind(this),

this[kReply],

this[kRequest],

this[kContentTypeParser],

config,

opts.errorHandler || this[kErrorHandler],

opts.bodyLimit,

opts.logLevel,

opts.logSerializers,

opts.attachValidation,

this[kReplySerializerDefault],

opts.schemaErrorFormatter || this[kSchemaErrorFormatter]

)

This object is not currently in the API docs. Do you suggest that the entire Context object be documented as part of the public API? Or instead, simply document portions of request.context that are relevant to a user. When I search the current docs for "context", I only find it mentioned in passing as a part of the Reply object.

https://github.com/fastify/fastify/blame/main/docs/Reply.md#L68-L76

It's odd, because the purpose of this PR seems to be solely about exposing routerPath, routerMethod and is404 getter properties on Request by proxying these to the context.

It's just a side effect that a context instance is now part of the public API, and I wonder if that's really what is intended since this PR did actually document what the expected API changes were. If it's intended, it's not clear what of the Context object should be publicly documented. If it's unintended, then it perhaps should not be documented? 🤷

I'm confused. This PR is merged and has doc changes that show the properties added in the request documentation. What is missing in the live docs?

@jsumners that was my question! It's not clear... This change introduced a context property on the Request object. But that was not the intent of the change, and so was never documented. Back in September, when I upgraded to a version containing this change, my application broke because of this new, undocumented property. When I figured out what had happened, I came here to mention it. Three days ago, someone else came here to mention the same thing and asked for doc changes. Then you said PRs are welcome, so I figured I would do my part and look into it. But, as I've said, it's not clear what exactly should be done.

It's sort of documented under https://www.fastify.io/docs/latest/Routes/#config

@mcollina posted some reasoning in another PR discussion around why we don't fully document it. But I don't recall which.

Many of the other properties in fastify use a Symbol as the property key in order to essentially hide these properties, preventing them from becoming a part of the published API. The `context` property on `request` was essentially an undocumented breaking change to the API. See: fastify#2506 This commit removes the `context` property from `Request` and replaces it with a `Symbol` to both revert this breaking change, and to remove the `context` property from the API surface area. This is potentially a breaking change, however, the `Request#context` property has never been documented, and is - based on my understanding of the intent in fastify#2506 - not meant to be a part of the public API.

github-actions · 2022-04-10T00:19:22Z

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.

Expose router config via getters on Request

7f03091

kibertoad requested review from Eomm and jsumners August 21, 2020 08:27

kibertoad commented Aug 21, 2020

View reviewed changes

mcollina approved these changes Aug 21, 2020

View reviewed changes

fox1t reviewed Aug 21, 2020

View reviewed changes

delvedor approved these changes Aug 21, 2020

View reviewed changes

RafaelGSS approved these changes Aug 21, 2020

View reviewed changes

delvedor requested changes Aug 21, 2020

View reviewed changes

jsumners requested changes Aug 21, 2020

View reviewed changes

test/router-options.test.js Outdated Show resolved Hide resolved

Address code review comments

952aa6e

jsumners reviewed Aug 21, 2020

View reviewed changes

docs/Request.md Outdated Show resolved Hide resolved

jsumners approved these changes Aug 21, 2020

View reviewed changes

kibertoad commented Aug 21, 2020

View reviewed changes

Minor text fix

469994a

kibertoad requested a review from delvedor August 21, 2020 12:24

fox1t approved these changes Aug 21, 2020

View reviewed changes

delvedor approved these changes Aug 22, 2020

View reviewed changes

kibertoad merged commit b9fcf3f into master Aug 22, 2020

kibertoad deleted the feature/2435-router-getters branch August 22, 2020 12:49

delvedor added semver-minor Issue or PR that should land as semver minor v3.x Issue or pr related to Fastify v3 labels Aug 22, 2020

This was referenced Aug 23, 2020

Update: pr#2017, 86e1364...b9fcf3f fastify/docs-chinese#230

Merged

文档持续更新List fastify/docs-chinese#18

Open

lance reviewed Sep 3, 2020

View reviewed changes

lance mentioned this pull request Apr 9, 2021

src: change request.context to use a symbol #2989

Closed

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose router config via getters on Request #2506

Expose router config via getters on Request #2506

kibertoad commented Aug 21, 2020 •

edited

kibertoad Aug 21, 2020

Eomm Aug 21, 2020

kibertoad Aug 21, 2020

kibertoad Aug 21, 2020

Eomm Aug 21, 2020

kibertoad Aug 21, 2020

Eomm Aug 21, 2020

kibertoad Aug 21, 2020

mcollina left a comment

fox1t left a comment

delvedor left a comment

RafaelGSS left a comment

delvedor left a comment

kibertoad commented Aug 21, 2020

jsumners left a comment

kibertoad Aug 21, 2020

Eomm Aug 21, 2020

kibertoad Aug 21, 2020

delvedor left a comment

kibertoad commented Aug 23, 2020

mcollina commented Aug 23, 2020

kibertoad commented Aug 25, 2020

mcollina commented Aug 25, 2020

lance Sep 3, 2020

mcollina Sep 3, 2020

lance Sep 3, 2020

pramodhts Apr 2, 2021

jsumners Apr 2, 2021

lance Apr 3, 2021 •

edited

jsumners Apr 5, 2021

lance Apr 5, 2021

jsumners Apr 5, 2021

lance Apr 9, 2021

github-actions bot commented Apr 10, 2022

		@@ -484,8 +491,8 @@ function fastify (options) {

		childLogger.info({ req }, 'incoming request')

		const request = new Request(id, req, null, childLogger)

	const context = new Context(
	opts.schema,
	opts.handler.bind(this),
	this[kReply],
	this[kRequest],
	this[kContentTypeParser],
	config,
	opts.errorHandler \|\| this[kErrorHandler],
	opts.bodyLimit,
	opts.logLevel,
	opts.logSerializers,
	opts.attachValidation,
	this[kReplySerializerDefault],
	opts.schemaErrorFormatter \|\| this[kSchemaErrorFormatter]
	)

Expose router config via getters on Request #2506

Expose router config via getters on Request #2506

Conversation

kibertoad commented Aug 21, 2020 • edited

Checklist

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

mcollina left a comment

Choose a reason for hiding this comment

fox1t left a comment

Choose a reason for hiding this comment

delvedor left a comment

Choose a reason for hiding this comment

RafaelGSS left a comment

Choose a reason for hiding this comment

delvedor left a comment

Choose a reason for hiding this comment

kibertoad commented Aug 21, 2020

jsumners 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

delvedor left a comment

Choose a reason for hiding this comment

kibertoad commented Aug 23, 2020

mcollina commented Aug 23, 2020

kibertoad commented Aug 25, 2020

mcollina commented Aug 25, 2020

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

lance Apr 3, 2021 • edited

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

github-actions bot commented Apr 10, 2022

kibertoad commented Aug 21, 2020 •

edited

lance Apr 3, 2021 •

edited