Author: epbernard Date: 2007-08-08 23:17:05 -0400 (Wed, 08 Aug 2007) New Revision: 12908 Modified: trunk/HibernateExt/search/src/java/org/hibernate/search/query/ScrollableResultsImpl.java trunk/HibernateExt/search/src/test/org/hibernate/search/test/query/LuceneQueryTest.java Log: HSEARCH-110 ScrollableResults handles out of bounds Modified: trunk/HibernateExt/search/src/java/org/hibernate/search/query/ScrollableResultsImpl.java =================================================================== --- trunk/HibernateExt/search/src/java/org/hibernate/search/query/ScrollableResultsImpl.java 2007-08-08 16:17:19 UTC (rev 12907) +++ trunk/HibernateExt/search/src/java/org/hibernate/search/query/ScrollableResultsImpl.java 2007-08-09 03:17:05 UTC (rev 12908) @@ -6,18 +6,17 @@ import java.math.BigInteger; import java.sql.Blob; import java.sql.Clob; +import java.util.ArrayList; import java.util.Calendar; import java.util.Date; import java.util.HashMap; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.TimeZone; -import java.util.List; -import java.util.ArrayList; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.apache.lucene.document.Document; import org.apache.lucene.search.Hits; import org.apache.lucene.search.IndexSearcher; import org.hibernate.HibernateException; @@ -33,6 +32,19 @@ * Implements scollable and paginated resultsets. * Contrary to query#iterate() or query#list(), this implementation is * exposed to returned null objects (if the index is out of date). + * <p/> + * <p/> + * + * The following methods that change the value of 'current' will check + * + * and set its value to either 'afterLast' or 'beforeFirst' depending + * + * on direction. This is to prevent rogue values from setting it outside + * + * the boundaries of the results. + * + * <ul> + * + * <li>next()</li> + * + * <li>previous()</li> + * + * <li>scroll(i)</li> + * + * <li>last()</li> + * + * <li>first()</li> + * + * </ul> * * @author Emmanuel Bernard * @author John Griffin @@ -69,9 +81,9 @@ this.fetchSize = fetchSize; } - // The 'cache' is a sliding window that moves back and - // forth over entityInfos loading values of size fetchSize - // as necessary. + // The 'cache' is a sliding window of size fetchSize that + // moves back and forth over entityInfos as directed loading + // values as necessary. private EntityInfo loadCache(int windowStart) { int windowStop; @@ -81,17 +93,17 @@ return info; } - if ( windowStart + fetchSize > max) { + if ( windowStart + fetchSize > max ) { windowStop = max; } else { windowStop = windowStart + fetchSize - 1; } - List<EntityInfo> entityInfosLoaded = new ArrayList<EntityInfo>(windowStop - windowStart + 1); + List<EntityInfo> entityInfosLoaded = new ArrayList<EntityInfo>( windowStop - windowStart + 1 ); for (int x = windowStart; x <= windowStop; x++) { try { - if (entityInfos[x - first] == null) { + if ( entityInfos[x - first] == null ) { //FIXME should check that clazz match classes but this complicates a lot the firstResult/maxResult entityInfos[x - first] = documentExtractor.extract( hits, x ); entityInfosLoaded.add( entityInfos[x - first] ); @@ -102,10 +114,10 @@ } } - //preload effitciently first + //preload efficiently first loader.load( entityInfosLoaded.toArray( new EntityInfo[entityInfosLoaded.size()] ) ); //load one by one to inject null results if needed - for ( EntityInfo slidingInfo : entityInfosLoaded ) { + for (EntityInfo slidingInfo : entityInfosLoaded) { if ( !resultContext.containsKey( slidingInfo ) ) { Object loaded = loader.load( slidingInfo ); if ( !loaded.getClass().isArray() ) loaded = new Object[] { loaded }; @@ -115,26 +127,77 @@ return entityInfos[windowStart - first]; } + /** + * Increases cursor pointer by one. If this places it > + * max + 1 (afterLast) then set it to afterLast and return + * false. + * + * @return booolean + * @throws HibernateException + */ public boolean next() throws HibernateException { - return ++current <= max; + if ( ++current > max ) { + afterLast(); + return false; + } + return true; } + /** + * Decreases cursor pointer by one. If this places it < + * first - 1 (beforeFirst) then set it to beforeFirst and + * return false. + * + * @return boolean + * @throws HibernateException + */ public boolean previous() throws HibernateException { - return --current >= first; + if ( --current < first ) { + beforeFirst(); + return false; + } + return true; } + /** + * Since we have to take into account that we can scroll any + * amount positive or negative, we perform the same tests that + * we performed in next() and previous(). + * + * @param i + * @return boolean + * @throws HibernateException + */ public boolean scroll(int i) throws HibernateException { current = current + i; - return current >= first && current <= max; + if ( current > max ) { + afterLast(); + return false; + } + else if ( current < first ) { + beforeFirst(); + return false; + } + else { + return true; + } } public boolean last() throws HibernateException { current = max; + if ( current < first ) { + beforeFirst(); + return false; + } return max >= first; } public boolean first() throws HibernateException { current = first; + if ( current > max ) { + afterLast(); + return false; + } return max >= first; } Modified: trunk/HibernateExt/search/src/test/org/hibernate/search/test/query/LuceneQueryTest.java =================================================================== --- trunk/HibernateExt/search/src/test/org/hibernate/search/test/query/LuceneQueryTest.java 2007-08-08 16:17:19 UTC (rev 12907) +++ trunk/HibernateExt/search/src/test/org/hibernate/search/test/query/LuceneQueryTest.java 2007-08-09 03:17:05 UTC (rev 12908) @@ -368,15 +368,15 @@ result = results.get(); assertNull( result ); - // Let's see if a bad reverse scroll screws things up - /** - * These instructions uncover problems with calculations - * of 'current'. It should be limited by first and max. - */ - results.scroll( -6 ); + results.scroll( -8 ); result = results.get(); assertNull( result ); + // And test a bad forward scroll. + results.scroll( 10 ); + result = results.get(); + assertNull( result ); + //cleanup for (Object element : s.createQuery( "from " + Employee.class.getName() ).list()) s.delete( element ); tx.commit(); @@ -409,7 +409,58 @@ s.close(); } + public void testCurrent() throws Exception { + FullTextSession s = Search.createFullTextSession( openSession() ); + prepEmployeeIndex( s ); + s.clear(); + Transaction tx = s.beginTransaction(); + QueryParser parser = new QueryParser( "dept", new StandardAnalyzer() ); + + Query query = parser.parse( "dept:ITech" ); + org.hibernate.search.FullTextQuery hibQuery = s.createFullTextQuery( query, Employee.class ); + hibQuery.setProjection( "id", "lastname", "dept" ); + + + + ScrollableResults results = hibQuery.scroll(); + results.beforeFirst(); + results.next(); + assertTrue("beforeFirst() pointer incorrect", results.isFirst()); + + results.afterLast(); + results.previous(); + assertTrue("afterLast() pointer incorrect", results.isLast()); + + // Let's see if a bad reverse scroll screws things up + results.scroll( -8 ); + results.next(); + assertTrue("large negative scroll() pointer incorrect", results.isFirst()); + + // And test a bad forward scroll. + results.scroll( 10 ); + results.previous(); + assertTrue("large positive scroll() pointer incorrect", results.isLast()); + + // Finally, let's test a REAL screwup. + hibQuery.setFirstResult( 3 ); + hibQuery.setMaxResults( 1 ); + + results = hibQuery.scroll(); + results.first(); + Object[] result = results.get(); + assertEquals(1004, result[0]); + + results.last(); + result = results.get(); + assertEquals(1004, result[0]); + + //cleanup + for (Object element : s.createQuery( "from " + Employee.class.getName() ).list()) s.delete( element ); + tx.commit(); + s.close(); + } + public void testMultipleEntityPerIndex() throws Exception { FullTextSession s = Search.createFullTextSession( openSession() ); Transaction tx = s.beginTransaction();