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

improve extension API #529

Merged
merged 1 commit into from
Sep 23, 2021
Merged

improve extension API #529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions api/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@


<properties>
<annotation.api.version>2.1.0-B1</annotation.api.version>
<atinject.api.version>2.0.0</atinject.api.version>
<uel.api.version>4.0.0</uel.api.version>
<interceptor.api.version>2.0.0</interceptor.api.version>
Expand All @@ -154,6 +155,12 @@
<version>6.8.8</version>
</dependency>

<dependency>
<groupId>jakarta.annotation</groupId>
<artifactId>jakarta.annotation-api</artifactId>
<version>${annotation.api.version}</version>
</dependency>

<dependency>
<groupId>jakarta.inject</groupId>
<artifactId>jakarta.inject-api</artifactId>
Expand Down Expand Up @@ -182,6 +189,11 @@
</dependencyManagement>

<dependencies>
<dependency>
<groupId>jakarta.annotation</groupId>
<artifactId>jakarta.annotation-api</artifactId>
</dependency>

<dependency>
<groupId>jakarta.el</groupId>
<artifactId>jakarta.el-api</artifactId>
Expand All @@ -190,6 +202,12 @@
<dependency>
<groupId>jakarta.interceptor</groupId>
<artifactId>jakarta.interceptor-api</artifactId>
<exclusions>
<exclusion>
<groupId>jakarta.annotation</groupId>
<artifactId>jakarta.annotation-api</artifactId>
</exclusion>
</exclusions>
</dependency>

<dependency>
Expand Down
76 changes: 42 additions & 34 deletions api/src/main/java/jakarta/enterprise/inject/build/compatible/spi/AnnotationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@
* <li>call {@link #build()} to create an {@link AnnotationInfo}.</li>
* </ol>
* One builder instance should not be used to create multiple annotations.
* <p>
* Note that values of all members of given annotation type must be defined before
* calling {@code build()}, except of annotation members that declare a default value.
* If a value is not defined for an annotation member that does not have a default value,
* {@code build()} will throw an exception. Defining values of members that do not
* exist on given annotation type is possible, but such values will be ignored.
*
* @since 4.0
*/
Expand Down Expand Up @@ -67,7 +73,7 @@ default AnnotationBuilder value(boolean value) {
* @param values the boolean array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(boolean... values) {
default AnnotationBuilder value(boolean[] values) {
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'm not 100% sure about this change, but when I tried to use the API, it was actually a bit surprising to me. I started wondering whether we should do something about single-element arrays, maybe automatically convert the type or so. In the end, I thought getting rid of varargs and forcing explicit arrays is a nice way out of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer varargs instead of the array. In the previous form, you can pass one value while the proposed change needs to convert the single value to an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is in fact incorrect, and shows exactly the same confusion I found myself in.

Note that we have overloaded methods value(boolean) and value(boolean...). If you call value(true), the overload resolution process will always select the first method (see https://docs.oracle.com/javase/specs/jls/se17/html/jls-15.html#jls-15.12.2: This ensures that a method is never chosen through variable arity method invocation if it is applicable through fixed arity method invocation).

Varargs here make sense if more than 1 argument is passed, or in case of an empty array. But for a single-argument array, you would always have to write new boolean[] { true }, which is inconsistent and, as demonstrated above, confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another method to eliminate the confusion would be, of course, to eliminate the overloads, so that the varargs accepting method would be called differently. I don't have a good name, personally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another method to eliminate the confusion would be, of course, to eliminate the overloads

That's what I was thinking when I saw your comment. Couldn't we have a value method name for single value and values for varargs? Or something like singleValue and value. Basically, I'd just look for some names so that we are able to keep varargs variants in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify further.

One reason why I don't want to force implementations to find types of annotation members is that I think it should be possible to define values for annotation members that don't exist (those values would be ignored). The reason is hypothetical forward compatibility. It should be possible to define a value for an annotation member that doesn't exist on an older version of the annotation, but does exist on a newer version of the annotation.

Maybe that's not entirely useful and we should fail in such situation -- I can see benefits in both ways and don't really mind. (I just pushed a change where I explicitly specify that nonexisting members are ignored. I can amend it again to say the opposite.)

Another reason is that I think callers should be explicit about which type of the annotation member they are defining. On other places, we moved away from type coercion, I don't think we should add it here. When I read value(true), I need to know immediately what type is it -- I don't want to look into the annotation definition to figure out if it's singular or array. Principle of locality.

Copy link
Contributor

Choose a reason for hiding this comment

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

personally I think we should have value(boolean) and value(boolean[]) it is not worth the hassle to support varargs and yes we 100% need the ability to specify meta-members that don't actually exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just out of curiosity @graemerocher, what is your reason to support specifying annotation members that don't exist? I admit mine is just the hypothetical forward compatibility -- I'd love to know if you have more.

Copy link
Contributor

@graemerocher graemerocher Sep 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We use it in Micronaut Data. We precompute the SQL/JPA-QL query and store it a meta annotation member that has no actual real equivalent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting one, thanks for sharing!

return member(AnnotationMember.VALUE, values);
}

Expand All @@ -87,7 +93,7 @@ default AnnotationBuilder value(byte value) {
* @param values the byte array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(byte... values) {
default AnnotationBuilder value(byte[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -107,7 +113,7 @@ default AnnotationBuilder value(short value) {
* @param values the short array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(short... values) {
default AnnotationBuilder value(short[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -127,7 +133,7 @@ default AnnotationBuilder value(int value) {
* @param values the int array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(int... values) {
default AnnotationBuilder value(int[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -147,7 +153,7 @@ default AnnotationBuilder value(long value) {
* @param values the long array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(long... values) {
default AnnotationBuilder value(long[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -167,7 +173,7 @@ default AnnotationBuilder value(float value) {
* @param values the float array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(float... values) {
default AnnotationBuilder value(float[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -187,7 +193,7 @@ default AnnotationBuilder value(double value) {
* @param values the double array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(double... values) {
default AnnotationBuilder value(double[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -207,7 +213,7 @@ default AnnotationBuilder value(char value) {
* @param values the char array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(char... values) {
default AnnotationBuilder value(char[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -227,7 +233,7 @@ default AnnotationBuilder value(String value) {
* @param values the String array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(String... values) {
default AnnotationBuilder value(String[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -247,7 +253,7 @@ default AnnotationBuilder value(Enum<?> value) {
* @param values the enum array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(Enum<?>... values) {
default AnnotationBuilder value(Enum<?>[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -269,7 +275,7 @@ default AnnotationBuilder value(Class<? extends Enum<?>> enumType, String enumVa
* @param enumValues names of the enum constants, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(Class<? extends Enum<?>> enumType, String... enumValues) {
default AnnotationBuilder value(Class<? extends Enum<?>> enumType, String[] enumValues) {
return member(AnnotationMember.VALUE, enumType, enumValues);
}

Expand All @@ -291,7 +297,7 @@ default AnnotationBuilder value(ClassInfo enumType, String enumValue) {
* @param enumValues names of the enum constants, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(ClassInfo enumType, String... enumValues) {
default AnnotationBuilder value(ClassInfo enumType, String[] enumValues) {
return member(AnnotationMember.VALUE, enumType, enumValues);
}

Expand All @@ -311,7 +317,7 @@ default AnnotationBuilder value(Class<?> value) {
* @param values the class array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(Class<?>... values) {
default AnnotationBuilder value(Class<?>[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -331,7 +337,7 @@ default AnnotationBuilder value(ClassInfo value) {
* @param values the class array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(ClassInfo... values) {
default AnnotationBuilder value(ClassInfo[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand Down Expand Up @@ -371,7 +377,7 @@ default AnnotationBuilder value(Type value) {
* @return this {@code AnnotationBuilder}
* @throws IllegalArgumentException if any given type is invalid, as described above
*/
default AnnotationBuilder value(Type... values) {
default AnnotationBuilder value(Type[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -391,7 +397,7 @@ default AnnotationBuilder value(AnnotationInfo value) {
* @param values the annotation array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(AnnotationInfo... values) {
default AnnotationBuilder value(AnnotationInfo[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand All @@ -411,7 +417,7 @@ default AnnotationBuilder value(Annotation value) {
* @param values the annotation array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
default AnnotationBuilder value(Annotation... values) {
default AnnotationBuilder value(Annotation[] values) {
return member(AnnotationMember.VALUE, values);
}

Expand Down Expand Up @@ -440,7 +446,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the boolean array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, boolean... values);
AnnotationBuilder member(String name, boolean[] values);

/**
* Adds a byte-valued annotation member with given {@code name}.
Expand All @@ -458,7 +464,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the byte array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, byte... values);
AnnotationBuilder member(String name, byte[] values);

/**
* Adds a short-valued annotation member with given {@code name}.
Expand All @@ -476,7 +482,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the short array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, short... values);
AnnotationBuilder member(String name, short[] values);

/**
* Adds an int-valued annotation member with given {@code name}.
Expand All @@ -494,7 +500,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the int array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, int... values);
AnnotationBuilder member(String name, int[] values);

/**
* Adds a long-valued annotation member with given {@code name}.
Expand All @@ -512,7 +518,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the long array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, long... values);
AnnotationBuilder member(String name, long[] values);

/**
* Adds a float-valued annotation member with given {@code name}.
Expand All @@ -530,7 +536,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the float array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, float... values);
AnnotationBuilder member(String name, float[] values);

/**
* Adds a double-valued annotation member with given {@code name}.
Expand All @@ -548,7 +554,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the double array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, double... values);
AnnotationBuilder member(String name, double[] values);

/**
* Adds a char-valued annotation member with given {@code name}.
Expand All @@ -566,7 +572,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the char array, must not be {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, char... values);
AnnotationBuilder member(String name, char[] values);

/**
* Adds a String-valued annotation member with given {@code name}.
Expand All @@ -584,7 +590,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the String array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, String... values);
AnnotationBuilder member(String name, String[] values);

/**
* Adds an enum-valued annotation member with given {@code name}.
Expand All @@ -602,7 +608,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the enum array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, Enum<?>... values);
AnnotationBuilder member(String name, Enum<?>[] values);

/**
* Adds an enum-valued annotation member with given {@code name}.
Expand All @@ -622,7 +628,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param enumValues names of the enum constants, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, Class<? extends Enum<?>> enumType, String... enumValues);
AnnotationBuilder member(String name, Class<? extends Enum<?>> enumType, String[] enumValues);

/**
* Adds an enum-valued annotation member with given {@code name}.
Expand All @@ -642,7 +648,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param enumValues names of the enum constants, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, ClassInfo enumType, String... enumValues);
AnnotationBuilder member(String name, ClassInfo enumType, String[] enumValues);

/**
* Adds a class-valued annotation member with given {@code name}.
Expand All @@ -660,7 +666,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the class array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, Class<?>... values);
AnnotationBuilder member(String name, Class<?>[] values);

/**
* Adds a class-valued annotation member with given {@code name}.
Expand All @@ -678,7 +684,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the class array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, ClassInfo... values);
AnnotationBuilder member(String name, ClassInfo[] values);

/**
* Adds a class-valued annotation member with given {@code name}.
Expand Down Expand Up @@ -717,7 +723,7 @@ default AnnotationBuilder value(Annotation... values) {
* @return this {@code AnnotationBuilder}
* @throws IllegalArgumentException if any given type is invalid, as described above
*/
AnnotationBuilder member(String name, Type... values);
AnnotationBuilder member(String name, Type[] values);

/**
* Adds an annotation-valued annotation member with given {@code name}.
Expand All @@ -735,7 +741,7 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the annotation array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, AnnotationInfo... values);
AnnotationBuilder member(String name, AnnotationInfo[] values);

/**
* Adds an annotation-valued annotation member with given {@code name}.
Expand All @@ -753,13 +759,15 @@ default AnnotationBuilder value(Annotation... values) {
* @param values the annotation array, must not be {@code null} or contain {@code null}
* @return this {@code AnnotationBuilder}
*/
AnnotationBuilder member(String name, Annotation... values);
AnnotationBuilder member(String name, Annotation[] values);

/**
* Returns an {@link AnnotationInfo} that includes all annotation members defined by previous method calls
* on this builder. After {@code build()} is called, this builder instance should be discarded.
*
* @return the built {@link AnnotationInfo}, never {@code null}
* @throws IllegalStateException if a value of some annotation member was not set, and that member
* does not declare a default value
*/
AnnotationInfo build();
}
Loading