feat(utils/body): add dot notation support for `parseBody` #2675

fzn0x · 2024-05-15T03:34:03Z

The author should do the following, if applicable

Add tests
Run tests
bun denoify to generate files for Deno
bun run format:fix && bun run lint:fix to format the code

MathurAditya724

These are some basic changes you can do to improve this. I haven't looked at the setNestedValue func and the test cases (this was all the time I had 😅), but I will try to look at it soon.

src/utils/body.ts

fzn0x · 2024-05-15T04:53:15Z

@MathurAditya724 Reviews resolved, to decide whether is it dot or transformDotNotation naming, we can wait @yusukebe first.

fzn0x · 2024-05-15T05:01:45Z

Dot notation support documentation will be added here: https://hono.dev/api/request#parsebody

Dot Notation

// assume you pass `obj.key1` in the request body
const body = await c.req.parseBody({ UNDECIDED: true })
body['obj'] // { key1: 'value' }

body['obj'] is a nested object.

All fields key with dot notation (e.g 'obj.key1.label' or 'obj.key1') will be parsed to nested objects.

MathurAditya724 · 2024-05-15T05:06:40Z

Yes, you will have to create a separate PR for that at https://github.com/honojs/website

yusukebe · 2024-05-15T08:00:36Z

Woooow. Super fast and quick! I'll review this now.

fzn0x · 2024-05-15T08:17:51Z

Thanks @yusukebe I will run denoify and lint once again

MathurAditya724

Just some minor changes for the improvements you did based on my review

src/utils/body.ts

yusukebe · 2024-05-15T08:44:35Z

Hey. The following test does not pass now. But is this expected? What do you think? @fzn0x @MathurAditya724

it('should parse data if both `all` and `dot` are set', async () => {
  const data = new FormData()
  data.append('obj.sub.foo', 'value1')
  data.append('obj.sub.foo', 'value2')
  data.append('key', 'value3')

  const req = createRequest(FORM_URL, 'POST', data)

  expect(await parseBody(req, { dot: true, all: true })).toEqual({
    obj: { sub: { foo: ['value1', 'value2'] } }, // Is tis expected??
    key: 'value3',
  })
})

Test test result:

- Expected
+ Received

  Object {
    "key": "value3",
    "obj": Object {
      "sub": Object {
-       "foo": Array [
-         "value1",
-         "value2",
-       ],
+       "foo": "value2",
      },
    },
  }

fzn0x · 2024-05-15T08:50:24Z

This is unexpected, let me fix that @yusukebe

yusukebe · 2024-05-15T08:55:58Z

Anyway, this is a new "feature," so it may not be merged into the main immediately. So you don't have to hurry up:)

vanodevium · 2024-05-15T08:57:23Z

@yusukebe

I need it for everyday tasks :)

MathurAditya724 · 2024-05-15T08:58:19Z

@fzn0x I believe handleNestedValues and handleParsingAllValues might be clashing. Do let me know if you require any help

fzn0x · 2024-05-15T08:59:50Z

Looks like

        if (
          !nestedForm[key] ||
          typeof nestedForm[key] !== 'object' ||
          Array.isArray(nestedForm[key])
        ) {
          nestedForm[key] = nestedForm[key] || {}
        }

This code only return the last value of the array @MathurAditya724 I will try to fix it and hope get back with good news

MathurAditya724 · 2024-05-15T09:01:48Z

Interesting, let me check

MathurAditya724 · 2024-05-15T09:19:01Z

Can I make changes to this PR? Or should I fork your repo?

fzn0x · 2024-05-15T09:25:21Z

I found the solution but the hard thing is to keep the code clean 😅 @MathurAditya724 @yusukebe

I guess we can continue with the review 🔥

fzn0x · 2024-05-15T09:37:05Z

Anyone can add more tests, I think current tests looks good now.

fzn0x · 2024-05-15T09:38:24Z

I believe we can add this in v4.4, what do you think? @yusukebe

yusukebe · 2024-05-15T10:02:36Z

I believe we can add this in v4.4, what do you think? @yusukebe

Yes. Let's include this in v4.4.

usualoma · 2024-05-15T20:38:19Z

Hi @fzn0x!
Thank you for the great PR.
I have two comments.

`all` is always enabled.

Since all is always enabled in the current branch, I think the code needs to be modified by adding the following test.

diff --git a/src/utils/body.test.ts b/src/utils/body.test.ts
index cc154ef4..f9f7e3c7 100644
--- a/src/utils/body.test.ts
+++ b/src/utils/body.test.ts
@@ -50,6 +50,20 @@ describe('Parse Body Util', () => {
     })
   })
 
+  it('should override value if `all` option is false', async () => {
+    const data = new FormData()
+    data.append('file', 'aaa')
+    data.append('file', 'bbb')
+    data.append('message', 'hello')
+
+    const req = createRequest(FORM_URL, 'POST', data)
+
+    expect(await parseBody(req)).toEqual({
+      file: 'bbb',
+      message: 'hello',
+    })
+  })
+
   it('should parse multiple values if `all` option is true', async () => {
     const data = new FormData()
     data.append('file', 'aaa')

Objects containing arbitrary data should be initialized using `Object.create(null)`

This would also require modification of existing code, but for objects that receive a request and put in arbitrary data, they should be initialized using Object.create(null).

diff --git a/src/utils/body.ts b/src/utils/body.ts
index a3382806..8ef63893 100644
--- a/src/utils/body.ts
+++ b/src/utils/body.ts
@@ -72,7 +72,7 @@ function convertFormDataToBodyData<T extends BodyData = BodyData>(
   formData: FormData,
   options: ParseBodyOptions
 ): T {
-  const form: BodyData = {}
+  const form: BodyData = Object.create(null)
 
   formData.forEach((value, key) => {
     const shouldParseAllValues = options.all || key.endsWith('[]')
@@ -140,7 +140,7 @@ const handleNestedValues = (
           typeof nestedForm[key] !== 'object' ||
           Array.isArray(nestedForm[key])
         ) {
-          nestedForm[key] = {}
+          nestedForm[key] = Object.create(null)
         }
         nestedForm = nestedForm[key] as BodyData
       }

fzn0x · 2024-05-15T22:27:32Z

Fix added! kindly review, thanks @usualoma

usualoma · 2024-05-16T00:33:03Z

@fzn0x Thank you! > 716de84

Sorry to add one more point. From a security standpoint, I think it would be better to prevent the File object's properties from being able to be updated.

diff --git a/src/utils/body.test.ts b/src/utils/body.test.ts
index 41235292..fb6da697 100644
--- a/src/utils/body.test.ts
+++ b/src/utils/body.test.ts
@@ -191,6 +191,32 @@ describe('Parse Body Util', () => {
     })
   })
 
+  it('should not update file object properties', async () => {
+    const file = new File(['foo'], 'file1', {
+      type: 'application/octet-stream',
+    })
+    const data = new FormData()
+
+    const req = createRequest(FORM_URL, 'POST', data)
+    vi.spyOn(req, 'formData').mockImplementation(
+      async () =>
+        ({
+          forEach: (cb) => {
+            cb(file, 'file', data)
+            cb('hoo', 'file.hoo', data)
+          },
+        } as FormData)
+    )
+
+    const parsedData = await parseBody(req, { dot: true })
+    expect(parsedData.file).not.instanceOf(File)
+    expect(parsedData).toEqual({
+      file: {
+        hoo: 'hoo',
+      },
+    })
+  })
+
   it('should parse multiple values if key ends with `[]`', async () => {
     const data = new FormData()
     data.append('file[]', 'aaa')
diff --git a/src/utils/body.ts b/src/utils/body.ts
index 1c77ab98..f07c6433 100644
--- a/src/utils/body.ts
+++ b/src/utils/body.ts
@@ -138,7 +138,8 @@ const handleNestedValues = (
         if (
           !nestedForm[key] ||
           typeof nestedForm[key] !== 'object' ||
-          Array.isArray(nestedForm[key])
+          Array.isArray(nestedForm[key]) ||
+          nestedForm[key] instanceof File
         ) {
           nestedForm[key] = Object.create(null)
         }

MathurAditya724

Hey @fzn0x, I have gone through the functions and made some improvements. All the tests are passing. Also, I added some pointers where we can improve this more. Let me know what you think

src/utils/body.ts

fzn0x · 2024-05-16T05:14:35Z

Thanks for the reviews,
I will submit a/some PR/s for the review above.

…ties

fzn0x · 2024-05-16T05:41:14Z

Adding tsdoc for better developer experience

src/utils/body.test.ts

src/utils/body.ts

yusukebe

LGTM!

yusukebe · 2024-05-22T20:49:06Z

Thanks y'all. Now ready to merge to the next branch for the v4.4.0.

fzn0x added 3 commits May 15, 2024 10:32

feat: add dot notation support for parseBody

948b82f

denoify & lint

5fd3fad

refactor: default value and jsdoc

2b8d4f4

fzn0x changed the title ~~feat: add dot notation support for parseBody~~ feat: add dot notation support for parseBody May 15, 2024

fzn0x added 2 commits May 15, 2024 10:42

denoify & lint

e55c2b4

test: more tests

ef160cf

MathurAditya724 reviewed May 15, 2024

View reviewed changes

src/utils/body.ts Outdated Show resolved Hide resolved

src/utils/body.ts Outdated Show resolved Hide resolved

src/utils/body.ts Outdated Show resolved Hide resolved

src/utils/body.ts Outdated Show resolved Hide resolved

src/utils/body.ts Outdated Show resolved Hide resolved

fzn0x added 3 commits May 15, 2024 11:35

fix: destruct option key value to false if value is undefined

288a418

types: remove object

1dd91dc

refactor: handle nested values inside a function

29f677c

fix: remove unusual import

9a268e9

yusukebe changed the title ~~feat: add dot notation support for parseBody~~ feat(utils/body): add dot notation support for parseBody May 15, 2024

denoify & lint

fab72f4

MathurAditya724 reviewed May 15, 2024

View reviewed changes

src/utils/body.ts Outdated Show resolved Hide resolved

src/utils/body.ts Outdated Show resolved Hide resolved

refactor: explicitly state just all and dot

e304c63

fix: issue raised by multiple append on the same dot notation key

ac0c47d

fzn0x added 2 commits May 15, 2024 16:33

types: dont use {}

27178c1

chore: denoify & lint

3461544

yusukebe added the v4.4 label May 15, 2024

fix(types): Deno compatible types

cd1055f

fzn0x added 2 commits May 16, 2024 05:26

fix: should override value if option is false

716de84

chore: denoify & lint

4500acf

MathurAditya724 reviewed May 16, 2024

View reviewed changes

fzn0x added 3 commits May 16, 2024 12:15

make option typings partials and should not update file object proper…

0265809

…ties

refactor: make code less complicated

64350b9

chore: add tsdoc

5dc896a

MathurAditya724 reviewed May 16, 2024

View reviewed changes

src/utils/body.test.ts Outdated Show resolved Hide resolved

fzn0x added 2 commits May 16, 2024 15:16

chore: remove comment

e300ddc

refactor(utils/body): reduce the complexity of O(mn)

29818f0

MathurAditya724 reviewed May 16, 2024

View reviewed changes

src/utils/body.ts Outdated Show resolved Hide resolved

chore: remove parseAllValues and change value to BodyDataValue type

2f631ca

yusukebe changed the base branch from main to next May 22, 2024 20:37

yusukebe approved these changes May 22, 2024

View reviewed changes

yusukebe merged commit 568f872 into honojs:next May 22, 2024
10 checks passed

MathurAditya724 mentioned this pull request May 24, 2024

refactor(utils/body): minor cleanup #2783

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(utils/body): add dot notation support for `parseBody` #2675

feat(utils/body): add dot notation support for `parseBody` #2675

fzn0x commented May 15, 2024 •

edited

MathurAditya724 left a comment

fzn0x commented May 15, 2024

fzn0x commented May 15, 2024 •

edited

MathurAditya724 commented May 15, 2024

yusukebe commented May 15, 2024

fzn0x commented May 15, 2024

MathurAditya724 left a comment

yusukebe commented May 15, 2024

fzn0x commented May 15, 2024

yusukebe commented May 15, 2024

vanodevium commented May 15, 2024

MathurAditya724 commented May 15, 2024

fzn0x commented May 15, 2024

MathurAditya724 commented May 15, 2024

MathurAditya724 commented May 15, 2024 •

edited

fzn0x commented May 15, 2024

fzn0x commented May 15, 2024

fzn0x commented May 15, 2024

yusukebe commented May 15, 2024

usualoma commented May 15, 2024

fzn0x commented May 15, 2024

usualoma commented May 16, 2024

MathurAditya724 left a comment

fzn0x commented May 16, 2024

fzn0x commented May 16, 2024 •

edited

yusukebe left a comment

yusukebe commented May 22, 2024

feat(utils/body): add dot notation support for parseBody #2675

feat(utils/body): add dot notation support for parseBody #2675

Conversation

fzn0x commented May 15, 2024 • edited

The author should do the following, if applicable

MathurAditya724 left a comment

Choose a reason for hiding this comment

fzn0x commented May 15, 2024

fzn0x commented May 15, 2024 • edited

Dot Notation

MathurAditya724 commented May 15, 2024

yusukebe commented May 15, 2024

fzn0x commented May 15, 2024

MathurAditya724 left a comment

Choose a reason for hiding this comment

yusukebe commented May 15, 2024

fzn0x commented May 15, 2024

yusukebe commented May 15, 2024

vanodevium commented May 15, 2024

MathurAditya724 commented May 15, 2024

fzn0x commented May 15, 2024

MathurAditya724 commented May 15, 2024

MathurAditya724 commented May 15, 2024 • edited

fzn0x commented May 15, 2024

fzn0x commented May 15, 2024

fzn0x commented May 15, 2024

yusukebe commented May 15, 2024

usualoma commented May 15, 2024

all is always enabled.

Objects containing arbitrary data should be initialized using Object.create(null)

fzn0x commented May 15, 2024

usualoma commented May 16, 2024

MathurAditya724 left a comment

Choose a reason for hiding this comment

fzn0x commented May 16, 2024

fzn0x commented May 16, 2024 • edited

yusukebe left a comment

Choose a reason for hiding this comment

yusukebe commented May 22, 2024

feat(utils/body): add dot notation support for `parseBody` #2675

feat(utils/body): add dot notation support for `parseBody` #2675

fzn0x commented May 15, 2024 •

edited

fzn0x commented May 15, 2024 •

edited

MathurAditya724 commented May 15, 2024 •

edited

`all` is always enabled.

Objects containing arbitrary data should be initialized using `Object.create(null)`

fzn0x commented May 16, 2024 •

edited