fix: cache duplicated symbols (fastify#4612)

* refactor: cleanup duplicated symbols

* refactor: rename to kRequestCacheValidateFns

* refactor: rename to kReplyCacheSerializeFns

* test: extend test cases

* test: add test case for context

Author: metcoder95

lib/context.js

@@ -11,8 +11,8 @@ const {
   kLogLevel,
   kContentTypeParser,
   kRouteByFastify,
-  kRequestValidateWeakMap,
-  kReplySerializeWeakMap,
+  kRequestCacheValidateFns,
+  kReplyCacheSerializeFns,
   kPublicRouteContext
 } = require('./symbols.js')
 
@@ -68,8 +68,8 @@ function Context ({
     defaultSchemaErrorFormatter
   this[kRouteByFastify] = isFastify
 
-  this[kRequestValidateWeakMap] = null
-  this[kReplySerializeWeakMap] = null
+  this[kRequestCacheValidateFns] = null
+  this[kReplyCacheSerializeFns] = null
   this.validatorCompiler = validatorCompiler || null
   this.serializerCompiler = serializerCompiler || null
 

lib/reply.js

@@ -18,7 +18,7 @@ const {
   kReplyNextErrorHandler,
   kDisableRequestLogging,
   kSchemaResponse,
-  kReplySerializeWeakMap,
+  kReplyCacheSerializeFns,
   kSchemaController,
   kOptions,
   kRouteContext
@@ -323,7 +323,7 @@ Reply.prototype.getSerializationFunction = function (schemaOrStatus, contentType
       serialize = this[kRouteContext][kSchemaResponse]?.[schemaOrStatus]
     }
   } else if (typeof schemaOrStatus === 'object') {
-    serialize = this[kRouteContext][kReplySerializeWeakMap]?.get(schemaOrStatus)
+    serialize = this[kRouteContext][kReplyCacheSerializeFns]?.get(schemaOrStatus)
   }
 
   return serialize
@@ -334,8 +334,8 @@ Reply.prototype.compileSerializationSchema = function (schema, httpStatus = null
   const { method, url } = request
 
   // Check if serialize function already compiled
-  if (this[kRouteContext][kReplySerializeWeakMap]?.has(schema)) {
-    return this[kRouteContext][kReplySerializeWeakMap].get(schema)
+  if (this[kRouteContext][kReplyCacheSerializeFns]?.has(schema)) {
+    return this[kRouteContext][kReplyCacheSerializeFns].get(schema)
   }
 
   const serializerCompiler = this[kRouteContext].serializerCompiler ||
@@ -360,11 +360,11 @@ Reply.prototype.compileSerializationSchema = function (schema, httpStatus = null
   // if it is not used
   // TODO: Explore a central cache for all the schemas shared across
   // encapsulated contexts
-  if (this[kRouteContext][kReplySerializeWeakMap] == null) {
-    this[kRouteContext][kReplySerializeWeakMap] = new WeakMap()
+  if (this[kRouteContext][kReplyCacheSerializeFns] == null) {
+    this[kRouteContext][kReplyCacheSerializeFns] = new WeakMap()
   }
 
-  this[kRouteContext][kReplySerializeWeakMap].set(schema, serializeFn)
+  this[kRouteContext][kReplyCacheSerializeFns].set(schema, serializeFn)
 
   return serializeFn
 }
@@ -393,8 +393,8 @@ Reply.prototype.serializeInput = function (input, schema, httpStatus, contentTyp
     }
   } else {
     // Check if serialize function already compiled
-    if (this[kRouteContext][kReplySerializeWeakMap]?.has(schema)) {
-      serialize = this[kRouteContext][kReplySerializeWeakMap].get(schema)
+    if (this[kRouteContext][kReplyCacheSerializeFns]?.has(schema)) {
+      serialize = this[kRouteContext][kReplyCacheSerializeFns].get(schema)
     } else {
       serialize = this.compileSerializationSchema(schema, httpStatus, contentType)
     }

lib/request.js

@@ -11,7 +11,7 @@ const {
   kSchemaQuerystring,
   kSchemaController,
   kOptions,
-  kRequestValidateWeakMap,
+  kRequestCacheValidateFns,
   kRouteContext,
   kPublicRouteContext
 } = require('./symbols')
@@ -254,16 +254,16 @@ Object.defineProperties(Request.prototype, {
         const symbol = HTTP_PART_SYMBOL_MAP[httpPartOrSchema]
         return this[kRouteContext][symbol]
       } else if (typeof httpPartOrSchema === 'object') {
-        return this[kRouteContext][kRequestValidateWeakMap]?.get(httpPartOrSchema)
+        return this[kRouteContext][kRequestCacheValidateFns]?.get(httpPartOrSchema)
       }
     }
   },
   compileValidationSchema: {
     value: function (schema, httpPart = null) {
       const { method, url } = this
 
-      if (this[kRouteContext][kRequestValidateWeakMap]?.has(schema)) {
-        return this[kRouteContext][kRequestValidateWeakMap].get(schema)
+      if (this[kRouteContext][kRequestCacheValidateFns]?.has(schema)) {
+        return this[kRouteContext][kRequestCacheValidateFns].get(schema)
       }
 
       const validatorCompiler = this[kRouteContext].validatorCompiler ||
@@ -287,11 +287,11 @@ Object.defineProperties(Request.prototype, {
       // if it is not used
       // TODO: Explore a central cache for all the schemas shared across
       // encapsulated contexts
-      if (this[kRouteContext][kRequestValidateWeakMap] == null) {
-        this[kRouteContext][kRequestValidateWeakMap] = new WeakMap()
+      if (this[kRouteContext][kRequestCacheValidateFns] == null) {
+        this[kRouteContext][kRequestCacheValidateFns] = new WeakMap()
       }
 
-      this[kRouteContext][kRequestValidateWeakMap].set(schema, validateFn)
+      this[kRouteContext][kRequestCacheValidateFns].set(schema, validateFn)
 
       return validateFn
     }
@@ -317,8 +317,8 @@ Object.defineProperties(Request.prototype, {
       }
 
       if (validate == null) {
-        if (this[kRouteContext][kRequestValidateWeakMap]?.has(schema)) {
-          validate = this[kRouteContext][kRequestValidateWeakMap].get(schema)
+        if (this[kRouteContext][kRequestCacheValidateFns]?.has(schema)) {
+          validate = this[kRouteContext][kRequestCacheValidateFns].get(schema)
         } else {
           // We proceed to compile if there's no validate function yet
           validate = this.compileValidationSchema(schema, httpPart)

lib/symbols.js

@@ -27,10 +27,9 @@ const keys = {
   kSchemaVisited: Symbol('fastify.schemas.visited'),
   // Request
   kRequest: Symbol('fastify.Request'),
-  kRequestValidateFns: Symbol('fastify.request.cache.validateFns'),
   kRequestPayloadStream: Symbol('fastify.RequestPayloadStream'),
   kRequestAcceptVersion: Symbol('fastify.RequestAcceptVersion'),
-  kRequestValidateWeakMap: Symbol('fastify.request.cache.validators'),
+  kRequestCacheValidateFns: Symbol('fastify.request.cache.validateFns'),
   // 404
   kFourOhFour: Symbol('fastify.404'),
   kCanSetNotFoundHandler: Symbol('fastify.canSetNotFoundHandler'),
@@ -51,7 +50,7 @@ const keys = {
   kReplyErrorHandlerCalled: Symbol('fastify.reply.errorHandlerCalled'),
   kReplyIsRunningOnErrorHook: Symbol('fastify.reply.isRunningOnErrorHook'),
   kReplySerializerDefault: Symbol('fastify.replySerializerDefault'),
-  kReplySerializeWeakMap: Symbol('fastify.reply.cache.serializeFns'),
+  kReplyCacheSerializeFns: Symbol('fastify.reply.cache.serializeFns'),
   // This symbol is only meant to be used for fastify tests and should not be used for any other purpose
   kTestInternals: Symbol('fastify.testInternals'),
   kErrorHandler: Symbol('fastify.errorHandler'),

test/internals/context.test.js

@@ -0,0 +1,33 @@
+'use strict'
+
+const { test } = require('tap')
+
+const { kRouteContext } = require('../../lib/symbols')
+const Context = require('../../lib/context')
+
+const Fastify = require('../..')
+
+test('context', context => {
+  context.plan(1)
+
+  context.test('Should not contain undefined as key prop', async t => {
+    const app = Fastify()
+
+    app.get('/', (req, reply) => {
+      t.type(req[kRouteContext], Context)
+      t.type(reply[kRouteContext], Context)
+      t.notOk('undefined' in reply[kRouteContext])
+      t.notOk('undefined' in req[kRouteContext])
+
+      reply.send('hello world!')
+    })
+
+    try {
+      await app.inject('/')
+    } catch (e) {
+      t.fail(e)
+    }
+
+    t.plan(4)
+  })
+})

test/internals/reply-serialize.test.js

@@ -1,7 +1,7 @@
 'use strict'
 
 const { test } = require('tap')
-const { kReplySerializeWeakMap, kRouteContext } = require('../../lib/symbols')
+const { kReplyCacheSerializeFns, kRouteContext } = require('../../lib/symbols')
 const Fastify = require('../../fastify')
 
 function getDefaultSchema () {
@@ -207,9 +207,9 @@ test('Reply#compileSerializationSchema', t => {
     fastify.get('/', (req, reply) => {
       const input = { hello: 'world' }
 
-      t.equal(reply[kRouteContext][kReplySerializeWeakMap], null)
+      t.equal(reply[kRouteContext][kReplyCacheSerializeFns], null)
       t.equal(reply.compileSerializationSchema(getDefaultSchema())(input), JSON.stringify(input))
-      t.type(reply[kRouteContext][kReplySerializeWeakMap], WeakMap)
+      t.type(reply[kRouteContext][kReplyCacheSerializeFns], WeakMap)
       t.equal(reply.compileSerializationSchema(getDefaultSchema())(input), JSON.stringify(input))
 
       reply.send({ hello: 'world' })
@@ -408,9 +408,9 @@ test('Reply#getSerializationFunction', t => {
 
     fastify.get('/', (req, reply) => {
       t.notOk(reply.getSerializationFunction(getDefaultSchema()))
-      t.equal(reply[kRouteContext][kReplySerializeWeakMap], null)
+      t.equal(reply[kRouteContext][kReplyCacheSerializeFns], null)
       t.notOk(reply.getSerializationFunction('200'))
-      t.equal(reply[kRouteContext][kReplySerializeWeakMap], null)
+      t.equal(reply[kRouteContext][kReplyCacheSerializeFns], null)
 
       reply.send({ hello: 'world' })
     })
@@ -684,9 +684,9 @@ test('Reply#serializeInput', t => {
 
     fastify.get('/', (req, reply) => {
       const input = { hello: 'world' }
-      t.equal(reply[kRouteContext][kReplySerializeWeakMap], null)
+      t.equal(reply[kRouteContext][kReplyCacheSerializeFns], null)
       t.equal(reply.serializeInput(input, getDefaultSchema()), JSON.stringify(input))
-      t.type(reply[kRouteContext][kReplySerializeWeakMap], WeakMap)
+      t.type(reply[kRouteContext][kReplyCacheSerializeFns], WeakMap)
 
       reply.send({ hello: 'world' })
     })

test/internals/reply.test.js

@@ -32,7 +32,7 @@ const doGet = function (url) {
 }
 
 test('Once called, Reply should return an object with methods', t => {
-  t.plan(13)
+  t.plan(14)
   const response = { res: 'res' }
   const context = {}
   const request = { [kRouteContext]: context }
@@ -50,6 +50,8 @@ test('Once called, Reply should return an object with methods', t => {
   t.same(reply.raw, response)
   t.equal(reply[kRouteContext], context)
   t.equal(reply.request, request)
+  // Aim to not bad property keys (including Symbols)
+  t.notOk('undefined' in reply)
 })
 
 test('reply.send will logStream error and destroy the stream', t => {

test/internals/request-validate.test.js

@@ -2,7 +2,7 @@
 
 const { test } = require('tap')
 const Ajv = require('ajv')
-const { kRequestValidateWeakMap, kRouteContext } = require('../../lib/symbols')
+const { kRequestCacheValidateFns, kRouteContext } = require('../../lib/symbols')
 const Fastify = require('../../fastify')
 
 const defaultSchema = {
@@ -231,11 +231,11 @@ test('#compileValidationSchema', subtest => {
       t.plan(5)
 
       fastify.get('/', (req, reply) => {
-        t.equal(req[kRouteContext][kRequestValidateWeakMap], null)
+        t.equal(req[kRouteContext][kRequestCacheValidateFns], null)
         t.type(req.compileValidationSchema(defaultSchema), Function)
-        t.type(req[kRouteContext][kRequestValidateWeakMap], WeakMap)
+        t.type(req[kRouteContext][kRequestCacheValidateFns], WeakMap)
         t.type(req.compileValidationSchema(Object.assign({}, defaultSchema)), Function)
-        t.type(req[kRouteContext][kRequestValidateWeakMap], WeakMap)
+        t.type(req[kRouteContext][kRequestCacheValidateFns], WeakMap)
 
         reply.send({ hello: 'world' })
       })
@@ -424,7 +424,7 @@ test('#getValidationFunction', subtest => {
       req.getValidationFunction(defaultSchema)
       req.getValidationFunction('body')
 
-      t.equal(req[kRouteContext][kRequestValidateWeakMap], null)
+      t.equal(req[kRouteContext][kRequestCacheValidateFns], null)
       reply.send({ hello: 'world' })
     })
 
@@ -724,9 +724,9 @@ test('#validate', subtest => {
       t.plan(3)
 
       fastify.get('/', (req, reply) => {
-        t.equal(req[kRouteContext][kRequestValidateWeakMap], null)
+        t.equal(req[kRouteContext][kRequestCacheValidateFns], null)
         t.equal(req.validateInput({ hello: 'world' }, defaultSchema), true)
-        t.type(req[kRouteContext][kRequestValidateWeakMap], WeakMap)
+        t.type(req[kRouteContext][kRequestCacheValidateFns], WeakMap)
 
         reply.send({ hello: 'world' })
       })

test/internals/request.test.js

@@ -66,6 +66,8 @@ test('Regular request', t => {
   t.equal(request.routerMethod, context.config.method)
   t.equal(request.routeConfig, context[kPublicRouteContext].config)
   t.equal(request.routeSchema, context[kPublicRouteContext].schema)
+  // Aim to not bad property keys (including Symbols)
+  t.notOk('undefined' in request)
 
   // This will be removed, it's deprecated
   t.equal(request.connection, req.connection)