fix: allocate less in interim (#973)

Author: ferhatelmas

src/http/plugins/header-validator.ts

@@ -28,12 +28,16 @@ export const headerValidator = (options: HeaderValidatorOptions = {}) =>
           return payload
         }
 
-        const headers = Object.entries(reply.getHeaders())
-        for (const [key, value] of headers) {
+        const headers = reply.getHeaders()
+        for (const key in headers) {
+          if (!Object.prototype.hasOwnProperty.call(headers, key)) {
+            continue
+          }
+          const value = headers[key]
           if (typeof value === 'string' && INVALID_HEADER_CHAR_PATTERN.test(value)) {
             throw ERRORS.InvalidHeaderChar(key, value)
           } else if (Array.isArray(value)) {
-            for (let item of value) {
+            for (const item of value) {
               if (typeof item === 'string' && INVALID_HEADER_CHAR_PATTERN.test(item)) {
                 throw ERRORS.InvalidHeaderChar(key, item)
               }

src/http/plugins/log-request.ts

@@ -57,20 +57,47 @@ export const logRequest = (options: RequestLoggerOptions) =>
        * Adds req.resources and req.operation to the request object
        */
       fastify.addHook('preHandler', async (req) => {
-        const resourceFromParams = Object.values(req.params || {}).join('/')
-        const resources = getFirstDefined<string[]>(
-          req.resources,
-          req.routeOptions.config.resources?.(req),
-          (req.raw as any).resources,
-          resourceFromParams ? [resourceFromParams] : ([] as string[])
-        )
+        let resources = req.resources
+
+        if (resources === undefined) {
+          resources = req.routeOptions.config.resources?.(req)
+        }
+
+        if (resources === undefined) {
+          resources = (req.raw as any).resources
+        }
+
+        if (resources === undefined) {
+          const params = req.params as Record<string, unknown> | undefined
+          let resourceFromParams = ''
+
+          if (params) {
+            let first = true
+            for (const key in params) {
+              if (!Object.prototype.hasOwnProperty.call(params, key)) {
+                continue
+              }
+
+              if (!first) {
+                resourceFromParams += '/'
+              }
+
+              const value = params[key]
+              resourceFromParams += value == null ? '' : String(value)
+              first = false
+            }
+          }
+
+          resources = resourceFromParams ? [resourceFromParams] : []
+        }
 
         if (resources && resources.length > 0) {
-          resources.map((resource, index) => {
+          for (let index = 0; index < resources.length; index++) {
+            const resource = resources[index]
             if (!resource.startsWith('/')) {
               resources[index] = `/${resource}`
             }
-          })
+          }
         }
 
         req.resources = resources
@@ -170,12 +197,3 @@ function doRequestLog(req: FastifyRequest, options: LogRequestOptions) {
     serverTimes: req.serverTimings,
   })
 }
-
-function getFirstDefined<T>(...values: any[]): T | undefined {
-  for (const value of values) {
-    if (value !== undefined) {
-      return value
-    }
-  }
-  return undefined
-}

src/http/routes/s3/index.ts

@@ -12,7 +12,7 @@ import {
   xmlParser,
 } from '../../plugins'
 import { s3ErrorHandler } from './error-handler'
-import { findArrayPathsInSchemas, getRouter, RequestInput } from './router'
+import { findArrayPathsInSchemas, getRouter, RequestInput, RouteQuery } from './router'
 
 const { s3ProtocolEnabled } = getConfig()
 
@@ -41,7 +41,7 @@ export default async function routes(fastify: FastifyInstance) {
             if (
               s3Router.matchRoute(route, {
                 type: req.isIcebergBucket ? 'iceberg' : undefined,
-                query: (req.query as Record<string, string>) || {},
+                query: (req.query as RouteQuery) || {},
                 headers: (req.headers as Record<string, string>) || {},
               })
             ) {

src/http/routes/s3/router.ts

@@ -100,11 +100,18 @@ type Handler<Req extends Schema, Context = unknown> = (
   ctx: Context
 ) => Promise<ResponseType>
 
+export type QuerystringMatch = {
+  key: string
+  value: string | undefined
+}
+
+export type RouteQuery = Record<string, string | undefined>
+
 type Route<S extends Schema, Context> = {
   method: HTTPMethod
   type?: string
   path: string
-  querystringMatches: { key: string; value: string }[]
+  querystringMatches: QuerystringMatch[]
   headersMatches: string[]
   handler?: Handler<S, Context>
   schema: S
@@ -241,7 +248,7 @@ export class Router<Context = unknown, S extends Schema = Schema> {
     this.registerRoute('head', url, options, handler as any)
   }
 
-  parseQueryMatch(query: string) {
+  parseQueryMatch(query: string): QuerystringMatch {
     const [key, value] = query.split('=')
     return { key, value }
   }
@@ -262,7 +269,7 @@ export class Router<Context = unknown, S extends Schema = Schema> {
 
   matchRoute(
     route: Route<S, Context>,
-    match: { query: Record<string, string>; headers: Record<string, string>; type?: string }
+    match: { query: RouteQuery; headers: Record<string, string>; type?: string }
   ) {
     const isOfType = match.type ? match.type === route.type : route.type === undefined
 
@@ -294,31 +301,46 @@ export class Router<Context = unknown, S extends Schema = Schema> {
     })
   }
 
-  protected matchQueryString(
-    matches: { key: string; value: string }[],
-    received?: Record<string, string>
-  ) {
-    const keys = Object.keys(received || {})
-    if (keys.length === 0 || !received) {
-      return matches.find((m) => m.key === '*')
+  protected matchQueryString(matches: QuerystringMatch[], received?: RouteQuery) {
+    let hasWildcard = false
+    for (const match of matches) {
+      if (match.key === '*') {
+        hasWildcard = true
+        break
+      }
     }
 
-    const foundMatches = matches.every((m) => {
-      const key = Object.keys(received).find((k) => k === m.key)
-      return (
-        (m.key === key && m.value !== undefined && m.value === received[m.key]) ||
-        (m.key === key && m.value === undefined)
-      )
-    })
+    if (!received) {
+      return hasWildcard
+    }
 
-    if (foundMatches) {
-      return true
+    let hasReceivedQuery = false
+    for (const key in received) {
+      if (Object.prototype.hasOwnProperty.call(received, key)) {
+        hasReceivedQuery = true
+        break
+      }
     }
 
-    if (!foundMatches && matches.find((m) => m.key === '*')) {
-      return true
+    if (!hasReceivedQuery) {
+      return hasWildcard
     }
-    return false
+
+    for (const match of matches) {
+      if (match.key === '*') {
+        continue
+      }
+
+      if (!Object.prototype.hasOwnProperty.call(received, match.key)) {
+        return hasWildcard
+      }
+
+      if (match.value !== undefined && match.value !== received[match.key]) {
+        return hasWildcard
+      }
+    }
+
+    return true
   }
 }
 

src/storage/protocols/s3/s3-handler.ts

@@ -1319,14 +1319,29 @@ export class S3ProtocolHandler {
   }
 
   parseMetadataHeaders(headers: Record<string, unknown>): Record<string, string> | undefined {
-    let metadata: Record<string, unknown> | undefined = undefined
+    let metadata: Record<string, string> | undefined
+    const metadataPrefix = 'x-amz-meta-'
 
-    Object.keys(headers)
-      .filter((key) => key.startsWith('x-amz-meta-'))
-      .forEach((key) => {
-        if (!metadata) metadata = {}
-        metadata[key.replace('x-amz-meta-', '')] = headers[key]
-      })
+    for (const key in headers) {
+      if (!key.startsWith(metadataPrefix)) {
+        continue
+      }
+
+      if (!Object.prototype.hasOwnProperty.call(headers, key)) {
+        continue
+      }
+
+      const value = headers[key]
+      if (typeof value !== 'string') {
+        continue
+      }
+
+      if (!metadata) {
+        metadata = {}
+      }
+
+      metadata[key.slice(metadataPrefix.length)] = value
+    }
 
     return metadata
   }

src/test/log-request.test.ts

@@ -0,0 +1,59 @@
+import Fastify, { FastifyInstance } from 'fastify'
+import { logRequest } from '../http/plugins/log-request'
+
+describe('log-request plugin', () => {
+  let app: FastifyInstance
+
+  beforeEach(async () => {
+    app = Fastify()
+    await app.register(logRequest({}))
+  })
+
+  afterEach(async () => {
+    await app.close()
+  })
+
+  it('derives resources from route params and prefixes them', async () => {
+    app.get('/bucket/:bucket/object/:name', async (request) => {
+      return {
+        resources: request.resources,
+      }
+    })
+
+    const response = await app.inject({
+      method: 'GET',
+      url: '/bucket/demo/object/file.txt',
+    })
+
+    expect(response.statusCode).toBe(200)
+    expect(response.json()).toEqual({
+      resources: ['/demo/file.txt'],
+    })
+  })
+
+  it('prefers configured resources and normalizes missing leading slashes', async () => {
+    app.get(
+      '/bucket/:bucket',
+      {
+        config: {
+          resources: () => ['bucket/demo', '/object/demo'],
+        },
+      },
+      async (request) => {
+        return {
+          resources: request.resources,
+        }
+      }
+    )
+
+    const response = await app.inject({
+      method: 'GET',
+      url: '/bucket/demo',
+    })
+
+    expect(response.statusCode).toBe(200)
+    expect(response.json()).toEqual({
+      resources: ['/bucket/demo', '/object/demo'],
+    })
+  })
+})