ronantakizawa/codereview-bench huggingface dataset:
Repository: fastify/fastify
File: test/request-error.test.js (JavaScript)

Code before:

    sget({
      method: 'POST',
      url: 'http://localhost:' + fastify.server.address().port + '/version',
      headers: { 'Content-Type': 'application/json', 'Accept-Version': '1.2.0' },
      body: [],
      json: true
    }, (err, response, body) => {
      t.error(err)
      t.equal(response.statusCode, 200)
    })

    sget({
      method: 'POST',
      url: 'http://localhost:' + fastify.server.address().port + '/version-undefined',
      headers: { 'Content-Type': 'application/json' },
      body: [],
      json: true
    }, (err, response, body) => {
      t.error(err)
      t.equal(response.statusCode, 200)
    })
  })
})

test('customErrorHandler should throw for json err and stream response', t => {
  t.plan(7)

  const logStream = split(JSON.parse)
  const fastify = Fastify({
    logger: {
      stream: logStream,
      level: 'error'
    }
  })

  t.teardown(fastify.close.bind(fastify))

  fastify.get('/', async (req, reply) => {
    const stream = new Readable({
      read () {
        this.push('hello')
      }
    })
    process.nextTick(() => stream.destroy(new Error('stream error')))

    reply.type('application/text')
    await reply.send(stream)
  })

  fastify.setErrorHandler((err, req, reply) => {

Review comment (bug):
I don't understand, you should be able to send custom data through `reply.send`. If it cannot, it should be a bug.

Code after:

  const result = await fetch(fastifyServer, {
    method: 'POST',
    headers: { 'Content-Type': 'application/json' },
    body: JSON.stringify([])
  })
  t.assert.ok(result.ok)
  t.assert.strictEqual(result.status, 200)
})

test('request.routeOptions.method is an array with uppercase string', async t => {
  t.plan(3)
  const fastify = Fastify()
  const handler = function (req, res) {
    t.assert.deepStrictEqual(['POST'], req.routeOptions.method)
    res.send({})
  }

  fastify.route({
    url: '/',
    method: ['pOSt'],
    bodyLimit: 1000,
    handler
  })
  const fastifyServer = await fastify.listen({ port: 0 })
  t.after(() => fastify.close())

  const result = await fetch(fastifyServer, {
    method: 'POST',
    headers: { 'Content-Type': 'application/json' },
    body: JSON.stringify([])
  })
  t.assert.ok(result.ok)
  t.assert.strictEqual(result.status, 200)
})

test('test request.routeOptions.version', async t => {
  t.plan(6)
  const fastify = Fastify()

  fastify.route({
    method: 'POST',
    url: '/version',
    constraints: { version: '1.2.0' },
    handler: function (request, reply) {
      t.assert.strictEqual('1.2.0', request.routeOptions.version)
      reply.send({})
    }
  })

  fastify.route({
Repository: fastify/fastify
File: lib/logger-factory.js (JavaScript)

Code before:
'use strict'

const { validateLogger } = require('./logger.js')

/**
 * Utility for creating a child logger with the appropriate bindings, logger factory
 * and validation.
 * @param {object} context
 * @param {import('../fastify').FastifyBaseLogger} logger
 * @param {import('../fastify').RawRequestDefaultExpression<any>} req
 * @param {string} reqId
 * @param {import('../types/logger.js').ChildLoggerOptions?} loggerOpts
 */
function createChildLogger (context, logger, req, reqId, loggerOpts) {
  const loggerBindings = {
    [context.requestIdLogLabel]: reqId
  }
  const child = context.childLoggerFactory.call(context.server, logger, loggerBindings, loggerOpts || {}, req)

  // Optimization: bypass validation if the factory is our own default factory
  if (context.childLoggerFactory !== defaultChildLoggerFactory) {
    validateLogger(child, true) // throw if the child is not a valid logger
  }

  return child
}

/**

Review comment (suggestion):
```suggestion
const { validateLogger } = require('./logger')
```

Code after:
'use strict'

const {
  FST_ERR_LOG_LOGGER_AND_LOGGER_INSTANCE_PROVIDED,
  FST_ERR_LOG_INVALID_LOGGER_CONFIG,
  FST_ERR_LOG_INVALID_LOGGER_INSTANCE,
  FST_ERR_LOG_INVALID_LOGGER
} = require('./errors')

/**
 * Utility for creating a child logger with the appropriate bindings, logger factory
 * and validation.
 * @param {object} context
 * @param {import('../fastify').FastifyBaseLogger} logger
 * @param {import('../fastify').RawRequestDefaultExpression<any>} req
 * @param {string} reqId
 * @param {import('../types/logger.js').ChildLoggerOptions?} loggerOpts
 *
 * @returns {object} New logger instance, inheriting all parent bindings,
 * with child bindings added.
 */
function createChildLogger (context, logger, req, reqId, loggerOpts) {
  const loggerBindings = {
    [context.requestIdLogLabel]: reqId
  }
  const child = context.childLoggerFactory.call(context.server, logger, loggerBindings, loggerOpts || {}, req)

  // Optimization: bypass validation if the factory is our own default factory
Repository: fastify/fastify
File: lib/logger-factory.js (JavaScript)

Code before:
'use strict'

const { FST_ERR_LOG_LOGGER_AND_LOGGER_INSTANCE_PROVIDED, FST_ERR_LOG_INVALID_LOGGER_CONFIG, FST_ERR_LOG_INVALID_LOGGER_INSTANCE, FST_ERR_LOG_INVALID_LOGGER } = require('./errors')

/**
 * Utility for creating a child logger with the appropriate bindings, logger factory
 * and validation.
 * @param {object} context
 * @param {import('../fastify').FastifyBaseLogger} logger
 * @param {import('../fastify').RawRequestDefaultExpression<any>} req
 * @param {string} reqId
 * @param {import('../types/logger.js').ChildLoggerOptions?} loggerOpts
 */
function createChildLogger (context, logger, req, reqId, loggerOpts) {
  const loggerBindings = {
    [context.requestIdLogLabel]: reqId
  }
  const child = context.childLoggerFactory.call(context.server, logger, loggerBindings, loggerOpts || {}, req)

  // Optimization: bypass validation if the factory is our own default factory
  if (context.childLoggerFactory !== defaultChildLoggerFactory) {
    validateLogger(child, true) // throw if the child is not a valid logger
  }

  return child
}

/**

Review comment (suggestion):
```suggestion
const {
  FST_ERR_LOG_LOGGER_AND_LOGGER_INSTANCE_PROVIDED,
  FST_ERR_LOG_INVALID_LOGGER_CONFIG,
  FST_ERR_LOG_INVALID_LOGGER_INSTANCE,
  FST_ERR_LOG_INVALID_LOGGER
} = require('./errors')
```

Code after:
'use strict'

const {
  FST_ERR_LOG_LOGGER_AND_LOGGER_INSTANCE_PROVIDED,
  FST_ERR_LOG_INVALID_LOGGER_CONFIG,
  FST_ERR_LOG_INVALID_LOGGER_INSTANCE,
  FST_ERR_LOG_INVALID_LOGGER
} = require('./errors')

/**
 * Utility for creating a child logger with the appropriate bindings, logger factory
 * and validation.
 * @param {object} context
 * @param {import('../fastify').FastifyBaseLogger} logger
 * @param {import('../fastify').RawRequestDefaultExpression<any>} req
 * @param {string} reqId
 * @param {import('../types/logger.js').ChildLoggerOptions?} loggerOpts
 *
 * @returns {object} New logger instance, inheriting all parent bindings,
 * with child bindings added.
 */
function createChildLogger (context, logger, req, reqId, loggerOpts) {
  const loggerBindings = {
    [context.requestIdLogLabel]: reqId
  }
  const child = context.childLoggerFactory.call(context.server, logger, loggerBindings, loggerOpts || {}, req)

  // Optimization: bypass validation if the factory is our own default factory
Repository: fastify/fastify
File: test/conditional-pino.test.js (JavaScript)

Code before:
'use strict'

const t = require('tap')
const nullLogger = require('abstract-logging')
const test = t.test

test("pino is not require'd if logger is not passed", t => {
  let called = false
  const loggerPinoMock = {
    createPinoLogger: () => {
      called = true
      return nullLogger
    }
  }
  t.plan(1)
  const fastify = t.mockRequire('..', {
    '../lib/logger-factory.js': t.mockRequire('../lib/logger-factory.js', {
      '../lib/logger-pino.js': loggerPinoMock
    }),
  })
  fastify()
  t.equal(called, false)
})

test("pino is require'd if logger is passed", t => {
  let called = false
  const loggerPinoMock = {
    createPinoLogger: () => {

Review comment (suggestion):
Please use `node:test` instead. We have started converting to it, and it does not make sense to write new tests that will need to be converted later.

Code after:
'use strict'

const { test } = require('node:test')

test("pino is not require'd if logger is not passed", t => {
  t.plan(1)

  const fastify = require('..')

  fastify()

  t.assert.equal(require.cache[require.resolve('pino')], undefined)
})

test("pino is require'd if logger is passed", t => {
  t.plan(1)

  const fastify = require('..')

  fastify({
    logger: true
  })

  t.assert.notEqual(require.cache[require.resolve('pino')], undefined)
})

test("pino is require'd if loggerInstance is passed", t => {
  t.plan(1)
Repository: fastify/fastify
File: test/conditional-pino.test.js (JavaScript)

Code before:
'use strict'

const t = require('node:test')
const test = t.test

test("pino is not require'd if logger is not passed", t => {
  t.plan(1)

  const fastify = require('..')

  fastify()

  t.assert.equal(!!require.cache[require.resolve('pino')], false)
})

test("pino is require'd if logger is passed", t => {
  t.plan(1)

  const fastify = require('..')

  fastify({
    logger: true
  })

  t.assert.equal(!!require.cache[require.resolve('pino')], true)
})

test("pino is require'd if loggerInstance is passed", t => {
  t.plan(1)

Review comment (suggestion):
```suggestion
const test = require('node:test')
```

Code after:
'use strict'

const { test } = require('node:test')

test("pino is not require'd if logger is not passed", t => {
  t.plan(1)

  const fastify = require('..')

  fastify()

  t.assert.equal(require.cache[require.resolve('pino')], undefined)
})

test("pino is require'd if logger is passed", t => {
  t.plan(1)

  const fastify = require('..')

  fastify({
    logger: true
  })

  t.assert.notEqual(require.cache[require.resolve('pino')], undefined)
})

test("pino is require'd if loggerInstance is passed", t => {
  t.plan(1)
Repository: fastify/fastify
File: test/conditional-pino.test.js (JavaScript)

Code before:
    logger: true
  })

  t.assert.ok(require.cache[require.resolve('pino')])
})

test("pino is require'd if loggerInstance is passed", t => {
  t.plan(1)

  const fastify = require('..')

  const loggerInstance = {
    fatal: (msg) => { },
    error: (msg) => { },
    warn: (msg) => { },
    info: (msg) => { },
    debug: (msg) => { },
    trace: (msg) => { },
    child: () => loggerInstance
  }

  fastify({
    loggerInstance
  })

  t.assert.ok(require.cache[require.resolve('pino')], true)
})


Review comment (suggestion):
Slight miss, here. The `assert.ok` method's second parameter is a message parameter. I would use `assert.equal` here.

Code after:
    logger: true
  })

  t.assert.notEqual(require.cache[require.resolve('pino')], undefined)
})

test("pino is require'd if loggerInstance is passed", t => {
  t.plan(1)

  const fastify = require('..')

  const loggerInstance = {
    fatal: (msg) => { },
    error: (msg) => { },
    warn: (msg) => { },
    info: (msg) => { },
    debug: (msg) => { },
    trace: (msg) => { },
    child: () => loggerInstance
  }

  fastify({
    loggerInstance
  })

  t.assert.notEqual(require.cache[require.resolve('pino')], undefined)
})
Repository: fastify/fastify
File: lib/request.js (JavaScript)

Code before:
      }
    }
  },
  host: {
    get () {
      let host = this.headers.host ?? this.headers[':authority']
      // support http.requireHostHeader === false
      if (this.server.server.requireHostHeader === false) host ??= ''
      return host
    }
  },
  hostname: {
    get () {
      return this.host.split(':')[0]
    }
  },
  port: {
    get () {
      // first try taking port from host
      const portFromHost = parseInt(this.host.split(':').slice(-1)[0])
      if (!isNaN(portFromHost)) {
        return portFromHost
      }
      // now fall back to port from host/:authority header
      let host = (this.headers.host ?? this.headers[':authority'])
      if (this.server.server.requireHostHeader === false) host ??= ''
      const portFromHeader = parseInt(host.split(':').slice(-1)[0])
      if (!isNaN(portFromHeader)) {
        return portFromHeader
      }
      // fall back to null
      return null
    }
  },
  protocol: {
    get () {
      if (this.socket) {
        return this.socket.encrypted ? 'https' : 'http'
      }
    }
  },
  headers: {
    get () {
      if (this.additionalHeaders) {
        return Object.assign({}, this.raw.headers, this.additionalHeaders)
      }
      return this.raw.headers
    },
    set (headers) {
      this.additionalHeaders = headers
    }

Review comment (suggestion):
Using the `http.Server` for checking the properties, it is better because it may exists in `http`, `https` or `http2` options. It should be always the same when we use multiple server instance.

Code after:
      }
    }
  },
  host: {
    get () {
      let host = this.raw.headers.host ?? this.raw.headers[':authority']
      // support http.requireHostHeader === false
      if (this.server.server.requireHostHeader === false) host ??= ''
      return host
    }
  },
  hostname: {
    get () {
      return this.host.split(':')[0]
    }
  },
  port: {
    get () {
      // first try taking port from host
      const portFromHost = parseInt(this.host.split(':').slice(-1)[0])
      if (!isNaN(portFromHost)) {
        return portFromHost
      }
      // now fall back to port from host/:authority header
      let host = (this.headers.host ?? this.headers[':authority'])
      if (this.server.server.requireHostHeader === false) host ??= ''
      const portFromHeader = parseInt(host.split(':').slice(-1)[0])
      if (!isNaN(portFromHeader)) {
        return portFromHeader
      }
      // fall back to null
      return null
    }
  },
  protocol: {
    get () {
      if (this.socket) {
        return this.socket.encrypted ? 'https' : 'http'
      }
    }
  },
  headers: {
    get () {
      if (this.additionalHeaders) {
        return Object.assign({}, this.raw.headers, this.additionalHeaders)
      }
      return this.raw.headers
    },
    set (headers) {
      this.additionalHeaders = headers
    }
Repository: fastify/fastify
File: lib/decorate.js (JavaScript)

Code before:
  if (fn && (typeof fn.getter === 'function' || typeof fn.setter === 'function')) {
    Object.defineProperty(instance, name, {
      get: fn.getter,
      set: fn.setter
    })
  } else {
    instance[name] = fn
  }
}

function getDecorator (instance, name) {
  if (checkExistence(instance, name)) {
    return instance[name]
  }

  const prop = getProp(instance[kRequest], name) ?? getProp(instance[kReply], name)
  if (!prop) {
    throw new FST_ERR_DEC_UNDECLARED(name)
  }

  if (prop.value) {
    return prop.value
  }

  return prop
}

function getDecorators (...args) {
  if (this[kState].started) {
    throw new FST_ERR_DEC_GET_ACCESS_AFTER_START()
  }

  let names = args
  if (Array.isArray(args[0])) {
    names = args[0]
  }

  const decorators = {}
  for (const name of names) {
    decorators[name] = getDecorator(this, name)
  }

  return decorators
}

function decorateConstructor (konstructor, name, fn, dependencies) {
  const instance = konstructor.prototype
  if (Object.hasOwn(instance, name) || hasKey(konstructor, name)) {
    throw new FST_ERR_DEC_ALREADY_PRESENT(name)
  }


Review comment (performance):
Maybe we can create a cache system with a key combining instance unique identifier with name.
Although it should not be a big performance issue because it is not meant to be use after start.


Code after:
    Object.defineProperty(instance, name, {
      get: fn.getter,
      set: fn.setter
    })
  } else {
    instance[name] = fn
  }
}

function getInstanceDecorator (name) {
  if (!checkExistence(this, name)) {
    throw new FST_ERR_DEC_UNDECLARED(name, 'instance')
  }

  if (typeof this[name] === 'function') {
    return this[name].bind(this)
  }

  return this[name]
}

function decorateConstructor (konstructor, name, fn, dependencies) {
  const instance = konstructor.prototype
  if (Object.hasOwn(instance, name) || hasKey(konstructor, name)) {
    throw new FST_ERR_DEC_ALREADY_PRESENT(name)
  }

  konstructor[kHasBeenDecorated] = true
  checkDependencies(konstructor, name, dependencies)

  if (fn && (typeof fn.getter === 'function' || typeof fn.setter === 'function')) {
    Object.defineProperty(instance, name, {
      get: fn.getter,
      set: fn.setter
    })
  } else if (typeof fn === 'function') {
    instance[name] = fn
  } else {
    konstructor.props.push({ key: name, value: fn })
  }
}

function checkReferenceType (name, fn) {
  if (typeof fn === 'object' && fn && !(typeof fn.getter === 'function' || typeof fn.setter === 'function')) {
    throw new FST_ERR_DEC_REFERENCE_TYPE(name, typeof fn)
  }
}

function decorateFastify (name, fn, dependencies) {
  assertNotStarted(this, name)
  decorate(this, name, fn, dependencies)
Repository: fastify/fastify
File: test/allowUnsafeRegex.test.js (JavaScript)

Code before:
'use strict'

const t = require('node:test')
const test = t.test
const Fastify = require('..')
const sget = require('simple-get').concat

test('allow unsafe regex', (t, done) => {
  t.plan(4)

  const fastify = Fastify({
    allowUnsafeRegex: false
  })
  t.after(() => fastify.close())

  fastify.get('/:foo(^[0-9]*$)', (req, reply) => {
    reply.send({ foo: req.params.foo })
  })

  fastify.listen({ port: 0 }, err => {
    t.assert.ifError(err)

    sget({
      method: 'GET',
      url: 'http://localhost:' + fastify.server.address().port + '/1234'
    }, (err, response, body) => {
      t.assert.ifError(err)
      t.assert.equal(response.statusCode, 200)
      t.assert.deepStrictEqual(JSON.parse(body), {

Review comment (suggestion):
I think we should match the other tests converted to `node:test` for consistency
```suggestion
const { test } = require('node:test')
```

Code after:
'use strict'

const { test } = require('node:test')
const Fastify = require('..')
const sget = require('simple-get').concat

test('allow unsafe regex', (t, done) => {
  t.plan(4)

  const fastify = Fastify({
    allowUnsafeRegex: false
  })
  t.after(() => fastify.close())

  fastify.get('/:foo(^[0-9]*$)', (req, reply) => {
    reply.send({ foo: req.params.foo })
  })

  fastify.listen({ port: 0 }, err => {
    t.assert.ifError(err)

    sget({
      method: 'GET',
      url: 'http://localhost:' + fastify.server.address().port + '/1234'
    }, (err, response, body) => {
      t.assert.ifError(err)
      t.assert.equal(response.statusCode, 200)
      t.assert.deepStrictEqual(JSON.parse(body), {
        foo: '1234'
Repository: fastify/fastify
File: test/buffer.test.js (JavaScript)

Code before:
'use strict'

const t = require('node:test')
const test = t.test
const Fastify = require('..')

test('Buffer test', async t => {
  const fastify = Fastify()
  fastify.addContentTypeParser('application/json', { parseAs: 'buffer' }, fastify.getDefaultJsonParser('error', 'ignore'))

  fastify.delete('/', async (request) => {
    return request.body
  })

  await test('should return 200 if the body is not empty', async t => {
    t.plan(3)

    const response = await fastify.inject({
      method: 'DELETE',
      url: '/',
      payload: Buffer.from('{"hello":"world"}'),
      headers: {
        'content-type': 'application/json'
      }
    })

    t.assert.ifError(response.error)
    t.assert.equal(response.statusCode, 200)
    t.assert.deepEqual(response.payload.toString(), '{"hello":"world"}')

Review comment (suggestion):
I think we should match the other tests converted to `node:test` for consistency
```suggestion
const { test } = require('node:test')
```

Code after:
'use strict'

const { test } = require('node:test')
const Fastify = require('..')

test('Buffer test', async t => {
  const fastify = Fastify()
  fastify.addContentTypeParser('application/json', { parseAs: 'buffer' }, fastify.getDefaultJsonParser('error', 'ignore'))

  fastify.delete('/', async (request) => {
    return request.body
  })

  await test('should return 200 if the body is not empty', async t => {
    t.plan(3)

    const response = await fastify.inject({
      method: 'DELETE',
      url: '/',
      payload: Buffer.from('{"hello":"world"}'),
      headers: {
        'content-type': 'application/json'
      }
    })

    t.assert.ifError(response.error)
    t.assert.equal(response.statusCode, 200)
    t.assert.deepEqual(response.payload.toString(), '{"hello":"world"}')
  })
Repository: fastify/fastify
File: test/case-insensitive.test.js (JavaScript)

Code before:
'use strict'

const t = require('node:test')
const test = t.test
const Fastify = require('..')
const sget = require('simple-get').concat

test('case insensitive', (t, done) => {
  t.plan(4)

  const fastify = Fastify({
    caseSensitive: false
  })
  t.after(() => fastify.close())

  fastify.get('/foo', (req, reply) => {
    reply.send({ hello: 'world' })
  })

  fastify.listen({ port: 0 }, err => {
    t.assert.ifError(err)

    sget({
      method: 'GET',
      url: 'http://localhost:' + fastify.server.address().port + '/FOO'
    }, (err, response, body) => {
      t.assert.ifError(err)
      t.assert.equal(response.statusCode, 200)
      t.assert.deepEqual(JSON.parse(body), {

Review comment (suggestion):
I think we should match the other tests converted to `node:test` for consistency
```suggestion
const { test } = require('node:test')
```

Code after:
'use strict'

const { test } = require('node:test')
const Fastify = require('..')
const sget = require('simple-get').concat

test('case insensitive', (t, done) => {
  t.plan(4)

  const fastify = Fastify({
    caseSensitive: false
  })
  t.after(() => fastify.close())

  fastify.get('/foo', (req, reply) => {
    reply.send({ hello: 'world' })
  })

  fastify.listen({ port: 0 }, err => {
    t.assert.ifError(err)

    sget({
      method: 'GET',
      url: 'http://localhost:' + fastify.server.address().port + '/FOO'
    }, (err, response, body) => {
      t.assert.ifError(err)
      t.assert.equal(response.statusCode, 200)
      t.assert.deepEqual(JSON.parse(body), {
        hello: 'world'
Repository: fastify/fastify
File: test/async-dispose.test.js (JavaScript)

Code before:
'use strict'

const { test } = require('node:test')
const Fastify = require('../fastify')

// asyncDispose doesn't exist in node <= 16
test('async dispose should close fastify', { skip: !('asyncDispose' in Symbol) }, async t => {
  t.plan(2)

  const fastify = Fastify()

  await fastify.listen({ port: 0 })

  t.assert.equal(fastify.server.listening, true)

  // the same as syntax sugar for
  // await using app = fastify()
  await fastify[Symbol.asyncDispose]()

  t.assert.equal(fastify.server.listening, false)
})


Review comment (suggestion):
```suggestion
  t.assert.strictEqual(fastify.server.listening, true)
```

Code after:
'use strict'

const { test } = require('node:test')
const Fastify = require('../fastify')

// asyncDispose doesn't exist in node <= 16
test('async dispose should close fastify', { skip: !('asyncDispose' in Symbol) }, async t => {
  t.plan(2)

  const fastify = Fastify()

  await fastify.listen({ port: 0 })

  t.assert.strictEqual(fastify.server.listening, true)

  // the same as syntax sugar for
  // await using app = fastify()
  await fastify[Symbol.asyncDispose]()
  t.assert.strictEqual(fastify.server.listening, false)
})