-
Notifications
You must be signed in to change notification settings - Fork 77
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
improve extension API #529
Conversation
api/src/main/java/jakarta/enterprise/inject/build/compatible/spi/AnnotationBuilder.java
Outdated
Show resolved
Hide resolved
@@ -67,7 +70,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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
* <p> | ||
* CDI implementations are not required to accept custom implementations of any {@code jakarta.enterprise.lang.model} | ||
* or {@code jakarta.enterprise.inject.build.compatible.spi} interface. In other words, users may only use instances | ||
* of these interfaces that they previously obtained from the corresponding API. If not, non-portable behavior results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't think of a better place. We should put it to the spec text for sure, but I thought the API javadoc should have it as well.
api/src/main/java/jakarta/enterprise/inject/build/compatible/spi/InterceptorInfo.java
Outdated
Show resolved
Hide resolved
* Similarly, annotation-typed parameters are always available as instances of the annotation | ||
* type, even if an instance of {@code AnnotationInfo} was passed to the builder. | ||
*/ | ||
public interface Parameters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original Map<String, Object>
-based API isn't exactly pleasant when you've got more than just 2 or 3 parameters. So I thought a dedicated API would be better (and would have much smaller surface).
Not sure if this style (get(String, Class)
) is better than the other suggested style (getInt(String)
, getString(String)
, getEnum(String)
, getAnnotation(String)
etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this is what we have in Micronaut that is equivalent https://github.com/micronaut-projects/micronaut-core/blob/19fef5801f31a729d88d8df47c3410fbfa442eda/core/src/main/java/io/micronaut/core/annotation/AnnotationValueResolver.java#L27
api/src/main/java/jakarta/enterprise/inject/build/compatible/spi/SyntheticBeanBuilder.java
Show resolved
Hide resolved
api/src/main/java/jakarta/enterprise/inject/build/compatible/spi/SyntheticBeanBuilder.java
Show resolved
Hide resolved
*/ | ||
ClassInfo declaration(); | ||
ClassType genericClass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from me thinking how to describe what a type is. Parameterized type isn't created by applying a list of type arguments to a class, but to a class type. So I thought that should be a first-class concept here. Allowing access to the declaration still makes sense, so that is retained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that I got this one wrong. The type arguments are not applied to the class type; it's applied to the type constructor that is introduced by the class declaration. I need to think more about this :-)
api/src/main/java/jakarta/enterprise/inject/build/compatible/spi/Enhancement.java
Show resolved
Hide resolved
Added comments where I thought some discussion is warranted. |
6522be7
to
dd20289
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped two separate comments to existing discussions but overall it looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a lot of changes is about updating varargs to array, which I think it is not a good change.
fb57939
to
ed6aed8
Compare
- explicitly specified that CDI implementations don't have to accept custom implemetations of the `jakarta.enterprise.lang.model` and `jakarta.enterprise.inject.build.compatible.spi` interfaces - moved to Jakarta Annotation 2.1.0-B1, removed `@ExtensionPriority` and specified that `@Priority` should be used on extension methods - specified that `AnnotationBuilder.build` will throw if some members of the annotation type were not defined - adjusted `AnnotationBuilder` so that its methods no longer accept varargs; arrays must be passed in explicitly to avoid ambiguity in case of single-element arrays - added `InterceptorInfo` (extending `BeanInfo`) to provide information about interceptors, and allowed `@Processing` methods to declare parameters of type `InterceptorInfo` - adjusted `@Enhancement` so that parameters of type `DeclarationInfo` and `DeclarationConfig` can no longer be declared; it is not clear what is the full set of matching values - replaced the use of `Map<String, Object>` in synthetic components functions by a dedicated `Parameters` interface - adjusted synthetic component builders to allow defining a parameter of an enum type, as well as using `ClassInfo` for defining a parameter of a `Class` type - improved `ParameterizedType` to provide access to the type (`ClassType`) of the generic class, in addition to the declaration (`ClassInfo`) - replaced abbreviated forms "isn't", "doesn't" and "can't" with their longer forms "is not", "does not" and "cannot"
ed6aed8
to
fa6d3a3
Compare
@Emily-Jiang I merged this PR, because it contains many other valuable changes. I'd be happy to debate the merits of varargs vs. plain arrays separately, if you're interested, and am open to changing it back if we find a good solution. |
@Ladicek can you explain why array is more preferred than varargs? I think it might be better if you explained and then merged rather than the other way round. |
I shared my reasoning in the comments extensively, but I'll repeat it for you in a condensed form. If we have If we only have We don't have a good alternative name for the array-accepting method; Having |
@Ladicek thanks for the further explanation! In my previous comments, I noticed not only a number of
I think forcing implementations doing something extra does give end uers a nicer API though. |
Yes, the same reasoning applies to the
My opinion is that this also leads to worse API. With explicit array creation, the code author must be explicit about the type of the annotation member. This means that if you read the code, you know exactly what type the member is supposed to have. If that information is implicit, you have to look at the annotation type when reading the code. Further, it would prevent certain idioms, such as keeping additional data in "non-existing" annotation members. |
custom implemetations of the
jakarta.enterprise.lang.model
andjakarta.enterprise.inject.build.compatible.spi
interfaces@ExtensionPriority
and specified that
@Priority
should be used on extension methodsAnnotationBuilder.build
will throw if some membersof the annotation type were not defined
AnnotationBuilder
so that its methods no longer acceptvarargs; arrays must be passed in explicitly to avoid ambiguity
in case of single-element arrays
InterceptorInfo
(extendingBeanInfo
) to provideinformation about interceptors, and allowed
@Processing
methodsto declare parameters of type
InterceptorInfo
@Enhancement
so that parameters of typeDeclarationInfo
and
DeclarationConfig
can no longer be declared; it is not clearwhat is the full set of matching values
Map<String, Object>
in synthetic componentsfunctions by a dedicated
Parameters
interfaceof an enum type, as well as using
ClassInfo
for defining a parameterof a
Class
typeParameterizedType
to provide access to the type (ClassType
)of the generic class, in addition to the declaration (
ClassInfo
)with their longer forms "is not", "does not" and "cannot"