armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Add `CreationMode` for annotated service dependencies

Open ks-yim opened this issue 2 years ago • 2 comments

Motivation:

Currently, armeria is unable to create annotated service dependencies specified in @Decorator, @DecoratorFactory, @RequestConverter, @ResponseConverter and @ExceptionHandler using the default constructor of the dependent classes when DependencyInjector is configured.

Related issues: #4521, #4534

Modifications:

  • Add CreationMode to @Decorator, @DecoratorFactory, @RequestConverter, @ResponseConverter and @ExceptionHandler.
  • Breaking) Use DependencyInjector only when the CreationMode.INJECTION is specified for annotated service dependencies.
  • Do not close dependencies create by their default constructor upon server shutdown.

Result:

  • Annotated service dependencies can be created with their default constructors even though a DependencyInjector is configured.
  • DependencyInjector is used only for those dependencies whose CreationMode is INJECTION.

ks-yim avatar Jan 02 '23 07:01 ks-yim

There are several things that I am concerned of with this PR and want to get some advice from maintainers.

  1. Regression) The dependencies added by their default constructor are no longer closed. ReflectiveDependencyInjector did handle this correctly but it's removed in this PR.
  2. Breaking) Having CreationMode.REFLECTION as default may silently break some applications that are using a non-default DependencyInjector as @jrhee17 stated in https://github.com/line/armeria/pull/4606#discussion_r1060300375.
  3. Adding annotations for the dependencies added via DependencyInjector becomes tedious as CreationMode.INJECTION must be specified in all annotations.
    @Decorator(value = FooDecorator.class, mode = CreationMode.INJECTION)
    

If I was the only one who felt uncomfortable with default constructor injection being disabled by enabling a custom DependencyInjector, I am willing to put this off for some time to collect more feedback about this behavior from the community. Actually, there were many "easy" workaround to get my work done without this change.

ks-yim avatar Jan 05 '23 02:01 ks-yim

API idea:

// Try Spring first, and then try reflection.
@Decorator(value=MyDecorator.class, injectors=[
    SpringDependencyInjector.class,
    ReflectiveDependencyInjector.class
])

// Try Spring only (no reflection)
@Decorator(value=MyDecorator.class, injectors=[
    SpringDependencyInjector.class
])

// Default behavior, which is:
// - Reflective only when spring-boot-autoconfigure is missing.
// - SpringOrReflectiveDependencyInjectotr when spring-boot-autoconfigure exists.
@Decorator(value=MyDecorator.class)

In application.yaml:

armeria:
  injection-mode: <one of: spring-only, reflection-only, spring-or-reflection, reflection-or-spring, none>
  # or:
  injectors: spring, reflection, ...

trustin avatar Nov 21 '23 09:11 trustin