@GeneratorType annotation

[gavinking]

Steve and I just discussed the question of configuring custom id generators, and we've converged on something I think works pretty well.

The problem we're trying to solve is the following:

• Using a custom id generator specified with@GenericGenerator involves two levels of stringly-typed indirection.
• Configuration of a custom id generator via @Parameter is even less typesafe.

And we're making the assumption that most of the time we don't really need to share a single id generator between multiple entities, and it's just as good for the id generator to belong to a single entity.

The idea is to add a meta-annotation, @GeneratorType or whatever, that identifies an annotation type as specifying configuration properties for a custom id generator. For example:

@GeneratorType(implementation = FibonacciGenerator.class)
@Target({METHOD, FIELD})
@Retention(RUNTIME)
public @interface Fibonacci {
    public int first() default 1;
    public int second() default 1;
}

The custom id generator class consumes this annotation type in its constructor, something like:

class FibonacciGenerator implements IdentifierGenerator {
    int penultimate;
    int ultimate;
    FibonacciGenerator(Fibonacci config) {
        penultimate = config.first();
        ultimate = config.second();
    }
    Object generate(SharedSessionContractImplementor session, Object object) {
        int next = ultimate + penultimate;
        penultimate = ultimate;
        ultimate = next;
        return next;
    }
}

Now, to make use of the custom id generator, I just annotate my @Id property:

@Id @Fibonacci(first=2, second=2) int id;

Note that all of the above is just to illustrate the ideas, and the details of this are still up for grabs.

[gavinking]

Ooops, I just noticed that what I've written up here is pretty much isomorphic to the already-existing @ValueGenerationType annotation and AnnotationValueGeneration interface.

So we should look for a way to unify these things. This isn't quite trivial given the structure of the current code, but it looks conceptually possible. I mean, ValueGenerator and IdentifierGenerator really aren't that different.

I'm not saying we should get rid of the distinction between a ValueGenerator and an IdentifierGenerator, but I think there's got to be a way to refactor this so that an instance of ValueGeneration can be used to set up an IdentifierGenerator.

@sebersole WDYT?

[gavinking]

Well no, what I'm trying to say—not very clearly precisely because I don't know exactly what form this would take—is that I would like there to be something, some interface type, that's common between a value generator and an id generator that @ValueGenerationType.generatedBy could point to that type.

I know that with the way things are currently structured it's not easy to see what that thing would be. But since you're already touching that part of the design of id generators, perhaps there's some way we could make it work.

[sebersole]

As I keep saying, yes there is a fair amount of overlap here; but the differences are real.

Plus, the other thing I would like to get into @IdGeneratorType is the ability to split the generator from the determination of any needed database structure (table, sequence, trigger, etc). This is important for CDI integration and is yet another deviation that just does not make sense to unify.

So let's keep thinking about it - but at the moment I am not seeing a good solution that makes me want to do this change

[sebersole]

Stepping back, I'd state my concern like this...

I think (Annotation)ValueGeneration, as is, is not a great contract for describing the id-generation case, not to mention that it is still injection-based rather than constructor-injection. Which is a concern here for me. I'm just not seeing the argument for changing an existing API just so we can unify it with a new feature. In a white-room would we? Sure, it would make sense.

Thinking about this strictly from the id-gen side, specifically thinking about the CDI case, I started down the path of this:

interface IdGeneratorDescriptor<G extends IdentifierGenerator, C extends Annotation> {
    <G extends IdentifierGenerator> createGenerator(
            C config,
            Member annotatedMember, 
            CustomIdGeneratorCreationContext creationContext);

    default void exportDatabaseObjects(Database database, ...) {
        // nothing to do by default
    }
}

And started thinking that the #exportDatabaseObjects could be useful for ValueGeneration as well. So maybe if we start layering such additional capabilities on top of (Annotation)ValueGeneration I'd be a little more willing to consider more drastic changes to it

Considering CustomIdGeneratorCreationContext exposes the Database reference, IdGeneratorDescriptor could actually register its database objects immediately upon creation. Instead, we'd have this:

interface IdGeneratorDescriptor<G extends IdentifierGenerator, C extends Annotation> {
    <G extends IdentifierGenerator> createGenerator();
}

With the stated assumption that IdGeneratorDescriptor implementations define a constructor with the signature { C, Member, CustomIdGeneratorCreationContext }

[sebersole]

@hibernate/orm-dev What do y'all think about changing (Annotation)ValueGeneration at this point?

Yes, 6.0 is a major change. But we don't have to do around changing everything just to change it. Does this change (however it looks) make sense?

[sebersole]

BTW, I've been using Member here mainly to give access to (1) property Java type and (2) ability to locate "supporting annotations". It might be better though to instead use HCANN refs like XProperty (ugh) or the KeyValue itself. Considering these are custom generators (at least at the moment), Member is probably fine.

[gavinking]

FTR, @sebersole has already implemented this proposal here:

#4429

[beikov]

So this can be closed?