SONAR-3755 Improve IssueFinder pagination

12 files changed, 206 insertions, 59 deletions

diff --git a/sonar-core/src/main/java/org/sonar/core/issue/IssueDao.java b/sonar-core/src/main/java/org/sonar/core/issue/IssueDao.java
index 941bf0873ae..3f2add7205f 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/IssueDao.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/IssueDao.java
@@ -22,7 +22,6 @@ package org.sonar.core.issue;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
-import org.apache.ibatis.session.RowBounds;
 import org.apache.ibatis.session.SqlSession;
 import org.sonar.api.BatchComponent;
 import org.sonar.api.ServerComponent;
@@ -30,6 +29,7 @@ import org.sonar.api.issue.IssueQuery;
 import org.sonar.core.persistence.MyBatis;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
@@ -102,28 +102,24 @@ public class IssueDao implements BatchComponent, ServerComponent {
   public List<IssueDto> select(IssueQuery query) {
     SqlSession session = mybatis.openSession();
     try {
-      return select(query, session);
+      return session.selectList("org.sonar.core.issue.IssueMapper.select", query);
     } finally {
       MyBatis.closeQuietly(session);
     }
   }
 
-  public List<IssueDto> select(IssueQuery query, SqlSession session) {
-    // TODO support ordering
-
-    return session.selectList("org.sonar.core.issue.IssueMapper.select", query, new RowBounds(query.offset(), query.limit()));
-  }
-
   /**
    * The returned IssueDto list contains only the issue id and the resource id
    */
   public List<IssueDto> selectIssueIdsAndComponentsId(IssueQuery query, SqlSession session) {
     // TODO support ordering
-
     return session.selectList("org.sonar.core.issue.IssueMapper.selectIssueIdsAndComponentsId", query);
   }
 
   public Collection<IssueDto> selectByIds(Collection<Long> ids, SqlSession session) {
+    if (ids.isEmpty()) {
+      return Collections.emptyList();
+    }
     List <List<Long>> idsPartition = Lists.partition(newArrayList(ids), 1000);
     Map<String, List <List<Long>>> params = ImmutableMap.of("ids", idsPartition);
     return session.selectList("org.sonar.core.issue.IssueMapper.selectByIds", params);
diff --git a/sonar-core/src/main/java/org/sonar/core/resource/ResourceDao.java b/sonar-core/src/main/java/org/sonar/core/resource/ResourceDao.java
index 5e5d0f8d435..79e5d1b13c0 100644
--- a/sonar-core/src/main/java/org/sonar/core/resource/ResourceDao.java
+++ b/sonar-core/src/main/java/org/sonar/core/resource/ResourceDao.java
@@ -19,12 +19,14 @@
  */
 package org.sonar.core.resource;
 
+import com.google.common.collect.Lists;
 import org.apache.ibatis.session.SqlSession;
 import org.sonar.api.component.Component;
 import org.sonar.core.component.ComponentDto;
 import org.sonar.core.persistence.MyBatis;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Date;
 import java.util.List;
 
@@ -133,9 +135,13 @@ public class ResourceDao {
   }
 
   public Collection<Component> findByIds(Collection<Integer> ids) {
+    if (ids.isEmpty()) {
+      return Collections.emptyList();
+    }
     SqlSession session = mybatis.openSession();
     try {
-      Collection<ResourceDto> resources =  session.getMapper(ResourceMapper.class).selectResourcesById(ids);
+      List <List<Integer>> idsPartition = Lists.partition(newArrayList(ids), 1000);
+      Collection<ResourceDto> resources =  session.getMapper(ResourceMapper.class).selectResourcesById(idsPartition);
       Collection<Component> components = newArrayList();
       for (ResourceDto resourceDto : resources) {
         components.add(toComponent(resourceDto));
diff --git a/sonar-core/src/main/java/org/sonar/core/resource/ResourceMapper.java b/sonar-core/src/main/java/org/sonar/core/resource/ResourceMapper.java
index a5a1e603f7b..13093cce06c 100644
--- a/sonar-core/src/main/java/org/sonar/core/resource/ResourceMapper.java
+++ b/sonar-core/src/main/java/org/sonar/core/resource/ResourceMapper.java
@@ -22,7 +22,6 @@ package org.sonar.core.resource;
 import org.apache.ibatis.annotations.Param;
 import org.apache.ibatis.session.ResultHandler;
 
-import java.util.Collection;
 import java.util.List;
 
 public interface ResourceMapper {
@@ -54,7 +53,7 @@ public interface ResourceMapper {
   /**
    * @since3.6
    */
-  List<ResourceDto> selectResourcesById(@Param("ids") Collection<Integer> ids);
+  List<ResourceDto> selectResourcesById(@Param("ids") List <List<Integer>> ids);
 
   void insert(ResourceDto resource);
 
diff --git a/sonar-core/src/main/java/org/sonar/core/rule/DefaultRuleFinder.java b/sonar-core/src/main/java/org/sonar/core/rule/DefaultRuleFinder.java
index 50996b16219..667549554be 100644
--- a/sonar-core/src/main/java/org/sonar/core/rule/DefaultRuleFinder.java
+++ b/sonar-core/src/main/java/org/sonar/core/rule/DefaultRuleFinder.java
@@ -31,6 +31,7 @@ import org.sonar.jpa.session.DatabaseSessionFactory;
 import javax.persistence.Query;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -57,6 +58,9 @@ public class DefaultRuleFinder implements RuleFinder {
   }
 
   public Collection<Rule> findByIds(Collection<Integer> ruleIds) {
+    if (ruleIds.isEmpty()) {
+      return Collections.emptyList();
+    }
     DatabaseSession session = sessionFactory.getSession();
     StringBuilder hql = new StringBuilder().append("from ").append(Rule.class.getSimpleName()).append(" r where r.id in (:ids) and status<>:status ");
     Query hqlQuery = session.createQuery(hql.toString())
diff --git a/sonar-core/src/main/resources/org/sonar/core/resource/ResourceMapper.xml b/sonar-core/src/main/resources/org/sonar/core/resource/ResourceMapper.xml
index 24482461298..ded87d52b8f 100644
--- a/sonar-core/src/main/resources/org/sonar/core/resource/ResourceMapper.xml
+++ b/sonar-core/src/main/resources/org/sonar/core/resource/ResourceMapper.xml
@@ -76,9 +76,13 @@
   </select>
 
   <select id="selectResourcesById" parameterType="map" resultMap="resourceResultMap">
-    select * from projects p where p.enabled=${_true} and p.id in
-    <foreach item="id" index="index" collection="ids" open="(" separator="," close=")">#{id}
+    select * from projects p where p.enabled=${_true} and
+    <foreach collection="ids" open="p.id in (" close=")" item="list" separator=") or p.id in (" >
+      <foreach collection="list" item="element" separator=",">
+        #{element}
+      </foreach>
     </foreach>
+
   </select>
 
   <select id="selectSnapshot" parameterType="long" resultMap="snapshotResultMap">
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueFinder.java b/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueFinder.java
index e3c3b5a62ce..d40120f32db 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueFinder.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueFinder.java
@@ -47,6 +47,8 @@ public interface IssueFinder extends ServerComponent {
     Component component(Issue issue);
 
     Collection<Component> components();
+
+    Pagination pagination();
   }
 
   Results find(IssueQuery query, @Nullable Integer currentUserId, String role);
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueQuery.java b/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueQuery.java
index 4ad5bc1965b..b481db42961 100644
--- a/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueQuery.java
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/issue/IssueQuery.java
@@ -116,16 +116,8 @@ public class IssueQuery {
     return limit;
   }
 
-//  public int pages(int count) {
-//    int p = (count / limit);
-//    if ((count % limit) > 0) {
-//      p++;
-//    }
-//    return p;
-//  }
-
-  public int offset(){
-    return (page - 1) * limit;
+  public int page() {
+    return page;
   }
 
   @Override
diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/issue/Pagination.java b/sonar-plugin-api/src/main/java/org/sonar/api/issue/Pagination.java
new file mode 100644
index 00000000000..bacb6d2d637
--- /dev/null
+++ b/sonar-plugin-api/src/main/java/org/sonar/api/issue/Pagination.java
@@ -0,0 +1,62 @@
+/*
+ * SonarQube, open source software quality management tool.
+ * Copyright (C) 2008-2013 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * SonarQube is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * SonarQube is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+package org.sonar.api.issue;
+
+public class Pagination {
+
+  private int limit;
+  private int page;
+  private int count;
+
+  public Pagination(int limit, int page, int count) {
+    this.limit = limit;
+    this.page = page;
+    this.count = count;
+  }
+
+  public int page() {
+    return page;
+  }
+
+  public int limit() {
+    return limit;
+  }
+
+  public int count() {
+    return count;
+  }
+
+  public int offset(){
+    return (page - 1) * limit;
+  }
+
+  public int pages() {
+    int p = (count / limit);
+    if ((count % limit) > 0) {
+      p++;
+    }
+    return p;
+  }
+
+  public boolean isEmpty() {
+    return count == 0;
+  }
+}
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/issue/IssueQueryTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/issue/IssueQueryTest.java
index dd1a2a38990..8f46066a642 100644
--- a/sonar-plugin-api/src/test/java/org/sonar/api/issue/IssueQueryTest.java
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/issue/IssueQueryTest.java
@@ -59,6 +59,6 @@ public class IssueQueryTest {
     assertThat(query.createdAfter()).isNotNull();
     assertThat(query.createdBefore()).isNotNull();
     assertThat(query.limit()).isEqualTo(10);
-    assertThat(query.offset()).isEqualTo(10);
+    assertThat(query.page()).isEqualTo(2);
   }
 }
diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/issue/PaginationTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/issue/PaginationTest.java
new file mode 100644
index 00000000000..f93ab52ea5b
--- /dev/null
+++ b/sonar-plugin-api/src/test/java/org/sonar/api/issue/PaginationTest.java
@@ -0,0 +1,50 @@
+/*
+ * SonarQube, open source software quality management tool.
+ * Copyright (C) 2008-2013 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * SonarQube is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * SonarQube is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+package org.sonar.api.issue;
+
+import org.junit.Test;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+public class PaginationTest {
+
+  @Test
+  public void test_pagination(){
+    Pagination pagination = new Pagination(5, 1, 20);
+
+    assertThat(pagination.limit()).isEqualTo(5);
+    assertThat(pagination.page()).isEqualTo(1);
+    assertThat(pagination.count()).isEqualTo(20);
+    assertThat(pagination.isEmpty()).isFalse();
+
+    assertThat(pagination.offset()).isEqualTo(0);
+    assertThat(pagination.pages()).isEqualTo(4);
+  }
+
+  @Test
+  public void test_pagination_on_second_page(){
+    Pagination pagination = new Pagination(5, 2, 20);
+
+    assertThat(pagination.offset()).isEqualTo(5);
+    assertThat(pagination.pages()).isEqualTo(4);
+  }
+
+}
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/ServerIssueFinder.java b/sonar-server/src/main/java/org/sonar/server/issue/ServerIssueFinder.java
index caeeb420aa7..cfe08b790d9 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/ServerIssueFinder.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/ServerIssueFinder.java
@@ -21,7 +21,6 @@ package org.sonar.server.issue;
 
 import com.google.common.base.Predicate;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import org.apache.ibatis.session.SqlSession;
 import org.slf4j.Logger;
@@ -30,6 +29,7 @@ import org.sonar.api.component.Component;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.IssueFinder;
 import org.sonar.api.issue.IssueQuery;
+import org.sonar.api.issue.Pagination;
 import org.sonar.api.rules.Rule;
 import org.sonar.core.issue.IssueDao;
 import org.sonar.core.issue.IssueDto;
@@ -45,6 +45,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import static com.google.common.collect.Lists.newArrayList;
 import static com.google.common.collect.Maps.newHashMap;
 
 /**
@@ -72,43 +73,53 @@ public class ServerIssueFinder implements IssueFinder {
     SqlSession sqlSession = myBatis.openSession();
     try {
       List<IssueDto> allIssuesDto = issueDao.selectIssueIdsAndComponentsId(query, sqlSession);
-
-      Set<Integer> componentIds = Sets.newLinkedHashSet();
-      for (IssueDto issueDto : allIssuesDto) {
-        componentIds.add(issueDto.getResourceId());
-      }
+      Set<Integer> componentIds = getResourceIds(allIssuesDto);
       Set<Integer> authorizedComponentIds = authorizationDao.keepAuthorizedComponentIds(componentIds, currentUserId, role, sqlSession);
-      Set<Long> issueIds = getAutorizedIssueIds(allIssuesDto, authorizedComponentIds, query);
+      List<IssueDto> authorizedIssues = getAuthorizedIssues(allIssuesDto, authorizedComponentIds);
+      Pagination pagination = new Pagination(query.limit(), query.page(), authorizedIssues.size());
+      Set<Long> paginatedAuthorizedIssueIds = getPaginatedAuthorizedIssueIds(authorizedIssues, pagination);
 
-      Collection<IssueDto> dtos = issueDao.selectByIds(issueIds, sqlSession);
+      Collection<IssueDto> dtos = issueDao.selectByIds(paginatedAuthorizedIssueIds, sqlSession);
       Set<Integer> ruleIds = Sets.newLinkedHashSet();
-      List<Issue> issues = Lists.newArrayList();
+      List<Issue> issues = newArrayList();
       for (IssueDto dto : dtos) {
         if (authorizedComponentIds.contains(dto.getResourceId())) {
           issues.add(dto.toDefaultIssue());
           ruleIds.add(dto.getRuleId());
         }
       }
-      if (!issues.isEmpty()) {
-        return new DefaultResults(issues, getRulesByIssue(issues, ruleIds), getComponentsByIssue(issues, componentIds));
-      } else {
-        return new DefaultResults(issues);
-      }
+
+      return new DefaultResults(issues, getRulesByIssue(issues, ruleIds), getComponentsByIssue(issues, componentIds), pagination);
     } finally {
       MyBatis.closeQuietly(sqlSession);
     }
   }
 
-  private Set<Long> getAutorizedIssueIds(List<IssueDto> allIssuesDto, Set<Integer> authorizedComponentIds, IssueQuery query){
+  private Set<Integer> getResourceIds(List<IssueDto> allIssuesDto) {
+    Set<Integer> componentIds = Sets.newLinkedHashSet();
+    for (IssueDto issueDto : allIssuesDto) {
+      componentIds.add(issueDto.getResourceId());
+    }
+    return componentIds;
+  }
+
+  private List<IssueDto> getAuthorizedIssues(List<IssueDto> allIssuesDto, final Set<Integer> authorizedComponentIds) {
+    return newArrayList(Iterables.filter(allIssuesDto, new Predicate<IssueDto>() {
+      @Override
+      public boolean apply(IssueDto issueDto) {
+        return authorizedComponentIds.contains(issueDto.getResourceId());
+      }
+    }));
+  }
+
+  private Set<Long> getPaginatedAuthorizedIssueIds(List<IssueDto> authorizedIssues, Pagination pagination) {
     Set<Long> issueIds = Sets.newLinkedHashSet();
     int index = 0;
-    for (IssueDto issueDto : allIssuesDto) {
-      if (authorizedComponentIds.contains(issueDto.getResourceId())) {
-        if (index >= query.offset() && issueIds.size() < query.limit()) {
-          issueIds.add(issueDto.getId());
-        } else if (issueIds.size() >= query.limit()) {
-          break;
-        }
+    for (IssueDto issueDto : authorizedIssues) {
+      if (index >= pagination.offset() && issueIds.size() < pagination.limit()) {
+        issueIds.add(issueDto.getId());
+      } else if (issueIds.size() >= pagination.limit()) {
+        break;
       }
       index++;
     }
@@ -158,20 +169,15 @@ public class ServerIssueFinder implements IssueFinder {
 
   static class DefaultResults implements Results {
     private final List<Issue> issues;
+    private final Pagination pagination;
     private final Map<Issue, Rule> rulesByIssue;
     private final Map<Issue, Component> componentsByIssue;
-    // TODO add nb total issues and maybe some pagination methods
 
-    DefaultResults(List<Issue> issues, Map<Issue, Rule> rulesByIssue, Map<Issue, Component> componentsByIssue) {
+    DefaultResults(List<Issue> issues, Map<Issue, Rule> rulesByIssue, Map<Issue, Component> componentsByIssue, Pagination pagination) {
       this.issues = issues;
       this.rulesByIssue = rulesByIssue;
       this.componentsByIssue = componentsByIssue;
-    }
-
-    DefaultResults(List<Issue> issues) {
-      this.issues = issues;
-      this.rulesByIssue = newHashMap();
-      this.componentsByIssue = newHashMap();
+      this.pagination = pagination;
     }
 
     @Override
@@ -195,5 +201,8 @@ public class ServerIssueFinder implements IssueFinder {
       return componentsByIssue.values();
     }
 
+    public Pagination pagination() {
+      return pagination;
+    }
   }
 }
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/ServerIssueFinderTest.java b/sonar-server/src/test/java/org/sonar/server/issue/ServerIssueFinderTest.java
index c043ebe1208..9a72ad109fd 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/ServerIssueFinderTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/ServerIssueFinderTest.java
@@ -113,7 +113,33 @@ public class ServerIssueFinderTest {
     IssueFinder.Results results = finder.find(issueQuery, null, UserRole.USER);
 
     verify(issueDao).selectByIds(eq(newHashSet(1L)), any(SqlSession.class));
-    assertThat(results.issues()).hasSize(1);
+  }
+
+  @Test
+  public void should_find_paginate_result() {
+    grantAccessRights();
+
+    IssueQuery issueQuery = mock(IssueQuery.class);
+    when(issueQuery.limit()).thenReturn(1);
+    when(issueQuery.page()).thenReturn(1);
+
+    IssueDto issue1 = new IssueDto().setId(1L).setRuleId(50).setResourceId(123)
+      .setComponentKey_unit_test_only("Action.java")
+      .setRuleKey_unit_test_only("squid", "AvoidCycle");
+    IssueDto issue2 = new IssueDto().setId(2L).setRuleId(50).setResourceId(135)
+      .setComponentKey_unit_test_only("Phases.java")
+      .setRuleKey_unit_test_only("squid", "AvoidCycle");
+    List<IssueDto> dtoList = newArrayList(issue1, issue2);
+    when(issueDao.selectIssueIdsAndComponentsId(eq(issueQuery), any(SqlSession.class))).thenReturn(dtoList);
+    when(issueDao.selectByIds(anyCollection(), any(SqlSession.class))).thenReturn(dtoList);
+
+    IssueFinder.Results results = finder.find(issueQuery, null, UserRole.USER);
+    assertThat(results.pagination().offset()).isEqualTo(0);
+    assertThat(results.pagination().count()).isEqualTo(2);
+    assertThat(results.pagination().pages()).isEqualTo(2);
+
+    // Only one result is expected because the limit is 1
+    verify(issueDao).selectByIds(eq(newHashSet(1L)), any(SqlSession.class));
   }
 
   @Test
@@ -194,9 +220,6 @@ public class ServerIssueFinderTest {
     assertThat(results.issues()).isEmpty();
     assertThat(results.rules()).isEmpty();
     assertThat(results.components()).isEmpty();
-
-    verifyZeroInteractions(ruleFinder);
-    verifyZeroInteractions(resourceDao);
   }
 
   private void grantAccessRights() {