Hello, Thanks for the review, the use cases and the explanation ! 1 beer for you ! :-)
2011/8/18 Tamás Cservenák <[email protected]>: > 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] > > -- Olivier Lamy Talend : http://talend.com http://twitter.com/olamy | http://linkedin.com/in/olamy --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
