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

Add an annotation overlay #255

Closed
Ladicek opened this issue Sep 8, 2022 · 9 comments · Fixed by #361
Closed

Add an annotation overlay #255

Ladicek opened this issue Sep 8, 2022 · 9 comments · Fixed by #361
Assignees
Milestone

Comments

@Ladicek
Copy link
Contributor

Ladicek commented Sep 8, 2022

Jandex is immutable, but it is often required to have a mutable layer for annotations. This is because annotations are very often used to represent framework-specific metadata, and frameworks that use Jandex as a language model require the ability to mutate the metadata.

ArC and RESTEasy Reactive, both parts of Quarkus, have their own annotation overlay mechanism (called AnnotationStore). Hibernate also has interest in this (#117). Over time, I grew convinced that Jandex should just carry a similar mechanism, which I'd like to call AnnotationOverlay.

I'm actually thinking of 2 kinds of an annotation overlay:

  1. An immutable overlay that is created from an IndexView and a collection of lazily-applied annotation transformations. This is what ArC and RESTEasy Reactive currently have.
  2. A mutable overlay created from an IndexView that exposes operations to directly mutate annotations. This kind of overlay would expose a freeze() operation, returning an annotation transformation that is suitable for creating the 1st kind of overlay. After freezing, this kind of annotation overlay should be thrown away. It is useful for implementing things like CDI Build Compatible Extensions.

Internally, an annotation overlay would contain a Map from an EquivalenceKey to a set of annotations present on the declaration. When the annotation overlay is first asked for information about given declaration, the annotation transformations are run and the result is stored to the map. In case no annotation transformation affected given declaration, we should be able to store a sentinel value that means "just look it up from the passed annotation target". That would conserve memory.

Extra care needs to be taken when it comes to annotations on nested annotation targets. I'm inclined to say that annotation overlays should only store/return annotations present directly on given declaration and ignore annotations present on nested annotation targets. However, ArC currently (being based on Jandex 2.x) sometimes uses method annotation transformations to transform annotations on the method parameters. So this might require some extra wiggle room.

@Ladicek Ladicek changed the title add an annotation overlay Add an annotation overlay Sep 12, 2022
@sebersole
Copy link

Are these "lazily-applied annotation transformations" already defined as a contract? Trying to visualize what this would look like and how it would work; maybe seeing the actually interface would help.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 5, 2023

I don't have anything concrete yet, but I expect the immutable overlay (item 1) to be fairly similar to what ArC and RESTEasy Reactive have today.

EDIT: I mean, the annotation overlay itself would likely look like this: https://github.com/quarkusio/quarkus/blob/main/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/AnnotationStore.java and the "lazily applied transformation function" would look like this: https://github.com/quarkusio/quarkus/blob/main/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/AnnotationsTransformation.java

@sebersole
Copy link

Will both of these ultimately result in a Index? Or will the result be a IndexView?

Trying to account for this possibility in the proof-of-concept work I am doing and this would be good to know; specifically the Collection/List return difference.

Thanks!

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 5, 2023

The annotation overlay won't be an Index nor an IndexView. The object graph coming from an index is immutable, so this would require expensive copying. Instead, the idea is that you'll obtain the objects from an index as usual, and if you want to obtain annotations for that object, you'll call into the overlay.

(The overlay will be built on top of an IndexView though.)

@sebersole
Copy link

Sorry, I wasn't asking is the overlay will be an Index or IndexView. I was asking whether it would produce either.

Or are you saying it is a completely separate beast and we would just consume the overlay instead of a Index or IndexView?

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 6, 2023

Sorry for confusion, I considered "be" or "produce" the same in this context. Neither is possible without massively breaking changes to Jandex I'm afraid.

So this will be a totally separate beast sitting on top of an IndexView. You'd have to first have an index, then you'd create the overlay that represents annotation alterations, and then you'd consume both: obtain classes/methods/fields from the index, and their annotations from the overlay.

@sebersole
Copy link

So for what it is worth, I'd not worry about this for Hibernate. I think anything you could do here would not really fit our needs. There's this aspect (XML overlay) plus stuff like dynamic models (no Class), auto-discovery, etc that we are just simply going to have to handle on our own. And if we need to handle these aspects ourselves, I'm not sure we are getting a lot of benefit from this feature in Jandex. Since these other projects already have working support for overlay I just wanted to make sure you aren't adding this just for Hibernate.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 9, 2023

No worries, it certainly isn't only for Hibernate. I think the idea is to centralize language modelling in Quarkus to Jandex, and this metadata (annotation) overlaying is a big missing piece there. So it's for Quarkus overall. (Whether there will be a single overlay for all Quarkus extensions, or each will maintain their own instance, I have no idea. The first option is very powerful, but also somewhat dangerous. We'll see.)

@geoand
Copy link
Contributor

geoand commented May 16, 2024

cc @FroMage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants