Revert "Revert "perf: optimize required props serialization (#675)""

This reverts commit 8e83aae.
fastify · Feb 5, 2024 · 1af56e3 · 1af56e3
1 parent 35346a7
commit 1af56e3
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 68 deletions.
diff --git a/index.js b/index.js
@@ -27,8 +27,6 @@ const validLargeArrayMechanisms = [
   'json-stringify'
 ]
 
-const addComma = '!addComma && (addComma = true) || (json += \',\')'
-
 let schemaIdCounter = 0
 
 const mergedSchemaRef = Symbol('fjs-merged-schema-ref')
@@ -262,7 +260,7 @@ function inferTypeByKeyword (schema) {
   return schema.type
 }
 
-function buildExtraObjectPropertiesSerializer (context, location) {
+function buildExtraObjectPropertiesSerializer (context, location, addComma) {
   const schema = location.schema
   const propertiesKeys = Object.keys(schema.properties || {})
 
@@ -321,88 +319,76 @@ function buildExtraObjectPropertiesSerializer (context, location) {
 }
 
 function buildInnerObject (context, location) {
-  let code = ''
   const schema = location.schema
-  const required = schema.required || []
 
   const propertiesLocation = location.getPropertyLocation('properties')
+  const requiredProperties = schema.required || []
+
+  // Should serialize required properties first
+  const propertiesKeys = Object.keys(schema.properties || {}).sort(
+    (key1, key2) => {
+      const required1 = requiredProperties.includes(key1)
+      const required2 = requiredProperties.includes(key2)
+      return required1 === required2 ? 0 : required1 ? -1 : 1
+    }
+  )
+  const hasRequiredProperties = requiredProperties.includes(propertiesKeys[0])
 
-  const requiredWithDefault = []
-  const requiredWithoutDefault = []
-  if (schema.properties) {
-    for (const key of Object.keys(schema.properties)) {
-      if (required.indexOf(key) === -1) {
-        continue
-      }
-      let propertyLocation = propertiesLocation.getPropertyLocation(key)
-      if (propertyLocation.schema.$ref) {
-        propertyLocation = resolveRef(context, propertyLocation)
-      }
-
-      const sanitizedKey = JSON.stringify(key)
+  let code = ''
 
-      // Using obj['key'] !== undefined instead of obj.hasOwnProperty(prop) for perf reasons,
-      // see https://github.com/mcollina/fast-json-stringify/pull/3 for discussion.
-      const defaultValue = propertyLocation.schema.default
-      if (defaultValue === undefined) {
-        code += `if (obj[${sanitizedKey}] === undefined) throw new Error('${sanitizedKey} is required!')\n`
-        requiredWithoutDefault.push(key)
-      }
-      requiredWithDefault.push(key)
+  for (const key of requiredProperties) {
+    if (!propertiesKeys.includes(key)) {
+      code += `if (obj['${key}'] === undefined) throw new Error('"${key}" is required!')\n`
     }
   }
 
-  // handle extraneous required fields
-  for (const requiredProperty of required) {
-    if (requiredWithDefault.indexOf(requiredProperty) !== -1) continue
-    code += `if (obj['${requiredProperty}'] === undefined) throw new Error('"${requiredProperty}" is required!')\n`
-  }
+  code += 'let json = \'{\'\n'
 
-  code += `
-    let addComma = false
-    let json = '{'
-  `
+  let addComma = ''
+  if (!hasRequiredProperties) {
+    code += 'let addComma = false\n'
+    addComma = '!addComma && (addComma = true) || (json += \',\')'
+  }
 
-  if (schema.properties) {
-    for (const key of Object.keys(schema.properties)) {
-      let propertyLocation = propertiesLocation.getPropertyLocation(key)
-      if (propertyLocation.schema.$ref) {
-        propertyLocation = resolveRef(context, propertyLocation)
-      }
+  for (const key of propertiesKeys) {
+    let propertyLocation = propertiesLocation.getPropertyLocation(key)
+    if (propertyLocation.schema.$ref) {
+      propertyLocation = resolveRef(context, propertyLocation)
+    }
 
-      const sanitizedKey = JSON.stringify(key)
+    const sanitizedKey = JSON.stringify(key)
+    const defaultValue = propertyLocation.schema.default
+    const isRequired = requiredProperties.includes(key)
 
-      if (requiredWithoutDefault.indexOf(key) !== -1) {
-        code += `
+    code += `
+      if (obj[${sanitizedKey}] !== undefined) {
         ${addComma}
         json += ${JSON.stringify(sanitizedKey + ':')}
         ${buildValue(context, propertyLocation, `obj[${sanitizedKey}]`)}
+      }`
+
+    if (defaultValue !== undefined) {
+      code += ` else {
+        ${addComma}
+        json += ${JSON.stringify(sanitizedKey + ':' + JSON.stringify(defaultValue))}
+      }
       `
-      } else {
-        // Using obj['key'] !== undefined instead of obj.hasOwnProperty(prop) for perf reasons,
-        // see https://github.com/mcollina/fast-json-stringify/pull/3 for discussion.
-        code += `
-        if (obj[${sanitizedKey}] !== undefined) {
-          ${addComma}
-          json += ${JSON.stringify(sanitizedKey + ':')}
-          ${buildValue(context, propertyLocation, `obj[${sanitizedKey}]`)}
-        }
-        `
-        const defaultValue = propertyLocation.schema.default
-        if (defaultValue !== undefined) {
-          code += `
-        else {
-          ${addComma}
-          json += ${JSON.stringify(sanitizedKey + ':' + JSON.stringify(defaultValue))}
-        }
-        `
-        }
+    } else if (isRequired) {
+      code += ` else {
+        throw new Error('${sanitizedKey} is required!')
       }
+      `
+    } else {
+      code += '\n'
+    }
+
+    if (hasRequiredProperties) {
+      addComma = 'json += \',\''
     }
   }
 
   if (schema.patternProperties || schema.additionalProperties) {
-    code += buildExtraObjectPropertiesSerializer(context, location)
+    code += buildExtraObjectPropertiesSerializer(context, location, addComma)
   }
 
   code += `

diff --git a/test/allof.test.js b/test/allof.test.js
@@ -121,7 +121,7 @@ test('object with allOf and multiple schema on the allOf', (t) => {
     id: 1,
     name: 'string',
     tag: 'otherString'
-  }), '{"name":"string","tag":"otherString","id":1}')
+  }), '{"name":"string","id":1,"tag":"otherString"}')
 })
 
 test('object with allOf and one schema on the allOf', (t) => {

diff --git a/test/required.test.js b/test/required.test.js
@@ -126,17 +126,18 @@ test('object with multiple required field not in properties schema', (t) => {
     stringify({})
     t.fail()
   } catch (e) {
-    t.equal(e.message, '"num" is required!')
+    t.equal(e.message, '"key1" is required!')
     t.pass()
   }
 
   try {
     stringify({
-      num: 42
+      key1: 42,
+      key2: 42
     })
     t.fail()
   } catch (e) {
-    t.equal(e.message, '"key1" is required!')
+    t.equal(e.message, '"num" is required!')
     t.pass()
   }