[hibernate-dev] Oracle12cDialect identity support

[Steve Ebersole]

On Tue, Jan 5, 2016 at 12:42 AM Gail Badner <gbadner at redhat.com> wrote:

>
>
>>  Native is a Hibernate-only concept and so we can really choose whatever
>> we want for native generators.  Also native is an outdated concept,
>> replaced by AUTO and SequenceStyleGenerator.  IMO we should never be
>> choosing IDENTITY for identifier generation unless a user explicitly asks
>> for it.
>>
>
> Just to make sure I understand, are you saying we should not be choosing
> IDENTITY by default for *new* dialects only?
>

Well in an ideal world where we can time-travel and retroactively apply our
gained 20/20 hindsight I would change that everywhere.  However we don't
have that luxury :)  So, yes, I would do that just for new Dialects.

So moving forward, any new Dialect should never chose {"native" ->
IDENTITY} and should never chose {AUTO -> IDENTITY}.



>
>
>> So for the first piece, Oracle12cDialect should
>> override getNativeIdentifierGeneratorClass to
>> return SequenceStyleGenerator.  The Dialect implementation
>> of getNativeIdentifierGeneratorClass really assumes databases which support
>> IDENTITY but not SEQUENCE.
>>
>
> Thanks for clarifying that. I was wondering about that.
>
>
>> Oracle12cDialect is the first case we have had where a database that
>> historically supported SEQUENCES has added support for IDENTITY and we just
>> did not account for that properly.  In fact, the "proper" solution would be
>> to go into the base Oracle Dialect(s) and override
>> getNativeIdentifierGeneratorClass to just always
>> return SequenceStyleGenerator.  But doing that in SequenceStyleGenerator
>> only is viable too.
>>
>
> Did you mean, "doing that in Oracle12cDialect only is viable too"?
>

Yes.  Considering I typed that on my phone I am shocked that was my only
typo :)


>
>>
>> As for how to get IDENTITY generation to work consistently with Oracle,
>> that I cannot say.  I asked Oscar (the reporter of HHH-9983) for find out
>> the way to get IDENTITY generation to work with Oracle 12c *via JDBC*.
>>  "Via JDBC" is the operative part.  All of the Oracle documentation and
>> google hits at that time only discussed using IDENTITY via command told
>> (SQLPlus, etc).  According to his findings we seem to have to correct.
>>
>
> I believe Oracle12cDialect identity support is working properly in master
> now.
>
> IIUC you are saying that the problem is porting that from 5.1 (master) to
>> 5.0?  Due to an SPI change?  What exactly is the SPI that changed?  We did
>> make lots of changes to "IdentityColumnSupport" (adding that as a
>> first-class contract), but that is really just cosmetic grouping of stuff
>> already available on Dialect.  So what change specifically stops you from
>> porting those changes to 5.0?
>>
>
> I've looked over the changes again and I think the main problem is that
> the refactoring done for HHH-10084 [1] will break custom dialects that
> override the Dialect methods that are deprecated for 5.1. The deprecated
> Dialect methods are no longer used; Hibernate obtains the identity support
> information from Dialect#getIdentityColumnSupport only.
>

Then the deprecated Dialect methods ought to be removed.  Leaving them only
causes confusion


>
> I've created a test to illustrate: [2].
>
> Custom dialects will need to be modified to override
> getIdentityColumnSupport() to work. Would this type of breaking change be
> OK for 5.1 (I'm guessing no...). IMO, it is not OK to introduce this
> breaking change in 5.0.
>

I am not sure what you are asking when you say "Would this type of breaking
change be OK for 5.1"



>
> HHH-10084 ([1]) changed the deprecated Dialect methods to delegate to the
> IdentityColumnSupport. It also extracted the overridden methods from
> Dialect subclasses into dialect-specific IdentityColumnSupport classes.
>
> If I backport HHH-9983 to 5.0, I think I would only backport part of
> HHH-10084. I would change IdentityColumnSupportImpl methods to delegate to
> the deprecated Dialect methods; I would not extract the overridden methods
> into dialect-specific IdentityColumnSupport classes. That way, custom
> dialects would not be broken.
>

This kind of multi-dispatch makes my head hurt.  And I bet users don't find
it any less confusing...