Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -663,14 +663,6 @@ else if ( comparatorSort != null ) {
}
}

if ( isSortedCollection ) {
if ( ! hadExplicitSort && !hadOrderBy ) {
throw new AnnotationException(
"A sorted collection must define an ordering or sorting : " + safeCollectionRole()
);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually compared to explicit @SortNatural case and found everything is the same(only hadExplicitSort values are different but it is used internally in this method and its main usage is the above code snippet), so removing the above code snippet seems enough to achieve the goal of this PR, unless I were wrong.

collection.setSorted( isSortedCollection || hadExplicitSort );

if ( comparatorClass != null ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,15 @@ public void createData(SessionFactoryScope scope) {
entityContainingMaps.addBasicByBasic( "someKey", "someValue" );
entityContainingMaps.addBasicByBasic( "anotherKey", "anotherValue" );

entityContainingMaps.addSortedBasicByBasic( "key1", "value1" );
entityContainingMaps.addSortedBasicByBasic( "key2", "value2" );
entityContainingMaps.addSortedBasicByBasic( "key1", "value1" );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to insert testing data with the natural order. Ideally we are supposed to populate big number of data to ensure the order is not maintained not by accident.


entityContainingMaps.addSortedBasicByBasicWithComparator( "kEy1", "value1" );
entityContainingMaps.addSortedBasicByBasicWithComparator( "KeY2", "value2" );

entityContainingMaps.addSortedBasicByBasicWithSortNaturalByDefault( "key2", "value2" );
entityContainingMaps.addSortedBasicByBasicWithSortNaturalByDefault( "key1", "value1" );

entityContainingMaps.addBasicByEnum( EnumValue.ONE, "one" );
entityContainingMaps.addBasicByEnum( EnumValue.TWO, "two" );

Expand Down Expand Up @@ -164,6 +167,30 @@ public void testSortedMapWithComparatorAccess(SessionFactoryScope scope) {
);
}

@Test
public void testSortedMapWithSortNaturalByDefaultAccess(SessionFactoryScope scope) {
scope.inTransaction(
session -> {
final EntityOfMaps entity = session.get( EntityOfMaps.class, 1 );
assertThat( entity.getSortedBasicByBasicWithSortNaturalByDefault(), InitializationCheckMatcher.isNotInitialized() );

// trigger the init
Hibernate.initialize( entity.getSortedBasicByBasicWithSortNaturalByDefault() );
assertThat( entity.getSortedBasicByBasicWithSortNaturalByDefault(), InitializationCheckMatcher.isInitialized() );
assertThat( entity.getSortedBasicByBasicWithSortNaturalByDefault().size(), is( 2 ) );
assertThat( entity.getBasicByEnum(), InitializationCheckMatcher.isNotInitialized() );

final Iterator<Map.Entry<String, String>> iterator = entity.getSortedBasicByBasicWithSortNaturalByDefault().entrySet().iterator();
final Map.Entry<String, String> first = iterator.next();
final Map.Entry<String, String> second = iterator.next();
assertThat( first.getKey(), is( "key1" ) );
assertThat( first.getValue(), is( "value1" ) );
assertThat( second.getKey(), is( "key2" ) );
assertThat( second.getValue(), is( "value2" ) );
}
);
}

@Test
public void testOrderedMap(SessionFactoryScope scope) {
// atm we can only check the fragment translation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,17 @@ public void testSets(SessionFactoryScope scope) {
final MappingMetamodel domainModel = scope.getSessionFactory().getDomainModel();
final EntityMappingType containerEntityDescriptor = domainModel.getEntityDescriptor( EntityOfSets.class );

assertThat( containerEntityDescriptor.getNumberOfAttributeMappings(), is( 11 ) );
assertThat( containerEntityDescriptor.getNumberOfAttributeMappings(), is( 12 ) );

final AttributeMapping setOfBasics = containerEntityDescriptor.findAttributeMapping( "setOfBasics" );
assertThat( setOfBasics, notNullValue() );

final AttributeMapping sortedSetOfBasics = containerEntityDescriptor.findAttributeMapping( "sortedSetOfBasics" );
assertThat( sortedSetOfBasics, notNullValue() );

final AttributeMapping sortedSetOfBasicsWithSortNaturalByDefault = containerEntityDescriptor.findAttributeMapping( "sortedSetOfBasicsWithSortNaturalByDefault" );
assertThat( sortedSetOfBasicsWithSortNaturalByDefault, notNullValue() );

final AttributeMapping orderedSetOfBasics = containerEntityDescriptor.findAttributeMapping( "orderedSetOfBasics" );
assertThat( orderedSetOfBasics, notNullValue() );

Expand Down Expand Up @@ -113,8 +116,7 @@ public void testMaps(SessionFactoryScope scope) {
final MappingMetamodel domainModel = scope.getSessionFactory().getDomainModel();
final EntityMappingType containerEntityDescriptor = domainModel.getEntityDescriptor( EntityOfMaps.class );

// 14 for now, until entity-valued map keys is supported
assertThat( containerEntityDescriptor.getNumberOfAttributeMappings(), is( 14 ) );
assertThat( containerEntityDescriptor.getNumberOfAttributeMappings(), is( 16 ) );

final PluralAttributeMapping basicByBasic = (PluralAttributeMapping) containerEntityDescriptor.findAttributeMapping( "basicByBasic" );
assertThat( basicByBasic, notNullValue() );
Expand All @@ -126,6 +128,11 @@ public void testMaps(SessionFactoryScope scope) {
assertThat( sortedBasicByBasic.getKeyDescriptor(), notNullValue() );
assertThat( sortedBasicByBasic.getElementDescriptor(), notNullValue() );

final PluralAttributeMapping sortedBasicByBasicWithSortNaturalByDefault = (PluralAttributeMapping) containerEntityDescriptor.findAttributeMapping( "sortedBasicByBasicWithSortNaturalByDefault" );
assertThat( sortedBasicByBasicWithSortNaturalByDefault, notNullValue() );
assertThat( sortedBasicByBasicWithSortNaturalByDefault.getKeyDescriptor(), notNullValue() );
assertThat( sortedBasicByBasicWithSortNaturalByDefault.getElementDescriptor(), notNullValue() );

final PluralAttributeMapping basicByEnum = (PluralAttributeMapping) containerEntityDescriptor.findAttributeMapping( "basicByEnum" );
assertThat( basicByEnum, notNullValue() );
assertThat( basicByEnum.getKeyDescriptor(), notNullValue() );
Expand Down Expand Up @@ -166,6 +173,11 @@ public void testMaps(SessionFactoryScope scope) {
assertThat( sortedManyToManyByBasic.getKeyDescriptor(), notNullValue() );
assertThat( sortedManyToManyByBasic.getElementDescriptor(), notNullValue() );

final PluralAttributeMapping sortedManyToManyByBasicWithSortNaturalByDefault = (PluralAttributeMapping) containerEntityDescriptor.findAttributeMapping( "sortedManyToManyByBasicWithSortNaturalByDefault" );
assertThat( sortedManyToManyByBasicWithSortNaturalByDefault, notNullValue() );
assertThat( sortedManyToManyByBasicWithSortNaturalByDefault.getKeyDescriptor(), notNullValue() );
assertThat( sortedManyToManyByBasicWithSortNaturalByDefault.getElementDescriptor(), notNullValue() );

final PluralAttributeMapping componentByBasicOrdered = (PluralAttributeMapping) containerEntityDescriptor.findAttributeMapping( "componentByBasicOrdered" );
assertThat( componentByBasicOrdered, notNullValue() );
assertThat( componentByBasicOrdered.getKeyDescriptor(), notNullValue() );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@
*/
package org.hibernate.orm.test.metamodel.mapping.collections;

import java.util.Iterator;

import org.hibernate.Hibernate;
import org.hibernate.persister.collection.CollectionPersister;

import org.hibernate.testing.hamcrest.InitializationCheckMatcher;
import org.hibernate.testing.orm.domain.StandardDomainModel;
import org.hibernate.testing.orm.domain.gambit.EntityOfSets;
import org.hibernate.testing.orm.domain.gambit.EnumValue;
Expand All @@ -25,6 +28,7 @@

import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.hibernate.testing.hamcrest.CollectionMatchers.hasSize;
import static org.hibernate.testing.hamcrest.InitializationCheckMatcher.isInitialized;
import static org.hibernate.testing.hamcrest.InitializationCheckMatcher.isNotInitialized;
Expand Down Expand Up @@ -56,6 +60,9 @@ public void createData(SessionFactoryScope scope) {
entity.addSortedBasicWithComparator( "Efg" );
entity.addSortedBasicWithComparator( "aBC" );

entity.addSortedBasicWithSortNaturalByDefault( "def" );
entity.addSortedBasicWithSortNaturalByDefault( "abc" );

entity.addEnum( EnumValue.ONE );
entity.addEnum( EnumValue.TWO );

Expand Down Expand Up @@ -206,6 +213,28 @@ public void testSortedSetWithComparatorAccess(SessionFactoryScope scope) {
);
}

@Test
public void testSortedSetWithSortNaturalByDefaultAccess(SessionFactoryScope scope) {
scope.inTransaction(
session -> {
final EntityOfSets entity = session.get( EntityOfSets.class, 1 );
assertThat( entity.getSortedSetOfBasicsWithSortNaturalByDefault(), InitializationCheckMatcher.isNotInitialized() );

// trigger the init
Hibernate.initialize( entity.getSortedSetOfBasicsWithSortNaturalByDefault() );
assertThat( entity.getSortedSetOfBasicsWithSortNaturalByDefault(), InitializationCheckMatcher.isInitialized() );
assertThat( entity.getSortedSetOfBasicsWithSortNaturalByDefault().size(), is( 2 ) );
assertThat( entity.getSetOfEnums(), InitializationCheckMatcher.isNotInitialized() );

final Iterator<String> iterator = entity.getSortedSetOfBasicsWithSortNaturalByDefault().iterator();
final String first = iterator.next();
final String second = iterator.next();
assertThat( first, is( "abc" ) );
assertThat( second, is( "def" ) );
}
);
}

@Test
public void testOrderedSet(SessionFactoryScope scope) {
// atm we can only check the fragment translation
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
package org.hibernate.orm.test.metamodel.mapping.collections;

import java.util.Arrays;
import java.util.Objects;
import java.util.SortedSet;
import java.util.TreeSet;
import javax.persistence.CascadeType;
import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
import javax.persistence.Id;
import javax.persistence.OneToMany;

import org.hamcrest.collection.IsIterableContainingInOrder;

import org.hibernate.testing.TestForIssue;
import org.hibernate.testing.orm.junit.DomainModel;
import org.hibernate.testing.orm.junit.ServiceRegistry;
import org.hibernate.testing.orm.junit.SessionFactory;
import org.hibernate.testing.orm.junit.SessionFactoryScope;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;

/**
* Tests Hibernate specific feature that {@link org.hibernate.annotations.SortNatural @SortNatural} will take effect implicitly
* when no <i>sort</i> or <i>order</i> related annotations exist, including:
* <ul>
* <li>{@link org.hibernate.annotations.SortNatural @SortNatural}</li>
* <li>{@link org.hibernate.annotations.SortComparator @SortComparator}</li>
* <li>{@link org.hibernate.annotations.Sort @Sort}</li>
* <li>{@link org.hibernate.annotations.OrderBy @OrderBy(from hibernate)}</li>
* <li>{@link javax.persistence.OrderBy @OrderBy(from JPA)}</li>
* </ul>
*
* @author Nathan Xu
*/
@ServiceRegistry
@DomainModel(
annotatedClasses = {
SortNaturalByDefaultTests.Person.class,
SortNaturalByDefaultTests.Phone.class
}
)
@SessionFactory
@TestForIssue( jiraKey = "HHH-13877" )
public class SortNaturalByDefaultTests {

@Test
void test(SessionFactoryScope scope) {
scope.inTransaction(
session -> {
final Person person = session.createQuery( "select p from Person p", Person.class ).getSingleResult();
final SortedSet<Phone> phones = person.getPhones();
assertThat( phones, IsIterableContainingInOrder.contains(
new Phone( "123-456-789" ),
new Phone( "234-567-891" ),
new Phone( "345-678-912" ),
new Phone( "456-789-123" ),
new Phone( "567-891-234" ),
new Phone( "678-912-345" ),
new Phone( "789-123-456" ),
new Phone( "891-234-567" ),
new Phone( "912-345-678" ) )
);
}
);
}

@BeforeEach
void createTestData(SessionFactoryScope scope) {
scope.inTransaction(
session -> {
final Person person = new Person();
final SortedSet<Phone> phones = new TreeSet<>(
Arrays.asList(
new Phone( "678-912-345" ),
new Phone( "234-567-891" ),
new Phone( "567-891-234" ),
new Phone( "456-789-123" ),
new Phone( "123-456-789" ),
new Phone( "912-345-678" ),
new Phone( "789-123-456" ),
new Phone( "345-678-912" ),
new Phone( "891-234-567" )
)
);
person.setPhones( phones );
session.persist( person );
}
);
}

@AfterEach
void cleanUpTestData(SessionFactoryScope scope) {
scope.inTransaction(
session -> {
session.createQuery( "delete Person" ).executeUpdate();
}
);
}

@Entity(name = "Person")
public static class Person {

@Id
@GeneratedValue
private Long id;

@OneToMany(cascade = CascadeType.ALL)
private SortedSet<Phone> phones = new TreeSet<>(); // no '@SortNatural', '@SortComparator' or '@OrderBy' annotation present

public Long getId() {
return id;
}

public void setId(Long id) {
this.id = id;
}

public SortedSet<Phone> getPhones() {
return phones;
}

public void setPhones(SortedSet<Phone> phones) {
this.phones = phones;
}
}

@Entity(name = "Phone")
public static class Phone implements Comparable<Phone> {

@Id
@GeneratedValue
private Long id;

private String number;

public Phone() {
}

public Phone(String number) {
this.number = number;
}

public Long getId() {
return id;
}

public void setId(Long id) {
this.id = id;
}

public String getNumber() {
return number;
}

public void setNumber(String number) {
this.number = number;
}

@Override
public int compareTo(Phone o) {
return number.compareTo( o.getNumber() );
}

@Override
public boolean equals(Object o) {
if ( this == o ) {
return true;
}
if ( o == null || getClass() != o.getClass() ) {
return false;
}
Phone phone = (Phone) o;
return Objects.equals( number, phone.number );
}

@Override
public int hashCode() {
return Objects.hash( number );
}
}
}
Loading