[Meta] [Major] Execution Context

[dantman]

It took me a long time to fully understand the ExecutionContext/ArgumentsHost that gets passed to guards, exception handlers, etc... there is no documentation on it and the type/usage does not adequately explain how it works. The "Execution Context" documentation page does not even document ExecutionContext.

    getClass<T = any>(): Type<T>;
    getHandler(): Function;
    getArgs<T extends Array<any> = any[]>(): T;
    getArgByIndex<T = any>(index: number): T;
    switchToRpc(): RpcArgumentsHost;
    switchToHttp(): HttpArgumentsHost;
    switchToWs(): WsArgumentsHost;

When I first read the many examples in documentation showing context.switchToHttp(). I expected this was a "I am coded to work in Http, return the http context, throw (or return undefined) if this is not Http". Not "I already know I am in Http, add some helpers to this arguments array".

So when it came down to writing an AuthGuard with a @User param decorator, AdminGuard, and permissions guard all doing the same getUser logic and a UseAuth helper that composed AuthGuard with some other decorators... and then a GraphQL AuthGuard, GraphQL User param decorator, GraphQL AdminGuard, and GraphQL permissions guard; and then in the future writing a new one of each for microservices. I of course decided it would be much smarter to build getRequest(context: ExecutionContext) and getUser(context: ExecutionContext) helpers that new where request/user was in each type of context and would abstract that so universal guards/decorators can be written. After all this is a general best practice.

This of course was a naive assumption that fell apart once I realized that my app was breaking because context.switchToHttp().getRequest() was returning the @Parent() in GraphQL parameter resolver context.

Proposal

I suggest changing ExecutionContext so instead of assuming the code it is passed to always knows what type of context it is, the ExecutionContext's creator tells the ExecutionContext what type it is.

To reflect this the ArgumentHost interface would have a getType(): string added to it (or getHostType if you want to be more strict). And ExecutionContextHost will need a way to provide this to the constructor. This will allow for universal code to be written by switching on getType(). For built-in types it would probably be best to keep these types consistent with the API, e.g. When you'd expect to use context.switchToHttp() then context.getType() === 'Http'.

To my understanding, current code calling the switch* methods only run in the correct contexts and generally break if this is not the case. So I think it is safe if the switch* methods to throw an error (make it a specific subclass and give it a .code) when the type is not correct. e.g. If context.getType() === 'Ws' then calling context.switchToHttp() will throw an ArgumentsHostTypeError`.

I'm not sure what type exactly is passed to createParamDecorator factories, but it would be preferable if it too would know what the type is. Since it looks exactly like the args for an ArgumentsHost for some context types it would be preferable if it were actually one. However I'm not sure how easy it would be to square that with current factories assuming that the argument is a Request or indexed array.

Internally this would be a breaking change worth packaging into a major version. All the official nestjs libraries creating an ExecutionContext would need to be updated to declare their context type. And if there are any 3rd party libraries doing so they too would need to be updated.

However I believe for all the nestjs users nothing would break and no changes would be necessary. Since the getType is a feature addition. And switch* is generally not called in cases where it would start throwing.

That might not be the case if we chose to change createParamDecorator's arguments though. I won't decide whether it's worth requiring a small refactor to clean up the arguments.

Bonus

As an extra note. For the non-builtin class based execution contexts like GqlExecutionContext it would be nice if they had a pattern of defining something like static type = Symbol('GqlExecutionContext');. Then it would be possible to create smoother coding patterns like:

const getUser = (context: ExecutionContext): User => switchArgument<User>(context, {
  Http: (context) => context.switchToHttp().getRequest().user,
  [GqlExecutionContext.type]: (context) => GqlExecutionContext.create(context).getContext().req
});

However if you don't like the split of internal string-based context hosts with switch* methods and external class/symbol based context hosts with create static methods, and want to do a bigger refactor cleaning things up. Then I can propose a different API where internal and external execution contexts work the same using HttpExecutionContext.switch(context)/WsExecutionContext.switch(context)/GqlExecutionContext.switch(context)/... and don't need a hardcoded set of switch* helpers hardcoded in the ExecutionContext interface.

...actually I suppose if we do it that way there may be a small chance of keeping a backwards compatibility later in for one version that allows for a slow transition.

[kamilmysliwiec]

I like the idea of adding type field. Also, we could add some warnings (throw an exception?) when someone tries to switch to the incorrect context.

[ricardovf]

@kamilmysliwiec is there a reason why ExecutionContext is not available on Exception Filters?

[victordidenko]

@kamilmysliwiec As for now, is there any reliable way to distinguish ExecutionContext without type field?

[hexonaut]

@victordidenko Not the nicest way to do it, but I've been using context.getArgs().length >= 4 as a proxy to differentiate between Express and GraphQL. Definitely not the safest way, but it works for my purposes. GraphQL sends 4 arguments and Express sends 2. I'll be happy to switch to getType() when it becomes available.

[victordidenko]

@hexonaut Thank you, yeah, I made it like this exactly:

import { ExecutionContext, ArgumentsHost } from '@nestjs/common';
import { GqlExecutionContext } from '@nestjs/graphql';

export enum ExecutionContextType {
  GQL,
  HTTP,
}

/**
 * Get Execution Context type
 * @see https://github.com/nestjs/nest/issues/1581
 */
export const getType = (context: ArgumentsHost) =>
  context.getArgs().length === 4
    ? ExecutionContextType.GQL
    : ExecutionContextType.HTTP;

/**
 * Get HTTP request object from Execution Context
 */
export const getRequest = (context: ExecutionContext) =>
  getType(context) === ExecutionContextType.GQL
    ? GqlExecutionContext.create(context).getContext().req // `req` object put here by Apollo server
    : context.switchToHttp().getRequest();

[dlipeles]

@victordidenko thank you very much for this solution, this is very underrated. Any chance we can document this in nestjs, or even build it in? I think it would alleviate a lot of confusion for folks.

[hexonaut]

@dlipeles The solution above is very brittle and should only be used temporarily until a "type" variable is introduced. It should definitely not be documented.

[kamilmysliwiec]

getType() has been added in 6.7.0. We'll also throw exceptions when the invalid context is being used, but this was postponed until 7.0.0 release (breaking change).

[spali]

Did I miss something in this commit?
For graphql I get http as type, same as if I use it in a controller.
I thought the plan is to allow to distinguish between graphql and rest too.
even with 6.7.0 I still need the workaround in #1581 (comment)

My use case would be to have a AuthGuard, that can be used in graphql resolvers and in controllers, by just check getType() in getRequest().

So it looks like this is not a complete solution for this issue as meant by the OP and several others.

[kamilmysliwiec]

@spali GraphQL package wasn't updated yet. We'll do it asap.