[hibernate-dev] Oracle12cDialect identity support

[Gail Badner]

Just to bring everyone up-to-date, the following were fixed in Hibernate
ORM 5.0.7:
* HHH-10421: "native" ID generator for Oracle12cDialect changed to
SequenceStyleGenerator; this change make the "native" ID generator for
Oracle12cDialect consistent with earlier Oracle dialects;
* HHH-10422: fix identity IDs using Oracle12cDialect.

HHH-10421 includes deprecation of the following dialect methods in 5.0:
supportsIdentityColumns()
supportsInsertSelectIdentity()
hasDataTypeInIdentityColumn()
appendIdentitySelectToInsert(String insertString)
getIdentitySelectString(String table, String column, int type)
getIdentityColumnString(int type)
getIdentityInsertString()

These method are removed from master.

On Tue, Jan 5, 2016 at 6:52 PM, Gail Badner <gbadner at redhat.com> wrote:

> I was mistaken when I said that an IDENTITY ID will be created when using
> @GeneratedValue or @GeneratedValue(strategy= GenerationType.AUTO). With
> hibernate.id.new_generator_mappings set to true by default in 5.0 and 5.1,
> a SequenceStyleGenerator will be used instead.
>
> Currently, the default for "native" ID generator mapped in hbm.xml uses an
> IDENTITY if the dialect supports an IDENTITY. This will be overridden in
> Oracle12cDialect as we've been discussing.
>
> On Tue, Jan 5, 2016 at 9:46 AM, Steve Ebersole <steve at hibernate.org>
> wrote:
>
>>
>>
>> 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...
>>
>>
>
>