odavid / typeorm-transactional-cls-hooked

A Transactional Method Decorator for typeorm that uses cls-hooked to handle and propagate transactions between different repositories and service methods. Inpired by Spring Trasnactional Annotation and Sequelize CLS
MIT License
524 stars 86 forks source link

Allow to use @Transactional() annotation to entire service (apply to all methods) #41

Open herbertpimentel opened 4 years ago

herbertpimentel commented 4 years ago

give us a possibility to anotate the entire service class as transactional instead of only methods. and then apply it to each method of the service class;

odavid commented 4 years ago

Hi @herbertpimentel, PR would be happily accepted...

cassinaooo commented 3 years ago

I extended the Transactional decorator for entire classes using this custom decorator:

export interface TransactionalOptions {
    propagation: Propagation
    isolationLevel: IsolationLevel
}

const TransactionalService = (options: TransactionalOptions): ClassDecorator => {
    return (target: Function) => {
        for (const key of Object.getOwnPropertyNames(target.prototype)) {
            const methodDescriptor = Object.getOwnPropertyDescriptor(target.prototype, key)

            // assumes that only async methods need a transaction
            const isAsyncMethod =
                methodDescriptor &&
                methodDescriptor.value instanceof Object.getPrototypeOf(async function() {}).constructor

            if (!methodDescriptor || !isAsyncMethod) {
                continue
            }

            Transactional(options)(target, key, methodDescriptor)

            Object.defineProperty(target.prototype, key, methodDescriptor)
        }
    }
}

It only adds transactions to async methods, since that is what made sense in my application.

@odavid Should this be a feature of the lib itself? How could I go about implementing it?

odavid commented 3 years ago

Hey @cassinaooo, Can you please open a PR for that decorator?

For this code to be added, I believe we need:

Let me know what you think... Cheers!

cassinaooo commented 3 years ago

@odavid Is there a way for the @Transactional decorator to be aware of this new class decorator, or vice-versa? I'm not familiar with meta programming on typescript, maybe Reflect Metadata?

Then we just need to decide on the override semantics instead of implementing a new decorator.

I don't really have anything against @IgnoreTransactionalServiceMethod or similar, just generally inclined to keep APIs as minimal as possible.

odavid commented 3 years ago

@cassinaooo - I believe reflect metadata should help listing methods that are decorated with Ignore

I believe you can start without the ignore feature.

It was just a suggestion, since setting Transactional on all async methods seems to me a bit "brutal", but a user can always remove the class decorator and put the Transactional explicitly.

Hope it helps...

cassinaooo commented 3 years ago

@odavid I see and agree with your point.

What should be expected of @Transactional when we have @TransactionalService active, specially in nested calls? This will eventually happen in large enough applications when services have methods that are called in multiple contexts.

I'm trying to understand how Transactional should behave in regards to propagation in the presence of TransactionalService, and if that helps avoid using @Ignore explicitly.

Suppose we have a class annotated with @TransactionalService({propagation: REQUIRED}) and decide to use @Transactional({propagation: NOT_SUPORTED}) in a given method. Would the ignore/override semantics be implicit, but clear? What are combinations of TransactionalService / Transactional that could be problematic?

mscottnelson commented 3 years ago

Just weighing in here since I would like to see this. It seems to me that @Transactional would always override @TransactionalService for defined options, and use the options declared in @TransactionalService as defaults. At least, that would be my personal expectation.

As an additional alternative to a decorator like @NotTransactional(), perhaps @TransactionalService could also take an array of method names to exclude, eg:

@TransactionalService({ excludeMethods: ["myMethod"] })

odavid commented 3 years ago

Agree with @mscottnelson

cassinaooo commented 3 years ago

I'll try and submit a PR in the next few days. Thanks for your inputs @mscottnelson! I'll will follow your suggestion of @Transactional always overriding the class decorator. We can use @Transactional({propagation: NOT_SUPORTED}) as a substitute for @NotTransactional().