Provides a way for DecoratorFactory to be applied only once

[my4-dev]

Motivation:

Please refer to issue #5759

Modifications:

• Refactored original logics of collecting decorator in order to remove side effects as much as possible.
• Added addtivity flag to DecoratorFactory which makes us possible to control that the annotation that has the highest priority to be only applied within annotations annotaed by this DecoratorFactory.

Result:

• Closes Provides a way for DecoratorFactory to be applied only once #5759
• If additivity=false, the decorator with a higher priority will only be selected and others are not applied. In the following example, focusing on get() method, Permission READ is only applied and Permission WRITE is ignored.

@RequiresPermission(value = Permission.WRITE, order = 2)
static class SensitiveService {
    @RequiresPermission(value = Permission.READ, order = 1)
    @Get("/")
    public String get() { ... }

    @Post("/")
    public String create() { ... }

    @Put("/")
    public String update() { ... }
}

@DecoratorFactory(value = PermissionCheckerFactory.class, additivity = false)
@Retention(RetentionPolicy.RUNTIME)
@interface RequiresPermission {
    Permission value() default Permission.READ;
    int order() default 0;
}

enum Permission {
    READ, WRITE
}

[github-actions]

🔍 Build Scan® (commit: d584448)

Job name	Status	Build Scan®
build-ubicloud-standard-8-jdk-8	✅	https://ge.armeria.dev/s/e7xherl65wlyc
build-ubicloud-standard-8-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/j22qpd37fs6y4
build-ubicloud-standard-8-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/shna3i2prfxl4
build-ubicloud-standard-8-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/f3y75k4ypz3my
build-ubicloud-standard-8-jdk-17-leak	✅	https://ge.armeria.dev/s/epzvopzryrjh2
build-ubicloud-standard-8-jdk-11	✅	https://ge.armeria.dev/s/4pt3ke7licmw6
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/i5tht56yfdrji

[jrhee17]

Before going into code review, I think it's better if some points are clarified cc. @ikhoon (the original issue creator)

1. Can repeatable be set for decorators also? It seems odd that only DecoratorFunctions only are considered. I think we can probably use the same trick that was used for order to determine additivity of decorators added by factories.

e.g.

@RequiresPermission(Permission.WRITE)
static class SensitiveService {
    @RequiresPermission(Permission.READ)
    @Decorator(PermissionDecorator.class)
    @Get("/")
    public String get() { ... }
}

2. Additivity originally turns off the cumulative behavior. Do we following this rule? or do we just apply the highest order decorator if any one of the decorators have additivity=true?

e.g. Assume that decorator1(order=0), decorator2(order=50, additivity = false), decorator3(order=100). Then only decorator1, decorator2 will be applied following the original cumulativity rule.

[ikhoon]

Can repeatable be set for decorators also?

SGTM

or do we just apply the highest order decorator if any one of the decorators have additivity=true?

Did you mean additivity=false?

e.g. Assume that decorator1(order=0), decorator2(order=50, additivity = false), decorator3(order=100). Then only decorator1, decorator2 will be applied following the original cumulativity rule.

@RequiresPermission(Permission.WRITE)
static class SensitiveService {
    @RequiresPermission(Permission.READ)
    @Decorator(PermissionDecorator.class)
    @Get("/")
    public String get() { ... }
}

A class decorator has a lower priority (order) than a method decorator. IIUC, if we follow the rule, isn't @RequiresPermission(Permission.WRITE) chosen?

Initially, I intended to provide a way to apply the innermost decorator when the same decorators are applied repeatedly.

[jrhee17]

A class decorator has a lower priority (order) than a method decorator. IIUC, if we follow the rule, isn't @RequiresPermission(Permission.WRITE) chosen?

I thought the order was determined by the order attribute value eventually

armeria/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DecoratorAnnotationUtil.java

Line 59 in 906617b

decorators.sort(Comparator.comparing(DecoratorAndOrder::order));

Initially, I intended to provide a way to apply the innermost decorator when the same decorators are applied repeatedly.

Perhaps I didn't understand the original intention of the repeatable field you proposed. So if repeatable=false is set for a decorator (decorator3) in the middle of decorators (decorator1~5), how does it affect which decorators are selected?

[ikhoon]

I thought the order was determined by the order attribute value eventually

I was talking about cases where the order is determined by the declared position of the decorator without explicitly declaring the order property.

armeria/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DecoratorAnnotationUtil.java

Lines 54 to 56 in 906617b

	// Class-level decorators are applied before method-level decorators.
	collectDecorators(decorators, AnnotationUtil.getAllAnnotations(clazz));
	collectDecorators(decorators, AnnotationUtil.getAllAnnotations(method));

So if repeatable=false is set for a decorator (decorator3) in the middle of decorators (decorator1~5), how does it affect which decorators are selected?

I thought about collecting decorators starting from the highest priority (innermost decorator) to the lowest priority until finding additivity = false. According to this rule, decorator3~5 will be applied.

[jrhee17]

I thought about collecting decorators starting from the highest priority (innermost decorator)

To be clear, by priority do you mean order? I feel like it's more natural that the decorator application order also applies to additivity (or repeatable). Reversing this order feels counter-intuitive to me.

I think I'm out of ideas at the moment.

[ikhoon]

We have already introduced a simpler approach in MetricCollectingService in which the decorator application order is ignored. If multiple MetricCollectingServices are applied the last applied decorator is used.

armeria/core/src/main/java/com/linecorp/armeria/server/metric/MetricCollectingService.java

Lines 123 to 124 in 906617b

	// An inner `MetricCollectingService` takes precedence over an outer one. Delegate to the inner
	// `MetricCollectingService` if exists.

So I didn't think it was odd.

Anyway, I also need to think more about the direction and API design.

[jrhee17]

We have already introduced a simpler approach in MetricCollectingService in which the decorator application order is ignored. If multiple MetricCollectingServices are applied the last applied decorator is used.

I think there is a misunderstanding 😅 I'm not against ignoring the order of decorators as long as the change makes sense.

It seems like @ikhoon already has a good idea on how to proceed with this PR, so let me review this later after the PR is more developed 🙏

[my4-dev]

I'm sorry, I misundertood the requirement of this issue🙏
In my understanding, order simply means the order for decorators to be applied, but through the perspective of priority the decorator applied last is more prioritized than the others. (I implemented vice versa.)

But the requirement may be more complicated than I thought.

Anyway, I also need to think more about the direction and API design.

Thank you. Please let me know if they are fixed!