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

Alternative NestJS Approach #44

Open
parisholley opened this issue Nov 6, 2021 · 9 comments
Open

Alternative NestJS Approach #44

parisholley opened this issue Nov 6, 2021 · 9 comments

Comments

@parisholley
Copy link

In moving to a persist/flush model system like Mikro, I find it important to prevent developers on my team from making mistakes like using some global entity manager with a persisted identity map across requests. At the moment, it is very easy to inject Mikro into any service, controller, etc without realizing the repercussions. At a high level, these are my requirements:

  • No one should be able access a singleton instance of EntityManager except for the Mikro module itself.
  • If there is ever a need to access the application-level singleton, it should be done through some obvious abstraction like orm.getGlobalEntityManager()
  • No magic should be involved with request scopes such as node.js domains (deprecated) or pulling things from global statics. Given that a service does not know if it needs to be ran in a transaction or not, guidance/best practice should be to always pass an instance of EntityManager to your downstream services via function arguments
  • The Mikro NestJS module should provide interceptors and decorators for making request scoped work easier (i'm using this for both REST and GraphQL endpoints), example:

EntityManagerInterceptor.ts

import { createParamDecorator, ExecutionContext } from '@nestjs/common';
import { EntityManager } from '@mikro-orm/postgresql';
import { CallHandler, ExecutionContext, NestInterceptor } from '@nestjs/common';
import { Observable } from 'rxjs';
import { mergeMap } from 'rxjs/operators';

export default class EntityManagerInterceptor implements NestInterceptor {
  constructor(private em: EntityManager) {}

  async intercept(context: ExecutionContext, next: CallHandler<any>): Promise<Observable<any>> {
    context['em'] = this.em.fork();

    return next.handle().pipe(
      mergeMap(async (output) => {
        await context['em'].flush();

        return output;
      })
    );
  }
}

RequestEntityManager.ts

export default createParamDecorator((data: unknown, context: ExecutionContext) => {
  if (!context['em']) {
    throw new Error('Could not find a request-scoped EntityManager');
  }

  return context['em'];
});
// graphql
@Mutation()
myMutation(@RequestEntityManager() em: EntityManager){}

// rest
@Get()
async myEndpoint(@RequestEntityManager() em: EntityManager){}
@B4nan
Copy link
Member

B4nan commented Nov 6, 2021

No one should be able access a singleton instance of EntityManager except for the Mikro module itself.

In v5 there will be a validation for working with the global EM's identity map aware API. I was originally thinking of making it opt-in, mainly because I was lazy to rewrite all the tests, but if we make it configurable via env vars, I could at least disable that globally there :D

If there is ever a need to access the application-level singleton, it should be done through some obvious abstraction like orm.getGlobalEntityManager()

Good idea.

No magic should be involved with request scopes such as node.js domains (deprecated) or pulling things from global statics. Given that a service does not know if it needs to be ran in a transaction or not, guidance/best practice should be to always pass an instance of EntityManager to your downstream services via function arguments

v5 uses ALS instead of domain API. I will definitely keep it there as it is very convenient and universal. On the other hand, this is optional, you can work without this magic if you don't like it.

The Mikro NestJS module should provide interceptors and decorators for making request scoped work easier (i'm using this for both REST and GraphQL endpoints), example:

Sure, we can add the proposed @RequestEntityManager() as an alternative approach. PR welcome.

Will move this to the nest repository to track it there.

@B4nan B4nan transferred this issue from mikro-orm/mikro-orm Nov 6, 2021
@parisholley
Copy link
Author

somehow, i completely missed the ALS announcement in v14, looks neat but may impact performance at scale (probably not a concern for most projects)

that being said, would the ALS implementation account for transactions? eg: i usually put my orchestration at the controller/resolver level, and wrap all the service calls into a transaction. it isn't clear in the case of nested services (or perhaps in some peoples case, nested transactions if supported by the db) how that implementation would work.

other question w/r/t ALS, if I have a web request come in, and i run to different service calls with two different transactions like so, does it break (probably not the best example, but i'm more-so poking at if there are parallel promises using entity manager)?

@Get()
endpoint(){
await Promise.all([
  this.service.a(),
  this.service.b();
];
}

@B4nan
Copy link
Member

B4nan commented Nov 6, 2021

somehow, i completely missed the ALS announcement in v14, looks neat but may impact performance at scale (probably not a concern for most projects)

ALS is actually very fast, compared to the domain API. When I was perf testing this last time, there was quite a small penalty for ALS, few percent, maybe up to 10-20%, but I don't really remember now.

that being said, would the ALS implementation account for transactions? eg: i usually put my orchestration at the controller/resolver level, and wrap all the service calls into a transaction.

ALS is actually used internally to hold the transaction context in v5 - again opt in, you can still use the EM from parameter explicitly, only if you work with the global instance the ALS is checked (this works based on the useContext parameter of EM, which defaults to false in em.fork()).

it isn't clear in the case of nested services (or perhaps in some peoples case, nested transactions if supported by the db) how that implementation would work.

Each transaction has its own transaction context (own EM fork), including the nested ones.

other question w/r/t ALS, if I have a web request come in, and i run to different service calls with two different transactions like so, does it break (probably not the best example, but i'm more-so poking at if there are parallel promises using entity manager)?

Using Promise.all() on a single EM instance is not supported, it will throw a validation error. But if you have two separate transactions created in each of a() and b(), it should work fine.

@parisholley
Copy link
Author

ALS makes sense, your pretty much use it everywhere or not

Using Promise.all() on a single EM instance is not supported, it will throw a validation error

can you expand on that?

@B4nan
Copy link
Member

B4nan commented Nov 6, 2021

I mean you can't flush in parallel (for that you need forks with their own UoW instance), for reading it should be fine.

@wSedlacek
Copy link

wSedlacek commented Oct 29, 2022

Just tossing this here in case anyone finds it useful.
I am using it for @nestjs/microservices since Middleware doesn't run in this context.

import type { CallHandler, ExecutionContext, NestInterceptor } from '@nestjs/common';
import { Injectable } from '@nestjs/common';

import type { EntityManager } from '@mikro-orm/core';
import { MikroORM, RequestContext } from '@mikro-orm/core';
import type { AsyncLocalStorage } from 'async_hooks';
import { Observable } from 'rxjs';

interface ExposedRequestContextInterface {
  createContext: (em: EntityManager) => RequestContext;
  storage: AsyncLocalStorage<RequestContext>;
}

const ExposedRequestContext = RequestContext as unknown as ExposedRequestContextInterface;

/**
 * Because the MikroORM RequestContext only works as a middleware for HTTP Traffic, this is a
 * monkey patch to inject context at the interceptor level
 */
@Injectable()
export class MikroOrmInterceptor implements NestInterceptor {
  constructor(private readonly orm: MikroORM) {}

  public intercept(_context: ExecutionContext, next: CallHandler): Observable<unknown> {
    const requestContext = ExposedRequestContext.createContext(this.orm.em);
    return new Observable((subscriber) => {
      const subscription = ExposedRequestContext.storage.run(requestContext, () =>
        next.handle().subscribe(subscriber)
      );

      return () => {
        subscription.unsubscribe();
      };
    });
  }
}

It of course has the limitation (being at the interceptor level) of not affecting guards, but for my use case I actually don't need that as all my guards are at the gateway level which issue request to other services which would do their business logic past any interceptors.

A note on a PR I may look into introducing, the RequestContext.createAsync() uses async so it can only handle promises, but the AsyncLocalStorage.prototype.run() can handle any type of returned value, so I am thinking of introducing a RequestContext.bind() or something like that which just calls this.storage.run() without the async and promises.

@ancyrweb
Copy link

ancyrweb commented Apr 8, 2024

I've implemented a NestJS interceptor to flush the EM automatically upon request completion. It leverages RequestContext that itself leverage ALS.

@B4nan does that look good or may I run into problems ?

@Injectable()
class DatabaseInterceptor implements NestInterceptor {
  intercept(
    context: ExecutionContext,
    next: CallHandler<any>,
  ): Observable<any> {
    return next.handle().pipe(
      tap(() => {
        return RequestContext.currentRequestContext()?.em?.flush();
      }),
    );
  }
}

@B4nan
Copy link
Member

B4nan commented Apr 8, 2024

Not entirely sure how this works, I remember tap was just a side effect without waiting, if so, I would rather wait for the flush to finish before you resolve the interceptor/send the response.

@ancyrweb
Copy link

ancyrweb commented Apr 8, 2024

My RxJS is very rusty. This update seems to be correct.

@Injectable()
class DatabaseInterceptor implements NestInterceptor {
  intercept(
    context: ExecutionContext,
    next: CallHandler<any>,
  ): Observable<any> {
    return next.handle().pipe(
      mergeMap(async (response) => {
        await RequestContext.currentRequestContext()?.em?.flush();
        return response;
      }),
    );
  }
}

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

No branches or pull requests

4 participants