tsedio / tsed

:triangular_ruler: Ts.ED is a Node.js and TypeScript framework on top of Express to write your application with TypeScript (or ES6). It provides a lot of decorators and guideline to make your code more readable and less error-prone. ⭐️ Star to support our work!
https://tsed.io/
MIT License
2.84k stars 286 forks source link

FormData property not available from BodyParams in a Middleware #2745

Closed csnate closed 2 months ago

csnate commented 3 months ago

Describe the bug

This is working in v7.33.0 and I'm not sure when it broke.

Trying to access a FormData property from @BodyParams('key') does not work from within a Middleware. However, I am able to access the property from the Controller method.

Example

I have a frontend form that sets a file for upload and some additional form information in a FormData object, and then sends that to the server.

Ex pseudo-code (frontend):

const formData = new FormData();
formData.append('list', JSON.stringify({ foo: 'bar' });
formData.append('file', file); // This is a File object from when a user uploads a file
await fetch('https://my-url.com', {
  method: 'POST',
  body: formData
});

Ex code (server):

@Middleware()
export class TestBodyParamsMiddleware implements MiddlewareMethods {
    public use(@BodyParams('list') list: string, @Next() next: Next) {
        console.log('List', list);
        next();
    }
}

// Controller method
@Post()
@MulterOptions({ storage: multer.diskStorage({}) })
@Use(TestBodyParamsMiddleware)
@Returns(200)
public async testBodyParams(
    @MultipartFile('file') file: PlatformMulterFile,
    @BodyParams('list') list: string,
): Promise<object> {
    const listObj = JSON.parse(list);
    console.log('List', listObj);
    console.log('file', file);
    return {};
}

With this code, the following happens in the Middleware - list is undefined: Screenshot 2024-07-02 at 1 56 09 PM

However in the actual controller method, list is available as well as the file from the FormData. Screenshot 2024-07-02 at 1 57 27 PM

To Reproduce

See description for steps

Expected behavior

@BodyParams('key') gets the request data from a FormData object from within a Middleware

Code snippets

No response

Repository URL example

No response

OS

macOS, Docker

Node version

Node 20.11.1

Library version

v7.75.0

Additional context

No response

Romakita commented 3 months ago

Hello @csnate

Accessing to body content when you have multipart file isn't possible inside a middleware because multipart content are processed by multer. Isn't a bug, but a limitation related to multer usage on this route. I'm not sure how to solve that. I suggest you to use Interceptor instead middleware.

csnate commented 3 months ago

Odd that it used to work with v7.33.0 though. Did anything change with the order of processing related to multer?

I'll investigate switching to use an Interceptor instead, or just move the logic into the Controller method.

Romakita commented 3 months ago

Hum sorry, I probably missed something on your issue. I don't know what exactly changed. Maybe it's just a configuration issue. I'll take a time to check the changes.

Romakita commented 3 months ago

Maybe you create a small repository to repro the issue please ;)

csnate commented 3 months ago

No rush (I figured out a workaround) but it could be something another person runs into.

I'll try and write up a small repo for it, but I may not be able to get to it until next week.

csnate commented 3 months ago

@Romakita ok, I got some free time today and was able to get an example repo for you

https://github.com/csnate/tsed-multi-formdata

The main branch is using 7.75.0, the tsed-7.33.0 branch is using 7.33.0. There is a single controller that has a SetNameMiddleware applied to the route that will read in the name from BodyParams and set it to the Locals. The body of the controller then checks if @Locals('name') and the file exists. If it does, respond with 200. If it does not, then respond with 500

Steps to reproduce:

The following CURL request should work as well, just replace the file with a valid local file:

curl  -X POST \
  'http://localhost:8083/rest/multi' \
  --header 'Accept: */*' \
  --form 'name="test"' \
  --form 'file=@/PATH/TO/SOME/FILE'
ochrstn commented 2 months ago

I had the same issues and came up with the following Pipe to handle JSON with multipart/form-data requests if send along with other (binary) files:

import { UsePipe } from '@tsed/platform-params';
import { Type, useDecorators } from '@tsed/core';
import { Inject, Injectable, Req, ValidationError } from '@tsed/common';
import {
  InFile,
  JsonParameterStore,
  type PipeMethods,
  getJsonSchema,
  Groups,
} from '@tsed/schema';
import { deserialize } from '@tsed/json-mapper';
import Ajv from 'ajv';

export interface MultipartJsonProps<T> {
  type: Type<T>;
  name: string;
  groups?: string[];
}

@Injectable()
class MultipartJsonValidatorPipe<T> implements PipeMethods<Request, T> {
  @Inject()
  ajv: Ajv;

  transform(req: Request, metadata: JsonParameterStore) {
    const options = metadata.store.get<MultipartJsonProps<T>>(
      MultipartJsonValidatorPipe
    );

    if (!req.body || !req.body[options.name]) {
      throw new ValidationError('EMPTY BODY', this.ajv.errors!);
    }

    const dto = JSON.parse(req.body[options.name]);
    const schema = getJsonSchema(options.type, { groups: options?.groups });
    if (!this.ajv.validate(schema, dto)) {
      throw new ValidationError('Error while validation', this.ajv.errors!);
    }
    return deserialize(dto, { type: options.type, groups: options?.groups });
  }
}

export function MultipartJson<T>(
  options: MultipartJsonProps<T>
): ParameterDecorator {
  return useDecorators(
    InFile(options.name),
    Req(),
    UsePipe(MultipartJsonValidatorPipe<T>, options),
    Groups(...(options.groups || []))
  );
}

Usage:

@Controller({
  path: "/car-models",
  children: [CarModelGalleryController],
})
export class CarModelController {
  @Put("/:id")
  @Auth({ allowedRoles: [UserRole.SUPERADMIN, UserRole.ADMIN] })
  @OperationId("updateCarModel")
  @Returns(200, Pagination).Of(CarModelEntity).Groups("create")
  async update(
    @PathParams("id") id: string,
    @MultipartJson({
      type: CarModelEntity,
      name: "model",
      groups: ["create"],
    })
    model: CarModelEntity,
    @MultipartFile("catalogImage") catalogImageMulterFile?: PlatformMulterFile
  ) {
    // ....
  }
}

It works but i'm sure that it can be improved. Maybe @Romakita has some ideas.

Romakita commented 2 months ago

@ochrstn Why do you not have added @BodyParams decorator ?


@Controller({
  path: "/car-models",
  children: [CarModelGalleryController],
})
export class CarModelController {
  @Put("/:id")
  @Auth({ allowedRoles: [UserRole.SUPERADMIN, UserRole.ADMIN] })
  @OperationId("updateCarModel")
  @Returns(200, Pagination).Of(CarModelEntity).Groups("create")
  async update(
    @PathParams("id") id: string,
    @BodyParams("model") @Groups("create")
    model: CarModelEntity,
    @MultipartFile("catalogImage") catalogImageMulterFile?: PlatformMulterFile
  ) {
    // ....
  }
}

I'll try to take time ASAP to check your issue @csnate`. Thanks for your repository example !

ochrstn commented 2 months ago

@ochrstn Why do you not have added @BodyParams decorator ?

@Controller({
  path: "/car-models",
  children: [CarModelGalleryController],
})
export class CarModelController {
  @Put("/:id")
  @Auth({ allowedRoles: [UserRole.SUPERADMIN, UserRole.ADMIN] })
  @OperationId("updateCarModel")
  @Returns(200, Pagination).Of(CarModelEntity).Groups("create")
  async update(
    @PathParams("id") id: string,
    @BodyParams("model") @Groups("create")
    model: CarModelEntity,
    @MultipartFile("catalogImage") catalogImageMulterFile?: PlatformMulterFile
  ) {
    // ....
  }
}

I'll try to take time ASAP to check your issue @csnate`. Thanks for your repository example !

Because the object arrives as a string:

Bad request on parameter \"request.body.model\".\nCarModelEntity must be object. Given value: \"{\\\"name\\\":\\\"Test\\\}\"

This is the client code sending the request:

export const createCarModel = (
  createCarModelBody: CreateCarModelBody,
  options?: SecondParameter<typeof customClient>
) => {
  const formData = new FormData();
  if (createCarModelBody.model !== undefined) {
    formData.append("model", JSON.stringify(createCarModelBody.model));
  }
  if (createCarModelBody.catalogGallery !== undefined) {
    createCarModelBody.catalogGallery.forEach((value) =>
      formData.append("catalogGallery", value)
    );
  }
  if (createCarModelBody.catalogImage !== undefined) {
    formData.append("catalogImage", createCarModelBody.catalogImage);
  }

  return customClient<CreateCarModel200>(
    {
      url: `/api/v1/car-models`,
      method: "POST",
      headers: { "Content-Type": "multipart/form-data" },
      data: formData,
    },
    options
  );
};
Romakita commented 2 months ago

@ochrstn Oh yes I remember!. Maybe you can use something shorter. Ts.ED support since a long time a priority option on Pipe. So the main idea is to create JsonPipe to process the value before validating / deserialize it:

@Injectable({
  priority: -900
})
export class JsonPipe implements PipeMethods {
   transform(value: string) {
      return JSON.parse(value);
   }
}

export function Json() {
   return usePipe(JsonPipe);
}

@Controller({
  path: "/car-models",
  children: [CarModelGalleryController],
})
export class CarModelController {
  @Put("/:id")
  @Auth({ allowedRoles: [UserRole.SUPERADMIN, UserRole.ADMIN] })
  @OperationId("updateCarModel")
  @Returns(200, Pagination).Of(CarModelEntity).Groups("create")
  async update(
    @PathParams("id") id: string,
    @BodyParams("model") @Json() @Groups("create")
    model: CarModelEntity,
    @MultipartFile("catalogImage") catalogImageMulterFile?: PlatformMulterFile
  ) {
    // ....
  }
}

It should work as expected :D and I'm sorry for not giving you this solution sooner.

See you

Romakita commented 2 months ago

@csnate I see where the problem. Ok I'm sorry, to fix an issue with the middleware order execution on endpoint I add also a prioritary option on middleware decorator. Order isn't easy to determine when decorators can be applied on class or on method (or twice). For example, the authentication middleware must be applied before all other middlewares like PlatformMulterMiddleware as is decribed in this issue https://github.com/tsedio/tsed/issues/865. The prioritary option solve that.

Unfortunately your middleware is called before PlatformMulterMiddleware. Adding {prioritary: 11} on your middleware decorator option will solve your issue ^^. If somebody have time to add this point on the multipart page documentation would be great and help me a lot ;)

Note: @ochrstn could be also a good idea to add the JsonPipe solution for multipart page documentation ?

Here the final solution:

@Middleware({ prioritary: 11 })
export class TestBodyParamsMiddleware implements MiddlewareMethods {
    public use(@BodyParams('list') list: string, @Next() next: Next) {
        console.log('List', list);
        next();
    }
}

See you

github-actions[bot] commented 2 months ago

🎉 Are you happy?

If you appreciated the support, know that it is free and is carried out on personal time ;)

A support, even a little bit makes a difference for me and continues to bring you answers!

github opencollective

ochrstn commented 2 months ago
export function Json() {
  return usePipe(JsonPipe); // UsePipe
}

Great, that works! (except it is UsePipe instead of usePipe)

Note: @ochrstn could be also a good idea to add the JsonPipe solution for multipart page documentation ?

I think that it is a common usecase and should be documented. Maybe tsed could provide the @Json decorator out of the box?

Also, I guess it would be helpful to document the priority option. Why did you choose -900 above? (I guess the default is 0 and there is another one at -1000 🤔 . Maybe there could be some tooling to visualize the final order inside the application 😇

Romakita commented 2 months ago

@ochrstn agree to document all of that but I'm already on some new features for tsed + the new website. Any help are welcome.

-1000 priority is actual value for the ParseExpressionPipe ;) this why I set -900 xD. Ok to add `@Jsonˋ if the explain why you need it ;).

A tool visualise priority order 😅 why not but it's a huge feature.

See you!

csnate commented 2 months ago

@Romakita - "If somebody have time to add this point on the multipart page documentation would be great and help me a lot ;)" - are you referring to https://tsed.io/docs/upload-files.html or is there another docs page you meant?

Romakita commented 2 months ago

@csnate yes multipart page should be the good place ;)

csnate commented 2 months ago

@Romakita here ya go - https://github.com/tsedio/tsed/pull/2770