Use Arc AnnotationStore to build Hibernate ORM metadata about quarkus

[Ladicek]

If anyone wants to discuss annotation overlay, here's a good place: smallrye/jandex#255

But I feel compelled to address one point here:

And last, I've seen many bugs where parts of an extension was using the overlay API and other parts using the Jandex API, and I really think the overlay should not be a different API to Jandex's main API.

Sounds good but might be tricky in terms of implementation.

It's not tricky, it's impossible. Not without a massive breaking change.

I've been toying with the idea of moving the ArC implementation of CDI Language Model to Jandex (as a separate module, not to the core) and recommend gradually migrating to that. The reason is that the CDI Language Model is a set of interfaces, not classes, so you can put an annotation overlay on top of Jandex there without breaking the consumers. The downsides are obvious, of course.

[geoand]

If I parsed your comments above incorrectly, my bad :)

[quarkus-bot]

/cc @Ladicek (arc), @gsmet (hibernate-orm), @manovotn (arc), @mkouba (arc), @yrodiere (hibernate-orm)

[Ladicek]

I don't really know how Hibernate is integrated into Quarkus, but I would naively advise against using the ArC annotation overlay, because using that would tie Hibernate's lifecycle to ArC's lifecycle to certain degree (at least on the level of build step ordering). Hibernate could have its own annotation overlay, just like RESTEasy Reactive.

[verjan-isencia]

What I actually am looking for, is a way to decorate the JPA entities at build-time, similar to what ArC allows through their annotation overlay. Whether it's a good idea or not to reuse the same overlay is only a secondary implementation decision. I would be inclined to say it is: as I understand it, this is an early step in the build process, where the jandex metadata is manipulated before consumers of these metadata start using it.

If reusing the same overlay is not acceptable, I would suggest to allow to add a similar overlay for Hibernate, as it seems to me as a powerful way to customize the orm metadata.

[yrodiere]

IMO there are two ways to look at this:

1. This annotation transformation stuff could make sense, but should be completely generic and allow transforming any annotation before feeding them to any extension in Quarkus (e.g. through bytecode transformation). It has no business involving Arc or the Hibernate ORM extension, and doing so would be a maintenance nightmare.
2. The problems you're mentioning could be solved directly in Hibernate ORM instead of introducing a whole new feature simply for the purpose of writing workarounds.

With that in mind, I'd suggest the following ways forward:

1. Discuss some generic way of transforming annotations in Quarkus with the relevant people. I don't know who that would be, but @geoand might know. @FroMage's recent work on annotation processors may or may not be relevant.
2. Discuss some of your problems with the Hibernate team. For example the following features feel very reasonable to add directly to Hibernate ORM:
   • Ability to take advantage of @TemporalUnit(SECONDS) in custom types
   • Ability to see soft-deleted entities in a particular session

[verjan-isencia]

1. I agree that a generic way of transforming annotations is the right way to go: I was surprised in the first place to see 2 completely parallel implementation for Arc and for RestEasy (buildItem, transformer, transformercontext,...). I think bean decoration at build time is very useful, independent of the consumer of these decorations.
2. the example of the @TemporalUnit annotation for Duration conversion is just one use-case of many, where a CustomType could be parameterized through custom annotations, and I am definitely not the only one that has problems with the new @SoftDelete implementation because of the inability to view deleted entities in many-to-one associations. But I think that should be discussed in a different issue.

[geoand]

For 1, @FroMage and I have have advocated for having an overlay for Jandex in Smallrye :).

I don't think it should be the same overlay for all extensions, just the same API

[yrodiere]

But I think that should be discussed in a different issue

Yep, I was suggesting to open feature requests to the Hibernate ORM project directly, or continue the discussion on existing ones https://hibernate.atlassian.net/browse/HHH

[yrodiere]

I don't think it should be the same overlay for all extensions, just the same API

Feel free to try, but Hibernate ORM itself (not the Quarkus extension) doesn't rely on Jandex for annotation processing, so... short of bytecode transformation (which necessarily applies to every extension), I don't think you'll get anywhere.

There is work in progress to make Hibernate ORM leverage Jandex, but that'll probably only land in ORM 7.0, and I suspect full support will take even more time (due to legacy APIs/SPIs that require exposing Class/Annotation types).

[Ladicek]

Even if Jandex contained an implementation of the annotation overlay (I still intend to do this, but it's been low priority), the important question is: should there be a single instance shared across all Quarkus extensions, or should each extension have its own instance of the overlay? There are good arguments for both answers.

[geoand]

or should each extension have its own instance of the overlay? There are good arguments for both answers.

Yeah, I don't have a good answer... My feeling is that they should be different in the interest of keeping backward compatibility (which theoretically could break if extensions start meddling with a shared index)

[mkouba]

Practically speaking, there is one annoying issue with annotation overlays - transformations ordering. Ideally, transformations should be run in deterministic order and transformation authors should be able to express that their transformations should be run before/after others (which is easier said than done). For example, AnnotationsTransformer from ArC has the concept of priority but that's not ideal. So if it's global/shared then we could expect more problems like that because more transformations can step on each other's feet.

That said, a global overlay would be probably more correct and efficient 🤷.

[FroMage]

Every ordering problem has been solved with @Priority 😂 If it's good enough for them, it's good enough for us ;)

And I'm strongly in favour of having a single overlay for every extension, because I've had to add overlays for Arc and Hibernate Reactive at the same time, and that's confusing as hell.

Also, for good or evil, some extensions look at annotations from other extensions, so this would avoid surprises where one extension would not see the overlaid annotation from the other extension.

And last, I've seen many bugs where parts of an extension was using the overlay API and other parts using the Jandex API, and I really think the overlay should not be a different API to Jandex's main API.

But those are all points that belong in a different issue discussing the Jandex overlay API, probably not this one :)

[mkouba]

Every ordering problem has been solved with @Priority 😂 If it's good enough for them, it's good enough for us ;)

Hehe, a good one :D

And I'm strongly in favour of having a single overlay for every extension,

Just to be clear - you're in favor of a "global" overlay shared by all extensions, right? Your wording can be IMO interpreted in different ways...

because I've had to add overlays for Arc and Hibernate Reactive at the same time, and that's confusing as hell.

Hibernate Reactive has an overlay?

Also, for good or evil, some extensions look at annotations from other extensions, so this would avoid surprises where one extension would not see the overlaid annotation from the other extension.

That's a good point.

And last, I've seen many bugs where parts of an extension was using the overlay API and other parts using the Jandex API, and I really think the overlay should not be a different API to Jandex's main API.

Sounds good but might be tricky in terms of implementation. Furthermore, there are cases where an extension needs to obtain the original metadata and not the transformed ones. Even the transformers sometimes need to access the original index. In other words, you will still need the original Jandex index around and so this kind of issue is not going to disappear. At best there will be two Jandex indexes - original and transfomed (overlay).

But those are all points that belong in a different issue discussing the Jandex overlay API, probably not this one :)

💯

[geoand]

Not without a massive breaking change

That's also the reason I disagree with @mkouba and @FroMage and think we should not do it

[mkouba]

If anyone wants to discuss annotation overlay, here's a good place: smallrye/jandex#255

👍

Not without a massive breaking change

That's also the reason I disagree with @mkouba and @FroMage and think we should not do it

@geoand I'm sorry but what exactly do you disagree with? I mean I didn't say we should do one or the other ;-)

[verjan-isencia]

I have checked smallrye/jandex#255 and smallrye/jandex#117: the ideas of having some global overlay for annotations in Jandex, and to use Jandex for Hibernate have been hovering around for some time already.

I fit into the profile of those framework builders that have their own annotations and want to (re)decorate beans/entities during build-time. For CDI and for RestEasy, that is already possible now in Quarkus, but each using its own overlay. And my original question in this issue was if something similar would be possible for Hibernate.

As I understand so far, that would require:

1. A global overlay for annotations, ideally part of Jandex itself. That would replace the Arc and RestEasy AnnotationStores.
2. Hibernate using that overlay to get metadata from annotations. That would be prior to any XML override logic.

It is a lot to ask, and won't be possible short-term, I know. But my suggestion is to at least think in that direction.

[FroMage]

Just to be clear - you're in favor of a "global" overlay shared by all extensions, right? Your wording can be IMO interpreted in different ways...

Yes

Hibernate Reactive has an overlay?

Sorry, I meant RESTEasy Reactive.

Furthermore, there are cases where an extension needs to obtain the original metadata and not the transformed ones. Even the transformers sometimes need to access the original index.

Really? In which cases? I can't think of one, but I'm probably overlooking something.

[mkouba]

Even the transformers sometimes need to access the original index.

Really? In which cases? I can't think of one, but I'm probably overlooking something.

For example, here we need to analyze the class hierarchy to find out if there is an injection point or so: https://github.com/quarkusio/quarkus/blob/main/extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/AutoAddScopeProcessor.java#L77-L79. It's not 100% correct but it's a good enough approximation 🤷.

[FroMage]

Well this looks like the overlay doesn't have the API to look up that info, so you turn to the index. But if the index had the annotation overlay inside, you wouldn't need two APIs, no?

Unless again, I'm missing the point of why would anyone want to look for non-overlayed info.

[mkouba]

Well this looks like the overlay doesn't have the API to look up that info, so you turn to the index. But if the index had the annotation overlay inside, you wouldn't need two APIs, no?

The problem is that you need to build/initialize the overlay first. Imagine that your transformer (or whatever we call it) transforms annotations of a class and at the same time you need to inspect a class hierarchy for annotations inside the transformer (which is what we do in the example above)... and boom, a cycle/chicken-egg problem is here.

[FroMage]

Oh, I see. But that's an API issue. An overlay has to be able to indeed build upon the previous index in order to compose on top of it.

I thought there was a use-case for extensions to access the non-overlaid index, which is different, IMO.

[gsmet]

I thought there was a use-case for extensions to access the non-overlaid index, which is different, IMO.

I actually used this property here: https://quarkus.io/blog/solving-problems-with-extensions-2/#annotationtransformers-to-the-rescue .

Agreed it's an ugly workaround but I would have been stuck otherwise.

[FroMage]

we could hide the annotations from ArC using an annotations transformer while keeping them available for Airline to consume them via reflection

If this is the goal, this does not conflict with what we're saying here. Only an annotation transformer would need access to the non-overlaid index, and every index user would see the overlaid index.

Unless you're saying you need to hide the annotation from the Arc extension, but keep it visible to other extensions? Not talking about reflection and modification of bytecode here. Just how other extensions see it.