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.

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

Implement @Attribute Injection. #5547

Merged
merged 41 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
eac6431
skeleton code for @Attribute injection.
chickenchickenlove Mar 29, 2024
2801cdd
Implement Attribute Injection : Apply review
chickenchickenlove Apr 1, 2024
889d887
make test code for Attribute injection.
chickenchickenlove Apr 1, 2024
94a93af
remove duplicate code from AnnotatedValueResolverTest
chickenchickenlove Apr 1, 2024
42ef9a2
Implement Attribute Injection : Apply review2
chickenchickenlove Apr 2, 2024
ab8a08a
Implement Attribute Injection : Apply review3
chickenchickenlove Apr 3, 2024
3fac84e
Implement Attribute Injection : Apply review3. remove useless comments.
chickenchickenlove Apr 3, 2024
f3f8b74
Fix lint, Apply review
chickenchickenlove Apr 3, 2024
821e18f
Add user docs
chickenchickenlove Apr 3, 2024
dce9520
Update core/src/main/java/com/linecorp/armeria/server/annotation/Attr…
chickenchickenlove Apr 5, 2024
fc5771a
Update core/src/main/java/com/linecorp/armeria/server/annotation/Attr…
chickenchickenlove Apr 5, 2024
f017563
Update core/src/main/java/com/linecorp/armeria/server/annotation/Attr…
chickenchickenlove Apr 5, 2024
1927e46
Update core/src/main/java/com/linecorp/armeria/server/annotation/Attr…
chickenchickenlove Apr 5, 2024
ee7a466
Use @default.class and fix link of java docs.
chickenchickenlove Apr 5, 2024
65868b7
Merge branch 'main' into 240329-try1
chickenchickenlove Apr 5, 2024
1d0b6c7
fix lint error
chickenchickenlove Apr 5, 2024
1877117
Update site/src/pages/docs/server-annotated-service.mdx
chickenchickenlove Apr 9, 2024
bdb0088
Update site/src/pages/docs/server-annotated-service.mdx
chickenchickenlove Apr 9, 2024
29543f9
Apply comment to server-annotated-service docs.
chickenchickenlove Apr 9, 2024
957378f
Update core/src/main/java/com/linecorp/armeria/server/annotation/Attr…
chickenchickenlove Apr 12, 2024
2893d3d
modify misstypo
chickenchickenlove Apr 12, 2024
c70c2fb
remove findName().
chickenchickenlove Apr 12, 2024
c1de466
Throw ClassCastException on AttributeResolver.
chickenchickenlove Apr 12, 2024
e6632fa
modify miss typoe
chickenchickenlove Apr 17, 2024
065e43a
Apply code review
chickenchickenlove Apr 17, 2024
e80075f
apply code review
chickenchickenlove Apr 18, 2024
5874745
apply review
chickenchickenlove Apr 18, 2024
c6519ab
apply review
chickenchickenlove Apr 18, 2024
bb087a7
apply review
chickenchickenlove Apr 18, 2024
4560943
apply review
chickenchickenlove Apr 22, 2024
0dd3e77
Update core/src/main/java/com/linecorp/armeria/server/annotation/Attr…
chickenchickenlove Apr 23, 2024
65bc886
throw immedetely if valued is failed to be cast.
chickenchickenlove Apr 23, 2024
c27629b
Update core/src/main/java/com/linecorp/armeria/internal/server/annota…
chickenchickenlove Apr 24, 2024
3943f70
Update core/src/main/java/com/linecorp/armeria/internal/server/annota…
chickenchickenlove Apr 24, 2024
3df348b
Update core/src/test/java/com/linecorp/armeria/internal/server/annota…
chickenchickenlove Apr 24, 2024
7230534
apply review
chickenchickenlove Apr 24, 2024
72d26b2
fix lint error.
chickenchickenlove Apr 24, 2024
8f3631c
Update core/src/main/java/com/linecorp/armeria/server/annotation/Attr…
chickenchickenlove May 2, 2024
9013388
apply review
chickenchickenlove May 2, 2024
588c8bb
Merge branch 'main' into 240329-try1
chickenchickenlove May 7, 2024
70d76cb
fix lint error
chickenchickenlove May 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.base.Ascii;
import com.google.common.base.CaseFormat;

import com.linecorp.armeria.server.annotation.Attribute;
import com.linecorp.armeria.server.annotation.Header;
import com.linecorp.armeria.server.annotation.Param;

Expand All @@ -46,6 +47,16 @@
return getName(nameRetrievalTarget);
}

static String findName(Attribute attribute, Object nameRetrievalTarget) {
trustin marked this conversation as resolved.
Show resolved Hide resolved
requireNonNull(nameRetrievalTarget, "nameRetrievalTarget");

Check warning on line 51 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedElementNameUtil.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedElementNameUtil.java#L51

Added line #L51 was not covered by tests

final String value = attribute.value();

Check warning on line 53 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedElementNameUtil.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedElementNameUtil.java#L53

Added line #L53 was not covered by tests
if (DefaultValues.isSpecified(value)) {
checkArgument(!value.isEmpty(), "value is empty.");
return value;

Check warning on line 56 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedElementNameUtil.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedElementNameUtil.java#L56

Added line #L56 was not covered by tests
}
return getName(nameRetrievalTarget);

Check warning on line 58 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedElementNameUtil.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedElementNameUtil.java#L58

Added line #L58 was not covered by tests
}
/**
* Returns the value of the {@link Header} annotation which is specified on the {@code element} if
* the value is not blank. If the value is blank, it returns the name of the specified
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -84,6 +85,7 @@
import com.linecorp.armeria.internal.server.FileAggregatedMultipart;
import com.linecorp.armeria.internal.server.annotation.AnnotatedBeanFactoryRegistry.BeanFactoryId;
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.annotation.Attribute;
import com.linecorp.armeria.server.annotation.ByteArrayRequestConverterFunction;
import com.linecorp.armeria.server.annotation.Default;
import com.linecorp.armeria.server.annotation.Delimiter;
Expand All @@ -99,6 +101,7 @@
import com.linecorp.armeria.server.docs.DescriptionInfo;

import io.netty.handler.codec.http.HttpConstants;
import io.netty.util.AttributeKey;
import scala.concurrent.ExecutionContext;

final class AnnotatedValueResolver {
Expand Down Expand Up @@ -444,6 +447,13 @@
requireNonNull(dependencyInjector, "dependencyInjector");

final DescriptionInfo description = findDescription(annotatedElement);

final Attribute attr = annotatedElement.getAnnotation(Attribute.class);
if (attr != null) {
final String name = findName(attr, typeElement);
return ofAttribute(name, annotatedElement, typeElement, type, description);

Check warning on line 454 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L453-L454

Added lines #L453 - L454 were not covered by tests
}

final Param param = annotatedElement.getAnnotation(Param.class);
if (param != null) {
final String name = findName(param, typeElement);
Expand Down Expand Up @@ -520,6 +530,7 @@

private static boolean isAnnotationPresent(AnnotatedElement element) {
return element.isAnnotationPresent(Param.class) ||
element.isAnnotationPresent(Attribute.class) ||
element.isAnnotationPresent(Header.class) ||
element.isAnnotationPresent(RequestObject.class);
}
Expand Down Expand Up @@ -621,6 +632,25 @@
.build();
}

private static AnnotatedValueResolver ofAttribute(String name,
AnnotatedElement annotatedElement,
AnnotatedElement typeElement, Class<?> type,
DescriptionInfo description) {
return new Builder(annotatedElement, type, name)
.annotationType(Attribute.class)
.typeElement(typeElement)
.supportDefault(true)
.supportContainer(true)
.description(description)
.resolver(resolverAttribute((ctx, clazz) -> {

Check warning on line 645 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L639-L645

Added lines #L639 - L645 were not covered by tests
return ctx.context.attr(AttributeKey.valueOf(clazz, name)) != null ?
ctx.context.attr(AttributeKey.valueOf(clazz, name)) :
ctx.context.attr(AttributeKey.valueOf(name));

Check warning on line 648 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L647-L648

Added lines #L647 - L648 were not covered by tests
chickenchickenlove marked this conversation as resolved.
Show resolved Hide resolved
}
))
.build();

Check warning on line 651 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L651

Added line #L651 was not covered by tests
}

@Nullable
private static AnnotatedValueResolver ofInjectableTypes(String name, AnnotatedElement annotatedElement,
Class<?> type, boolean useBlockingExecutor) {
Expand Down Expand Up @@ -775,6 +805,61 @@
};
}

private static BiFunction<AnnotatedValueResolver, ResolverContext, Object>
resolverAttribute(BiFunction<ResolverContext, Class<?>, Object> getter) {
chickenchickenlove marked this conversation as resolved.
Show resolved Hide resolved
return (resolver, ctx) -> {

Check warning on line 810 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L810

Added line #L810 was not covered by tests
final Class<?> type = resolver.elementType().isPrimitive() ?
PrimitiveToReference.primitiveToReference(resolver.elementType.getName()) :
resolver.elementType();

Check warning on line 813 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L812-L813

Added lines #L812 - L813 were not covered by tests
// Primitive to Reference.
trustin marked this conversation as resolved.
Show resolved Hide resolved
Object value = getter.apply(ctx, type);

Check warning on line 815 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L815

Added line #L815 was not covered by tests
if (value != null) {
return value;

Check warning on line 817 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L817

Added line #L817 was not covered by tests
}
return resolver.defaultOrException();

Check warning on line 819 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L819

Added line #L819 was not covered by tests
};
}

enum PrimitiveToReference {
trustin marked this conversation as resolved.
Show resolved Hide resolved
BOOLEAN("boolean", Boolean.class),
BYTE("byte", Byte.class),
CHAR("char", Character.class),
SHORT("short", Short.class),
INTEGER("int", Integer.class),
LONG("long", Long.class),
FLOAT("float", Float.class),
DOUBLE("double", Double.class);

Check warning on line 831 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L823-L831

Added lines #L823 - L831 were not covered by tests

PrimitiveToReference(String name, Class<?> clazz) {
this.name = name;
this.clazz = clazz;
}

Check warning on line 836 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L833-L836

Added lines #L833 - L836 were not covered by tests

public String getName() {
return this.name;

Check warning on line 839 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L839

Added line #L839 was not covered by tests
}

public Class<?> getClazz() {
return this.clazz;

Check warning on line 843 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L843

Added line #L843 was not covered by tests
}

static Class<?> primitiveToReference(String name) {
final Class<?> clazz = PrimitiveToReference.map.get(name);

Check warning on line 847 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L847

Added line #L847 was not covered by tests
if (clazz != null) {
return clazz;

Check warning on line 849 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L849

Added line #L849 was not covered by tests
}
throw new IllegalStateException("There is no Reference type corresponding to " + name);

Check warning on line 851 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L851

Added line #L851 was not covered by tests
}

private final String name;
private final Class<?> clazz;
private static final Map<String, Class<?>> map = Arrays.stream(values())
.collect(Collectors.toMap(
e -> e.getName(),
e -> e.getClazz()));

Check warning on line 859 in core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java#L856-L859

Added lines #L856 - L859 were not covered by tests
}


/**
* Returns a bean resolver which retrieves a value using request converters. If the target element
* is an annotated bean, a bean factory of the specified {@link BeanFactoryId} will be used for creating an
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.linecorp.armeria.server.annotation;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import com.linecorp.armeria.internal.server.annotation.DefaultValues;

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.PARAMETER)
public @interface Attribute {
chickenchickenlove marked this conversation as resolved.
Show resolved Hide resolved

String value() default DefaultValues.UNSPECIFIED;
trustin marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the value is required anyways, it's probably better to enforce this at compile-time rather than handling it as an error later.

Suggested change
String value() default DefaultValues.UNSPECIFIED;
String value();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. i committed your suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's probably better to enforce this at compile-time rather than handling it as an error later.

Do you think that inferring the name of AttributeKey from Parameter.getName() is not a good practice?

Copy link
Contributor Author

@chickenchickenlove chickenchickenlove Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikhoon nim, thanks for your comment. 🙇‍♂️
I don't think so. I believe each has its own strengths.
I tend to prefer explicit aspects.
However, I am ready to adapt to the preferences of the framework developer, as well!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked about this with other maintainers and they thought the naming convention of AttributeKey is different from method parameter names.

Let's keep it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! thanks a lot 🙇‍♂️

}
Loading