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

Feature: Extendable fileUploader #248

Open
dotMortis opened this issue Feb 26, 2020 · 5 comments
Open

Feature: Extendable fileUploader #248

dotMortis opened this issue Feb 26, 2020 · 5 comments
Labels
enhancement New feature or request

Comments

@dotMortis
Copy link

Hello again :)

I'm going to use multer to upload files, but there are some specific issues with your file uploader.
I want to handle the file upload with multer and then the file should be uploaded to an AWS S3 Bucket.

When I use your fileUploader, the file is only saved in my local folder. It would be really nice if I could add a middleware to your fileUploader so that I can process the new File after multer.

Another option is to set the fileUploader to false and add my own middleware logic. However, if the fileUploader is set false, your validator will no longer validate my openapi definitions for files.

Is there a workaround or am I missing something?

Thank you!

@cdimascio cdimascio added the enhancement New feature or request label Feb 26, 2020
@dotMortis
Copy link
Author

dotMortis commented Feb 27, 2020

  • However, if the fileUploader is set false, your validator will no longer validate my openapi definitions for files.

This is wrong. It's more like your validator can't validate the body anymore. It will fail if the fileUploader is false and the body property "image" is required even if the correct body is sent.

    requestBody:
        content:
          multipart/form-data:
            schema:
              description: default
              type: object
              additionalProperties: false
              required:
                -  image
              properties:
                image:
                  type: string
                  format: binary

In addition, middleware should be able to be attached to the beginning and end of the fileUploader so that authorization logic can be added.

@cdimascio cdimascio added v4 version 4 and removed v4 version 4 labels Jun 6, 2020
@cdimascio
Copy link
Owner

cdimascio commented Aug 23, 2020

@Haruki-Mortis apologies for the late reply. this is certainly something that can use improvement. we'll need to provide a way to handle the validation, then call next to enable the implementor to provide a custom file upload implementation. i will think on this. let me know if you have suggestions. ultimately, it means improving how the multipart middleware works

PRs are welcome, if you want to take a stab at it

@dotMortis
Copy link
Author

dotMortis commented Oct 16, 2020

@cdimascio sry for the late reply, too.

How about something like this (just a quick example):

types.ts

// extended fileuploader options
export type FileUploaderOptions = {
  multer: multer.Options;
  preMiddleware?: FileUploaderMiddlewareOptions[];
  postMiddleware?: FileUploaderMiddlewareOptions[];
};

// define a middlware
// optional for specific path(s)
export type FileUploaderMiddlewareOptions = {
  middleware: OpenApiRequestHandler;
  onPath?: RegExp | string;
};

export interface OpenApiValidatorOpts {
  apiSpec: OpenAPIV3.Document | string;
  validateResponses?: boolean | ValidateResponseOpts;
  validateRequests?: boolean | ValidateRequestOpts;
  validateSecurity?: boolean | ValidateSecurityOpts;
  ignorePaths?: RegExp;
  securityHandlers?: SecurityHandlers;
  coerceTypes?: boolean | 'array';
  unknownFormats?: true | string[] | 'ignore';
  fileUploader?: boolean | FileUploaderOptions; // Update this one
  multerOpts?: multer.Options;
  $refParser?: {
    mode: 'bundle' | 'dereference';
  };
  operationHandlers?: false | string | OperationHandlerOptions;
  validateFormats?: false | 'fast' | 'full';
}

openapi.multipart.ts

// add some validation logic to middlewares
export function toMultipartMiddleware(
  middlewareOpts: FileUploaderMiddlewareOptions[],
  req: OpenApiRequest,
  pContext: Promise<OpenApiContext>,
): OpenApiRequestHandler[] {
  return middlewareOpts?.map<OpenApiRequestHandler>(
    (middlewareOpts: FileUploaderMiddlewareOptions) => (
      req: OpenApiRequest,
      res: Response,
      next: NextFunction,
    ) =>
      pContext
        .then(() => {
          // default isOnPath should be true
          let isOnPath = true;
          // if onPath is set validate the 
          if (middlewareOpts.onPath) {
            // get current requested path
            const currentPath = (<OpenApiRequestMetadata>req.openapi)
                .openApiRoute;
            //validate the path
            isOnPath =
              typeof middlewareOpts.onPath === 'string'
                ? currentPath.includes(middlewareOpts.onPath)
                : currentPath.match(middlewareOpts.onPath)
                ? true
                : false;
          }
          
          
          if (isMultipart(req) && isOnPath) {
            //call the pre/postmiddleware if all requirements are valid
            return middlewareOpts.middleware(req, res, next);
          } else {
            // else just call next
            next();
          }
        })
        .catch(next),
  ) || [];
}

openapi.validator.ts

export class OpenApiValidator {
  ...
  constructor {
    ...
    if (options.fileUploader == null) options.fileUploader = true
    ...
  }
  ...
  installMiddleware(spec: Promise<Spec>): OpenApiRequestHandler[] {
    ...
    if (this.options.fileUploader) {
      // multipart middleware
      const {
        multer,
        preMiddleware = [],
        postMiddleware = [],
      }: FileUploaderOptions =
        typeof this.options.fileUploader === 'boolean'
          ? undefined
          : this.options.fileUploader;

      // install premiddlwares
      middlewares.push(...toMultipartMiddleware(preMiddleware, pContext));

      // install multer
      let fumw;
      middlewares.push((req, res, next) =>
        pContext
          .then((context) => {
            fumw = fumw || this.multipartMiddleware(context);
            return fumw(req, res, next);
          })
          .catch(next),
      );

      // install postmiddlewares
      middlewares.push(...toMultipartMiddleware(postMiddleware, pContext));
    }
    ...
  }
  ...
  private multipartMiddleware(context: OpenApiContext) {
    return middlewares.multipart(context, {
      // get multer options if set
      multerOpts: this.options.fileUploader['multer'],
      unknownFormats: this.options.unknownFormats,
    });
  }
  ...
}

@cdimascio
Copy link
Owner

cdimascio commented Oct 17, 2020

@Haruki-Mortis Thanks so much for this proposal. Would you mind putting together a PR with your idea? It will be great to expand the capability here.

Note: I generally try to apply this rule of thumb:
"always use a standard express mechanism when possible. add a proprietary mechanism only when no other option exists".

With the rule of thumb in mind, here are some comments:

  • Can we do this without pre op middleware?
    Given the scenario, it does seem that we must introduce a non-standard express mechanism to allow a user to take full control of the file upload. It will be great if we can pull this off with a single middleware function, rather than two (pre/post).

  • Is there a way we can avoid the user having to provide a path match in the options i.e. onPath. I would be good if user can instead add that logic within their provided middleware function.

In terms of interface, can we get closer to the following?

export type FileUploaderOptions = {
  multer: multer.Options;
  handler: (req: Request, res: Response, next: NextFunction) => void 
};

@dotMortis
Copy link
Author

I'm working on it this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants