[JENKINS-42645] Case insensitive search by default for new and anonym…

…ous 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
jenkinsci · Apr 7, 2017 · b831acd · b831acd
1 parent 25ef87d
commit b831acd
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 48 deletions.
diff --git a/core/src/main/java/hudson/search/FixedSet.java b/core/src/main/java/hudson/search/FixedSet.java
@@ -27,12 +27,15 @@
 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 FixedSet(SearchItem... items) {
 
     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);
+            }
         }
     }
 }
diff --git a/core/src/main/java/hudson/search/UserSearchProperty.java b/core/src/main/java/hudson/search/UserSearchProperty.java
@@ -11,7 +11,9 @@
 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 boolean getInsensitiveSearch() {
 
     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 String getDisplayName() {
         }
 
         public UserProperty newInstance(User user) {
-            return new UserSearchProperty(false); //default setting is case-sensitive searching
+            return new UserSearchProperty(DEFAULT_SEARCH_CASE_INSENSITIVE_MODE);
         }
 
         @Override

diff --git a/core/src/test/java/hudson/model/ViewTest.java b/core/src/test/java/hudson/model/ViewTest.java
@@ -13,6 +13,7 @@
 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 void getAllItems() throws Exception {
         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};
 

diff --git a/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java b/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java
@@ -37,8 +37,8 @@
 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,40 +326,39 @@ public void test_search_runs_by_build_number() throws IOException {
     }
 
     @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")));
         assertOneMatchingBuildForGivenSearchStringAndRunItems("dummyEnv", runs);
     }
 
     @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")));
         assertOneMatchingBuildForGivenSearchStringAndRunItems("dummyEnv", runs);
     }
 
     @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"));

diff --git a/...storyPageFilterInsensitiveSearchTest.java → ...oryPageFilterCaseSensitiveSearchTest.java b/...storyPageFilterInsensitiveSearchTest.java → ...oryPageFilterCaseSensitiveSearchTest.java
@@ -8,6 +8,7 @@
 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,40 +26,57 @@
 
 /**
  * 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";
 
     @Rule
     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 @@ private void assertOneMatchingBuildForGivenSearchStringAndRunItems(String search
         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 int getNumber() {
             return (int) queueId;
         }
     }
+
+    //Waiting for Java 8... - coming soon - April 2017?
+    private interface SearchResultAssertFunction {
+        void doAssertion(HistoryPageFilter<ModelObject> historyPageFilter);
+    }
 }