nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
66.9k stars 7.55k forks source link

A not responding route when using a middleware #1227

Closed hypeofpipe closed 5 years ago

hypeofpipe commented 5 years ago

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request

Current behavior

When adding a middleware to the module, it breaks and whole route (to which a middleware was added) is broken and don't respond too.

Expected behavior

I expect to get an error about it because it doesn't even throw any exceptions and it's very hard to detect using a debugger.

Minimal reproduction of the problem with instructions

  1. Clone this repo https://github.com/nestjs/typescript-starter
  2. Add the next piece of code: app.module.ts
    @Module({
    imports: [SampleMiddleware],
    controllers: [AppController],
    providers: [AppService],
    })
    export class AppModule implements NestModule {
    configure(consumer: MiddlewareConsumer) {
    consumer
      .apply(SampleMiddleware)
      .forRoutes('whatever');
    }
    }

    sample.middleware.ts

@Injectable()
export class SampleMiddleware implements NestMiddleware {
  resolve(..._args: any[]): MiddlewareFunction {
    return (req: Request, _res, next: Function | undefined) => {
      console.log('Well, hello from typevalidator')
      if (next) {
       next();
      }
    };
  }
}

What is the motivation/use case for changing the behavior?

Unfortunately, I don't know. The code seems legit though.

Environment


Nest version: ^5.4.0

For Tooling issues:
- Node version: v8.12.0
- Platform: macOS High Sierra

Others:
yarn v1.10.1
VSCode Version 1.28.1
barretojs commented 5 years ago

tried here, it's working fine. you forgot the closing brace on the config method on your app module.

BrunnerLivio commented 5 years ago

I’ve run in a similar problem when I incorrectly returned the observable of a interceptor. This caused the whole application to silently fail / not respond at all to web requests. As a user this can be really hard to debug, since there is no error thrown.

I think this issue describes a similar behavior?

riadhriadh commented 5 years ago

this my code can help you

in app.module.ts

` import { Module , NestModule,MiddlewareConsumer , RequestMethod } from '@nestjs/common'; import { AppController } from 'app.controller'; import { AppService } from 'app.service'; import { CatsController } from 'cats/cats.controller'; import { CatsService } from 'cats/cats.service'; import { CatsModule } from 'cats/cats.module'; import { TypeOrmModule } from '@nestjs/typeorm'; import { Connection } from 'typeorm'; import { UsersController } from 'users/users.controller'; import { UsersService } from 'users/users.service'; import { UsersModule } from 'users/users.module'; import { AuthService } from 'auth/auth.service'; import { AuthController } from 'auth/auth.controller'; import { LoggerMiddleware } from 'logger.middleware';

@Module({ imports: [TypeOrmModule.forRoot(),CatsModule, UsersModule], controllers: [AppController, UsersController, AuthController], providers: [AppService, UsersService,AuthService], }) export class AppModule {

// constructor(private readonly connection: Connection) {

// } configure(consumer: MiddlewareConsumer) { consumer .apply(LoggerMiddleware) .forRoutes({ path: '/cats', method: RequestMethod.GET });

} }

`

in logger.middleware.ts

` import { Injectable, NestMiddleware, MiddlewareFunction, HttpException, HttpStatus, } from '@nestjs/common'; import as passport from 'passport'; import { JwtStrategy } from 'auth/passport/jwt.strategy'; import as jwt from 'jsonwebtoken'; const config_projet = require('./projet_config'); import { UsersService } from 'users/users.service'; import { User } from 'users/user.entity';

@Injectable() export class LoggerMiddleware implements NestMiddleware { constructor(private readonly usersService: UsersService) {} public resolve() { return async (req, res, next) => { if (req.headers.authorization) { console.log('token : ', req.headers.authorization); const token = req.headers.authorization; const decoded=null; try { const decoded: any = jwt.verify(token, config_projet.secret); } catch (error) { throw new HttpException( { status: HttpStatus.FORBIDDEN, error: 'This is a custom message', }, 403, ); }

    if (decoded) {
      console.log('decoded :', decoded);
      if (decoded.email) {
        const user = await this.usersService.getUserByEmail(decoded.email);
        if (user) {
          console.log('user', user);
          next();
        } else {
          throw new HttpException(
            {
              status: HttpStatus.FORBIDDEN,
              error: 'This is a custom message',
            },
            403,
          );
        }
      } else {
        throw new HttpException(
          {
            status: HttpStatus.FORBIDDEN,
            error: 'This is a custom message',
          },
          403,
        );
      }
    } else {
      throw new HttpException(
        {
          status: HttpStatus.FORBIDDEN,
          error: 'This is a custom message',
        },
        403,
      );
    }
  }else{
    throw new HttpException(
      {
        status: HttpStatus.FORBIDDEN,
        error: 'This is a custom message',
      },
      403,
    );
  }
};

} } `

riadhriadh commented 5 years ago

i think this method false in documentation https://docs.nestjs.com/middleware is not import SampleMiddleware and in configure(consumer: MiddlewareConsumer) { is add MiddlewareConsumer

barretojs commented 5 years ago

can you say in what situation the error is happening? what does the usersService.getUserByEmail() returns? no error is appearing at all?

barretojs commented 5 years ago

i see one problem with your logic, not the middleware. if the usersService.getUserByEmail is making a query in a DB, like with MongooseModule for example, and the user don't exist, a error will be thrown. since your usersService.getUserByEmail() call is not wrapped in a try/catch block on your middleware, the lines following the getUserByEmail will not be executed. so, with this you won't enter the if(user) neither its else, and you will not enter the other elses, because the JWT was decoded and the email is present on the token.

do you think this can be the problem?

TL;DR: try wrapping the userService.getUserByEmail in a try/catch

edit: after messing around with the middleware code, try doing this in your configure function in app.module to see if the apply returned properly. i think if it shows your middleware and route, it should be set up properly const logger: any = consumer.apply(LoggerMiddleware).forRoutes('/'); for (let [key, value] of logger.middlewareCollection.entries()) console.log(value);

riadhriadh commented 5 years ago

usersService.getUserByEmail() returns if this email in data base

riadhriadh commented 5 years ago

@barretojs but if token not verified

hypeofpipe commented 5 years ago

tried here, it's working fine. you forgot the closing brace on the config method on your app module.

Yeah, I just forgot to close the brace in my bug report, in the code, it's normally closed, so the issue isn't in it.

barretojs commented 5 years ago

your code is only this? try putting if (next) { next(); } else { throw new HttpException('next error', 400); } on linux with npm is working totally fine. if your code is only this and it's not working maybe it's a platform specific error.

hypeofpipe commented 5 years ago

i think this method false in documentation https://docs.nestjs.com/middleware is not import SampleMiddleware and in configure(consumer: MiddlewareConsumer) { is add MiddlewareConsumer

But in the documentation - there is a module with that middleware, but as You can see, my middleware is without a module.

hypeofpipe commented 5 years ago

tried here, it's working fine. you forgot the closing brace on the config method on your app module.

You know what, I think, that it might be related to platform, but I'm not sure, if I didn't make a mistake.

barretojs commented 5 years ago

@hypeofpipe if this is the only code and you're getting a error, it probably is platform-related.

hypeofpipe commented 5 years ago

@barretojs it's not.

I found out that if you remove forRoutes('whatever') in AppModule at configure method - it would work. I don't know why. It needs more investigations!

barretojs commented 5 years ago

but if you remove the forRoutes your middleware is not getting applied. if you could share a repository with your real code it'd be easier to help

hypeofpipe commented 5 years ago

@barretojs Yeah, I've understood it recently... Um, okay, I'll share it. Here is the link https://github.com/hypeofpipe/images-into-ascii-nestjs

hypeofpipe commented 5 years ago

UPD: I think the problem is in my middleware because, besides it, everything works fine.

If you have the same problem, just use "Functional middleware" like this: logger.middleware.js

export function logger(req, res, next) {
  console.log(`Request...`);
  next();
};
barretojs commented 5 years ago

@hypeofpipe yeah man, sorry that i wasn't able to help, this is one weird behavior. i tried debugging here but nothing seemed to work, and you middleware seems to be defined properly.

but it's good to know that functional middlewares are working better.

hypeofpipe commented 5 years ago

@hypeofpipe yeah man, sorry that i wasn't able to help, this is one weird behavior. i tried debugging here but nothing seemed to work, and you middleware seems to be defined properly.

but it's good to know that functional middlewares are working better.

Thank you a lot for Your engagement in this problem :)

Yeah, it might be a platform problem, @kamilmysliwiec - what do you think?

hypeofpipe commented 5 years ago

UPD: Pipes aren't working too. I don't know what am I doing wrong... The app is the same. It's not even responding to the console. I'll try to debug it more, but I can't understand what's wrong.

app.controller.ts

@Controller('image')
export class ImageController {
  constructor(private readonly appService: AppService) {}

  @Post()
  @UseInterceptors(FileInterceptor('file', { storage: multer.memoryStorage() }))
  @UsePipes(new FileValidatorPipe())
  @UseGuards(new KeyGuard())
  transform(@UploadedFile() file: Express.Multer.File): string {
    const res = new FileToBase64Pipe().transform(file);
    return this.appService.transform(res)
  }
}

file-validator.pipe.ts

@Injectable()
export class FileValidatorPipe implements PipeTransform<any> {
   transform(_value: any, _metadata: ArgumentMetadata) {
    console.log('what')
    return _value
  }
}
kamilmysliwiec commented 5 years ago

Please, use StackOverflow for such questions. We are not here to explain a normal framework behavior.

kiwikern commented 5 years ago

The question was reposted on StackOverflow.

hypeofpipe commented 5 years ago

The problem was in my tsconfig, I should use target at least es6

jbjhjm commented 5 years ago

I'm experiencing the same problem. No way to make injectable middleware work. Switching to the functional middleware suggested by hypeofpipe works though.

@hypeofpipe were you able to solve the initial issue? I'm not sure if your comment to use target=es6 was related to the whole problem or only the use of pipes. For me, es6 configuration did not solve the issue. Going to use functional middleware for now, though it would be of interest to find out why it's not working as intended.

josephcarington commented 5 years ago

I am experiencing the same problem. Injectable middleware causes requests matching the url to never return. My middleware is never called. Functional middleware works. I created a minimal test and was able to reproduce it. @nest/common@6.0.2, Debian 9.8

mcblum commented 5 years ago

@hypeofpipe holy shit dude thank you. This was driving me absolutely insane.

lock[bot] commented 5 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.