ronantakizawa/codereview-bench huggingface dataset:
Repository: honojs/hono
File: src/middleware/ip-restriction/index.ts (TypeScript)

Code before:
/**
 * Rules for IP Limit Middleware
 */
export interface IPRestrictionRules {
  denyList?: IPRestrictionRule[]
  allowList?: IPRestrictionRule[]
}

/**
 * IP Limit Middleware
 *
 * @param getIP function to get IP Address
 */
export const ipRestriction = (
  getIP: GetIPAddr,
  { denyList = [], allowList = [] }: IPRestrictionRules,
  onError?: (remote: { addr: string; type: AddressType }) => Response | Promise<Response>
): MiddlewareHandler => {
  const allowLength = allowList.length

  const denyMatcher = buildMatcher(denyList)
  const allowMatcher = buildMatcher(allowList)

  const blockError = (): HTTPException =>
    new HTTPException(403, {
      res: new Response('Unauthorized', {
        status: 403,
      }),
    })

  return async function (c, next) {
    const connInfo = getIP(c)
    const addr = typeof connInfo === 'string' ? connInfo : connInfo.remote.address
    if (!addr) {
      throw blockError()
    }
    const type =
      (typeof connInfo !== 'string' && connInfo.remote.addressType) || distinctRemoteAddr(addr)

    const remoteData = { addr, type, isIPv4: type === 'IPv4' }

    if (denyMatcher(remoteData)) {
      if (onError) {
        return onError({ addr, type })
      }
      throw blockError()
    }
    if (allowMatcher(remoteData)) {
      return await next()
    }


Review comment (security):
I think `Forbidden` is better than `Unauthorized`.

Code after:
/**
 * Rules for IP Limit Middleware
 */
export interface IPRestrictionRules {
  denyList?: IPRestrictionRule[]
  allowList?: IPRestrictionRule[]
}

/**
 * IP Limit Middleware
 *
 * @param getIP function to get IP Address
 */
export const ipRestriction = (
  getIP: GetIPAddr,
  { denyList = [], allowList = [] }: IPRestrictionRules,
  onError?: (
    remote: { addr: string; type: AddressType },
    c: Context
  ) => Response | Promise<Response>
): MiddlewareHandler => {
  const allowLength = allowList.length

  const denyMatcher = buildMatcher(denyList)
  const allowMatcher = buildMatcher(allowList)

  const blockError = (c: Context): HTTPException =>
    new HTTPException(403, {
      res: c.text('Forbidden', {
        status: 403,
      }),
    })

  return async function (c, next) {
    const connInfo = getIP(c)
    const addr = typeof connInfo === 'string' ? connInfo : connInfo.remote.address
    if (!addr) {
      throw blockError(c)
    }
    const type =
      (typeof connInfo !== 'string' && connInfo.remote.addressType) || distinctRemoteAddr(addr)

    const remoteData = { addr, type, isIPv4: type === 'IPv4' }

    if (denyMatcher(remoteData)) {
      if (onError) {
        return onError({ addr, type }, c)
      }
      throw blockError(c)
    }
    if (allowMatcher(remoteData)) {
Repository: honojs/hono
File: src/context.ts (TypeScript)

Code before:
        this.res.headers.set(name, value)
      }
    }
  }

  status = (status: StatusCode): void => {
    this.#isFresh = false
    this.#status = status
  }

  /**
   * `.set()` can set the value specified by the key.
   *
   * @see {@link https://hono.dev/api/context#set-get}
   *
   * @example
   * ```ts
   * app.use('*', async (c, next) => {
   *   c.set('message', 'Hono is cool!!')
   *   await next()
   * })
   * ```
```
   */
  set: Set<E> = (key: string, value: unknown) => {
    // @ts-expect-error this._var is initialized as {}
    this._var[key] = value
  }

  /**
   * `.get()` can use the value specified by the key.
   *
   * @see {@link https://hono.dev/api/context#set-get}
   *
   * @example
   * ```ts
   * app.get('/', (c) => {
   *   const message = c.get('message')
   *   return c.text(`The message is "${message}"`)
   * })
   * ```
   */
  get: Get<E> = (key: string) => {
    // @ts-expect-error this._var is initialized as {}
    return this._var[key]
  }

  /**
   * `.var` can access the value of a variable.
   *
   * @see {@link https://hono.dev/api/context#var}

Review comment (suggestion):
It throws a type error that points to the possibility of `undefined`. But it has been initialized above. So ignore it.

Code after:
        this.res.headers.set(name, value)
      }
    }
  }

  status = (status: StatusCode): void => {
    this.#isFresh = false
    this.#status = status
  }

  /**
   * `.set()` can set the value specified by the key.
   *
   * @see {@link https://hono.dev/api/context#set-get}
   *
   * @example
   * ```ts
   * app.use('*', async (c, next) => {
   *   c.set('message', 'Hono is cool!!')
   *   await next()
   * })
   * ```
```
   */
  set: Set<E> = (key: string, value: unknown) => {
    this._var ??= {}
    this._var[key as string] = value
  }

  /**
   * `.get()` can use the value specified by the key.
   *
   * @see {@link https://hono.dev/api/context#set-get}
   *
   * @example
   * ```ts
   * app.get('/', (c) => {
   *   const message = c.get('message')
   *   return c.text(`The message is "${message}"`)
   * })
   * ```
   */
  get: Get<E> = (key: string) => {
    return this._var ? this._var[key] : undefined
  }

  /**
   * `.var` can access the value of a variable.
   *
   * @see {@link https://hono.dev/api/context#var}
   *
Repository: honojs/hono
File: src/adapter/cloudflare-pages/handler.ts (TypeScript)

Code before:
  return async (executionCtx) => {
    const context = new Context(executionCtx.request, {
      env: executionCtx.env,
      executionCtx,
    })

    let response: Response | void = undefined

    try {
      response = await middleware(context, async () => {
        context.res = await executionCtx.next()
      })
    } catch (error) {
      if (error instanceof HTTPException) {
        response = error.getResponse()
      } else {
        throw error
      }
    }

    return response ?? context.res
  }
}

declare abstract class FetcherLike {
  fetch(input: RequestInfo, init?: RequestInit): Promise<Response>
}

/**
 *
 * @description `serveStatic()` is for advanced mode:
 * https://developers.cloudflare.com/pages/platform/functions/advanced-mode/#set-up-a-function
 *
 */
export const serveStatic = (): MiddlewareHandler => {
  return async (c) => {
    const env = c.env as { ASSETS: FetcherLike }
    const res = await env.ASSETS.fetch(c.req.raw)
    if (res.status === 404) {
      return c.notFound()
    }
    return res
  }
}


Review comment (suggestion):
I included the code to handle `HTTPException`, but I wasn't sure what should happen with other kinds of errors, so it will re-throw everything else

Looking at the documentation, there is also a [`Context.error`](https://hono.dev/docs/api/context#error) property. Should we handle that here too? Similar to `Context.res`

Code after:
  return async (executionCtx) => {
    const context = new Context(executionCtx.request, {
      env: executionCtx.env,
      executionCtx,
    })

    let response: Response | void = undefined

    try {
      response = await middleware(context, async () => {
        try {
          context.res = await executionCtx.next()
        } catch (error) {
          if (error instanceof Error) {
            context.error = error
          } else {
            throw error
          }
        }
      })
    } catch (error) {
      if (error instanceof Error) {
        context.error = error
      } else {
        throw error
      }
    }

    if (response) {
      return response
    }

    if (context.error instanceof HTTPException) {
      return context.error.getResponse()
    }

    if (context.error) {
      throw context.error
    }

    return context.res
  }
}

declare abstract class FetcherLike {
  fetch(input: RequestInfo, init?: RequestInit): Promise<Response>
}

/**
 *
 * @description `serveStatic()` is for advanced mode:
Repository: honojs/hono
File: src/adapter/cloudflare-pages/handler.ts (TypeScript)

Code before:
import type { Hono } from '../../hono'
import type { MiddlewareHandler } from '../../types'

// Ref: https://github.com/cloudflare/workerd/blob/main/types/defines/pages.d.ts

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type Params<P extends string = any> = Record<P, string | string[]>

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type EventContext<Env = {}, P extends string = any, Data = {}> = {
  request: Request
  functionPath: string
  waitUntil: (promise: Promise<unknown>) => void
  passThroughOnException: () => void
  next: (input?: Request | string, init?: RequestInit) => Promise<Response>
  env: Env & { ASSETS: { fetch: typeof fetch } }
  params: Params<P>
  data: Data
}

interface HandleInterface {
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  (app: Hono<any, any, any>): (eventContext: EventContext) => Response | Promise<Response>
}

export const handle: HandleInterface = (app: Hono) => (eventContext: EventContext) => {
  return app.fetch(
    eventContext.request,
    { ...eventContext.env, eventContext },
    {
      waitUntil: eventContext.waitUntil,
      passThroughOnException: eventContext.passThroughOnException,
    }
  )
}

declare abstract class FetcherLike {
  fetch(input: RequestInfo, init?: RequestInit): Promise<Response>
}

/**
 *
 * @description `serveStatic()` is for advanced mode:
 * https://developers.cloudflare.com/pages/platform/functions/advanced-mode/#set-up-a-function
 *
 */
export const serveStatic = (): MiddlewareHandler => {
  return async (c) => {
    const env = c.env as { ASSETS: FetcherLike }
    const res = await env.ASSETS.fetch(c.req.raw)
    if (res.status === 404) {
      return c.notFound()
    }
    return res
  }
}


Review comment (suggestion):
I'm still not sure all of this is necessary.. I added all the test cases I could think of, so hopefully all the main use cases are covered

Code after:
            throw error
          }
        }
      })
    } catch (error) {
      if (error instanceof Error) {
        context.error = error
      } else {
        throw error
      }
    }

    if (response) {
      return response
    }

    if (context.error instanceof HTTPException) {
      return context.error.getResponse()
    }

    if (context.error) {
      throw context.error
    }

    return context.res
  }
}

declare abstract class FetcherLike {
  fetch(input: RequestInfo, init?: RequestInit): Promise<Response>
}

/**
 *
 * @description `serveStatic()` is for advanced mode:
 * https://developers.cloudflare.com/pages/platform/functions/advanced-mode/#set-up-a-function
 *
 */
export const serveStatic = (): MiddlewareHandler => {
  return async (c) => {
    const env = c.env as { ASSETS: FetcherLike }
    const res = await env.ASSETS.fetch(c.req.raw)
    if (res.status === 404) {
      return c.notFound()
    }
    return res
  }
}
Repository: honojs/hono
File: src/adapter/cloudflare-pages/handler.test.ts (TypeScript)

Code before:
import type { EventContext } from './handler'
import { handle, handleMiddleware } from './handler'

type Env = {
  Bindings: {
    TOKEN: string
    eventContext: EventContext
  }
}

describe('Adapter for Cloudflare Pages', () => {
  it('Should return 200 response', async () => {
    const request = new Request('http://localhost/api/foo')
    const env = {
      TOKEN: 'HONOISCOOL',
    }
    const app = new Hono<Env>()
    app.get('/api/foo', (c) => {
      const reqInEventContext = c.env.eventContext.request
      return c.json({ TOKEN: c.env.TOKEN, requestURL: reqInEventContext.url })
    })
    const handler = handle(app)
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore
    const res = await handler({ request, env })
    expect(res.status).toBe(200)
    expect(await res.json()).toEqual({
      TOKEN: 'HONOISCOOL',
      requestURL: 'http://localhost/api/foo',
    })
  })

  it('Should not use `basePath()` if path argument is not passed', async () => {
    const request = new Request('http://localhost/api/error')
    const app = new Hono().basePath('/api')

    app.onError((e) => {
      throw e
    })
    app.get('/error', () => {
      throw new Error('Custom Error')
    })

    const handler = handle(app)
    // It does throw the error if app is NOT "subApp"
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore
    expect(() => handler({ request })).toThrowError('Custom Error')
  })
})


Review comment (suggestion):
Creating an `EventContext` in each test was getting quite repetitive, so I added this function that turns a `Partial<EventContext>` into an `EventContext` with all the necessary properties.

Code after:
import type { EventContext } from './handler'
import { handle, handleMiddleware, serveStatic } from './handler'

type Env = {
  Bindings: {
    TOKEN: string
  }
}

function createEventContext(
  context: Partial<EventContext<Env['Bindings']>>
): EventContext<Env['Bindings']> {
  return {
    data: {},
    env: {
      ...context.env,
      ASSETS: { fetch: vi.fn(), ...context.env?.ASSETS },
      TOKEN: context.env?.TOKEN ?? 'HONOISCOOL',
    },
    functionPath: '_worker.js',
    next: vi.fn(),
    params: {},
    passThroughOnException: vi.fn(),
    request: new Request('http://localhost/api/foo'),
    waitUntil: vi.fn(),
    ...context,
  }
}

describe('Adapter for Cloudflare Pages', () => {
  it('Should return 200 response', async () => {
    const request = new Request('http://localhost/api/foo')
    const env = {
      ASSETS: { fetch },
      TOKEN: 'HONOISCOOL',
    }
    const waitUntil = vi.fn()
    const passThroughOnException = vi.fn()
    const eventContext = createEventContext({
      request,
      env,
      waitUntil,
      passThroughOnException,
    })
    const app = new Hono<Env>()
    const appFetchSpy = vi.spyOn(app, 'fetch')
    app.get('/api/foo', (c) => {
      return c.json({ TOKEN: c.env.TOKEN, requestURL: c.req.url })
    })
    const handler = handle(app)
    const res = await handler(eventContext)
Repository: honojs/hono
File: src/adapter/cloudflare-pages/handler.test.ts (TypeScript)

Code before:
import { getCookie } from '../../helper/cookie'
import { Hono } from '../../hono'
import { HTTPException } from '../../http-exception'
import type { EventContext } from './handler'
import { handle, handleMiddleware } from './handler'

type Env = {
  Bindings: {
    TOKEN: string
    eventContext: EventContext
  }
}

describe('Adapter for Cloudflare Pages', () => {
  it('Should return 200 response', async () => {
    const request = new Request('http://localhost/api/foo')
    const env = {
      TOKEN: 'HONOISCOOL',
    }
    const app = new Hono<Env>()
    app.get('/api/foo', (c) => {
      const reqInEventContext = c.env.eventContext.request
      return c.json({ TOKEN: c.env.TOKEN, requestURL: reqInEventContext.url })
    })
    const handler = handle(app)
    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
    // @ts-ignore
    const res = await handler({ request, env })
    expect(res.status).toBe(200)
    expect(await res.json()).toEqual({
      TOKEN: 'HONOISCOOL',
      requestURL: 'http://localhost/api/foo',
    })
  })

  it('Should not use `basePath()` if path argument is not passed', async () => {
    const request = new Request('http://localhost/api/error')
    const app = new Hono().basePath('/api')

    app.onError((e) => {
      throw e
    })
    app.get('/error', () => {
      throw new Error('Custom Error')
    })

    const handler = handle(app)
    // It does throw the error if app is NOT "subApp"

Review comment (suggestion):
From my understanding, this was ensuring that the original `eventContext` was added to `c.env`

I replaced this by adding an assertion that `vi.spyOn(app, 'fetch')` was called with `{ ...env, eventContext }`

Code after:
import { getCookie } from '../../helper/cookie'
import { Hono } from '../../hono'
import { HTTPException } from '../../http-exception'
import type { EventContext } from './handler'
import { handle, handleMiddleware, serveStatic } from './handler'

type Env = {
  Bindings: {
    TOKEN: string
  }
}

function createEventContext(
  context: Partial<EventContext<Env['Bindings']>>
): EventContext<Env['Bindings']> {
  return {
    data: {},
    env: {
      ...context.env,
      ASSETS: { fetch: vi.fn(), ...context.env?.ASSETS },
      TOKEN: context.env?.TOKEN ?? 'HONOISCOOL',
    },
    functionPath: '_worker.js',
    next: vi.fn(),
    params: {},
    passThroughOnException: vi.fn(),
    request: new Request('http://localhost/api/foo'),
    waitUntil: vi.fn(),
    ...context,
  }
}

describe('Adapter for Cloudflare Pages', () => {
  it('Should return 200 response', async () => {
    const request = new Request('http://localhost/api/foo')
    const env = {
      ASSETS: { fetch },
      TOKEN: 'HONOISCOOL',
    }
    const waitUntil = vi.fn()
    const passThroughOnException = vi.fn()
    const eventContext = createEventContext({
      request,
      env,
      waitUntil,
      passThroughOnException,
    })
    const app = new Hono<Env>()
Repository: honojs/hono
File: src/middleware/request-id/request-id.ts (TypeScript)

Code before:
 *
 * @example
 * ```ts
 * type Variables = RequestIDVariables
 * const app = new Hono<{Variables: Variables}>()
 *
 * app.use(requestID())
 * app.get('/', (c) => {
 *   console.log(c.get('requestID')) // Debug
 *   return c.text('Hello World!')
 * })
 * ```
 */
export const requestID = (options?: RequesIDOptions): MiddlewareHandler => {
  const limitLength = options?.limitLength ?? 255
  const headerName = options?.headerName ?? 'X-Request-Id'

  return async function requestID(c, next) {
    // If `headerName` is empty string, req.header will return the object
    let requestId = headerName ? c.req.header(headerName) : undefined

    if (requestId) {
      requestId = requestId.replace(/[^\w\-]/g, '')
      requestId = limitLength > 0 ? requestId.substring(0, limitLength) : requestId
    } else {
      requestId = options?.generator?.() ?? crypto.randomUUID()
    }

    c.set('requestID', requestId)
    if (headerName) {
      c.header(headerName, requestId)
    }
    await next()
  }
}


Review comment (suggestion):
Note that, `crypto.randomUUID()` may not be a stable method to use if Hono plan to focus on [supporting Web Browsers](https://x.com/honojs/status/1714947374886637883)

Code after:
 *
 * @example
 * ```ts
 * type Variables = RequestIdVariables
 * const app = new Hono<{Variables: Variables}>()
 *
 * app.use(requestId())
 * app.get('/', (c) => {
 *   console.log(c.get('requestId')) // Debug
 *   return c.text('Hello World!')
 * })
 * ```
 */
export const requestId = ({
  limitLength = 255,
  headerName = 'X-Request-Id',
  generator = () => crypto.randomUUID(),
}: RequestIdOptions = {}): MiddlewareHandler => {
  return async function requestId(c, next) {
    // If `headerName` is empty string, req.header will return the object
    let reqId = headerName ? c.req.header(headerName) : undefined
    if (!reqId || reqId.length > limitLength || /[^\w\-]/.test(reqId)) {
      reqId = generator(c)
    }

    c.set('requestId', reqId)
    if (headerName) {
      c.header(headerName, reqId)
    }
    await next()
  }
}
Repository: honojs/hono
File: src/types.ts (TypeScript)

Code before:
  F extends ResponseFormat = T extends string
    ? 'text'
    : T extends JSONValue
    ? 'json'
    : ResponseFormat
> = {
  _data: T
  _status: U
  _format: F
}

type MergeTypedResponse<T> = T extends Promise<infer T2>
  ? T2 extends TypedResponse
    ? T2
    : TypedResponse
  : T extends TypedResponse
  ? T
  : TypedResponse

////////////////////////////////////////
//////                             /////
//////      ValidationTargets      /////
//////                             /////
////////////////////////////////////////

export type FormValue = string | File | Blob
export type ParsedFormValue = string | File

export type ValidationTargets<T extends FormValue = ParsedFormValue> = {
  json: any
  form: Record<string, T | T[]>
  query: Record<string, string | string[]>
  param: Record<string, string> | Record<string, string | undefined>
  header: Record<string, string>
  cookie: Record<string, string>
}

////////////////////////////////////////
//////                            //////
//////      Path parameters       //////
//////                            //////
////////////////////////////////////////

type ParamKeyName<NameWithPattern> = NameWithPattern extends `${infer Name}{${infer Rest}`
  ? Rest extends `${infer _Pattern}?`
    ? `${Name}?`
    : Name
  : NameWithPattern

type ParamKey<Component> = Component extends `:${infer NameWithPattern}`
  ? ParamKeyName<NameWithPattern>

Review comment (suggestion):
I think `File` is not necessary:

```ts
export type FormValue = string | Blob
```

Code after:
  F extends ResponseFormat = T extends string
    ? 'text'
    : T extends JSONValue
    ? 'json'
    : ResponseFormat
> = {
  _data: T
  _status: U
  _format: F
}

type MergeTypedResponse<T> = T extends Promise<infer T2>
  ? T2 extends TypedResponse
    ? T2
    : TypedResponse
  : T extends TypedResponse
  ? T
  : TypedResponse

////////////////////////////////////////
//////                             /////
//////      ValidationTargets      /////
//////                             /////
////////////////////////////////////////

export type FormValue = string | Blob
export type ParsedFormValue = string | File

export type ValidationTargets<T extends FormValue = ParsedFormValue> = {
  json: any
  form: Record<string, T | T[]>
  query: Record<string, string | string[]>
  param: Record<string, string> | Record<string, string | undefined>
  header: Record<string, string>
  cookie: Record<string, string>
}

////////////////////////////////////////
//////                            //////
//////      Path parameters       //////
//////                            //////
////////////////////////////////////////

type ParamKeyName<NameWithPattern> = NameWithPattern extends `${infer Name}{${infer Rest}`
  ? Rest extends `${infer _Pattern}?`
    ? `${Name}?`
    : Name
  : NameWithPattern

type ParamKey<Component> = Component extends `:${infer NameWithPattern}`
  ? ParamKeyName<NameWithPattern>