Register Hibernate GenericGenerator strategies for reflection #1767

geoand · 2019-03-29T16:23:11Z

Note: I'm not sure if where I added the reflection is the "proper" place or it should be moved to the JpaJandexScavenger class for example.

Sanne

Thanks! Could you add a test?

Sanne · 2019-03-29T17:02:11Z

...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java

+        Set<String> strategies = new HashSet<>();
+        for (AnnotationInstance annotation : index.getIndex().getAnnotations(GENERIC_GENERATOR)) {
+            String strategy = annotation.value("strategy").asString();
+            if (strategy.contains(".")) {


wondering: why checking for the dot being contained in the strategy string?

If I am not mistaken, strategy could be a simple string, but maybe I am mixing this with the strategy from GenericValue?

you're right it could be a special keyword, but also it could be a "fully qualified name" with no package. (That's far fetched but some people do it!)

We also need to deal with the opposite problem: custom identifier generators can be registered with any name, I'm not aware of a rule that prohibits these to have a "." in the name.

sorry I know that's all bad news but that's how it is :)

It might help to peek into org.hibernate.id.factory.internal.DefaultIdentifierGeneratorFactory#getIdentifierGeneratorClass.

you could also - instead of registering these for reflection - instantiate the various identifierGenerators , store them in an immutable map, and insert a custom IdentifierGeneratorFactory implementation in the ORM bootstrap registry.

Not a problem at all, I'll revisit later on tonight or over the weekend if you don't mind :)

sure, thanks a lot!

WRT the second approach IIUC I would have to change this with an Initiator that would return the custom IdentifierGeneratorFactory, correct?
I'm asking because I haven't looked at the Hibernate part of Quarkus before ( this PR is a great opportunity to do so :) ) and I don't want to stray down a completely incorrect path :P.

Sanne · 2019-03-29T17:02:42Z

...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java

+        // register the strategies of @GenericGenerator for reflection
+        Set<String> strategies = new HashSet<>();
+        for (AnnotationInstance annotation : index.getIndex().getAnnotations(GENERIC_GENERATOR)) {
+            String strategy = annotation.value("strategy").asString();


I think the asString() method could hit an NPE ?

I thought it couldn't because the strategy field is necessary for GenericGenerator, but I guess we can be defensive, it doesn't hurt :)

geoand · 2019-03-29T17:08:29Z

I will also go ahead and add a test for this

geoand · 2019-03-29T17:42:34Z

It looks like my test is not passing locally so I need to dig in more

geoand · 2019-03-29T17:59:37Z

Should be good to go now

Fixes: quarkusio#1743

geoand · 2019-03-31T17:25:28Z

@Sanne I added another commit that implements the strategy you propose - to the best of my understanding. There are probably a few things wrong since I am not really familiar with the Hibernate internals :).

Update
The native test seems to be failing, I clearly haven't understood how RecordableBootstrap and FastBootMetadataBuilder#registerIdentifierGenerators come into play here.

Sanne · 2019-04-01T10:45:06Z

thanks @geoand , I'll have a look.

geoand · 2019-04-01T10:45:38Z

Looking forward to it :)

Sanne · 2019-04-01T15:35:51Z

@geoand great work on figuring out how the ORM boostrap works 👍 And I'm sorry I didn't put more comments, there's a lot to do still :)

I found a couple little problems:

since you're re-implementing the mapping name -> class, this approach isn't allowing to use any of the embedded IdGenerators liste in org.hibernate.id.factory.internal.DefaultIdentifierGeneratorFactory#generatorStrategyToClassNameMap
as far as I can see, not dealing with generators implementing Configurable, might break some implementations
it's not applied as the "captured state" in this case needs to be folded into the Metadata instance; sorry I mentioned the service registry, but it seems like these are already initiatiated by the time the second service registry is created. Apparently listing it in PreconfiguredServiceRegistryBuilder is unnecessary.

I'll clean this up, I have a local branch already..

geoand · 2019-04-01T15:41:45Z

Thanks for the explanation @Sanne, it is highly appreciated! I am looking forward to seeing how it all comes together since it's a great opportunity to learn about Hibernate internals 😉

Sanne · 2019-04-01T21:48:53Z

closing this one, replaced by #1809

geoand force-pushed the #1743 branch from 37ff151 to 585ea17 Compare March 29, 2019 16:33

Sanne self-assigned this Mar 29, 2019

Sanne requested changes Mar 29, 2019

View reviewed changes

Sanne added the area/persistence OBSOLETE, DO NOT USE label Mar 29, 2019

geoand force-pushed the #1743 branch from 529f0a8 to e4a7e4a Compare March 29, 2019 17:59

geoand requested a review from Sanne March 29, 2019 17:59

geoand added the coding-in-progress label Mar 29, 2019

Sanne mentioned this pull request Mar 29, 2019

Support cleanup of deleted resources during live reload #1772

Closed

geoand added 3 commits March 31, 2019 15:17

Register Hibernate GenericGenerator strategies for reflection

aabe693

Fixes: quarkusio#1743

Apply review comments

68d57e0

Move IdentifierGeneration creation to static init

087ce2e

geoand force-pushed the #1743 branch from e4a7e4a to 087ce2e Compare March 31, 2019 17:23

Sanne mentioned this pull request Apr 1, 2019

IdGeneration strategies initialized as part of the pre-recorded Metadata #1809

Merged

Sanne closed this Apr 1, 2019

Sanne removed coding-in-progress area/persistence OBSOLETE, DO NOT USE labels Apr 1, 2019

geoand deleted the #1743 branch April 2, 2019 07:51

gsmet added the triage/duplicate This issue or pull request already exists label Apr 2, 2019

Register Hibernate GenericGenerator strategies for reflection #1767

Register Hibernate GenericGenerator strategies for reflection #1767

Uh oh!

Conversation

geoand commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sanne left a comment

Choose a reason for hiding this comment

Uh oh!

Sanne Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

geoand Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sanne Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

geoand Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

Sanne Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

geoand Mar 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sanne Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

geoand Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoand commented Mar 29, 2019

Uh oh!

geoand commented Mar 29, 2019

Uh oh!

geoand commented Mar 29, 2019

Uh oh!

geoand commented Mar 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sanne commented Apr 1, 2019

Uh oh!

geoand commented Apr 1, 2019

Uh oh!

Sanne commented Apr 1, 2019

Uh oh!

geoand commented Apr 1, 2019

Uh oh!

Sanne commented Apr 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

geoand commented Mar 29, 2019 •

edited

Loading

geoand Mar 29, 2019 •

edited

Loading

geoand Mar 31, 2019 •

edited

Loading

geoand Mar 29, 2019 •

edited

Loading

geoand commented Mar 31, 2019 •

edited

Loading