Creating IdentifierGenerator instances - timing

[sebersole]

For a number of reasons I am working on making a single point of entry ("eye of the needle" as my mentor used to say) for the creation of IdentifierGenerators. Currently that creation can happen multiple times, sometimes even expecting failures and ignoring them expecting that a "later attempt" will succeed.

This causes me some difficulty working on HHH-14506, #3823 and friends.

The "expecting failures and ignoring them" happens during the building of MetadataImpl. Building the "boot model" reference is supposed to not attempt reflection at all. However, in existing code (5.x and earlier), the very last "phase" in building the Metadata is attempting to create these generators which often needs access to the Java type - is it UUID? Integer? String? ... This often fails and the code block in question uses try/catch and ignores any errors because the same exact thing (sans ignoring exceptions) happens in the SessionFactory constructor later on.

My initial attempts to create the generators at a single point focused on the point in building Metadata, but this time with no try/catch/ignore handling. This worked great, but causes one test (org.hibernate.orm.test.legacy.NonReflectiveBinderTest) to fail because the test explicitly tries to make sure that no reflection happens building the Metadata (it tries to map non-existent classes).

But interestingly enough, the existing try/catch/ignore approach expects all Exportable references (Sequence or Table) needed for the generator get registered during this try/catch/ignore processing. So it expects certain cases can (and do) fail and be caught and ignored. So in trying to keep the same semantic, I next tried to move to this creation phase to the SessionFactoryBuilder handling which happens just before constructing the SessionFactory (duh). Again this works fine, but causes test failures. Specifically any test that expects the built Metadata to contain all Sequence and Table, including those for identifier) to be registered already, fail. They fail specifically because they only build a Metadata reference and expect the id-gen Exportables to be registered

In my opinion this is a logical inconsistency and either one case or the other should "win". Personally I vote that we handle this id-gen creation as part of building the Metadata. I think that limiting reflection is perfect while building the actual mapping classes (PersistentClass, etc); but I think it is overkill once we get to the point of creating the actual Metadata instance in certain cases, and I think this id-gen creation is one of those.

[dreab8]

I am not against it but I'm not sure about the consequences/problems if any of using reflection while building the Metadata.

[sebersole]

Well there is a balance, which I tried to delineate but apparently did a poor job of doing...

Metadata is the thing that holds the objects like PersistentClass, Component, Value, etc - stuff from the org.hibernate.mapping package.

There are 2 distinct points: (1) creating these org.hibernate.mapping objects and then (2) moving from InFlightMetadataCollectorImpl to the "finalized" MetadataImpl

With regard to (1), we for sure want to continue to make sure that reflection is not triggered as part of building the org.hibernate.mapping objects. Those are used in places (tooling mainly) where they are often built piecemeal and referenced types may not even yet exist. E.g. if you think about reverse engineering, as we create a PersistentClass from a database table that class does not yet exist so any reflection would cause an exception. So building these should not trigger reflection.

The process of InFlightMetadataCollectorImpl -> MetadataImpl (2) is a finalization of the boot/mapping model. At that point we expect everything to be fully configured and ready to use. To me it is completely reasonable to allow reflection at that point.

Either way, my point is simply that we have a mismatch and that to me the hacky approach of "let's just try to do something (configure generators to register database objects) that may trigger reflection and catch-ignore reflection failures and retry later" is simply that - hacky.

As I said, the code does this process twice

1. on this InFlightMetadataCollectorImpl -> MetadataImpl transition with try-catch-ignore
2. in SF ctor presumably to process any generators that failed during (1)

Many tests fully expect to be able to find them there after building just the MetadataImpl, so clearly the intention with (1) is that all id-related database objects get registered with the MetadataImpl. Though there are clearly cases where they won't be (or else there is no need for the try-catch-ignore and then retry in SF ctor).

My point is simply that this should be consistent. The expectation should be either :

1. MetadataImpl contains all of the database objects (including those for id-generators)
2. MetadataImpl does not contain database objects pertaining to id-generators (with the expectation that those are only available after building the SF

[sebersole]

Not to mention, we have this try-catch-ignore for a reason - some of those do fail. In the tests I was working on, composite ids trigger this to fail (and get caught and ignored). So at the very least we will be missing database objects related with partially generated composite ids.

So if we aren't getting all generator database objects, then that is inconsistent

[dreab8]

Thanks @sebersole for the great explanation, I also prefer to handle the id-gen creation as part of building the Metadata and therefore using reflection in the process InFlightMetadataCollectorImpl -> MetadataImpl.

[sebersole]

Ok. In the absence of feedback I am going to have the transition from InFlightMetadataCollectorImpl -> MetadataImpl to be the threshold for allowing reflection