From 7579d91505f66d4c3bfcbe3ef79e57a39a532517 Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Wed, 21 Apr 2021 16:19:16 +0200 Subject: [PATCH] Fix limit with negative integer for SearchUtil (#1627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: René Pfeuffer --- gradle/changelog/search_util_limit.yaml | 2 + .../java/sonia/scm/search/SearchUtil.java | 185 ++++---------- .../java/sonia/scm/search/SearchUtilTest.java | 234 ++++++++---------- .../sonia/scm/group/DefaultGroupManager.java | 25 +- .../sonia/scm/user/DefaultUserManager.java | 24 +- 5 files changed, 160 insertions(+), 310 deletions(-) create mode 100644 gradle/changelog/search_util_limit.yaml diff --git a/gradle/changelog/search_util_limit.yaml b/gradle/changelog/search_util_limit.yaml new file mode 100644 index 0000000000..5d9921aa1b --- /dev/null +++ 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)) diff --git a/scm-core/src/main/java/sonia/scm/search/SearchUtil.java b/scm-core/src/main/java/sonia/scm/search/SearchUtil.java index 9c09461cdb..30a2783fcb 100644 --- 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 - * - * @return - */ public static Collection search(SearchRequest searchRequest, - Collection collection, TransformFilter filter) - { + Collection collection, TransformFilter filter) { List items = new ArrayList<>(); int index = 0; int counter = 0; - Iterator 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(); diff --git a/scm-core/src/test/java/sonia/scm/search/SearchUtilTest.java b/scm-core/src/test/java/sonia/scm/search/SearchUtilTest.java index f309839243..d83921ecfc 100644 --- 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 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 search = SearchUtil.search( + searchRequest, + userList("sometester", "maintester", "anotherOne"), + applySearchFilter(searchRequest) + ); + + assertThat(search).hasSize(2); + } + + private Collection userList(String... userNames) { + return Arrays.stream(userNames).map(User::new).collect(toList()); + } + + private TransformFilter applySearchFilter(SearchRequest searchRequest) { + return item -> { + if (SearchUtil.matchesOne(searchRequest, item.getName())) { + return item; + } + return null; + }; } } diff --git a/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupManager.java b/scm-webapp/src/main/java/sonia/scm/group/DefaultGroupManager.java index 8a05eb6c96..b19cc14e50 100644 --- 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 check = GroupPermissions.read(); return SearchUtil.search(searchRequest, groupDAO.getAll(), - new TransformFilter() - { - @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) diff --git a/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java b/scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java index 00b0216818..8f21293fbe 100644 --- 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 check = UserPermissions.read(); - return SearchUtil.search(searchRequest, userDAO.getAll(), new TransformFilter() { - @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 getAll(Comparator comaparator, int start, int limit) { final PermissionActionCheck check = UserPermissions.read(); return Util.createSubCollection(userDAO.getAll(), comaparator, - new CollectionAppender() - { - @Override - public void append(Collection collection, User item) - { + (collection, item) -> { if (check.isPermitted(item)) { collection.add(item.clone()); } - } - }, start, limit); + }, start, limit); } /**