Fix limit with negative integer for SearchUtil (#1627)

Co-authored-by: René Pfeuffer <rene.pfeuffer@cloudogu.com>
2026-07-07 13:03:34 +02:00 · 2021-04-21 16:19:16 +02:00
parent 05ef203038
commit 7579d91505
5 changed files with 160 additions and 310 deletions
--- a/gradle/changelog/search_util_limit.yaml
+++ b/gradle/changelog/search_util_limit.yaml
@@ -0,0 +1,2 @@
+- type: fixed
+  description: Fix limit with negativ integer for searchUtil ([#1627](https://github.com/scm-manager/scm-manager/pull/1627))
--- a/scm-core/src/main/java/sonia/scm/search/SearchUtil.java
+++ b/scm-core/src/main/java/sonia/scm/search/SearchUtil.java
@@ -21,152 +21,75 @@
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 */
-    
-package sonia.scm.search;

-//~--- non-JDK imports --------------------------------------------------------
+package sonia.scm.search;

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import sonia.scm.TransformFilter;
-import sonia.scm.util.Util;
-
-//~--- JDK imports ------------------------------------------------------------

 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;

-/**
- *
- * @author Sebastian Sdorra
- */
-public final class SearchUtil
-{
+public final class SearchUtil {

-  /**
-   * the logger for SearchUtil
-   */
-  private static final Logger logger =
-    LoggerFactory.getLogger(SearchUtil.class);
+  private static final Logger LOG = LoggerFactory.getLogger(SearchUtil.class);

-  //~--- constructors ---------------------------------------------------------
+  private SearchUtil() {
+  }

-  /**
-   * Constructs ...
-   *
-   */
-  private SearchUtil() {}
-
-  //~--- methods --------------------------------------------------------------
-
-  /**
-   * Method description
-   *
-   *
-   * @param request
-   * @param value
-   * @param other
-   *
-   * @return
-   */
  public static boolean matchesAll(SearchRequest request, String value,
-    String... other)
-  {
-    boolean result = false;
+                                   String... other) {
    String query = createStringQuery(request);

-    if (value.matches(query))
-    {
-      result = true;
-
-      for (String o : other)
-      {
-        if ((o == null) ||!o.matches(query))
-        {
-          result = false;
-
-          break;
+    if (value.matches(query)) {
+      for (String o : other) {
+        if ((o == null) || !o.matches(query)) {
+          return false;
        }
      }
+      return true;
    }

-    return result;
+    return false;
  }

-  /**
-   * Method description
-   *
-   *
-   * @param request
-   * @param value
-   * @param other
-   *
-   * @return
-   */
  public static boolean matchesOne(SearchRequest request, String value,
-    String... other)
-  {
-    boolean result = false;
+                                   String... other) {
    String query = createStringQuery(request);

-    if (!value.matches(query))
-    {
-      for (String o : other)
-      {
-        if ((o != null) && o.matches(query))
-        {
-          result = true;
-
-          break;
+    if (!value.matches(query)) {
+      for (String o : other) {
+        if ((o != null) && o.matches(query)) {
+          return true;
        }
      }
-    }
-    else
-    {
-      result = true;
+    } else {
+      return true;
    }

-    return result;
+    return false;
  }

-  /**
-   * Method description
-   *
-   *
-   * @param searchRequest
-   * @param collection
-   * @param filter
-   * @param <T>
-   *
-   * @return
-   */
  public static <T, R> Collection<R> search(SearchRequest searchRequest,
-    Collection<T> collection, TransformFilter<T, R> filter)
-  {
+                                            Collection<T> collection, TransformFilter<T, R> filter) {
    List<R> items = new ArrayList<>();
    int index = 0;
    int counter = 0;
-    Iterator<T> it = collection.iterator();

-    while (it.hasNext())
-    {
-      R item = filter.accept(it.next());
+    for (T t : collection) {
+      R item = filter.accept(t);

-      if (item != null)
-      {
+      if (item != null) {
        index++;

-        if (searchRequest.getStartWith() <= index)
-        {
+        if (searchRequest.getStartWith() <= index) {
          items.add(item);
          counter++;

-          if (searchRequest.getMaxResults() <= counter)
-          {
+          if (searchRequest.getMaxResults() > 0 && searchRequest.getMaxResults() <= counter) {
            break;
          }
        }
@@ -176,107 +99,87 @@ public final class SearchUtil
    return items;
  }

-  /**
-   * Method description
-   *
-   *
-   * @param pattern
-   * @param c
-   */
-  private static void appendChar(StringBuilder pattern, char c)
-  {
-    switch (c)
-    {
-      case '*' :
+  private static void appendChar(StringBuilder pattern, char c) {
+    switch (c) {
+      case '*':
        pattern.append(".*");

        break;

-      case '?' :
+      case '?':
        pattern.append(".");

        break;

-      case '(' :
+      case '(':
        pattern.append("\\(");

        break;

-      case ')' :
+      case ')':
        pattern.append("\\)");

        break;

-      case '{' :
+      case '{':
        pattern.append("\\{");

        break;

-      case '}' :
+      case '}':
        pattern.append("\\}");

        break;

-      case '[' :
+      case '[':
        pattern.append("\\[");

        break;

-      case ']' :
+      case ']':
        pattern.append("\\]");

        break;

-      case '|' :
+      case '|':
        pattern.append("\\|");

        break;

-      case '.' :
+      case '.':
        pattern.append("\\.");

        break;

-      case '\\' :
+      case '\\':
        pattern.append("\\\\");

        break;

-      default :
+      default:
        pattern.append(c);
    }
  }

-  /**
-   * Method description
-   *
-   *
-   * @param request
-   *
-   * @return
-   */
-  private static String createStringQuery(SearchRequest request)
-  {
+  private static String createStringQuery(SearchRequest request) {
    String query = request.getQuery().trim();

    StringBuilder pattern = new StringBuilder();

-    if (request.isIgnoreCase())
-    {
+    if (request.isIgnoreCase()) {
      pattern.append("(?i)");
      query = query.toLowerCase(Locale.ENGLISH);
    }

    pattern.append(".*");

-    for (char c : query.toCharArray())
-    {
+    for (char c : query.toCharArray()) {
      appendChar(pattern, c);
    }

    pattern.append(".*");

-    logger.trace("converted query \"{}\" to regex pattern \"{}\"",
+    LOG.trace("converted query \"{}\" to regex pattern \"{}\"",
      request.getQuery(), pattern);

    return pattern.toString();
--- a/scm-core/src/test/java/sonia/scm/search/SearchUtilTest.java
+++ b/scm-core/src/test/java/sonia/scm/search/SearchUtilTest.java
@@ -21,171 +21,135 @@
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 */
-    
+
 package sonia.scm.search;

-//~--- non-JDK imports --------------------------------------------------------
-
 import org.junit.Test;
+import sonia.scm.TransformFilter;
+import sonia.scm.user.User;

-import static org.junit.Assert.*;
+import java.util.Arrays;
+import java.util.Collection;

-/**
- *
- * @author Sebastian Sdorra
- */
-public class SearchUtilTest
-{
+import static java.util.stream.Collectors.toList;
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class SearchUtilTest {

-  /**
-   * Method description
-   *
-   */
  @Test
-  public void testMultiMatchesAll()
-  {
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test"), "test",
-      "test hello", "hello test", "hello test hello"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test"), "test",
-      "test hello", "hello test", "hello test hello", "ka"));
+  public void testMultiMatchesAll() {
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "test", "test hello", "hello test", "hello test hello")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "test", "test hello", "hello test", "hello test hello", "ka")).isFalse();
  }

-  /**
-   * Method description
-   *
-   */
  @Test
-  public void testMultiMatchesOne()
-  {
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test"), "test",
-      "test hello", "hello test", "hello test hello"));
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test"), "test",
-      "test hello", "hello test", "hello test hello", "ka"));
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test"), "hans", "uew",
-      "klaus", "hello test hello", "ka"));
+  public void testMultiMatchesOne() {
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "test", "test hello", "hello test", "hello test hello")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "test", "test hello", "hello test", "hello test hello", "ka")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "hans", "uew", "klaus", "hello test hello", "ka")).isTrue();
  }

-  /**
-   * Method description
-   *
-   */
  @Test
-  public void testSingleMatchesAll()
-  {
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test"), "test"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test"), "hello test"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test"), "test hello"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test"),
-      "hello test hello"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test"),
-      "hello hello"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test"),
-      "hello te hello"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test"),
-      "hello TEST hello"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test"),
-      "hello TesT hello"));
+  public void testSingleMatchesAll() {
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "test")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "hello test")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "test hello")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "hello test hello")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "hello hello")).isFalse();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "hello te hello")).isFalse();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "hello TEST hello")).isFalse();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test"), "hello TesT hello")).isFalse();
  }

-  /**
-   * Method description
-   *
-   */
  @Test
-  public void testSingleMatchesAllIgnoreCase()
-  {
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test", true), "tEsT"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test", true),
-      "heLLo teSt"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test", true),
-      "TEST hellO"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test", true),
-      "hEllO tEsT hEllO"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test", true),
-      "heLLo heLLo"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test", true),
-      "heLLo te heLLo"));
+  public void testSingleMatchesAllIgnoreCase() {
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test", true), "tEsT")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test", true), "heLLo teSt")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test", true), "TEST hellO")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test", true), "hEllO tEsT hEllO")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test", true), "heLLo heLLo")).isFalse();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test", true), "heLLo te heLLo")).isFalse();
  }

-  /**
-   * Method description
-   *
-   */
  @Test
-  public void testSingleMatchesOne()
-  {
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test"), "test"));
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test"), "hello test"));
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test"), "test hello"));
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test"),
-      "hello test hello"));
-    assertFalse(SearchUtil.matchesOne(new SearchRequest("test"),
-      "hello hello"));
-    assertFalse(SearchUtil.matchesOne(new SearchRequest("test"),
-      "hello te hello"));
-    assertFalse(SearchUtil.matchesOne(new SearchRequest("test"),
-      "hello TEST hello"));
-    assertFalse(SearchUtil.matchesOne(new SearchRequest("test"),
-      "hello TesT hello"));
+  public void testSingleMatchesOne() {
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "test")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "hello test")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "test hello")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "hello test hello")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "hello hello")).isFalse();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "hello te hello")).isFalse();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "hello TEST hello")).isFalse();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test"), "hello TesT hello")).isFalse();
  }

-  /**
-   * Method description
-   *
-   */
  @Test
-  public void testSingleMatchesOneIgnoreCase()
-  {
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test", true), "tEsT"));
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test", true),
-      "heLLo teSt"));
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test", true),
-      "TEST hellO"));
-    assertTrue(SearchUtil.matchesOne(new SearchRequest("test", true),
-      "hEllO tEsT hEllO"));
-    assertFalse(SearchUtil.matchesOne(new SearchRequest("test", true),
-      "heLLo heLLo"));
-    assertFalse(SearchUtil.matchesOne(new SearchRequest("test", true),
-      "heLLo te heLLo"));
+  public void testSingleMatchesOneIgnoreCase() {
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test", true), "tEsT")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test", true), "heLLo teSt")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test", true), "TEST hellO")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test", true), "hEllO tEsT hEllO")).isTrue();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test", true), "heLLo heLLo")).isFalse();
+    assertThat(SearchUtil.matchesOne(new SearchRequest("test", true), "heLLo te heLLo")).isFalse();
  }

  /**
   * Test for issue 441
-   *
   */
  @Test
-  public void testSpecialCharacter()
-  {
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test\\hansolo"),
-      "test\\hansolo"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("*\\hansolo"),
-      "test\\hansolo"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test\\*"),
-      "test\\hansolo"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("test\\hansolo"),
-      "abc test\\hansolo abc"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test\\hansolo"),
-      "testhansolo"));
-    assertFalse(SearchUtil.matchesAll(new SearchRequest("test\\hansolo"),
-      "test\\hnsolo"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("{test,hansolo} tst"),
-      "{test,hansolo} tst"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("(test,hansolo) tst"),
-      "(test,hansolo) tst"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("[test,hansolo] tst"),
-      "[test,hansolo] tst"));
+  public void testSpecialCharacter() {
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test\\hansolo"), "test\\hansolo")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("*\\hansolo"), "test\\hansolo")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test\\*"), "test\\hansolo")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test\\hansolo"), "abc test\\hansolo abc")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test\\hansolo"), "testhansolo")).isFalse();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("test\\hansolo"), "test\\hnsolo")).isFalse();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("{test,hansolo} tst"), "{test,hansolo} tst")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("(test,hansolo) tst"), "(test,hansolo) tst")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("[test,hansolo] tst"), "[test,hansolo] tst")).isTrue();
  }

-  /**
-   * Method description
-   *
-   */
  @Test
-  public void testWildcardMatches()
-  {
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("*test*"),
-      "hello test hello"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("?es?"), "test"));
-    assertTrue(SearchUtil.matchesAll(new SearchRequest("t*t"), "test"));
+  public void testWildcardMatches() {
+    assertThat(SearchUtil.matchesAll(new SearchRequest("*test*"), "hello test hello")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("?es?"), "test")).isTrue();
+    assertThat(SearchUtil.matchesAll(new SearchRequest("t*t"), "test")).isTrue();
+  }
+
+  @Test
+  public void shouldReturnLimitedResults() {
+    SearchRequest searchRequest = new SearchRequest("*test*", true, 2);
+    Collection<User> search = SearchUtil.search(
+      searchRequest,
+      userList("sometester", "maintester", "anotherTester"),
+      applySearchFilter(searchRequest)
+    );
+
+    assertThat(search).hasSize(2);
+  }
+
+  @Test
+  public void shouldReturnAllResultsIfLimitIsNegativInt() {
+    SearchRequest searchRequest = new SearchRequest("*test*", true, -1);
+    Collection<User> search = SearchUtil.search(
+      searchRequest,
+      userList("sometester", "maintester", "anotherOne"),
+      applySearchFilter(searchRequest)
+    );
+
+    assertThat(search).hasSize(2);
+  }
+
+  private Collection<User> userList(String... userNames) {
+    return Arrays.stream(userNames).map(User::new).collect(toList());
+  }
+
+  private TransformFilter<User, User> applySearchFilter(SearchRequest searchRequest) {
+    return item -> {
+      if (SearchUtil.matchesOne(searchRequest, item.getName())) {
+        return item;
+      }
+      return null;
+    };
  }
 }
--- a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupManager.java
+++ b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupManager.java
@@ -21,7 +21,7 @@
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 */
-    
+
 package sonia.scm.group;

 //~--- non-JDK imports --------------------------------------------------------
@@ -187,23 +187,14 @@ public class DefaultGroupManager extends AbstractGroupManager

    final PermissionActionCheck<Group> check = GroupPermissions.read();
    return SearchUtil.search(searchRequest, groupDAO.getAll(),
-      new TransformFilter<Group, Group>()
-    {
-      @Override
-      public Group accept(Group group)
-      {
-        Group result = null;
-
-        if (check.isPermitted(group) && matches(searchRequest, group))
-        {
-          result = group.clone();
+      group -> {
+        if (check.isPermitted(group) && matches(searchRequest, group)) {
+          return group.clone();
        }
-
-        return result;
-      }
-    });
+        return null;
+      });
  }
-  
+
  private boolean matches(SearchRequest searchRequest, Group group) {
    return SearchUtil.matchesOne(searchRequest, group.getName(), group.getDescription());
  }
@@ -222,7 +213,7 @@ public class DefaultGroupManager extends AbstractGroupManager
  public Group get(String id)
  {
    GroupPermissions.read(id).check();
-    
+
    Group group = groupDAO.get(id);

    if (group != null)
--- a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java
+++ b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java
@@ -21,7 +21,7 @@
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 */
-    
+
 package sonia.scm.user;

 import com.github.sdorra.ssp.PermissionActionCheck;
@@ -217,16 +217,11 @@ public class DefaultUserManager extends AbstractUserManager
    }

    final PermissionActionCheck<User> check = UserPermissions.read();
-    return SearchUtil.search(searchRequest, userDAO.getAll(), new TransformFilter<User, User>() {
-      @Override
-      public User accept(User user)
-      {
-        User result = null;
-        if (check.isPermitted(user) && matches(searchRequest, user)) {
-          result = user.clone();
-        }
-        return result;
+    return SearchUtil.search(searchRequest, userDAO.getAll(), user -> {
+      if (check.isPermitted(user) && matches(searchRequest, user)) {
+        return user.clone();
      }
+      return null;
    });
  }

@@ -313,16 +308,11 @@ public class DefaultUserManager extends AbstractUserManager
  public Collection<User> getAll(Comparator<User> comaparator, int start, int limit) {
    final PermissionActionCheck<User> check = UserPermissions.read();
    return Util.createSubCollection(userDAO.getAll(), comaparator,
-      new CollectionAppender<User>()
-    {
-      @Override
-      public void append(Collection<User> collection, User item)
-      {
+      (collection, item) -> {
        if (check.isPermitted(item)) {
          collection.add(item.clone());
        }
-      }
-    }, start, limit);
+      }, start, limit);
  }

  /**