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

improve extension API #529

Merged
merged 1 commit into from Sep 23, 2021
Merged

improve extension API #529

merged 1 commit into from Sep 23, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Sep 20, 2021

  • 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"

@@ -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) {
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

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!

* <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.
Copy link
Contributor Author

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.

* 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 {
Copy link
Contributor Author

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.).

Copy link
Contributor

Choose a reason for hiding this comment

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

*/
ClassInfo declaration();
ClassType genericClass();
Copy link
Contributor Author

@Ladicek Ladicek Sep 20, 2021

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.

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 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 :-)

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 20, 2021

Added comments where I thought some discussion is warranted.

Copy link
Contributor

@manovotn manovotn left a 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.

Copy link
Contributor

@Emily-Jiang Emily-Jiang left a 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.

@Ladicek Ladicek force-pushed the api-improvements branch 2 times, most recently from fb57939 to ed6aed8 Compare September 22, 2021 09:14
- 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"
@Ladicek Ladicek merged commit 602f89c into jakartaee:master Sep 23, 2021
@Ladicek Ladicek deleted the api-improvements branch September 23, 2021 15:12
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 23, 2021

@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.

@Emily-Jiang
Copy link
Contributor

@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.

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 27, 2021

I shared my reasoning in the comments extensively, but I'll repeat it for you in a condensed form.

If we have value(boolean) and value(boolean...), we also have an ambiguity for a single-element array, which needs to be resolved by the caller by creating an array explicitly.

If we only have value(boolean...), we force implementations to look up the type of the annotation member, which is undesirable.

We don't have a good alternative name for the array-accepting method; values is wrong, because the method is used to set the value annotation member (similarly for the member method -- members is also wrong, because it's still for a single member).

Having value(boolean) and value(boolean[]) is perhaps not so nice when you can write down the array as a sequence of literals (can't estimate how common that is, but you sometimes need to compute the array, in which case it doesn't matter), but it makes it immediately obvious what method is being called and what is the intended type.

@Emily-Jiang
Copy link
Contributor

Emily-Jiang commented Sep 27, 2021

@Ladicek thanks for the further explanation! In my previous comments, I noticed not only a number of values being impacted but also members.

If we only have value(boolean...), we force implementations to look up the type of the annotation member, which is undesirable.

I think forcing implementations doing something extra does give end uers a nicer API though.

@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 27, 2021

Yes, the same reasoning applies to the member methods. I keep referring to value(boolean) / value(boolean...), but the same problem exists for member(String, int) / member(String, int...) and all the other combinations.

I think forcing implementations doing something extra does give end uers a nicer API though.

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.

@Ladicek Ladicek added Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal labels Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Lite Related to CDI Lite lite-extension-api Issues related to CDI Lite extension API proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants