Nest: Guards and Pipes Execution order

[amirsaber]

I'm submitting a...

[ ] Regression 
[ ] Bug report
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Guards are executed after each middleware, but before any pipe.

Expected behavior

Being able to order them or use pipes before guards.

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

I am using nest in different applications and I am noticing in some cases guards are dependent of what is inside body. In those cases I would want to have the validationpipe before any guard so that if the request is not fully compliant with what we expect, block it from there.

I believe this will have some performance impacts too since guards are using services and talking to databases before making decisions so by running validationpipe before guards we can avoid unnecessary calls.

Thanks for this amazing framework.

[cojack]

@amirsaber totally agree!

[amirsaber]

@kamilmysliwiec any thought on this?

[kamilmysliwiec]

I totally understand your point here. Nevertheless, giving an ability to switch execution hierarchy may bring a lot of mess to the framework and make the codebases less consistent since the order might by totally inverted. Any ideas how the API could hypothetically look like?

[amirsaber]

I am wondering what is the initial thought about executing pipes after guards. That might help me to understand what is the best solution.
In terms of API, I can think of a variable as options in Pipes and Guards that will define their priority. Something like

  @UsePipes(new ValidationPipe({ transform: true }), {
       order: 1,
  })
  @UseGuards(FooGuard, {
       order: 2,
  })
  public async FooFunction......

If the order is not defined or equal we can follow a static rule of which one to execute first.

We can also stick to the idea of pipes being used for transforming data to desired output and provide a ValidationGuard that takes care of throwing error if body is not in correct format.

[maroon1]

how abount this solution? just like interceptor. @kamilmysliwiec

@Injectable()
export class Guard implements CanActivate {
  canActivate(
    context: ExecutionContext,
    pipe$: Observable<any>,
  ): boolean | Promise<boolean> | Observable<boolean> {
    /* guard before pipe */
    if (false) return of(false);

    return pipe$.pipe(map(() => {
      /* guard after pipe */
      return true;
    }));
  }
}

[marcus-sa]

@amirasaber
Just identify which order the decoraters have been applied by using Reflect.getMetadataKeys instead of an order option.

EDIT:
Nevermind I see that requires some extra work due to how the metadata is just extended.
https://github.com/nestjs/nest/blob/ce498e86150f7de4a260f0c393d47ec4cc920ea1/packages/common/decorators/core/use-guards.decorator.ts#L17-L37

[ozeas]

@kamilmysliwiec great point, although some features make it easier to work, in some cases, they can break the default structure of the framework and cause problems in the of use.

[cojack]

@kamilmysliwiec any future research?

[kamilmysliwiec]

Even though this change could sometimes, potentially make life easier, we cannot break the default request pipeline and the natural behavior of the framework. As I've mentioned, giving an ability to switch execution hierarchy may bring a lot of mess to the framework and make the codebases less consistent since the order might by totally inverted. Every time when someone would face an issue and ask a question on either StackOverflow or Discord, we would first have to ask if the default order has been changed. Additionally, some packages/solutions/tutorials might stop working correctly if one decides to switch an execution order leading to some strange side-effect. So sadly, because I love the flexibility, this change would very likely bring us more problems than benefits.

[lock[bot]]

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.