Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provides a way for DecoratorFactory to be applied only once #5759

Open
ikhoon opened this issue Jun 13, 2024 · 4 comments · May be fixed by #5918
Open

Provides a way for DecoratorFactory to be applied only once #5759

ikhoon opened this issue Jun 13, 2024 · 4 comments · May be fixed by #5918
Labels
new feature

Comments

@ikhoon
Copy link
Contributor

ikhoon commented Jun 13, 2024 •
edited
Loading

Say a decorator checks permission of request. I want to give all API write permissions in the class and optionally give read permissions. In this way, when a new API is added to the class, write permission is given by default. This will prevent the new API from being accidentally exposed with lower permissions.

This logic cannot be implemented using the DecoratorFactory. Because the decorators in the method and class are applied repeatedly even if it is the same class type.

In the SensitiveService, get() method can't be executed with READ permission. Both READ and WRITE are required instead.

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

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

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

@DecoratorFactory(PermissionCheckerFactory.class)
@Retention(RetentionPolicy.RUNTIME)
@interface RequiresPermission {
    Permission value() default Permission.READ;
}

enum Permission {
    READ, WRITE
}

To solve this case, I suggest adding the repeatable option to DecoratorFactory. If repeatable=false, the decorator with a higher priority will be selected.

public @interface DecoratorFactory {

    Class<? extends DecoratorFactoryFunction<?>> value();
    
    boolean repeatable() default true;
}

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

If there is any other good name other than repeatable, please recommend it.
@jrhee17
Copy link
Contributor

jrhee17 commented Jun 14, 2024

Although not a widely used term, this kind of reminds me of additivity in the logging community.
ref: https://logback.qos.ch/manual/architecture.html#additivity

@seonWKim
Copy link
Contributor

I love the way the maintainers explain the details of the issue and extra information. Just reading the issues sometimes help me a lot 😄

@my4-dev
Copy link
Contributor

my4-dev commented Sep 12, 2024

Hi! I'm interested in handling this issue.
I have some questions to ask.

Is it ideal that an annotation that has @DecoratorFactory annotation whose repeatable flag is true has higher priority than an annotation whose repeatable flag is false?

One more question.
Do you imagine that priority comparison applies only to annotations that have @DecoratorFactory annotation whose value's class is the same?

@ikhoon
Copy link
Contributor Author

ikhoon commented Sep 13, 2024

@DecoratorFactory annotation whose repeatable flag is true has higher priority than an annotation whose repeatable flag is false?

No, I don't think so. We need to keep the original priority. The innermost @DecoratorFactory or @Decorator will take precedence.

Do you imagine that priority comparison applies only to annotations that have @DecoratorFactory annotation whose value's class is the same?

Right. repeatable should be limited to the same DecoratorFactoryFunction.class.

@my4-dev my4-dev linked a pull request Sep 18, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Provides a way for DecoratorFactory to be applied only once my4-dev/armeria
4 participants
@ikhoon @jrhee17 @my4-dev @seonWKim