diff --git a/orm/src/main/java/org/hibernate/search/query/hibernate/impl/CriteriaObjectInitializer.java b/orm/src/main/java/org/hibernate/search/query/hibernate/impl/CriteriaObjectInitializer.java index 9e38474d797..5f3b1a0573c 100644 --- a/orm/src/main/java/org/hibernate/search/query/hibernate/impl/CriteriaObjectInitializer.java +++ b/orm/src/main/java/org/hibernate/search/query/hibernate/impl/CriteriaObjectInitializer.java @@ -39,6 +39,7 @@ * * @author Emmanuel Bernard * @author Gunnar Morling + * @author Guillaume Smet */ public class CriteriaObjectInitializer implements ObjectInitializer { @@ -84,7 +85,7 @@ public void initializeObjects(List entityInfos, if ( documentBuilder == null ) { // the query result can contain entities which are not indexed. This can for example happen if // the targeted entity type is a superclass with indexed and un-indexed sub classes - // entities which don't have an document builder can be ignores (HF) + // entities which don't have a document builder can be ignored (HF) continue; } XMember idProperty = documentBuilder.getIdGetter(); @@ -117,18 +118,20 @@ private void setCriteriaTimeout(Criteria criteria, TimeoutManager timeoutManager * case all the entity infos originate from the same id space. */ private List buildUpCriteria(List entityInfos, ObjectInitializationContext objectInitializationContext) { - Map, List> infosByIdSpace = groupInfosByIdSpace( entityInfos, objectInitializationContext ); + Map, EntityInfoIdSpace> infosByIdSpace = groupInfosByIdSpace( entityInfos, objectInitializationContext ); // all entities from same id space -> single criteria if ( infosByIdSpace.size() == 1 ) { + EntityInfoIdSpace idSpace = infosByIdSpace.values().iterator().next(); + // no explicitly user specified criteria query, define one Criteria criteria = objectInitializationContext.getCriteria(); if ( criteria == null ) { - criteria = objectInitializationContext.getSession().createCriteria( infosByIdSpace.keySet().iterator().next() ); + criteria = objectInitializationContext.getSession().createCriteria( idSpace.getMostSpecificEntityType() ); } - criteria.add( getIdListCriterion( infosByIdSpace.values().iterator().next(), objectInitializationContext ) ); + criteria.add( getIdListCriterion( idSpace.getEntityInfos(), objectInitializationContext ) ); return Collections.singletonList( criteria ); } @@ -141,9 +144,11 @@ private List buildUpCriteria(List entityInfos, ObjectIniti List criterias = new ArrayList<>( infosByIdSpace.size() ); - for ( Entry, List> infosOfIdSpace : infosByIdSpace.entrySet() ) { - Criteria criteria = objectInitializationContext.getSession().createCriteria( infosOfIdSpace.getKey() ); - criteria.add( getIdListCriterion( infosOfIdSpace.getValue(), objectInitializationContext ) ); + for ( Entry, EntityInfoIdSpace> infosOfIdSpace : infosByIdSpace.entrySet() ) { + EntityInfoIdSpace idSpace = infosOfIdSpace.getValue(); + + Criteria criteria = objectInitializationContext.getSession().createCriteria( idSpace.getMostSpecificEntityType() ); + criteria.add( getIdListCriterion( idSpace.getEntityInfos(), objectInitializationContext ) ); criterias.add( criteria ); } @@ -188,13 +193,13 @@ private Criterion getIdListCriterion(List entityInfos, ObjectInitial * * @return The given entity infos, keyed by the root entity type of id spaces */ - private Map, List> groupInfosByIdSpace(List entityInfos, ObjectInitializationContext objectInitializationContext) { + private Map, EntityInfoIdSpace> groupInfosByIdSpace(List entityInfos, ObjectInitializationContext objectInitializationContext) { ServiceManager serviceManager = objectInitializationContext.getExtendedSearchIntegrator().getServiceManager(); IdUniquenessResolver resolver = serviceManager.requestService( IdUniquenessResolver.class ); SessionFactoryImplementor sessionFactory = (SessionFactoryImplementor) objectInitializationContext.getSession().getSessionFactory(); try { - Map, List> idSpaces = new HashMap<>(); + Map, EntityInfoIdSpace> idSpaces = new HashMap<>(); for ( EntityInfo entityInfo : entityInfos ) { addToIdSpace( idSpaces, entityInfo, resolver, sessionFactory ); @@ -214,9 +219,9 @@ private Class getRootEntityType(SessionFactoryImplementor sessionFactory, Cla return sessionFactory.getEntityPersister( rootEntityName ).getMappedClass(); } - private void addToIdSpace(Map, List> idSpaces, EntityInfo entityInfo, IdUniquenessResolver resolver, SessionFactoryImplementor sessionFactory) { + private void addToIdSpace(Map, EntityInfoIdSpace> idSpaces, EntityInfo entityInfo, IdUniquenessResolver resolver, SessionFactoryImplementor sessionFactory) { // add to existing id space if possible - for ( Entry, List> idSpace : idSpaces.entrySet() ) { + for ( Entry, EntityInfoIdSpace> idSpace : idSpaces.entrySet() ) { if ( resolver.areIdsUniqueForClasses( entityInfo.getClazz(), idSpace.getKey() ) ) { idSpace.getValue().add( entityInfo ); return; @@ -224,8 +229,8 @@ private void addToIdSpace(Map, List> idSpaces, EntityInfo e } // otherwise create a new id space, using the root entity as key - List idSpace = new ArrayList<>(); - idSpace.add( entityInfo ); + Class rootType = getRootEntityType( sessionFactory, entityInfo.getClazz() ); + EntityInfoIdSpace idSpace = new EntityInfoIdSpace( rootType, entityInfo ); idSpaces.put( getRootEntityType( sessionFactory, entityInfo.getClazz() ), idSpace ); } @@ -240,4 +245,58 @@ private DocumentBuilderIndexedEntity getDocumentBuilder(Class entityType, Ext return null; } } + + /** + * Container used to store all the {@code EntityInfo}s for entities which share the same id space (typically all the + * subtypes of the same root type). + * + * Determines the most specific entity type we can use to build a Criteria to get the entities associated with these + * {@code EntityInfo}s. + */ + private static class EntityInfoIdSpace { + private final Class rootType; + + private Class mostSpecificEntityType; + + private List entityInfos = new ArrayList<>(); + + private EntityInfoIdSpace(Class rootType, EntityInfo entityInfo) { + this.rootType = rootType; + this.entityInfos.add( entityInfo ); + this.mostSpecificEntityType = entityInfo.getClazz(); + } + + private void add(EntityInfo entityInfo) { + entityInfos.add( entityInfo ); + mostSpecificEntityType = getMostSpecificCommonSuperClass( mostSpecificEntityType, entityInfo.getClazz() ); + } + + private Class getMostSpecificCommonSuperClass(Class class1, Class class2) { + if ( rootType.equals( class1 ) || rootType.equals( class2 ) ) { + return rootType; + } + Class superClass = class1; + while ( !superClass.isAssignableFrom( class2 ) ) { + superClass = superClass.getSuperclass(); + } + return superClass; + } + + private List getEntityInfos() { + return entityInfos; + } + + /** + * Returns the most specific possible type to build the criteria with. In case of a hierarchy using a join inheritance, + * it makes a huge difference if we are targeting only one subtype as we will avoid the joins on all the subtypes + * of the root entity type. + * + * @return the most specific entity type we can use for the Criteria + */ + private Class getMostSpecificEntityType() { + return mostSpecificEntityType; + } + + } + } diff --git a/orm/src/test/java/org/hibernate/search/test/query/initandlookup/CriteriaObjectInitializerAndHierarchyInheritanceTest.java b/orm/src/test/java/org/hibernate/search/test/query/initandlookup/CriteriaObjectInitializerAndHierarchyInheritanceTest.java new file mode 100644 index 00000000000..79b4eeca9a3 --- /dev/null +++ b/orm/src/test/java/org/hibernate/search/test/query/initandlookup/CriteriaObjectInitializerAndHierarchyInheritanceTest.java @@ -0,0 +1,207 @@ +/* + * Hibernate Search, full-text search for your domain model + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later + * See the lgpl.txt file in the root directory or . + */ +package org.hibernate.search.test.query.initandlookup; + +import static org.fest.assertions.Assertions.assertThat; + +import java.util.List; +import java.util.Locale; + +import javax.persistence.Entity; +import javax.persistence.Id; +import javax.persistence.Inheritance; +import javax.persistence.InheritanceType; +import javax.persistence.MappedSuperclass; +import javax.persistence.Table; + +import org.apache.lucene.index.Term; +import org.apache.lucene.search.BooleanClause.Occur; +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.Sort; +import org.apache.lucene.search.SortField; +import org.apache.lucene.search.TermQuery; +import org.hibernate.Session; +import org.hibernate.Transaction; +import org.hibernate.search.FullTextSession; +import org.hibernate.search.Search; +import org.hibernate.search.annotations.DocumentId; +import org.hibernate.search.annotations.Field; +import org.hibernate.search.annotations.Indexed; +import org.hibernate.search.annotations.SortableField; +import org.hibernate.search.test.SearchTestBase; +import org.hibernate.search.testsupport.BytemanHelper; +import org.hibernate.search.testsupport.BytemanHelperStateCleanup; +import org.hibernate.search.testsupport.TestForIssue; +import org.jboss.byteman.contrib.bmunit.BMRule; +import org.jboss.byteman.contrib.bmunit.BMUnitRunner; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; + +@RunWith(BMUnitRunner.class) +public class CriteriaObjectInitializerAndHierarchyInheritanceTest extends SearchTestBase { + + @Rule + public BytemanHelperStateCleanup bytemanState = new BytemanHelperStateCleanup(); + + @Override + public Class[] getAnnotatedClasses() { + return new Class[]{ BaseEntity.class, A.class, AA.class, AAA.class, AAB.class, AB.class, ABA.class, AC.class, B.class, BA.class }; + } + + @BMRule( + name = "trackCriteriaEntityType", + targetClass = "org.hibernate.search.query.hibernate.impl.CriteriaObjectInitializer", + targetMethod = "buildUpCriteria(java.util.List, org.hibernate.search.query.hibernate.impl.ObjectInitializationContext)", + targetLocation = "EXIT", + helper = "org.hibernate.search.testsupport.BytemanHelper", + binding = "c : org.hibernate.internal.CriteriaImpl = $!.get(0);", + action = "pushEvent(c.getEntityOrClassName())" + ) + @Test + @TestForIssue(jiraKey = "HSEARCH-2301") + @SuppressWarnings("unchecked") + public void testJoinsAreOnlyOnUsefulEntityTypes() throws Exception { + Session s = openSession(); + + Transaction tx = s.beginTransaction(); + int i = 1; + createInstance( s, A.class, i++, "A" ); + createInstance( s, A.class, i++, "A" ); + createInstance( s, AA.class, i++, "A AA" ); + createInstance( s, AA.class, i++, "A AA" ); + createInstance( s, AAA.class, i++, "A AA AAA" ); + createInstance( s, AAA.class, i++, "A AA AAA" ); + createInstance( s, AAA.class, i++, "A AA AAA" ); + createInstance( s, AAB.class, i++, "A AA AAB" ); + createInstance( s, AAB.class, i++, "A AA AAB" ); + createInstance( s, AB.class, i++, "A AB" ); + createInstance( s, AB.class, i++, "A AB" ); + createInstance( s, ABA.class, i++, "A AB ABA" ); + createInstance( s, ABA.class, i++, "A AB ABA" ); + createInstance( s, AC.class, i++, "A AC" ); + createInstance( s, AC.class, i++, "A AC" ); + createInstance( s, B.class, i++, "B" ); + createInstance( s, B.class, i++, "B" ); + createInstance( s, BA.class, i++, "B BA" ); + createInstance( s, BA.class, i++, "B BA" ); + tx.commit(); + s.clear(); + + FullTextSession session = Search.getFullTextSession( s ); + + List results = getResults( session, AAA.class ); + assertThat( results ).onProperty( "name" ).containsOnly( "A AA AAA" ); + assertThat( BytemanHelper.consumeNextRecordedEvent() ).isEqualTo( AAA.class.getName() ); + + results = getResults( session, AAA.class, AAB.class ); + assertThat( results ).onProperty( "name" ).containsOnly( "A AA AAA", "A AA AAB" ); + assertThat( BytemanHelper.consumeNextRecordedEvent() ).isEqualTo( AA.class.getName() ); + + results = getResults( session, AAA.class, AB.class ); + assertThat( results ).onProperty( "name" ).containsOnly( "A AA AAA", "A AB", "A AB ABA" ); + assertThat( BytemanHelper.consumeNextRecordedEvent() ).isEqualTo( A.class.getName() ); + + results = getResults( session, AAA.class, BA.class ); + assertThat( results ).onProperty( "name" ).containsOnly( "A AA AAA", "B BA" ); + // here, we have 2 Criterias returned: we only test the first one + assertThat( BytemanHelper.consumeNextRecordedEvent() ).isIn( AAA.class.getName(), BA.class.getName() ); + + s.close(); + } + + private void createInstance(Session s, Class clazz, Integer id, String name) throws Exception { + BaseEntity entity = clazz.newInstance(); + entity.id = id; + entity.name = name; + s.persist( entity ); + } + + @SuppressWarnings("unchecked") + private List getResults(FullTextSession session, Class... classes) { + BooleanQuery.Builder bqb = new BooleanQuery.Builder(); + for ( Class clazz : classes ) { + bqb.add( new TermQuery( new Term( "name", clazz.getSimpleName().toLowerCase( Locale.ENGLISH ) ) ), Occur.SHOULD ); + } + return session.createFullTextQuery( bqb.build(), BaseEntity.class ) + .setSort( new Sort( new SortField( "idSort", SortField.Type.INT ) ) ) + .list(); + } + + @MappedSuperclass + @Inheritance(strategy = InheritanceType.JOINED) + @Table(name = "BaseEntity") + public abstract static class BaseEntity { + @Id + @DocumentId + @Field(name = "idSort") + @SortableField(forField = "idSort") + Integer id; + + @Field + String name; + + public String getName() { + return name; + } + } + + @Entity + @Indexed + @Table(name = "A") + public static class A extends BaseEntity { + } + + @Entity + @Indexed + @Table(name = "AA") + public static class AA extends A { + } + + @Entity + @Indexed + @Table(name = "AAA") + public static class AAA extends AA { + } + + @Entity + @Indexed + @Table(name = "AAB") + public static class AAB extends AA { + } + + @Entity + @Indexed + @Table(name = "AB") + public static class AB extends A { + } + + @Entity + @Indexed + @Table(name = "ABA") + public static class ABA extends AB { + } + + @Entity + @Indexed + @Table(name = "AC") + public static class AC extends A { + } + + @Entity + @Indexed + @Table(name = "B") + public static class B extends BaseEntity { + } + + @Entity + @Indexed + @Table(name = "BA") + public static class BA extends B { + } + +}