Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array of object with file and a string field throwing "string field must be defined" message always. #4777

Open
mdthansil opened this issue Oct 26, 2024 · 12 comments
Assignees
Labels
Semver: Major Has breaking changes Type: Enhancement Improving an existing feature

Comments

@mdthansil
Copy link

mdthansil commented Oct 26, 2024

Package version

Adonis Core Version: 6.14.1, vineJS version: 2.1.0

Describe the bug

Using Adonisjs with inertia + vue(SSR)

// router.ts
router.post('/upload', async ({ request, response }) => {
  const data = await request.validateUsing(
    vine.compile(
      vine.object({
        images: vine.array(vine.object({ file: vine.file({ size: '2mb' }), title: vine.string() })),
      })
    )
  )
  return response.json(data)
})

//home.vue
<script setup lang="ts">
import { Head, useForm } from '@inertiajs/vue3'

const getError = (error: any, key: string) => {
  return error?.[key]?.join(',')
}

const form = useForm<{
  images: { file: File | undefined; title: string }[]
}>({
  images: [{ file: undefined, title: '' }],
})
</script>

<template>
  <Head>
    <title>Home</title>
  </Head>
  <h1>Form</h1>
  <div>
    <form
      @submit.prevent="form.post('/upload')"
      style="display: flex; flex-direction: column; gap: 1rem; padding: 3rem"
    >
      <template v-for="(_, index) in form.images" :key="index">
        <input type="text" placeholder="Title" v-model="form.images[index].title" />
        <span>{{ getError(form.errors, `images.${index}.title`) }}</span>
        <input
          type="file"
          @input="form.images[index].file = ($event.target as HTMLInputElement)?.files?.[0]"
        />
        <span>{{ getError(form.errors, `images.${index}.file`) }}</span>
      </template>
      <button type="button" @click="form.images.push({ file: undefined, title: '' })">
        Add image
      </button>
      <button type="submit" :disabled="form.processing">Submit</button>
    </form>
  </div>
</template>
`

//Respone
"props": {
        "errors": {
            "images.0.title": [
                "The title field must be defined"
            ],
            "images.1.title": [
                "The title field must be defined"
            ],
            "images.2.title": [
                "The title field must be defined"
            ]
        }
    },

Reproduction repo

No response

@mdthansil mdthansil changed the title Array of object with file and a string field throwing "string field must be defined" message. Array of object with file and a string field throwing "string field must be defined" message always. Oct 26, 2024
@RomainLanz
Copy link
Member

Hey @mdthansil! 👋🏻

What's the content of request.all() in your controller?

@mdthansil
Copy link
Author

Hey @mdthansil! 👋🏻

What's the content of request.all() in your controller?

When I use console.log(request.all()), I'm getting the following.

image

{
  images: [ { title: 'Title 1' }, { title: 'Title 2' }, { title: 'Title 3' } ]
}

@mdthansil
Copy link
Author

Hi @RomainLanz any updates on this?

@RomainLanz
Copy link
Member

It looks like if should work fine.
Could you create a repository with the minimum amount of code to reproduce the issue?

@mdthansil
Copy link
Author

mdthansil commented Oct 28, 2024

hi @RomainLanz

Here is the link to the repository.
https://github.com/mdthansil/adonisjs-inertial-multi-fileupload

@mdthansil
Copy link
Author

Hey @RomainLanz 👋

Please note that

The issue only occurs when we use vine.file() together with other types like vine.string(). There is no problem with other combination; it works perfectly fine. For example, this setup:

tags: vine.array(vine.object({ key: vine.string(), value: vine.string() }))

functions correctly. However, when we attempt something like this:

images: vine.array(vine.object({ file: vine.file({ size: '2mb' }), title: vine.string() }))

we receive an error stating that the field must be defined.

@thetutlage
Copy link
Member

I think this issue was reported earlier as-well (maybe in the Discord). The files and the request body are two different objects and hence when you expect them to be present together than you will have to self define the data property for validation and deep merge request.all and request.files.

For example, you can use the deepmerge package from npm and use it follows.

import merge from 'deepmerge'

request.validateUsing(validator, {
  data: merge(request.all(), request.allFiles())
})

@thetutlage thetutlage self-assigned this Nov 5, 2024
@thetutlage thetutlage added the Type: Enhancement Improving an existing feature label Nov 5, 2024
@ThisIsMissEm
Copy link

Try setting enctype on the form to "multipart/form-data" then you should get both the file and title, per: https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/enctype

This form is currently submitting as application/x-www-form-urlencoded

Copy link

This issue has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still need help on this issue

@dunhamjared
Copy link

Just ran into this issue.

@thetutlage is correct, you would have to deep merge the request all() and allFiles() objects.

@ThisIsMissEm -- setting the enctype to multipart/form-data did not resolve the issue for me.

Source

Here is the code @thetutlage may be referencing:

const data = validatorOptions.data || {
...this.#ctx.request.all(),
...this.#ctx.request.allFiles(),
params: this.#ctx.request.params(),
headers: this.#ctx.request.headers(),
cookies: this.#ctx.request.cookiesList(),
}

Example

If both all() and allFiles() had the following objects:

const data = { 
   ...this.#ctx.request.all(),      // { foo: { name: bar }}
   ...this.#ctx.request.allFiles(), // { foo: { file: MultipartFile }}
} 

The const data would then be:

{ foo: { file: MultipartFile }}

But we expected:

{ foo: { name: bar, file: MultipartFile }}

@dunhamjared
Copy link

dunhamjared commented Feb 5, 2025

The bodyparser request toJSON() seems to separate the body and files, like so:

toJSON {
  id: 'example',
  url: '/foo/1/bar/2',
  query: null,
  body: { title: 'asdf', description: null }, // <--
  params: { space: '1', item: '2' },
  headers: { host: 'localhost:3333' },
  method: 'PUT',
  protocol: 'http',
  hostname: 'localhost',
  ip: '::1',
  subdomains: {},
  files: { custom: { asdf: [MultipartFile] } } // <--
}

With that in mind, could we instead update the code to this?

const data = validatorOptions.data || { 
  ...this.#ctx.request.all(), 
  files: this.#ctx.request.allFiles(), // <-- Files under its own 'file' key
  params: this.#ctx.request.params(), 
  headers: this.#ctx.request.headers(), 
  cookies: this.#ctx.request.cookiesList(), 
} 

Or possibly implement the deep merge into the validateUsing method?

@thetutlage
Copy link
Member

Deep merging will be implemented, but since it will be a breaking change, this has to wait a little :)

@thetutlage thetutlage added the Semver: Major Has breaking changes label Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Semver: Major Has breaking changes Type: Enhancement Improving an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants