unifying generators (id, version, value generation)

[gavinking]

Perhaps I'm wrong, or perhaps it's simply not worth the effort, but I feel like VersionJavaType is a bit of a hangover from ancient days, and that there should be some way to unify version generation with the whole ValueGeneration infrastructure.

In particular, the ugly @Source annotation, and the DbTimestampType (which is deprecated, but still underlies @Source(DB)) stick out like sore thumbs.

@beikov wrote in DbTimestampType that we should:

Use {@link org.hibernate.tuple.ValueGeneration} instead

but as far as I can tell it's not actually possible at present to use ValueGeneration to implement @Source.

Nor can I see any way to achieve the effect of @Source using @Generated or @GeneratedColumn.

Am I missing something there? I would love to deprecate @Source or reimplement it as a @ValueGenerationType.

Thoughts?

[beikov]

Using the annotation @CurrentTimestamp which uses CurrentTimestampGeneration would be the "equivalent" in the ValueGeneration realm. AFAIR this can only be applied through annotations though. So to support the special named type dbtimestamp, I assume we'd have to special case this type name somewhere in a binder and use the timestamp type instead + apply the CurrentTimestampGeneration.

[gavinking]

Using the annotation @CurrentTimestamp which uses CurrentTimestampGeneration would be the "equivalent" in the ValueGeneration realm.

Right, what I'm suggesting it to integrate ValueGeneration with the optimistic locking stuff. Currently you can't use a value generated via ValueGeneration to do optimistic locking.

[beikov]

The VersionJavaType stuff is what I introduced as replacement for VersionType. This is used for optimistic version generation. The logic was only dependent on the java type and since we wanted to get rid of the special BasicType implementations anyway, the move to the JavaType hierarchy for this was natural.

[gavinking]

Sure, and that's all perfectly reasonable. The problem long predates the migration of VersionType to VersionJavaType.

[gavinking]

So after today's work, this should now be straightforward. We can make the move anytime we like, since all the machinery is there.

[gavinking]

A remaining task is to fix StandardTableExporter so that it can correctly detect and export identity columns, wherever they occur in the mappings. Currently there is a badly-hardcoded special case for a single-column PK. It should be up to the Column to know it is an IDENTITY column.

[gavinking]

Another remaining task is: investigate whether IdentityColumnSupport.appendIdentitySelectToInsert(String identityColumnName, String insertString) could be generalized to accept an array of column names. Looks like we could definitely do this on h2, not sure about any other database.

[gavinking]

Looks like we could definitely do this on h2, not sure about any other database.

Well, on postgres it's returning, I knew that. And it looks like it's output on SQL Server?

[sebersole]

2. SelectGenerator isn't a real generator, it doesn't generate the value differently to IdentityGenerator

My point earlier was that really post-insert generators are not generators at all

[gavinking]

Yes, right, they weren't. Well, they are now :)

[gavinking]

So now I've arrived at an interesting question. I've reached the point where an "id generator" is just an InMemoryGenerator with GenerationTiming.INSERT.

So then what precisely is the difference between an @IdGeneratorType annotation and a @ValueGenerationType annotation? In principle we really don't need both of them, and in what I pushed yesterday, I let you use a @ValueGenerationType to generate ids.

But, since we already have both these annotations, and because I'm disinclined to deprecate the one we only just added in 6.0, probably this was a mistake. If you want to use an InMemoryGenerator for both things, it's easy enough to define an annotation with is both an @IdGeneratorType and a @ValueGenerationType. So I think that's the path I'm going to take for now. That is: id generators must be annotated @IdGeneratorType, and other field generators must be annotated @ValueGenerationType, just like in 6.0.

Thoughts?

[beikov]

One thing I'd say that makes id generators different is that they might require a sequence or table i.e. there is some "Exportable" thing related to it.

[gavinking]

Yes but in principle so could any other generator ... that we don't currently support that is a (fixable) limitation of the old ValueGeneration stuff.

[gavinking]

(Indeed, I want to see it fixed.)

[sebersole]

They might also want an exportable for non-id generation. This unification ultimately lets us address that

[sebersole]

I.e., this might be cool:

@Entity 
class ... {
    @Id private Integer id;

    @Basic
    @Generated( strategy = TriggerGenerator.class, ... )
    @TriggerGenerator( ... )
    private String status;
}

[gavinking]

So I'm now primarily worried about package organization. Related issue is this one: https://hibernate.atlassian.net/browse/HHH-15800

Basically, I want to move Generator and friends out of org.hibernate.tuple, partly because that package is sort of a mess. I've been through a bunch of options in my head, trying to find a way to do this without breaking. It's very tricky, basically due to the GenerationTime(ing) enums, but what I've finally come up with is this:

1. Remove getGenerationTiming() from Generator (it would remain on the now-legacy ValueGeneration) and replace it with two booleans. This makes perfect sense on its own; it was always a bit weird to have an enum for the type (boolean,boolean).
2. Move the new Generator interfaces to the new package org.hibernate.generator. I think this is important to do.
3. Leave ValueGeneration, GenerationTiming and friends where they are, in org.hibernate.tuple to retain compatibility, though they would now be in a semi-deprecated state.
4. Move concrete implementations of Generator to org.hibernate.generator.internal. (We could leave inheriting stubs in org.hibernate.tuple if you guys think that's important enough to bother with.)
5. Make a final decision as to whether @Generated and @CurrentTimestamp should be using annotations.GenerationTime or tuple.GenerationTiming. I had previously decided to standardize them on tuple.GenerationTiming, but with this proposal I guess it makes much more sense to reverse this decision, since the whole tuple package doesn't have a big future. The thing making this uncomfortable is that we introduced @CurrentTimestamp in 6.0, with timing given by a tuple.GenerationTiming. What I'm thinking now is: can we just call this a "bug" and fix it? I judge the potential breakiness to be quite small. People who have already migrated to 6 probably have not found this annotation yet.

WDYT?

[sebersole]

1. Remove getGenerationTiming() from Generator (it would remain on the now-legacy ValueGeneration) and replace it with two booleans. This makes perfect sense on its own; it was always a bit weird to have an enum for the type (boolean,boolean).

We'll agree to disagree ;) To me, modeling 3 potential outcomes as 4 options is weird

[sebersole]

Overall I agree about the tuple package.

I've been wanting to tackle that for 6.2, but to me that also includes cleaning org.hibernate.persister.walking. I got most of org.hibernate.persister.walking done, but the one remaining contract gets deeply tangled with the tuple package.

It also, again very unfortunately, gets tangled up with PropertyMapping which is another thing I want to get rid of (pretty sure we no longer use that in any meaningful way).

At a high-level, that was all a preliminary attempt at what has become the mapping model but twisted to fit into pre-6.0 codebase.

[gavinking]

We'll agree to disagree ;) To me, modeling 3 potential outcomes as 4 options is weird

Well I added support UPDATE to the enums in HHH-15672. https://hibernate.atlassian.net/browse/HHH-15672

I mean, I agree that generation that happens only on UPDATE is less common than the other options, but there's nothing conceptually wrong wit the idea, it's very easy to support, and surely someone, someday will find it useful.

[gavinking]

It also, again very unfortunately, gets tangled up with PropertyMapping which is another thing I want to get rid of (pretty sure we no longer use that in any meaningful way).

Yeah, looks 'bout obsolete to me.

[sebersole]

We'll agree to disagree ;) To me, modeling 3 potential outcomes as 4 options is weird

Well I added support UPDATE to the enums in HHH-15672. https://hibernate.atlassian.net/browse/HHH-15672

I mean, I agree that generation that happens only on UPDATE is less common than the other options, but there's nothing conceptually wrong wit the idea, it's very easy to support, and surely someone, someday will find it useful.

Yes, you added that VERY recently. It is so uncommon that no one has asked for that ever as far as I know.

But fine. You are correct that it is easy to support, usefulness and need aside.