Papooch / nestjs-cls

A continuation-local storage (async context) module compatible with NestJS's dependency injection.
https://papooch.github.io/nestjs-cls/
MIT License
393 stars 23 forks source link

Design Question: Storing Prisma transaction client in AsyncLocalStorage #42

Closed fuvidani closed 1 year ago

fuvidani commented 1 year ago

Hi @Papooch !

I am considering implementing decorator-based database transactions in our NestJs application using AsyncLocalStorage. I'm reaching out to you, because looking at your CLS implementation, you seem to have extensive experience with this.

I'd like to merge the concept from this issue for Prisma, but using AsyncLocalStorage instead of cls-hooked. The idea is to introduce a @Transaction decorator for service methods, that would store a transaction client in the storage and wrap the function s.t. the repositories within that call chain could retrieve it and execute their operations with it.

A given repository, when invoked, would try to first retrieve the transaction client from the store and use it if it's available. If not available, it would just use its own db-client (i.e. there is no composite transaction happening).

Do you think this is a sound approach? I'm not entirely sure about where the AsyncLocalStorage should be instantiated. Does it need to happen at bootstrap time or can it be spontaneous (when decorator is called)?

Any feedback would be much appreciated! 😊

PS.: I also considered to use your CLSService, but AFAIK injecting a custom service into a decorator would be challenging, so I'm not sure it'd be applicable, or would it? 🤔

Papooch commented 1 year ago

Hi there, it is absolutely doable! (It is basically what I have suggested before: https://github.com/prisma/prisma/issues/5729#issuecomment-959137819, and you can use pure AsyncLocalStorage in practically the same way).

In my library, the ClsService is instantiated outside of DI and can be retrieved outside of it, if needed.

The AsyncLocalStorage object can be instantiated anywhere, but to enter the context, you need to wrap the method call with the .run(store, ()=>{...}) call. This can be done in a middleware or an enhancer, or by replacing the method implementation in the decorator (similar to what I'm aiming for here).

fuvidani commented 1 year ago

Thank you @Papooch for the quick and detailed response! 🤩 I still have some doubts on how to reliably access the context if it's not wrapped into a .run() call. Using a @Transaction decorator is clear to me I think - I'd just need to wrap the the whole method into a run and everything inside would have access to the context.

But now let's consider a repository implementation that would look like this:

export abstract class BaseRepository {
    constructor(
          private readonly prisma: PrismaService, 
          private readonly cls: ClsService,) {}

    protected get prisma(): PrismaClient | Prisma.TransactionClient {
        const transactionClient = this.cls.get('transactionClient');
        return transactionClient ?? this.prisma;
    }

}

And a concrete repository:

export class PhotoRepository extends BaseRepository {

    public async create(photo: Photo): Promise<Photo> {
        // Persisting the photo ...
        return Photo.fromPrismaToEntity(
           await this.prisma.create({ data: photo })
        );
    }

}

Now I fear that this is not that simple, because if I understood the concept right, then the invocation should've already been wrapped into a run in order for the the line const transactionClient = this.cls.get('transactionClient'); to work reliably. My goal is to have encapsulation and isolation s.t. if the repository is simply called without an explicit transaction decorator, then it falls back to its own Prisma instance. If my suspicion is right, then actually every invocation of the repository would require a run wrap somewhere, regardless of an existing transaction or not. Am I wrong? 😬

Papooch commented 1 year ago

Yes, that's right. As I said, you can do that inside the @Transactional decorator by replacing the decorated method's implementatioj with a wrapped version. You would also have to modify the class to have the Prisma service injected so you could also wrap it in a transaction.

Otherwise you'd have to do all this in an enhancer or mideware, but that wouldn't work with invocations that don't go through a controller (because enhancers aren't available in that case).

EDIT: Actually now that I re-read the question, you if you call cls.get outside of a run context, it should just return undefined, so it would still work ourside of the context. But yes, the transaction will only work if the context has been set up before entering the transaction.

fuvidani commented 1 year ago

Thanks for the clarification! I came up with the following rough decorator implementation (haven't tested it yet, it's just a PoC). What do you think of it? I'm particularly interested in the order of wrapping. Should I start the transaction first and then do a cls.run() or the other way around (like in the code snippet below)?

Another question: prisma.$transaction() returns a promise. Does cls.run() takes care of resolving the Promise or do I need to await it explicitly? 🤔

import { Inject } from '@nestjs/common';
import { PrismaService } from 'our-own-custom-lib';
import { ClsServiceManager } from 'nestjs-cls';

export function Transaction() {
  const injectPrisma = Inject(PrismaService);

  return (target: any, propertyKey: string, descriptor: PropertyDescriptor) => {
    if (this.prisma) {
      console.log('Object already contains a PrismaService dep');
    } else {
      // this is equivalent to have a constructor like constructor(yourservice: YourServiceClass)
      // note that this will injected to the instance, while your decorator runs for the class constructor
      injectPrisma(target, 'prisma');
    }

    // we use a ref here so we can type it
    const originalMethod = descriptor.value;
    descriptor.value = function (...args: any[]) {
      const cls = ClsServiceManager.getClsService();
      if (cls.get('TRANSACTION_CLIENT')) {
        // A transaction has already been started. We just call the function.
        return originalMethod.apply(this, [...args]);
      }
      cls.run(async () => {
        const prisma: PrismaService = this.prisma;
        return prisma.$transaction(async (transactionClient) => {
          cls.set('TRANSACTION_CLIENT', transactionClient);
          try {
            // We can now call the function that had been decorated with the Transaction decorator.
            const result = await originalMethod.apply(this, [...args]);
            return result;
          } finally {
            // We just finished working with our transaction: we remove it rom the current context.
            cls.set('TRANSACTION_CLIENT', null);
          }
        });
      });
    };
  };
}
Papooch commented 1 year ago

Withouh having run any code, what you have there seems reasonable.

One more check I would do is run cls.isActive() and don't wrap the call if we're already running inside a cls context, so we're not nesting contexts, in which case you'd have to copy the contents of the ClsStore to the new context if other code relied on it.

cls.run only wraps the call and does not perform any awaiting or error handling itself. The return type of the wrapped callback is the same as the original one.

fuvidani commented 1 year ago

Thank you @Papooch!! I'll try it out tomorrow, I'm very excited if this can work reliably. We could finally get rid of leaking Prisma outside of the data access layer.

Papooch commented 1 year ago

@fuvidani did you come to a satisfactory solution?

fuvidani commented 1 year ago

@fuvidani did you come to a satisfactory solution?

Hi @Papooch! I've been working on it as a side-task and it's in a pretty good state. I'm just concerned about the multi-module usage in Nestjs. Let's say we have module A and module B, they are both importing your CLS implementation via ClsModule.register({global: false, middleware: { mount: false },}). Now consider the situation where module A is also importing module B in a way that module A calls a service method of module B. Which CLS store is going to be used? Is it guaranteed that calls to ClsServiceManager.getClsService(); will target the right store?

To give you more context, I created a separate BaseRepository module, which practically provides a base repository class to wrap the retrieval of the Prisma instance:

import { Injectable } from '@nestjs/common';
import { ClsService } from 'nestjs-cls';
import { Prisma, PrismaClient } from '@prisma/client';
// some imports omitted 

@Injectable()
export class BaseRepository {
  constructor(
    private readonly prismaClient: PrismaService, // this service is just a wrapper around PrismaClient
    private readonly cls: ClsService
  ) {}

  protected get prisma(): PrismaClient | Prisma.TransactionClient {
    const maybeTransactionClient = this.cls.get(
      TRANSACTION_CLIENT_KEY
    ) as PrismaService;
    if (this.cls.isActive() && maybeTransactionClient) {
      logger.info('Using transaction client from ALS');
      return maybeTransactionClient;
    }
    logger.info('Using basic Prisma client');
    return this.prismaClient;
  }
}

Again, if there are 2 decorated methods in 2 different modules and I start a call chain from the first module, will the decorator in the second module get the same CLSService instance or the one from its own module? Do you understand what I'm trying to say? My fear is that in runtime the ClsServiceManager.getClsService() invocation in the decorator returns two different instances of the service, s.t. transactionality across two modules could not be guaranteed. Am I completely misunderstanding something? If you could help me out here with the theory, I'd be very grateful. 🙇 🙏

Papooch commented 1 year ago

Since ClsServiceManager.getClsService() is a static method, it returns the same each time. Actually, it's the same instance as the injected ClsService that lives in the DI container. Even if they weren't, they share the same AsyncLocalStorage underneath.

Btw are you actually mounting the ClsMiddleware manually somewhere? If not, then it's redundant in the module registration.

fuvidani commented 1 year ago

Makes sense @Papooch, this is quite reassuring. Huge thank you @Papooch, really appreciate the support! 💪

fuvidani commented 1 year ago

Hi @Papooch! Do you know how this behaves in terms of concurrency? If the event loop switches between two asynchronous call chains, wouldn't ClsServiceManager.getClsService().get('transactionClient') return the same value to both chains? That would potentially mean that:

What do you think about that?

Papooch commented 1 year ago

That's the nice thing about AsyncLocalStorage. Each execution chain gets an unique instance of the store and it gets propoagated through promise chains and callbacks (it even works with asynchronous event emitters and nestjs/cqrs).

The only case in which there is a chance that 2 requests would access the wame store is when you use the enterWith function to set up the context, but otherwise all code that is invoked from wtihin the run callback gets exclusive access to an unique instance of the store.

Faithfinder commented 1 year ago

@fuvidani Do you have a final solution :)? I'm looking to implement something similar and looking at this, am wondering if I could just lift your work =P

fuvidani commented 1 year ago

Hey @Faithfinder 👋 I've implemented this at our organization, the final solution follows the same strategy that you see in the above code snippets. I'll reach out to my supervisors and ask for permission to share the final result. But in theory the available information should be enough... 😉 Let me know if you have specific questions!

Faithfinder commented 1 year ago

Nah, nothing specific. Just thought you might have found a bug or made an improvement or something since October that I don't need to learn on my own.

fuvidani commented 1 year ago

Hey @Faithfinder and @Papooch! What do you guys think of this?

Context: since the implementation of the @Transactional decorator with nestjs-cls more and more parts of our codebase have adopted it. However we still have a lot of components that pass transaction clients explicitly as a parameter. While I did implement bridges between the explicit passing and the decorator usage, they're kinda awkward at the moment and cause friction (especially for developers who aren't familiar what's happening under the hood with ALS and everything).

This got us thinking and we realized that it'd be so nice if we simply wrapped Prisma into a Proxy and returned the "correct" instance at runtime whenever someone does this.prisma or this.prisma.$transaction(). Coincidentally I came across the above issue that was posted a couple of months ago and to be honest I like the idea! I'm curious what you guys think. I'm thinking of marrying the two concepts: using the decorator to declaratively mark transactional blocks but also using a Proxy s.t. other parts of the codebase that haven't adopted the decorator would work out-of-the-box with each other.

Papooch commented 1 year ago

@fuvidani Yeah, Proxies are pretty powerful and ideal for this type of scenario. Coincidently, what you described is exactly how Proxy providers are implemented in this very library :)

uki1014 commented 6 months ago

I'm developing an application with clean architecture in Nest.js and I am tired of using Prisma's TransactionClient across modules and I came across this issue looking for a good and good solution. I implemented PrismaClientManager with nestjs-cls that makes it easier to use transaction, so if you have the same problem, please take a look at this.

https://github.com/uki1014/nestjs-prisma-client-manager