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 and resolve all remaining open questions #544

Merged
merged 1 commit into from Oct 21, 2021

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Oct 18, 2021

  • settled on the name "Build Compatible Extensions"
  • renamed @Processing to @Registration, because if we ever want
    to introduce an extension phase for modifying beans before they
    are registered, that should be called @Processing
  • moved the synthetic bean creation/destruction functions from
    the CreationalContext style to the Instance style
  • specified precisely how ClassInfo.methods and fields work,
    specified that ClassConfig behaves identically, and specified
    that an extension methods with parameters of type MethodInfo
    or FieldInfo also behave identically
  • removed the @ExactType and @SubtypesOf annotations and moved
    type queries directly to the @Enhancement and @Registration
    annotations
  • moved the MetaAnnotations API from accepting a callback
    to directly returning ClassConfig
  • specified lifecycle of SyntheticBean{Creator,Disposer} and
    SyntheticObserver
  • specified that Messages.error is treated as a deployment problem

@Ladicek Ladicek added lite-extension-api Issues related to CDI Lite extension API proposal Lite Related to CDI Lite labels Oct 18, 2021
@Ladicek
Copy link
Contributor Author

Ladicek commented Oct 18, 2021

@graemerocher There are 2 changes here that are worth highlighting.

The MetaAnnotations API now returns ClassConfig instead of accepting a Consumer<ClassConfig>. I investigated if that can be implemented in Quarkus and it can -- but if that would cause trouble for you, we can move back to the callback style.

The other thing is that SyntheticBean{Creator,Disposer} now take an Instance<Object> instead of a CreationalContext. As such, they are much nicer to use. If you already have the implementation using CreationalContext, then the Instance-based API can be implemented on top of it like this:

  1. The Instance is very similar to what BeanContainer.createInstance() returns, except:
    1. it must not have its own CreationalContext, it must use the CreationalContext for the synthetic bean;
    2. it must not have its own "current InjectionPoint", it must use the "current InjectionPoint" from when the creation function is called;
    3. if the synthetic bean is not @Dependent, trying to lookup InjectionPoint should throw;
    4. trying to lookup InjectionPoint in the disposer function should throw.
  2. The creator function that accepts CreationalContext must create the Instance per item 1 and invoke the creator function that accepts Instance.
  3. The disposer function that accepts CreationalContext must create the Instance per item 1, invoke the disposer function that accepts Instance, and then invoke CreationalContext.release().

@Ladicek
Copy link
Contributor Author

Ladicek commented Oct 18, 2021

BTW, this completes the extension API (at least from my side). I'll write the accompanying specification text next.

Copy link
Contributor

@graemerocher graemerocher left a comment

Choose a reason for hiding this comment

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

Perhaps you could clarify in a comment how an implementor of the synthetic bean creator API is to create a bean looking up any additional instances using the Instance parameter and then later dispose a bean plus diposing any beans created via the Instance paramerer. That part is not 100% clear to me.

*
* @return types of annotations that must be present on the <em>expected types</em>
*/
Class<? extends Annotation>[] withAnnotations() default Annotation.class;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still concerned this defaults to any annotation. It should maybe be clarified that if not specified then only types with at least one bean defining annotation will be visited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that a concern that visiting types with all annotations is impossible (in which case we may use Annotation.class as a marker for default value, whatever that is), or that visiting types with all annotations is usually bad but still possible (in which case we'd have to create our own marker type)?

I mean, I'm fine with restricting that, just not sure to what annotations. Bean defining annotations are probably a good start, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I just noticed that Portable Extensions' @WithAnnotations also treats a type annotated with given annotation if the type is annotated with some other annotation that is, in turn, meta-annotated with given annotation. Do we want to add that too?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes stereotypes meta-annotations are fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. This now looks like withAnnotations() default BeanDefiningAnnotations.class, where BeanDefiningAnnotations is a marker annotation type that represents the set of bean defining annotations.

@Ladicek
Copy link
Contributor Author

Ladicek commented Oct 18, 2021

Perhaps you could clarify in a comment how an implementor of the synthetic bean creator API is to create a bean looking up any additional instances using the Instance parameter and then later dispose a bean plus diposing any beans created via the Instance paramerer. That part is not 100% clear to me.

Do you mean here, or in the javadoc?

In any case, it works like this:

  1. The bean creator can call lookup.select(SomeBean.class).get() to obtain a dependency that it needs.
  2. If that dependency is only needed for something temporary to the creation function and is not passed to the constructor of the synthetic bean, then the bean creator can call lookup.destroy(theDependency) to eagerly destroy the bean.
  3. Alternatively, the bean creator can just let the dependency live on. That is especially useful if that dependency is passed to the constructor, or if the dependency is @Dependent.
  4. The bean disposer can use lookup.destroy() as well to destroy any dependencies that outlive the bean creator. Note that this isn't necessary for @Dependent dependencies, because those are destroyed automatically after the disposer finishes.

EDIT: I also updated the comment above that describes how to implement the API that accepts Instance on top of the API that accepts CreationalContext.

@Ladicek
Copy link
Contributor Author

Ladicek commented Oct 19, 2021

Added one more commit that moves the new extension API to the jakarta.enterprise.inject.build.spi package, which I think can be used as final location.

@Ladicek Ladicek force-pushed the api-improvements branch 3 times, most recently from 869c143 to 2c5c21f Compare October 20, 2021 15:09
- settled on the name "Build Compatible Extensions"
- renamed `@Processing` to `@Registration`, because if we ever want
  to introduce an extension phase for modifying beans before they
  are registered, _that_ should be called `@Processing`
- moved the synthetic bean creation/destruction functions from
  the `CreationalContext` style to the `Instance` style
- specified precisely how `ClassInfo.methods` and `fields` work,
  specified that `ClassConfig` behaves identically, and specified
  that an extension methods with parameters of type `MethodInfo`
  or `FieldInfo` also behave identically
- removed the `@ExactType` and `@SubtypesOf` annotations and moved
  type queries directly to the `@Enhancement` and `@Registration`
  annotations
- moved the `MetaAnnotations` API from accepting a callback
  to directly returning `ClassConfig`
- specified lifecycle of `SyntheticBean{Creator,Disposer}` and
  `SyntheticObserver`
- specified that `Messages.error` is treated as a deployment problem
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.

LGTM

@Ladicek Ladicek merged commit aa11e86 into jakartaee:master Oct 21, 2021
@Ladicek Ladicek deleted the api-improvements branch October 21, 2021 07:58
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

3 participants