[hibernate-dev] HHH-8999/HHH-10413 and Comparable IDs

Gail Badner gbadner at redhat.com
Mon Mar 14 03:38:14 EDT 2016 

In addition to affecting entity IDs [1], this also affects collection keys
[2].

I've also reproduced a similar problem when an implementation of UserType
in a composite ID or collection key will fail, unless the implementation
extends Comparator. This is because CustomType#getComparator returns
(Comparator) userType [3]; UserType and EnhancedUserType do not implement
Comparator.

IMO, we should not add a requirement that classes used for an ID or
collection key must implement Comparable. I'm also not crazy about using
IncomparableComparator for ID types that don't implement Comparable.

My preference would be for EntityAction#compareTo only compare the IDs if:

Comparable.class.isAssignableFrom(
    persister.getIdentifierType().getReturnedClass()
)

Similarly for CollectionAction#compareTo, the collection keys would only be
compared if:

Comparable.class.isAssignableFrom(
    persister.getKeyType().getReturnedClass()
)

Can the Javadoc for Type#compare be changed to say that Type#compare should
only be called if Comparable.class.isAssignableFrom(
getReturnedClass() )? The ways that Hibernate uses Type#compare is very
limited, I *think* this should be OK.

A comparator for VersionType is important when using the 2nd-level cache.

The Javadoc for org.hibernate.cache.spi.CacheDataDescription#isVersioned
says:

"If {@code true}, it is illegal for {@link #getVersionComparator} to return
{@code null}."  [4]

Since byte[] is used for versions, then BinaryType#getComparator needs to
return a non-null Comparator that actually compares the values (not
IncomparableComparator).

IMO, it would be a little strange for BinaryType#compare to throw an
exception but have BinaryType#getComparator return a non-null Comparator
that actually compares the values. I could be convinced it's OK though.

FWIW, an advantage of having BinaryType#getComparator return the Comparator
direction (instead of delegating to PrimitiveByteArrayTypeDescriptor) is
that the other types that use PrimitiveByteArrayTypeDescriptor, ImageType
and MaterializedBlobType, will not have a Comparator defined. I think that
makes sense since ImageType and MaterializedBlobType really should not be
compared.

Please let me know what you think about all this.

Unfortunately it looks like this fix will not make it into 5.0.9.

[1]
https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/action/internal/EntityAction.java#L154
[2]
https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/action/internal/CollectionAction.java#L159
[3]
https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/type/CustomType.java#L182
[4]
https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/cache/spi/CacheDataDescription.java#L29

On Fri, Mar 11, 2016 at 11:47 AM, Gail Badner <gbadner at redhat.com> wrote:

> Hibernate supports byte[] versions. You can see that org.hibernate.type.BinaryType implements VersionType<byte[]>. [1]
>
> The comment in BinaryType#seed says:
> 		// Note : simply returns null for seed() and next() as the only known
> 		// 		application of binary types for versioning is for use with the
> 		// 		TIMESTAMP datatype supported by Sybase and SQL Server, which
> 		// 		are completely db-generated values...
>
> There is also a unit test, org.hibernate.test.version.sybase.SybaseTimestampVersioningTest that maps:
>
> <version name="timestamp" type="binary" generated="always">
>     <column name="ts" sql-type="timestamp"/>
> </version>
>
> [1] https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/type/BinaryType.java
>
>
> On Fri, Mar 11, 2016 at 7:36 AM, Emmanuel Bernard <emmanuel at hibernate.org>
> wrote:
>
>> Version can be a byte[] ? I thought JPA was restricting version types
>> actually.
>>
>> On Thu 2016-03-10 12:19, Gail Badner wrote:
>> > As I mentioned before, it is not acceptable for the comparator for
>> > PrimitiveByteArrayTypeDescripter to be IncomparableComparator because a
>> > version attribute can be of type byte[]. We definitely do not want all
>> > byte[] version values to compare as equal. A Comparator needs to be
>> > implemented at least for PrimitiveByteArrayTypeDescripter.
>> >
>> > I'm OK with changing getComparator() for
>> PrimitiveCharacterArrayTypeDescriptor,
>> > ByteArrayTypeDescriptor, and
>> > CharacterArrayTypeDescriptor to return IncomparableComparator.INSTANCE.
>> I
>> > think that is a safer option than B) (making
>> > IncomparableComparator.INSTANCE the default if the type is not
>> assignable
>> > to Comparable).
>> >
>> > I'm also OK with using what I implemented in the pull request.
>> >
>> > On Thu, Mar 10, 2016 at 7:27 AM, Steve Ebersole <steve at hibernate.org>
>> wrote:
>> >
>> > > Personally I think using IncomparableComparator or null here makes the
>> > > most sense.  I'm not really sure what the sort ordering of a byte
>> array
>> > > would "mean".
>> > >
>> > > On Thu, Mar 10, 2016, 1:41 AM Gail Badner <gbadner at redhat.com> wrote:
>> > >
>> > >> I've created a pull request implements option A) (creates comparators
>> > >> for PrimitiveByteArrayTypeDescriptor,
>> > >> PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, and
>> > >> CharacterArrayTypeDescriptor.
>> > >>
>> > >> I'm still not sure if it makes sense to have a comparators for
>> arrays.
>> > >>
>> > >> Steve, please take a look at the pull request [1] and let me know if
>> this
>> > >> looks reasonable to you.
>> > >>
>> > >> Thanks,
>> > >> Gail
>> > >>
>> > >> [1] https://github.com/hibernate/hibernate-orm/pull/1294
>> > >>
>> > >> On Thu, Mar 3, 2016 at 2:44 AM, Gail Badner <gbadner at redhat.com>
>> wrote:
>> > >>
>> > >> > Missed something. Please see below...
>> > >> >
>> > >> > On Wed, Mar 2, 2016 at 11:49 PM, Gail Badner <gbadner at redhat.com>
>> > >> wrote:
>> > >> >
>> > >> >> ExecutableList#add attempts to keep track of whether the
>> Executable
>> > >> >> objects are sorted. [1]
>> > >> >>
>> > >> >> Except for entity insert actions (which use InsertActionSorter),
>> > >> >> ExecutableList#add uses the following to determine if adding the
>> > >> current
>> > >> >> Executable to the end invalidates the sort:
>> > >> >>
>> > >> >> if ( previousLast != null && previousLast.compareTo( executable )
>> > 0
>> > >> ) {
>> > >> >>     sorted = false;
>> > >> >> }
>> > >> >>
>> > >> >> EntityAction#compareTo compares the IDs for the current and
>> previous
>> > >> >> EntityAction if they have different entity names; similarly,
>> > >> >> CollectionAction compares the collection keys for the current and
>> > >> previous
>> > >> >> CollectionAction if they have different entity names.
>> > >> >>
>> > >> >> In most cases, the ID class implements Comparable, although I
>> don't
>> > >> know
>> > >> >> of any requirement that says this needs to be true.
>> > >> >>
>> > >> >> This breaks down when a byte[] ID is used as described by
>> HHH-8999. The
>> > >> >> reason is that AbstractTypeDescriptor#comparator is null because
>> it is
>> > >> >> assigned like this:
>> > >> >>
>> > >> >> this.comparator = Comparable.class.isAssignableFrom( type )
>> > >> >>         ? (Comparator<T>) ComparableComparator.INSTANCE
>> > >> >>         : null;
>> > >> >>
>> > >> >> PrimitiveByteArrayTypeDescriptor does not override
>> > >> >> AbstractTypeDescriptor#getComparator, so null is returned
>> ultimately
>> > >> >> causing a NullPointerException in
>> AbstractStandardBasicType#compare:
>> > >> >>
>> > >> >> public final int compare(Object x, Object y) {
>> > >> >>     return javaTypeDescriptor.getComparator().compare( (T) x, (T)
>> y );
>> > >> >> }
>> > >> >>
>> > >> >> The same happens for PrimitiveCharacterArrayTypeDescriptor,
>> > >> >> ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor. Are
>> there
>> > >> others?
>> > >> >>
>> > >> >> According to HHH-10413, this also affects version attributes.
>> > >> >>
>> > >> >> Here are a couple of alternatives:
>> > >> >>
>> > >> >> A) create comparators for PrimitiveCharacterArrayTypeDescriptor,
>> > >> >> ByteArrayTypeDescriptor, and CharacterArrayTypeDescriptor.
>> > >> >>
>> > >> >
>> > >> > PrimitiveByteArrayTypeDescript should also be in the list for A).
>> > >> >
>> > >> >
>> > >> >>
>> > >> >>
>> > >> >> B) Change AbstractTypeDescriptor#comparator to:
>> > >> >>
>> > >> >> this.comparator = Comparable.class.isAssignableFrom( type )
>> > >> >>         ? (Comparator<T>) ComparableComparator.INSTANCE
>> > >> >>         : IncomparableComparator.INSTANCE;
>> > >> >>
>> > >> >> IncomparableComparator#compare always returns 0, so that
>> comparison
>> > >> would
>> > >> >> never invalidate the sort.
>> > >> >>
>> > >> >>
>> > >> > B) is not acceptable for PrimitiveByteArrayTypeDescripter because a
>> > >> > version attribute can be of type byte[]. We definitely do not want
>> all
>> > >> > byte[] version values to compare as equal. A Comparator needs to be
>> > >> > implemented at least for PrimitiveByteArrayTypeDescripter.
>> > >> >
>> > >> > The following question only applies to
>> > >> > PrimitiveCharacterArrayTypeDescriptor, ByteArrayTypeDescriptor, and
>> > >> > CharacterArrayTypeDescriptor.
>> > >> >
>> > >> >
>> > >> >> Does it make sense to sort Executable objects by an ID or
>> collection
>> > >> key
>> > >> >> that is an array? In other words, would such IDs be generated in a
>> > >> natural
>> > >> >> order? If not, I think B) makes more sense.
>> > >> >>
>> > >> >
>> > >> >> Thoughts?
>> > >> >>
>> > >> >> Thanks,
>> > >> >> Gail
>> > >> >>
>> > >> >> [1]
>> > >> >>
>> > >>
>> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/engine/spi/ExecutableList.java#L194
>> > >> >>
>> > >> >
>> > >> >
>> > >> _______________________________________________
>> > >> hibernate-dev mailing list
>> > >> hibernate-dev at lists.jboss.org
>> > >> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> > >>
>> > >
>> > _______________________________________________
>> > hibernate-dev mailing list
>> > hibernate-dev at lists.jboss.org
>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>
>

More information about the hibernate-dev mailing list