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

A not responding route when using a middleware #1227

Closed
hypeofpipe opened this issue Oct 24, 2018 · 27 comments
Closed

A not responding route when using a middleware #1227

hypeofpipe opened this issue Oct 24, 2018 · 27 comments

Comments

@hypeofpipe
Copy link

hypeofpipe commented Oct 24, 2018

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
@hypeofpipe hypeofpipe changed the title Hidden error when using a middleware A not responding route when using a middleware Oct 24, 2018
@barretojs
Copy link

barretojs commented Oct 24, 2018

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

@BrunnerLivio
Copy link
Member

BrunnerLivio commented Oct 24, 2018

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
Copy link

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
Copy link

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
Copy link

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

@barretojs
Copy link

barretojs commented Oct 25, 2018

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
Copy link

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

@riadhriadh
Copy link

@barretojs
but if token not verified

@hypeofpipe
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link
Author

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
Copy link

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

@hypeofpipe
Copy link
Author

@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
Copy link

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
Copy link
Author

hypeofpipe commented Oct 25, 2018

@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
Copy link
Author

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
Copy link

@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
Copy link
Author

@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
Copy link
Author

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
Copy link
Member

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

@kiwikern
Copy link
Contributor

The question was reposted on StackOverflow.

@hypeofpipe
Copy link
Author

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

@jbjhjm
Copy link

jbjhjm commented Mar 11, 2019

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
Copy link

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
Copy link

mcblum commented May 24, 2019

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

@lock
Copy link

lock bot commented Sep 23, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants