Register arrays of Hibernate ORM's JDBC basic types for reflection

[yrodiere]

Fixes #34071

Essentially this is a workaround for https://hibernate.atlassian.net/browse/HHH-16809, which from the looks of it is not considered a priority and won't be solved anytime soon.

See also hibernate/hibernate-orm#6815 (comment)

And before anyone (i.e. @Sanne ;) ) tells me "you could have tried to detect which of those types are actually useful for a given application"... yes, I could have, but:

1. I'd need to process @Id, @IdClass/@EmbeddedId with a single column, XML mapping, ...
2. I couldn't do it with perfect accuracy, because it's the JDBC type that matters, and that's not the same as the property type. AttributeConverter being one example, but there are probably others.
3. GraalVM's "features" are not a viable option either as JDBC types are all reachable through a map (i.e. GraalVM cannot distinguish being one type being retrieved or another).
4. Frankly we're talking about just a few standard Java types (and not even those types, just their array types), so let's not bother.

[quarkus-bot]

/cc @Sanne (hibernate-orm), @gsmet (hibernate-orm)

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[gsmet]

Looks good as a workaround!

[Sanne]

Many thanks, nice to see this resolved.

For the future I'm thinking to propose an explicit SPI in ORM core: a service for ORM code itself to register elements for reflective access? It could be a no-op in regular use, but frameworks like Quarkus (and possibly others) would be able to collect types and register them on GraalVM explicitly. But I wonder if it's a good ideas, as its usefulness depends on how many of such cases we're having (certainly this case adds to an existing list), and testing for it to be thoroughly applied seems tricky - perhaps it's unnecessarily complex.

Thoughts?

[yrodiere]

I'm not sure this SPI really is necessary... Seems to me it would be exactly as effective, and perhaps more generic, to just have Hibernate ORM correctly avoid all dynamic, runtime reflection and instead store relevant Constructor/Method/Field/MethodHandle in its Metadata? Then IIUC GraalVM is perfectly able to deduce that those constructors/methods/fields will be accessed at runtime (at least in Quarkus, as they will be put on the heap during static init).

certainly this case adds to an existing list

Not sure about that. In this instance, the problem really is that Hibernate ORM does something like Arrays.newArray( <some variable containing a Class> ). So, it does dynamic, runtime reflection. If, instead, Hibernate ORM stored the array constructor somewhere in the metadata (e.g. in the BasicJavaType), or used some static constructor call (new Integer[<size>]), then we wouldn't have such a problem.

And if you think about it, changing Hibernate ORM to use the new SPI probably has very similar cost than changing Hibernate ORM to just avoid dynamic, runtime reflection.

[Sanne]

I'm not sure this SPI really is necessary... Seems to me it would be exactly as effective, and perhaps more generic, to just have Hibernate ORM correctly avoid all dynamic, runtime reflection and instead store relevant

I agree in theory, but as this case shows sometimes practical needs hit.

We had agreed with the ORM team to stick to pretty much the guidelines you're describing, but things will naturally fall through the cracks. Which is why I'm wondering if we could have integration tests coverage within ORM to flag such needs earlier - and if we had such an SPI it would be their responsibility to fix right away rather than a "problem for downstream consumers", which inevitably gets us into problems either of twisted release dances or such bugs.

[yrodiere]

and if we had such an SPI it would be their responsibility to fix right away rather than a "problem for downstream consumers",

IMO we'd just get in the same situation, just with an additional SPI adding complexity to the mix and (understandable) resentment/reluctance upstream because it creates additional work upstream for our (very) specific downstream needs.

At least resolving the Constructor/Method/etc. during metadata creation is arguably a good practice that might lead to (small) performance increases in general, even in JVM mode and with other frameworks, so it's not "just for those Quarkus folks".

I'm not saying "no, we should never do that", I just doubt it will solve the root of the problem, which is that Hibernate ORM doesn't get tested with the same constraints as Quarkus. That JVM agent you talked about some time ago to detect reflection that happens too late (after Metadata creation) would be a better angle of approach IMO. At least we'd be able to detect future problems (regressions) and have them fixed upstream as they appear, and we could add an allowlist for existing problems to work on them asynchronously, as time allows. That is, assuming the agent doesn't yield too many false-positives/false-negatives.

[Sanne]

Sure, being able to test for this correctly is key and that's what I was suggesting above as well. Without it, a new SPI won't be very useful, but if we combine them then it's a strong win.