src: change request.context to use a symbol

[lance]

Many of the other properties in fastify objects use a Symbol as the property key in order to hide these properties, preventing them from becoming a part of the public API. The context property on request was essentially an undocumented breaking change to the API.

See: #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 #2506 - not meant to be a part of the public API.

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

[mcollina]

I think request.context is both useful and dangerous to use. I'm pretty sure we use it in a few of the plugins in this organization.

Making it private will definitely be breaking for most folks.

[delvedor]

Agree with @mcollina, context is badly documented but there are real uses cases around.
An example if present in the docs.
The main issue is that context contains both private and public info. We should discuss about refactoring it in v4.

fastify/lib/context.js

Lines 7 to 29 in 0ae362b

	function Context (schema, handler, Reply, Request, contentTypeParser, config, errorHandler, bodyLimit, logLevel, logSerializers, attachValidation, replySerializer, schemaErrorFormatter) {
	this.schema = schema
	this.handler = handler
	this.Reply = Reply
	this.Request = Request
	this.contentTypeParser = contentTypeParser
	this.onRequest = null
	this.onSend = null
	this.onError = null
	this.onTimeout = null
	this.preHandler = null
	this.onResponse = null
	this.config = config
	this.errorHandler = errorHandler
	this._middie = null
	this._parserOptions = { limit: bodyLimit || null }
	this.logLevel = logLevel
	this.logSerializers = logSerializers
	this[kFourOhFourContext] = null
	this.attachValidation = attachValidation
	this[kReplySerializerDefault] = replySerializer
	this.schemaErrorFormatter = schemaErrorFormatter || defaultSchemaErrorFormatter
	}

[Eomm]

It is used for sure, like accessing the route config

eg: reply.context.config.url to get the generic route (aka /foo/:param)

[lance]

The examples referenced by @Eomm and @delvedor are both accessing the reply.context which is unchanged by this PR. It is still simply proxied from request[kContext].

The only thing that this PR changes is how context is accessed on request. It does not eliminate access to context.

I'm pretty sure we use it in a few of the plugins in this organization.

I did not check for usage in all of the organization repos. But just to be clear, if they were written before #2506 landed then they were accessing context via reply.context which has not changed with this PR.

[lance]

The main issue is that context contains both private and public info

+1000

[delvedor]

@lance yes, we referenced examples with reply, but the same could work with request.
I think we should fix the underlying issue, which is that we are leaking internals in a public object.
It's very useful that both request and reply can accept context, as you can do things like:

fastify.decorateRequest('getStuff', function () {
  if (this.context.config.something) {
    return 'stuff'
  }
  return 'not stuff'
})

[lance]

@delvedor I know that. It was the addition of context to the request object that broke my decoration nodeshift/faas-js-runtime@81a94e1



[lance]

For what it's worth, I understand if you don't want to land this PR. But I would say that if you don't it's very important that request.context be documented in some way. I had to dig through fastify code to figure out what went wrong with my decoration. There is no documentation of this property. At least not on the request object. I started to look into writing documentation, and realized that Context was both public and private. See the conversation in the PR where that landed. Because of that, I decided to submit this PR instead.

[lance]

fastify.decorateRequest('getStuff', function () {
  if (this.context.config.something) {

For a user to do this, they would have to know that there is a context property on the request object. That is not documented.

[mcollina]

I'm sorry if we broke you while adding that property.. however that was 8 months ago: a bit late for a revert.

I'm +1 in documenting the context property as "opaque".

[lance]

I'm sorry if we broke you while adding that property.. however that was 8 months ago: a bit late for a revert.

No apology needed. This only arose now because someone else recently complained about the situation in #2506 and was told "we accept PRs". I was just trying to do my part. 🤷

[jsumners]

I don't want this to seem like we don't accept PRs. We do accept them after considered review. The review in this case has determined that the status quo is now set and a revert is not viable. It has also determined that some work around this should be done against the next branch so we do not perpetuate this into v4 and beyond.

@lance would you be willing to spearhead that effort?

[lance]

I don't want this to seem like we don't accept PRs. We do accept them after considered review.

Understood - and I submitted this PR knowing that it would likely be rejected. I just had to defend it as best as I could.

@lance would you be willing to spearhead that effort?

Can you help me understand what that looks like? Based on this conversation, it sounds like there should be a publicly documented API for Context and the parts of Context that are considered private should be moved into some other less accessible object. Does that sound right?

[mcollina]

Not really. I would just document that a req.context exists and it is internal / opaque.

[lance]

Not really. I would just document that a req.context exists and it is internal / opaque.

Even for next? I was responding to this 👇

some work around this should be done against the next branch so we do not perpetuate this into v4 and beyond.

[chrskrchr]

I would just document that a req.context exists and it is internal / opaque.

I realize that I'm 7mo late to this conversation, but would the team be open to a PR that types FastifyRequest.context as unknown to more strongly communicate the fact that 1) it exists and 2) its internal/opaque and generally shouldn't be accessed or modified? e.g.,:

https://github.com/fastify/fastify/blob/main/types/request.d.ts

export interface FastifyRequest<...> {
    ...
    context: unknown;
}

While it didn't cause any major issues for us, my team was stashing away some request-scoped data in this field and were surprised to find a few weeks ago that it was already being used internally by Fastify. This was prior to the field being documented in #3201 and we didn't check via a debugger if it was already being used. But we got lucky and didn't clobber any of the existing fields. 😄

Let me know if y'all are open to that small PR and I'll submit it.

[climba03003]

Let me know if y'all are open to that small PR and I'll submit it.

You can update it to the same as reply.

fastify/types/reply.d.ts

Line 24 in 11c77b5

context: FastifyContext<ContextConfig>;

[chrskrchr]

You can update it to the same as reply.

I thought about proposing that, but it seems to run counter to things said in this thread by @mcollina, @delvedor, and @jsumners that this field is internal/opaque and shouldn't be carried forward as-is into v4 and beyond. That was the main reason for proposing it be typed as unknown.

I'm happy to submit a PR that goes either route depending on what the maintainers prefer. I just don't want others to get bit by this.

[climba03003]

The other property of context is internal and should not be visualize by the consumer, but not config. As config is passed by the users.

[chrskrchr]

@climba03003 - sounds good! Submitted a PR here that types FastifyRequest.context the same as FastifyReply.context:

#3472

[github-actions]

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.