[Feature]: Class Decorator support

[ehdgus094]

Overview

@Cache({
  // ...options(metadata value)
})
export class SomeService {
  some() {
    // ...
  }
}

It would be great if Class Decorator for AOP is supported like the code block above.
What the class decorator does is basically wrap all the methods of the class.

Describe the solution you'd like

I'm currently using the code below.

• Type Declaration

export type AopClassDecoratorOptions<T> = {
  excludeMethodNames?: string[];
  omitParentClass?: boolean;
} & T;

• createClassDecorator

// 상속관계의 조상 클래스에 AOP가 적용될 시, 중복 적용되는 것을 방지
const methodWrappedSymbol = Symbol('METHOD_WRAPPED');

export const createClassDecorator = (
  metadataKey: symbol | string,
  metadata?: AopClassDecoratorOptions<unknown>,
): ClassDecorator => {
  const aopSymbol = Symbol('AOP_DECORATOR');
  const excludeMethodNames = metadata?.excludeMethodNames ?? [];
  const omitParentClass = metadata?.omitParentClass ?? false;
  delete metadata?.excludeMethodNames;
  delete metadata?.omitParentClass;

  return (target: any) => {
    const recursive = (currentPrototype: any) => {
      const methodNames = Object.getOwnPropertyNames(currentPrototype);
      // currentPrototype이 최상위 객체인 Object일때 재귀 종료
      if (methodNames.includes('__proto__')) return;

      const decoratorStatus = Reflect.getMetadata(
        metadataKey,
        currentPrototype.constructor,
      );

      if (decoratorStatus !== methodWrappedSymbol) {
        methodNames
          .filter(
            (methodName) =>
              methodName !== 'constructor' &&
              !excludeMethodNames.includes(methodName),
          )
          .forEach((methodName) => {
            try {
              const originalFn = currentPrototype[methodName];

              // 1. Add metadata to the method
              if (!Reflect.hasMetadata(metadataKey, originalFn)) {
                Reflect.defineMetadata(metadataKey, [], originalFn);
              }
              const metadataValues = Reflect.getMetadata(
                metadataKey,
                originalFn,
              );
              metadataValues.push({ originalFn, metadata, aopSymbol });

              // 2. Wrap the method before the lazy decorator is executed
              Object.defineProperty(currentPrototype, methodName, {
                value: function (...args: unknown[]) {
                  const wrappedFn = this[aopSymbol]?.[methodName];
                  if (wrappedFn) {
                    // If there is a wrapper stored in the method, use it
                    return wrappedFn.apply(this, args);
                  }
                  // if there is no wrapper that comes out of method, call originalFn
                  return originalFn.apply(this, args);
                },
              });

              /**
               * There are codes that using `function.name`.
               * Therefore the codes below are necessary.
               *
               * ex) @nestjs/swagger
               */
              Object.defineProperty(currentPrototype[methodName], 'name', {
                value: methodName,
                writable: false,
              });
              Object.setPrototypeOf(currentPrototype[methodName], originalFn);
            } catch (_ignored) {}
          });
        Reflect.defineMetadata(
          metadataKey,
          methodWrappedSymbol,
          currentPrototype.constructor,
        );
      }
      if (!omitParentClass) {
        recursive(Object.getPrototypeOf(currentPrototype));
      }
    };
    recursive(target.prototype);
  };
};

• createCommonDecorator

/**
 * Class 와 Method 모두에 적용 가능한 Decorator
 */
export const createCommonDecorator = (
  metadataKey: symbol | string,
  metadata?: AopClassDecoratorOptions<unknown>,
) => {
  return function (
    target: any,
    propertyKey?: string | symbol,
    descriptor?: PropertyDescriptor,
  ) {
    const isMethodDecorator = typeof target === 'object';
    if (isMethodDecorator) {
      delete metadata?.excludeMethodNames;
      delete metadata?.omitParentClass;

      return createDecorator(metadataKey, metadata)(
        target,
        propertyKey!,
        descriptor!,
      );
    }
    return createClassDecorator(metadataKey, metadata)(target);
  };
};

It would be appreciated if you could point out any side effect that could occur by using the code above.

Thank you for this awesome library!

Additional context

[kys0213]

Hi there. Thanks for the good suggestion.

I didn't add the class decorator feature because human error can cause AOP to be applied unintentionally.

There are options like excludeMethodNames, but it seems like it could be missed if the amount of code written is large or if you don't know the exact context.

[ehdgus094]

I totally agree with your concern.

But there are cases where it is not possible appying AOP with method decorator.

For example,

@Injectable()
export class SomeRepository extends Repository<SomeEntity> {
  constructor(private readonly dataSource: DataSource) {
    super(SomeEntity, dataSource.createEntityManager());
  }

  // @SomeAopDecorator({
  //   // ...options(metadata value)
  // })
  // async find() {}
}

When there is a class that extends another class from external library like above, there is no way to apply AOP to a method from parent class.

We could apply AOP to the method by wrapping in another method as a workaround.

@SomeAopDecorator({
  // ...options(metadata value)
})
async wrapMethod() {
  await this.find();
}

But this could be annoying sometimes.
That is why I made createClassDecorator so that I could apply AOP directly to the method.

I'm pretty sure you've already thought about cases like this, so any ideas would be appreciated.

[kys0213]

If you are using a common class, I believe you can eliminate duplicate code by using the decorator pattern or implementing a separate custom class in the middle layer of the common class.

• used decorator pattern

class CacheRepository<T> implements Repository {
  consturcotr(someRepository: SomeRepository)
  
  @Cachable()
  find(...) {
    this.someRepository.find(...)
  }

  ....
}

• used extends and override

class CacheRepository<T> extends Repository { 
  @Cachable()
  find(...) { // override
    this.someRepository.find(...)
  }
}

class SomeService {
  constructor(private readonly repositry: CacheRepository)
  ...
}

Additionally, if you need to cache your repository in bulk, as in the example code, you may want to consider using the caching settings provided by your ORM.

[ehdgus094]

Thank you for your suggestion!
I agree that the way you suggested is much better than just using class decorator. I think I would change my code as you suggested.

Apart from the code quality, I would like to think about the purpose of a library itself.
Even though I may not use the class decorator, I still think that the class decorator feature should be added. Because I believe that libraries should not force developers(users)' code style whereas frameworks should force developers' code style.

The major reason why I think libraries should not force developers' code style or structure is because the developer who makes a library would never know the exact context or environment in which the users use. So, the more features the maker provides, the better the library would be as long as the features don't contaminate the primary purpose of the library.
Plus, I believe the users should take care of human errors, not the one who makes the library.

So my conclusion is, if this library code is only used internally, I wouldn't add the class decorator feature, but if it's a library for anyone, I think it's better adding the class decorator feature and more features if possible.

My opinion may be wrong because I'm a developer with a short experience. So could you share your thoughts on this?

Thanks! :)

[WhiteKiwi]

Thank you for your valuable feedback.
I agree that while the library provides a variety of features, developers should have the control over them.

I'm curious to know if other languages offer similar capabilities. Java has class decorators, but they operate at compile time.

In AOP, this approach seems to have a high potential for side effects, and it appears that the actual need for it is not significant.
Therefore, I have a negative view on adding this feature. If you have any further comments, I would appreciate hearing them.

[ehdgus094]

I failed to understand "Java decorators operate at compile time" because as far as I know, Java annotations are not functions and in Java/Spring environment the proxy beans for AOP are created on initialization of spring container at runtime. I might have misunderstood your word, so could you provide a little more details including the difference between Java and JavaScript environment for applying AOP please? Of course the mechanism of applying AOP for this library and spring-aop would be different but I still don't see the reason why it has a high potential for side effects in JavaScript but not in Java.
I found it difficult to apply AOP in JavaScript because of asynchronous codes though.

The only side effect that comes to my mind is applying AOP to the unexpected methods. Could you provide any other example/scenario of side effects? Sorry for my ignorance!

[ehdgus094]

I came up with an example.

In circumstances where there are various AOP decorators to be applied in a certain order, class decorator could be problematic because AOP with method decorator will always be applied before class decorator.

[WhiteKiwi]

Sorry I am not good at English.

as far as I understand it

1. Java's annotation works as a marking.
2. Lombok works at compilation time, which is similar to codegen.

I was wondering if there is a case where Annotation that applies to Class affects all methods at runtime :)

[ehdgus094]

I believe Lombok works with annotation processor which is totally different from AOP.

And spring-aop does support the feature!

[WhiteKiwi]

And spring-aop does support the feature!

I am not good at Java.
I would appreciate it if you could provide an example. 🙇

[ehdgus094]

This is a basic logging example with spring-aop.

• annotation

@Target({ElementType.TYPE, ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
public @interface Logging {
}

• aspect class

@Slf4j
@Aspect
@Component
@Order(0)
public class LoggingAspect {

    @Pointcut("@annotation(your.annotation.package.path.Logging)")
    private void methodPointcut() {}

    @Pointcut("@within(your.annotation.package.path.Logging)")
    private void classPointcut() {}

    @Around("methodPointcut() || classPointcut()")
    public Object execute(ProceedingJoinPoint joinPoint) throws Throwable {
        log.debug("before");

        Object result = joinPoint.proceed();

        log.debug("after");

        return result;
    }
}

• usage

// @Logging
@Service
public class SomeService {

    // @Logging
    public void some() {
        // ...
    }
}

Spring-aop supports even more features such as applying AOP without annotation like below. (high flexibility to define targets)

@Pointcut("execution(public * *(..))")
private void anyPublicOperationPointcut() {}

@Pointcut("execution(* your.service.package.path.SomeService.*(..))")
private void someServicePointcut() {}

I'm not so sure if this level of flexibility is possible in JavaScript though.

[WhiteKiwi]

Thank you for the detailed example
I have some urgent work this week, so I will discuss this next week and leave a comment then 🙇

[WhiteKiwi]

It took some time for discussion 🙏
I also agree that adding class aop similar to a pointcut would be beneficial.
However, I am hesitant to add this feature now due to potential side effects. Thus, I will keep this issue open for further discussion and reaction.
If more people need it, We will reconsider it then.
please leave any additional comments. Thank you

[kys0213]

hello.

I'm not sure if nestjs-aop should be considered a library, I think a library should have a lifecycle that runs independently of the outside world, and nestjs-aop is more of a framework because it basically runs on top of nestjs' lifecycle. Also, providing this functionality out of convenience makes it hard to defend against inheritance if it happens.

ex) library lifecycle

const someLibrary = new SomeLibray(...)
const someLibrary = someLibrary.getBuilder().add().build()

Also, I'm not sure if there's anything that should be enforced at the class level for cache or transactions, which are typically used for AOP - I think they both require different policies depending on what they do, and in the case of cache, which you asked about in your first question, I think you'd need to approach it in a different way.

For something like logging, I think you might want to consider an interceptor or middleware.

[ehdgus094]

Oh, the code block about the cache that I mentioned in my first comment was just an example that I brought from the README of this project.

I agree that this project could be considered more as a part of a framework.
Thanks for providing me your perspectives, it was a great discussion! :)