Skip to content

feat: introduce Body Limit Middleware using stream#2103

Merged
yusukebe merged 7 commits intomainfrom
feat/body-limit-stream
Mar 4, 2024
Merged

feat: introduce Body Limit Middleware using stream#2103
yusukebe merged 7 commits intomainfrom
feat/body-limit-stream

Conversation

@yusukebe
Copy link
Copy Markdown
Member

@yusukebe yusukebe commented Jan 27, 2024

This is an implementation of the feature proposed in #2077 using stream.

How to use

Simply specify the maximum size.

app.post(
  '/hello',
  bodyLimit({
    maxSize: 15 * Unit.b, // byte,
    onError: (c) => {
      return c.text('oveflow :(', 413)
    },
  }),
  (c) => {
    return c.text('pass :)')
  }
)

Using stream

The problem discussed in #2077 that a body must be read at a time is solved by using stream.

c.req.bodyCache

It uses c.req.bodyCache used by the Validator implementation. By setting the loaded ArrayBuffer to c.req.bodyCache.arrayBuffer, such as c.req.json() will use that cached body from then on.

Author should do the followings, if applicable

  • Add tests
  • Run tests
  • yarn denoify to generate files for Deno

yusukebe and others added 2 commits January 28, 2024 05:06
Co-authored-by: Ame_x <121654029+EdamAme-x@users.noreply.github.com>
@yusukebe yusukebe added the v4 label Jan 29, 2024
@yusukebe yusukebe removed the v4 label Feb 5, 2024
Base automatically changed from v4 to main February 9, 2024 05:53
@yusukebe yusukebe added the v4.1 label Feb 21, 2024
@rafaell-lycan
Copy link
Copy Markdown

Express has an effortless way to customize its body limit, defaulting to 100kb max.

…2109)

* fix(body-limit): fix typo

* feat(body-limit): Replace `c.req.raw` with body limit middleware proxy

* refactor(body-limit): we can trust content-length header

* fix(body-limit): call controller.error instead of throwing an error

* test(body-limit): add test for ReadableStream body

* chore: denoify

* refactor(middleware/body-limit): throw HTTPException instead of retuning c.text()
@yusukebe
Copy link
Copy Markdown
Member Author

Hey @usualoma

Could you review this merged code again?

@usualoma
Copy link
Copy Markdown
Member

usualoma commented Feb 27, 2024

Do we need data called Unit?

As a matter of fact, to me, the following statement seems cognitively loaded. Of course it is not incomprehensible, and I think it is wonderful that it comes with a type. But later, when you take a quick look at the code, what is Unit?" is not so obvious and requires knowledge of body-limit.

impot { bodyLimit, Unit } from "hono/middleware/body-limit"
bodyLimit({
  maxSize: 15 * Unit.mb,
})

I find it less cognitively demanding to write the following rather than using utility type.

impot { bodyLimit } from "hono/middleware/body-limit"
bodyLimit({
  maxSize: 15 * 1024 * 1024, // 15mb
})

Alternatively, if we need to provide a way to specify using units, I feel the following would be better

impot { bodyLimit } from "hono/middleware/body-limit"
bodyLimit({
  maxSize: '15mb',
})
diff --git a/src/middleware/body-limit/index.test.ts b/src/middleware/body-limit/index.test.ts
index 058ffc77..7be5c41b 100644
--- a/src/middleware/body-limit/index.test.ts
+++ b/src/middleware/body-limit/index.test.ts
@@ -1,5 +1,6 @@
 import { Hono } from '../../hono'
-import { Unit, bodyLimit } from '.'
+import { bodyLimit } from '.'
+import type { Size } from './index'
 
 const GlobalRequest = globalThis.Request
 globalThis.Request = class Request extends GlobalRequest {
@@ -35,7 +36,7 @@ describe('Body Limit Middleware', () => {
 
   beforeEach(() => {
     app = new Hono()
-    app.use('*', bodyLimit({ maxSize: 15 * Unit.b }))
+    app.use('*', bodyLimit({ maxSize: '15b' }))
     app.get('/', (c) => c.text('index'))
     app.post('/body-limit-15byte', async (c) => {
       return c.text(await c.req.raw.text())
@@ -112,45 +113,49 @@ describe('Body Limit Middleware', () => {
     })
   })
 
-  describe('custom error handler', () => {
+  describe('Size', () => {
+    it('should accept string with unit', () => {
+      expectTypeOf<Size>('1b').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1kb').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1mb').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1gb').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1tb').toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>('1pb').toEqualTypeOf<Size>('1b')
+
+      expectTypeOf<Size>('1.5gb').toEqualTypeOf<Size>('1b')
+    })
+    it('should accept number', () => {
+      expectTypeOf<Size>(1).toEqualTypeOf<Size>('1b')
+      expectTypeOf<Size>(1.1).toEqualTypeOf<Size>('1b')
+    })
+  })
+
+  describe('maxSize with string', () => {
     beforeEach(() => {
       app = new Hono()
-      app.post(
-        '/text-limit-15byte-custom',
-        bodyLimit({
-          maxSize: 15 * Unit.b,
-          onError: (c) => {
-            return c.text('no', 413)
-          },
-        }),
-        (c) => {
-          return c.text('yes')
-        }
-      )
     })
 
-    it('should return the custom error handler', async () => {
-      const res = await app.request(
-        '/text-limit-15byte-custom',
-        buildRequestInit({ body: exampleText2 })
+    test.each`
+      maxSize  | size
+      ${'15b'} | ${15}
+      ${'1kb'} | ${1 * 1024}
+      ${'1mb'} | ${1 * 1024 ** 2}
+    `('should accept $maxSize as byte unit', async ({ maxSize, size }) => {
+      app.post(
+        '/hello',
+        bodyLimit({
+          maxSize,
+        }),
+        (c) => c.text('pass :)')
       )
 
+      let res = await app.request('/hello', buildRequestInit({ body: 'a'.repeat(size) }))
+      expect(res).not.toBeNull()
+      expect(res.status).toBe(200)
+
+      res = await app.request('/hello', buildRequestInit({ body: 'a'.repeat(size + 1) }))
       expect(res).not.toBeNull()
       expect(res.status).toBe(413)
-      expect(await res.text()).toBe('no')
     })
   })
 })
-
-describe('Unit', () => {
-  it('should return the correct size', () => {
-    let beforeSize = 1 / 1024
-
-    for (let i = 0, keys = Object.keys(Unit), len = keys.length; i < len; i++) {
-      // @ts-expect-error: <safe access>
-      const size = Unit[keys[i]]
-      expect(size === beforeSize * 1024).toBeTruthy()
-      beforeSize = size
-    }
-  })
-})
diff --git a/src/middleware/body-limit/index.ts b/src/middleware/body-limit/index.ts
index 929de058..77610ca5 100644
--- a/src/middleware/body-limit/index.ts
+++ b/src/middleware/body-limit/index.ts
@@ -4,9 +4,15 @@ import type { MiddlewareHandler } from '../../types'
 
 const ERROR_MESSAGE = 'Payload Too Large'
 
+const metricPrefixes = ['', 'k', 'm', 'g', 't', 'p'] as const
+type Unit = `${typeof metricPrefixes[number]}b`
+
+export type Size = number | `${number}${Unit}`
+const sizeRe = new RegExp(`(.*?)(${metricPrefixes.join('|')})b$`)
+
 type OnError = (c: Context) => Response | Promise<Response>
 type BodyLimitOptions = {
-  maxSize: number
+  maxSize: Size
   onError?: OnError
 }
 
@@ -17,6 +23,15 @@ class BodyLimitError extends Error {
   }
 }
 
+const calculateSize = (maxSize: Size): number => {
+  if (typeof maxSize === 'number') {
+    return maxSize
+  }
+
+  const [, size, prefix] = maxSize.match(sizeRe) as [unknown, string, typeof metricPrefixes[number]]
+  return parseFloat(size) * 1024 ** metricPrefixes.indexOf(prefix)
+}
+
 /**
  * Body Limit Middleware
  *
@@ -25,7 +40,7 @@ class BodyLimitError extends Error {
  * app.post(
  *  '/hello',
  *  bodyLimit({
- *    maxSize: 15 * Unit.b,
+ *    maxSize: "15mb",
  *    onError: (c) => {
  *      return c.text('overflow :(', 413)
  *    }
@@ -45,7 +60,7 @@ export const bodyLimit = (options: BodyLimitOptions): MiddlewareHandler => {
       })
       throw new HTTPException(413, { res })
     })
-  const maxSize = options.maxSize
+  const maxSize = calculateSize(options.maxSize)
 
   return async function bodyLimit(c, next) {
     if (!c.req.raw.body) {
@@ -94,13 +109,3 @@ export const bodyLimit = (options: BodyLimitOptions): MiddlewareHandler => {
     }
   }
 }
-
-/**
- * Unit any
- * @example
- * ```ts
- * const limit = 100 * Unit.kb // 100kb
- * const limit2 = 1 * Unit.gb // 1gb
- * ```
- */
-export const Unit = { b: 1, kb: 1024, mb: 1024 ** 2, gb: 1024 ** 3, tb: 1024 ** 4, pb: 1024 ** 5 }

@usualoma
Copy link
Copy Markdown
Member

Personally, I have the above opinion about the current specifications of Unit.
But, well, it doesn't mean that it is no good. If the current specifications of Unit are good as they are, then I think it is good regarding other points.

@usualoma
Copy link
Copy Markdown
Member

(A few updated implementation examples)

@yusukebe
Copy link
Copy Markdown
Member Author

yusukebe commented Mar 1, 2024

Thanks, @usualoma !

As for Unit, I think removing it might be a good idea. I agree with the following opinion.

I find it less cognitively demanding to write the following rather than using utility type.

impot { bodyLimit } from "hono/middleware/body-limit"
bodyLimit({
  maxSize: 15 * 1024 * 1024, // 15mb
})

@yusukebe
Copy link
Copy Markdown
Member Author

yusukebe commented Mar 3, 2024

All works are finished!

@EdamAme-x If you are okay, I'll merge this PR to the "next" with you as a co-author!

@usualoma
Copy link
Copy Markdown
Member

usualoma commented Mar 3, 2024

@yusukebe
Copy link
Copy Markdown
Member Author

yusukebe commented Mar 3, 2024

@usualoma

You are right. Updated!

@yusukebe
Copy link
Copy Markdown
Member Author

yusukebe commented Mar 4, 2024

Hi @EdamAme-x! Now, I'll merge this into the "next". Thanks a lot!

@yusukebe yusukebe merged commit 2a9c9a1 into main Mar 4, 2024
@yusukebe yusukebe deleted the feat/body-limit-stream branch March 4, 2024 12:36
@yusukebe yusukebe restored the feat/body-limit-stream branch March 4, 2024 12:37
yusukebe added a commit that referenced this pull request Mar 4, 2024
@yusukebe
Copy link
Copy Markdown
Member Author

yusukebe commented Mar 4, 2024

Oops. Mistook. Merged into the main. Reverting.

@yusukebe yusukebe deleted the feat/body-limit-stream branch March 4, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants