[JENKINS-42645] Case insensitive search by default for new and anonymous users (#2801)

* [JENKINS-42645] Case insensitive search by default

* [JENKINS-42960] Search in FixedSet more locale friendly

String.equalsIgnoreCase is safer than toLowerCase when non English
locales are used.

* [JENKINS-42645] Review remarks

core/src/main/java/hudson/search/FixedSet.java

@@ -27,12 +27,15 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
 
+import org.apache.commons.lang.StringUtils;
+
 /**
  * Set of {@link SearchItem}s that are statically known upfront.
  *
  * @author Kohsuke Kawaguchi
  */
 public class FixedSet implements SearchIndex {
+
     private final Collection<? extends SearchItem> items;
 
     public FixedSet(Collection<? extends SearchItem> items) {
@@ -45,27 +48,21 @@ public class FixedSet implements SearchIndex {
 
     public void find(String token, List<SearchItem> result) {
         boolean caseInsensitive = UserSearchProperty.isCaseInsensitive();
-        for (SearchItem i : items){
+        for (SearchItem i : items) {
             String name = i.getSearchName();
-            if(caseInsensitive){
-                token=token.toLowerCase();
-                name=name.toLowerCase();
-            }
-            if(token.equals(i.getSearchName()))
+            if (name.equals(token) || (caseInsensitive && name.equalsIgnoreCase(token))) {
                 result.add(i);
+            }
         }
     }
 
     public void suggest(String token, List<SearchItem> result) {
         boolean caseInsensitive = UserSearchProperty.isCaseInsensitive();
-        for (SearchItem i : items){
+        for (SearchItem i : items) {
             String name = i.getSearchName();
-            if(caseInsensitive){
-                token=token.toLowerCase();
-                name=name.toLowerCase();
-            }
-            if(name.contains(token))
+            if (name.contains(token) || (caseInsensitive && StringUtils.containsIgnoreCase(name, token))) {
                 result.add(i);
+            }
         }
     }
 }

core/src/main/java/hudson/search/UserSearchProperty.java

@@ -11,7 +11,9 @@ import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.export.Exported;
 
 public class UserSearchProperty extends hudson.model.UserProperty {
-    
+
+    private static final boolean DEFAULT_SEARCH_CASE_INSENSITIVE_MODE = true;
+
     private final boolean insensitiveSearch;
 
     public UserSearchProperty(boolean insensitiveSearch) {
@@ -25,11 +27,12 @@ public class UserSearchProperty extends hudson.model.UserProperty {
     
     public static boolean isCaseInsensitive(){
         User user = User.current();
-        boolean caseInsensitive = false;
-        if(user!=null && user.getProperty(UserSearchProperty.class).getInsensitiveSearch()){//Searching for anonymous user is case-sensitive
-          caseInsensitive=true;
+
+        if (user == null) {
+            return DEFAULT_SEARCH_CASE_INSENSITIVE_MODE;
         }
-        return caseInsensitive;
+
+        return user.getProperty(UserSearchProperty.class).getInsensitiveSearch();
     }
     
 
@@ -40,7 +43,7 @@ public class UserSearchProperty extends hudson.model.UserProperty {
         }
 
         public UserProperty newInstance(User user) {
-            return new UserSearchProperty(false); //default setting is case-sensitive searching
+            return new UserSearchProperty(DEFAULT_SEARCH_CASE_INSENSITIVE_MODE);
         }
 
         @Override

core/src/test/java/hudson/model/ViewTest.java

@@ -13,6 +13,7 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 
 import javax.servlet.ServletException;
@@ -98,7 +99,7 @@ public class ViewTest {
         final TopLevelItem rightJob = createJob("rightJob");
 
         Mockito.when(leftView.getItems()).thenReturn(Arrays.asList(leftJob, sharedJob));
-        Mockito.when(rightView.getItems()).thenReturn(Arrays.asList(rightJob));
+        Mockito.when(rightView.getItems()).thenReturn(Collections.singletonList(rightJob));
 
         final TopLevelItem[] expected = new TopLevelItem[] {rootJob, sharedJob, leftJob, rightJob};
 

core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java

@@ -37,8 +37,8 @@ import hudson.model.Run;
 import hudson.model.StringParameterValue;
 
 import org.junit.Assert;
-import org.junit.Ignore;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.mockito.Mockito;
 
 import java.io.IOException;
@@ -326,24 +326,21 @@ public class HistoryPageFilterTest {
     }
 
     @Test
-    public void test_search_should_be_case_sensitive_for_anonymous_user() throws IOException {
-        //given
-        HistoryPageFilter<ModelObject> historyPageFilter = newPage(5, null, null);
-        //and
-        historyPageFilter.setSearchString("failure");
-        //and
+    @Issue("JENKINS-42645")
+    public void should_be_case_insensitive_by_default() throws IOException {
         List<ModelObject> runs = Lists.<ModelObject>newArrayList(new MockRun(2, Result.FAILURE), new MockRun(1, Result.SUCCESS));
-        List<Queue.Item> queueItems = newQueueItems(3, 4);
-
-        //when
-        historyPageFilter.add(runs, queueItems);
+        assertOneMatchingBuildForGivenSearchStringAndRunItems("failure", runs);
+    }
 
-        //then
-        Assert.assertEquals(0, historyPageFilter.runs.size());
+    @Test
+    public void should_lower_case_search_string_in_case_insensitive_search() throws IOException {
+        List<ModelObject> runs = Lists.<ModelObject>newArrayList(new MockRun(2, Result.FAILURE), new MockRun(1, Result.SUCCESS));
+        assertOneMatchingBuildForGivenSearchStringAndRunItems("FAILure", runs);
     }
 
     @Test
-    public void test_search_builds_by_build_variables() throws IOException {
+    @Issue("JENKINS-40718")
+    public void should_search_builds_by_build_variables() throws IOException {
         List<ModelObject> runs = ImmutableList.<ModelObject>of(
                 new MockBuild(2).withBuildVariables(ImmutableMap.of("env", "dummyEnv")),
                 new MockBuild(1).withBuildVariables(ImmutableMap.of("env", "otherEnv")));
@@ -351,7 +348,8 @@ public class HistoryPageFilterTest {
     }
 
     @Test
-    public void test_search_builds_by_build_params() throws IOException {
+    @Issue("JENKINS-40718")
+    public void should_search_builds_by_build_params() throws IOException {
         List<ModelObject> runs = ImmutableList.<ModelObject>of(
                 new MockBuild(2).withBuildParameters(ImmutableMap.of("env", "dummyEnv")),
                 new MockBuild(1).withBuildParameters(ImmutableMap.of("env", "otherEnv")));
@@ -359,7 +357,8 @@ public class HistoryPageFilterTest {
     }
 
     @Test
-    public void test_ignore_sensitive_parameters_in_search_builds_by_build_params() throws IOException {
+    @Issue("JENKINS-40718")
+    public void should_ignore_sensitive_parameters_in_search_builds_by_build_params() throws IOException {
         List<ModelObject> runs = ImmutableList.<ModelObject>of(
                 new MockBuild(2).withBuildParameters(ImmutableMap.of("plainPassword", "pass1plain")),
                 new MockBuild(1).withSensitiveBuildParameters("password", "pass1"));

test/src/test/java/jenkins/widgets/HistoryPageFilterInsensitiveSearchTest.java → test/src/test/java/jenkins/widgets/HistoryPageFilterCaseSensitiveSearchTest.java

@@ -8,6 +8,7 @@ import org.acegisecurity.providers.UsernamePasswordAuthenticationToken;
 import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.mockito.Mockito;
 
@@ -25,8 +26,11 @@ import hudson.security.AuthorizationStrategy;
 
 /**
  * TODO: Code partially duplicated with HistoryPageFilterTest in core
+ *
+ * Search in case insensitive mode is tested by unit tests in HistoryPageFilterTest.
  */
-public class HistoryPageFilterInsensitiveSearchTest {
+@Issue({"JENKINS-40718", "JENKINS-42645"})
+public class HistoryPageFilterCaseSensitiveSearchTest {
 
     private static final String TEST_USER_NAME = "testUser";
 
@@ -34,31 +38,45 @@ public class HistoryPageFilterInsensitiveSearchTest {
     public JenkinsRule j = new JenkinsRule();
 
     @Test
-    public void should_search_insensitively_when_enabled_for_user() throws IOException {
-        setUserContextAndAssertCaseInsensitivitySearchForGivenSearchString("failure");
+    public void should_search_case_sensitively_when_enabled_for_user() throws IOException {
+        setCaseSensitiveSearchForUserAndCheckAssertionForGivenSearchString("FAILURE", new SearchResultAssertFunction() {
+            @Override
+            public void doAssertion(HistoryPageFilter<ModelObject> historyPageFilter) {
+                Assert.assertEquals(1, historyPageFilter.runs.size());
+                Assert.assertEquals(HistoryPageEntry.getEntryId(2), historyPageFilter.runs.get(0).getEntryId());
+            }
+        });
     }
 
     @Test
-    public void should_also_lower_search_query_in_insensitive_search_enabled() throws IOException {
-        setUserContextAndAssertCaseInsensitivitySearchForGivenSearchString("FAILure");
+    public void should_skip_result_with_different_capitalization_when_case_sensitively_search_is_enabled_for_user() throws IOException {
+        setCaseSensitiveSearchForUserAndCheckAssertionForGivenSearchString("failure", new SearchResultAssertFunction() {
+            @Override
+            public void doAssertion(HistoryPageFilter<ModelObject> historyPageFilter) {
+                Assert.assertEquals(0, historyPageFilter.runs.size());
+            }
+        });
     }
 
-    private void setUserContextAndAssertCaseInsensitivitySearchForGivenSearchString(final String searchString) throws IOException {
+    private void setCaseSensitiveSearchForUserAndCheckAssertionForGivenSearchString(final String searchString,
+                                                                                    SearchResultAssertFunction assertionOnSearchResults) throws IOException {
         AuthorizationStrategy.Unsecured strategy = new AuthorizationStrategy.Unsecured();
         j.jenkins.setAuthorizationStrategy(strategy);
         j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
 
         UsernamePasswordAuthenticationToken testUserAuthentication = new UsernamePasswordAuthenticationToken(TEST_USER_NAME, "any");
-        try (ACLContext acl = ACL.as(testUserAuthentication)) {
-            User.get(TEST_USER_NAME).addProperty(new UserSearchProperty(true));
+        try (ACLContext ignored = ACL.as(testUserAuthentication)) {
+            User.get(TEST_USER_NAME).addProperty(new UserSearchProperty(false));
 
             //test logic
-            List<ModelObject> runs = ImmutableList.<ModelObject>of(new MockRun(2, Result.FAILURE), new MockRun(1, Result.SUCCESS));
-            assertOneMatchingBuildForGivenSearchStringAndRunItems(searchString, runs);
+            final List<ModelObject> runs = ImmutableList.<ModelObject>of(new MockRun(2, Result.FAILURE), new MockRun(1, Result.SUCCESS));
+            assertNoMatchingBuildsForGivenSearchStringAndRunItems(searchString, runs, assertionOnSearchResults);
         }
+
     }
 
-    private void assertOneMatchingBuildForGivenSearchStringAndRunItems(String searchString, List<ModelObject> runs) {
+    private void assertNoMatchingBuildsForGivenSearchStringAndRunItems(String searchString, List<ModelObject> runs,
+                                                                       SearchResultAssertFunction assertionOnSearchResults) {
         //given
         HistoryPageFilter<ModelObject> historyPageFilter = new HistoryPageFilter<>(5);
         //and
@@ -68,8 +86,7 @@ public class HistoryPageFilterInsensitiveSearchTest {
         historyPageFilter.add(runs, Collections.<Queue.Item>emptyList());
 
         //then
-        Assert.assertEquals(1, historyPageFilter.runs.size());
-        Assert.assertEquals(HistoryPageEntry.getEntryId(2), historyPageFilter.runs.get(0).getEntryId());
+        assertionOnSearchResults.doAssertion(historyPageFilter);
     }
 
     @SuppressWarnings("unchecked")
@@ -111,4 +128,9 @@ public class HistoryPageFilterInsensitiveSearchTest {
             return (int) queueId;
         }
     }
+
+    //Waiting for Java 8... - coming soon - April 2017?
+    private interface SearchResultAssertFunction {
+        void doAssertion(HistoryPageFilter<ModelObject> historyPageFilter);
+    }
 }