feat: request and reply decorators can not be a reference type (#5462)

docs/Reference/Decorators.md

@@ -59,8 +59,8 @@ close as possible to the value intended to be set dynamically in the future.
 Initialize a decorator as a `''` if the intended value is a string, and as
 `null` if it will be an object or a function.
 
-Remember this example works only with value types as reference types will be
-shared amongst all requests. See [decorateRequest](#decorate-request).
+Remember this example works only with value types as reference types will 
+thrown and error during the fastify startup. See [decorateRequest](#decorate-request).
 
 See [JavaScript engine fundamentals: Shapes and Inline
 Caches](https://mathiasbynens.be/notes/shapes-ics) for more information on this
@@ -83,7 +83,7 @@ fastify.decorate('utility', function () {
 })
 ```
 
-As mentioned above, non-function values can be attached:
+As mentioned above, non-function values can be attached to the server instance as:
 
 ```js
 fastify.decorate('conf', {
@@ -177,17 +177,18 @@ fastify.decorateReply('utility', function () {
 Note: using an arrow function will break the binding of `this` to the Fastify
 `Reply` instance.
 
-Note: using `decorateReply` will emit a warning if used with a reference type:
+Note: using `decorateReply` will throw and error if used with a reference type:
 
 ```js
 // Don't do this
 fastify.decorateReply('foo', { bar: 'fizz'})
 ```
-In this example, the reference of the object is shared with all the requests:
-**any mutation will impact all requests, potentially creating security
-vulnerabilities or memory leaks**. To achieve proper encapsulation across
-requests configure a new value for each incoming request in the [`'onRequest'`
-hook](./Hooks.md#onrequest). Example:
+In this example, the reference of the object would be shared with all the requests
+and **any mutation will impact all requests, potentially creating security
+vulnerabilities or memory leaks**, so Fastify blocks it.
+
+To achieve proper encapsulation across requests configure a new value for each
+incoming request in the [`'onRequest'` hook](./Hooks.md#onrequest). Example:
 
 ```js
 const fp = require('fastify-plugin')
@@ -219,18 +220,20 @@ fastify.decorateRequest('utility', function () {
 Note: using an arrow function will break the binding of `this` to the Fastify
 `Request` instance.
 
-Note: using `decorateRequest` will emit a warning if used with a reference type:
+Note: using `decorateRequest` will emit an error if used with a reference type:
 
 ```js
 // Don't do this
 fastify.decorateRequest('foo', { bar: 'fizz'})
 ```
-In this example, the reference of the object is shared with all the requests:
-**any mutation will impact all requests, potentially creating security
-vulnerabilities or memory leaks**.
+In this example, the reference of the object would be shared with all the requests
+and **any mutation will impact all requests, potentially creating security
+vulnerabilities or memory leaks**, so Fastify blocks it.
 
 To achieve proper encapsulation across requests configure a new value for each
-incoming request in the [`'onRequest'` hook](./Hooks.md#onrequest). Example:
+incoming request in the [`'onRequest'` hook](./Hooks.md#onrequest).
+
+Example:
 
 ```js
 const fp = require('fastify-plugin')
@@ -245,6 +248,29 @@ async function myPlugin (app) {
 module.exports = fp(myPlugin)
 ```
 
+The hook solution is more flexible and allows for more complex initialization
+because you can add more logic to the `onRequest` hook.
+
+Another approach is to use the getter/setter pattern, but it requires 2 decorators:
+
+```js
+fastify.decorateRequest('my_decorator_holder') // define the holder
+fastify.decorateRequest('user', {
+  getter () {
+    this.my_decorator_holder ??= {} // initialize the holder
+    return this.my_decorator_holder
+  }
+})
+
+fastify.get('/', async function (req, reply) {
+  req.user.access = 'granted'
+  // other code
+})
+```
+
+This ensures that the `user` property is always unique for each
+request.
+
 See [`decorate`](#decorate) for information about the `dependencies` parameter.
 
 #### `hasDecorator(name)`

docs/Reference/Errors.md

@@ -36,6 +36,7 @@
     - [FST_ERR_DEC_DEPENDENCY_INVALID_TYPE](#fst_err_dec_dependency_invalid_type)
     - [FST_ERR_DEC_MISSING_DEPENDENCY](#fst_err_dec_missing_dependency)
     - [FST_ERR_DEC_AFTER_START](#fst_err_dec_after_start)
+    - [FST_ERR_DEC_REFERENCE_TYPE](#fst_err_dec_reference_type)
     - [FST_ERR_HOOK_INVALID_TYPE](#fst_err_hook_invalid_type)
     - [FST_ERR_HOOK_INVALID_HANDLER](#fst_err_hook_invalid_handler)
     - [FST_ERR_HOOK_INVALID_ASYNC_HANDLER](#fst_err_hook_invalid_async_handler)
@@ -308,6 +309,7 @@ Below is a table with all the error codes that Fastify uses.
 | <a id="fst_err_dec_dependency_invalid_type">FST_ERR_DEC_DEPENDENCY_INVALID_TYPE</a> | The dependencies of decorator must be of type `Array`. | Use an array for the dependencies. | [#3090](https://github.com/fastify/fastify/pull/3090) |
 | <a id="fst_err_dec_missing_dependency">FST_ERR_DEC_MISSING_DEPENDENCY</a> | The decorator cannot be registered due to a missing dependency. | Register the missing dependency. | [#1168](https://github.com/fastify/fastify/pull/1168) |
 | <a id="fst_err_dec_after_start">FST_ERR_DEC_AFTER_START</a> | The decorator cannot be added after start. | Add the decorator before starting the server. | [#2128](https://github.com/fastify/fastify/pull/2128) |
+| <a id="fst_err_dec_reference_type">FST_ERR_DEC_REFERENCE_TYPE</a> | The decorator cannot be a reference type. | Define the decorator with a getter/setter interface or an empty decorator with a hook. | [#5462](https://github.com/fastify/fastify/pull/5462) |
 | <a id="fst_err_hook_invalid_type">FST_ERR_HOOK_INVALID_TYPE</a> | The hook name must be a string. | Use a string for the hook name. | [#1168](https://github.com/fastify/fastify/pull/1168) |
 | <a id="fst_err_hook_invalid_handler">FST_ERR_HOOK_INVALID_HANDLER</a> | The hook callback must be a function. | Use a function for the hook callback. | [#1168](https://github.com/fastify/fastify/pull/1168) |
 | <a id="fst_err_hook_invalid_async_handler">FST_ERR_HOOK_INVALID_ASYNC_HANDLER</a> | Async function has too many arguments. Async hooks should not use the `done` argument. | Remove the `done` argument from the async hook. | [#4367](https://github.com/fastify/fastify/pull/4367) |

docs/Reference/Warnings.md

@@ -9,7 +9,6 @@
     - [FSTWRN002](#FSTWRN002)
   - [Fastify Deprecation Codes](#fastify-deprecation-codes)
     - [FSTDEP005](#FSTDEP005)
-    - [FSTDEP006](#FSTDEP006)
     - [FSTDEP007](#FSTDEP007)
     - [FSTDEP008](#FSTDEP008)
     - [FSTDEP009](#FSTDEP009)
@@ -73,7 +72,6 @@ Deprecation codes are further supported by the Node.js CLI options:
 | Code | Description | How to solve | Discussion |
 | ---- | ----------- | ------------ | ---------- |
 | <a id="FSTDEP005">FSTDEP005</a> | You are accessing the deprecated `request.connection` property. | Use `request.socket`. | [#2594](https://github.com/fastify/fastify/pull/2594) |
-| <a id="FSTDEP006">FSTDEP006</a> | You are decorating Request/Reply with a reference type. This reference is shared amongst all requests. | Do not use Arrays/Objects as values when decorating Request/Reply. | [#2688](https://github.com/fastify/fastify/pull/2688) |
 | <a id="FSTDEP007">FSTDEP007</a> | You are trying to set a HEAD route using `exposeHeadRoute` route flag when a sibling route is already set. | Remove `exposeHeadRoutes` or explicitly set `exposeHeadRoutes` to `false` | [#2700](https://github.com/fastify/fastify/pull/2700) |
 | <a id="FSTDEP008">FSTDEP008</a> | You are using route constraints via the route `{version: "..."}` option.  |  Use `{constraints: {version: "..."}}` option.  | [#2682](https://github.com/fastify/fastify/pull/2682) |
 | <a id="FSTDEP009">FSTDEP009</a> | You are using a custom route versioning strategy via the server `{versioning: "..."}` option. |  Use `{constraints: {version: "..."}}` option.  | [#2682](https://github.com/fastify/fastify/pull/2682) |

lib/decorate.js

@@ -13,11 +13,10 @@ const {
   FST_ERR_DEC_ALREADY_PRESENT,
   FST_ERR_DEC_MISSING_DEPENDENCY,
   FST_ERR_DEC_AFTER_START,
+  FST_ERR_DEC_REFERENCE_TYPE,
   FST_ERR_DEC_DEPENDENCY_INVALID_TYPE
 } = require('./errors')
 
-const { FSTDEP006 } = require('./warnings')
-
 function decorate (instance, name, fn, dependencies) {
   if (Object.prototype.hasOwnProperty.call(instance, name)) {
     throw new FST_ERR_DEC_ALREADY_PRESENT(name)
@@ -58,7 +57,7 @@ function decorateConstructor (konstructor, name, fn, dependencies) {
 
 function checkReferenceType (name, fn) {
   if (typeof fn === 'object' && fn && !(typeof fn.getter === 'function' || typeof fn.setter === 'function')) {
-    FSTDEP006(name)
+    throw new FST_ERR_DEC_REFERENCE_TYPE(name, typeof fn)
   }
 }
 

lib/errors.js

@@ -151,6 +151,10 @@ const codes = {
     'FST_ERR_DEC_AFTER_START',
     "The decorator '%s' has been added after start!"
   ),
+  FST_ERR_DEC_REFERENCE_TYPE: createError(
+    'FST_ERR_DEC_REFERENCE_TYPE',
+    "The decorator '%s' of type '%s' is a reference type. Use the { getter, setter } interface instead."
+  ),
 
   /**
    * hooks

lib/warnings.js

@@ -5,7 +5,6 @@ const { createDeprecation, createWarning } = require('process-warning')
 /**
  * Deprecation codes:
  *   - FSTDEP005
- *   - FSTDEP006
  *   - FSTDEP007
  *   - FSTDEP008
  *   - FSTDEP009
@@ -26,11 +25,6 @@ const FSTDEP005 = createDeprecation({
   message: 'You are accessing the deprecated "request.connection" property. Use "request.socket" instead.'
 })
 
-const FSTDEP006 = createDeprecation({
-  code: 'FSTDEP006',
-  message: 'You are decorating Request/Reply with a reference type. This reference is shared amongst all requests. Use onRequest hook instead. Property: %s'
-})
-
 const FSTDEP007 = createDeprecation({
   code: 'FSTDEP007',
   message: 'You are trying to set a HEAD route using "exposeHeadRoute" route flag when a sibling route is already set. See documentation for more info.'
@@ -107,7 +101,6 @@ const FSTSEC001 = createWarning({
 
 module.exports = {
   FSTDEP005,
-  FSTDEP006,
   FSTDEP007,
   FSTDEP008,
   FSTDEP009,

test/decorator-namespace.test._js_

@@ -0,0 +1,31 @@
+'use strict'
+
+/* eslint no-prototype-builtins: 0 */
+
+const t = require('tap')
+const test = t.test
+const Fastify = require('..')
+const fp = require('fastify-plugin')
+
+test('plugin namespace', async t => {
+  t.plan(2)
+  const app = Fastify()
+
+  await app.register(async function plugin (app, opts) {
+    app.decorate('utility', function () {
+      return 'utility'
+    })
+
+    app.get('/', function (req, reply) {
+      // ! here the plugin would use app.utility()
+      // ! the plugin does not know about the namespace
+      reply.send({ utility: app.utility() })
+    })
+  }, { namepace: 'foo' })
+
+  // ! but outside the plugin the decorator would be app.foo.utility()
+  t.ok(app.foo.utility)
+
+  const res = await app.inject('/')
+  t.same(res.json(), { utility: 'utility' })
+})

test/decorator.test.js

@@ -8,7 +8,6 @@ const Fastify = require('..')
 const fp = require('fastify-plugin')
 const sget = require('simple-get').concat
 const symbols = require('../lib/symbols.js')
-const proxyquire = require('proxyquire')
 
 test('server methods should exist', t => {
   t.plan(2)
@@ -789,49 +788,44 @@ test('decorate* should throw if called after ready', async t => {
   await fastify.close()
 })
 
-test('decorate* should emit warning if an array is passed', t => {
-  t.plan(1)
+test('decorate* should emit error if an array is passed', t => {
+  t.plan(2)
 
-  function onWarning (name) {
-    t.equal(name, 'test_array')
+  const fastify = Fastify()
+  try {
+    fastify.decorateRequest('test_array', [])
+    t.fail('should not decorate')
+  } catch (err) {
+    t.same(err.code, 'FST_ERR_DEC_REFERENCE_TYPE')
+    t.same(err.message, "The decorator 'test_array' of type 'object' is a reference type. Use the { getter, setter } interface instead.")
   }
+})
 
-  const decorate = proxyquire('../lib/decorate', {
-    './warnings': {
-      FSTDEP006: onWarning
-    }
-  })
-  const fastify = proxyquire('..', { './lib/decorate.js': decorate })()
-  fastify.decorateRequest('test_array', [])
+test('server.decorate should not emit error if reference type is passed', async t => {
+  t.plan(1)
+
+  const fastify = Fastify()
+  fastify.decorate('test_array', [])
+  fastify.decorate('test_object', {})
+  await fastify.ready()
+  t.pass('Done')
 })
 
 test('decorate* should emit warning if object type is passed', t => {
-  t.plan(1)
+  t.plan(2)
 
-  function onWarning (name) {
-    t.equal(name, 'test_object')
+  const fastify = Fastify()
+  try {
+    fastify.decorateRequest('test_object', { foo: 'bar' })
+    t.fail('should not decorate')
+  } catch (err) {
+    t.same(err.code, 'FST_ERR_DEC_REFERENCE_TYPE')
+    t.same(err.message, "The decorator 'test_object' of type 'object' is a reference type. Use the { getter, setter } interface instead.")
   }
-
-  const decorate = proxyquire('../lib/decorate', {
-    './warnings': {
-      FSTDEP006: onWarning
-    }
-  })
-  const fastify = proxyquire('..', { './lib/decorate.js': decorate })()
-  fastify.decorateRequest('test_object', { foo: 'bar' })
 })
 
 test('decorate* should not emit warning if object with getter/setter is passed', t => {
-  function onWarning (warning) {
-    t.fail('Should not call a warn')
-  }
-
-  const decorate = proxyquire('../lib/decorate', {
-    './warnings': {
-      FSTDEP006: onWarning
-    }
-  })
-  const fastify = proxyquire('..', { './lib/decorate.js': decorate })()
+  const fastify = Fastify()
 
   fastify.decorateRequest('test_getter_setter', {
     setter (val) {
@@ -844,17 +838,74 @@ test('decorate* should not emit warning if object with getter/setter is passed',
   t.end('Done')
 })
 
-test('decorate* should not emit warning if string,bool,numbers are passed', t => {
-  function onWarning (warning) {
-    t.fail('Should not call a warn')
-  }
+test('decorateRequest with getter/setter can handle encapsulation', async t => {
+  t.plan(24)
 
-  const decorate = proxyquire('../lib/decorate', {
-    './warnings': {
-      FSTDEP006: onWarning
+  const fastify = Fastify({ logger: true })
+
+  fastify.decorateRequest('test_getter_setter_holder')
+  fastify.decorateRequest('test_getter_setter', {
+    getter () {
+      this.test_getter_setter_holder ??= {}
+      return this.test_getter_setter_holder
     }
   })
-  const fastify = proxyquire('..', { './lib/decorate.js': decorate })()
+
+  fastify.get('/', async function (req, reply) {
+    t.same(req.test_getter_setter, {}, 'a getter')
+    req.test_getter_setter.a = req.id
+    t.same(req.test_getter_setter, { a: req.id })
+  })
+
+  fastify.addHook('onResponse', async function hook (req, reply) {
+    t.same(req.test_getter_setter, { a: req.id })
+  })
+
+  await Promise.all([
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200))
+  ])
+})
+
+test('decorateRequest with getter/setter can handle encapsulation with arrays', async t => {
+  t.plan(24)
+
+  const fastify = Fastify({ logger: true })
+
+  fastify.decorateRequest('array_holder')
+  fastify.decorateRequest('my_array', {
+    getter () {
+      this.array_holder ??= []
+      return this.array_holder
+    }
+  })
+
+  fastify.get('/', async function (req, reply) {
+    t.same(req.my_array, [])
+    req.my_array.push(req.id)
+    t.same(req.my_array, [req.id])
+  })
+
+  fastify.addHook('onResponse', async function hook (req, reply) {
+    t.same(req.my_array, [req.id])
+  })
+
+  await Promise.all([
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200)),
+    fastify.inject('/').then(res => t.same(res.statusCode, 200))
+  ])
+})
+
+test('decorate* should not emit error if string,bool,numbers are passed', t => {
+  const fastify = Fastify()
 
   fastify.decorateRequest('test_str', 'foo')
   fastify.decorateRequest('test_bool', true)