Hi Olivier, the change http://svn.apache.org/viewvc?view=revision&revision=1158917
while properly fixing the stuff mentioned in commit log, sneaks in another change that is wrong. The "searchFlat can return empty results with multiple index if the first used returns empty result" fix is okay, and I agree with it (removal of "return 0"). But, what is sneaked in wrt "duplicates" is wrong, and would violate and even prevent some existing use cases. The passed in parameter Collection<ArtifactInfo> is already a Set with proper Comparator that is user selectable. In short, you set ArtifactInfo.VERSION_COMPARATOR always, but that's maybe not what user wants. To demonstrate, along with fix for your rev1158917 (it undoes that solution with a Set) I committed an extensive UT demonstrating what your fix was breaking (just re-implement your change wrt "duplicates" and run the UT, will fail). Never forget that Maven Indexer is not used in MRMs only, things like IDE integrations are using it too, and those use cases are wildly different from those used in MRMs. The change is r1159159 http://svn.apache.org/viewvc?view=revision&revision=1159159 Thanks, ~t~ On Wed, Aug 17, 2011 at 11:16 PM, <[email protected]> wrote: > Author: olamy > Date: Wed Aug 17 21:16:25 2011 > New Revision: 1158917 > > URL: http://svn.apache.org/viewvc?rev=1158917&view=rev > Log: > [MINDEXER-38] searchFlat can return empty results with multiple index if the > first used returns empty result > > Modified: > > maven/indexer/trunk/indexer-core/src/main/java/org/apache/maven/index/DefaultSearchEngine.java > > maven/indexer/trunk/indexer-core/src/test/java/org/apache/maven/index/SearchWithAnEmptyIndexTest.java > > Modified: > maven/indexer/trunk/indexer-core/src/main/java/org/apache/maven/index/DefaultSearchEngine.java > URL: > http://svn.apache.org/viewvc/maven/indexer/trunk/indexer-core/src/main/java/org/apache/maven/index/DefaultSearchEngine.java?rev=1158917&r1=1158916&r2=1158917&view=diff > ============================================================================== > --- > maven/indexer/trunk/indexer-core/src/main/java/org/apache/maven/index/DefaultSearchEngine.java > (original) > +++ > maven/indexer/trunk/indexer-core/src/main/java/org/apache/maven/index/DefaultSearchEngine.java > Wed Aug 17 21:16:25 2011 > @@ -164,17 +164,27 @@ public class DefaultSearchEngine > { > int hitCount = 0; > > + Set<ArtifactInfo> nonDuplicateResults = new TreeSet<ArtifactInfo>( > ArtifactInfo.VERSION_COMPARATOR ); > + > + > for ( IndexingContext context : participatingContexts ) > { > final TopScoreDocCollector collector = doSearchWithCeiling( req, > context.getIndexSearcher(), query ); > > + // olamy if the first context used doesn't find the other are > not used for search > + // so the result can probably returns duplicate as artifactInfo > doesn't implements hashCode/equals > + // so implements this in nonDuplicateResults > + /* > if ( collector.getTotalHits() == 0 ) > { > return 0; > } > + */ > > ScoreDoc[] scoreDocs = collector.topDocs().scoreDocs; > > + // uhm btw hitCount contains dups > + > hitCount += collector.getTotalHits(); > > int start = 0; // from == FlatSearchRequest.UNDEFINED ? 0 : from; > @@ -192,11 +202,14 @@ public class DefaultSearchEngine > > artifactInfo.context = context.getId(); > > - result.add( artifactInfo ); > + nonDuplicateResults.add( artifactInfo ); > + > } > } > } > > + result.addAll( nonDuplicateResults ); > + > return hitCount; > } > > > Modified: > maven/indexer/trunk/indexer-core/src/test/java/org/apache/maven/index/SearchWithAnEmptyIndexTest.java > URL: > http://svn.apache.org/viewvc/maven/indexer/trunk/indexer-core/src/test/java/org/apache/maven/index/SearchWithAnEmptyIndexTest.java?rev=1158917&r1=1158916&r2=1158917&view=diff > ============================================================================== > --- > maven/indexer/trunk/indexer-core/src/test/java/org/apache/maven/index/SearchWithAnEmptyIndexTest.java > (original) > +++ > maven/indexer/trunk/indexer-core/src/test/java/org/apache/maven/index/SearchWithAnEmptyIndexTest.java > Wed Aug 17 21:16:25 2011 > @@ -31,6 +31,7 @@ import org.codehaus.plexus.util.FileUtil > > import java.io.File; > import java.util.ArrayList; > +import java.util.Arrays; > import java.util.List; > > /** > @@ -70,10 +71,9 @@ public class SearchWithAnEmptyIndexTest > } > } > > - public void testWithTwoContextWithOneEmpty() > + public void testWithTwoContextWithOneEmptyFirstInContextsListSearchFlat() > throws Exception > { > - createIndex( "src/test/repo-with-osgi", > "target/test/repo-with-osgi/", INDEX_ID1 ); > > String repoPath = "target/test/empty-repo-for-searchtest"; > > @@ -89,6 +89,8 @@ public class SearchWithAnEmptyIndexTest > //createIndex( "/src/test/repo", repoPath + "/.index", INDEX_ID2 ); > createIndex( repoPath, repoPath, INDEX_ID2 ); > > + createIndex( "src/test/repo-with-osgi", > "target/test/repo-with-osgi/", INDEX_ID1 ); > + > try > { > BooleanQuery q = new BooleanQuery(); > @@ -99,7 +101,9 @@ public class SearchWithAnEmptyIndexTest > > FlatSearchRequest request = new FlatSearchRequest( q ); > assertEquals( 2, > nexusIndexer.getIndexingContexts().values().size() ); > - request.setContexts( new ArrayList( > nexusIndexer.getIndexingContexts().values() ) ); > + request.setContexts( Arrays.asList( > nexusIndexer.getIndexingContexts().get( INDEX_ID2 ), > + > nexusIndexer.getIndexingContexts().get( INDEX_ID1 ) ) ); > + > FlatSearchResponse response = nexusIndexer.searchFlat( request ); > > assertEquals( 1, response.getResults().size() ); > @@ -148,6 +152,58 @@ public class SearchWithAnEmptyIndexTest > } > } > > + /** > + * both repos contains commons-cli so ensure we don't return duplicates > + */ > + public void testSearchNoDuplicateArtifactInfo() > + throws Exception > + { > + > + String repoPathIndex = "target/test/repo-for-searchdupe"; > + > + File emptyRepo = new File( getBasedir(), repoPathIndex ); > + > + if ( emptyRepo.exists() ) > + { > + FileUtils.deleteDirectory( emptyRepo ); > + } > + > + emptyRepo.mkdirs(); > + > + //createIndex( "/src/test/repo", repoPath + "/.index", INDEX_ID2 ); > + createIndex( "/src/test/repo", repoPathIndex, INDEX_ID2 ); > + > + createIndex( "src/test/repo-with-osgi", > "target/test/repo-with-osgi/", INDEX_ID1 ); > + > + try > + { > + BooleanQuery q = new BooleanQuery(); > + > + q.add( nexusIndexer.constructQuery( MAVEN.GROUP_ID, new > StringSearchExpression( "commons-cli" ) ), > + BooleanClause.Occur.MUST ); > + > + q.add( nexusIndexer.constructQuery( MAVEN.PACKAGING, new > StringSearchExpression( "jar" ) ), > + BooleanClause.Occur.MUST ); > + > + q.add( nexusIndexer.constructQuery( MAVEN.CLASSIFIER, new > StringSearchExpression( "sources" ) ), > + BooleanClause.Occur.MUST ); > + > + FlatSearchRequest request = new FlatSearchRequest( q ); > + assertEquals( 2, > nexusIndexer.getIndexingContexts().values().size() ); > + request.setContexts( Arrays.asList( > nexusIndexer.getIndexingContexts().get( INDEX_ID2 ), > + > nexusIndexer.getIndexingContexts().get( INDEX_ID1 ) ) ); > + > + FlatSearchResponse response = nexusIndexer.searchFlat( request ); > + > + assertEquals( 1, response.getResults().size() ); > + > + } > + finally > + { > + closeAllIndexs(); > + } > + } > + > private void closeAllIndexs() > throws Exception > { > > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
