SONAR-3893 Fixed snapshot source and data querying for better performance

6 files changed, 52 insertions, 21 deletions

diff --git a/sonar-core/src/main/java/org/sonar/core/source/HtmlSourceDecorator.java b/sonar-core/src/main/java/org/sonar/core/source/HtmlSourceDecorator.java
index e8772ee357c..1e3143fba3e 100644
--- a/sonar-core/src/main/java/org/sonar/core/source/HtmlSourceDecorator.java
+++ b/sonar-core/src/main/java/org/sonar/core/source/HtmlSourceDecorator.java
@@ -20,6 +20,7 @@
 
 package org.sonar.core.source;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import org.sonar.core.persistence.MyBatis;
 import org.sonar.core.source.jdbc.SnapshotDataDao;
@@ -37,23 +38,31 @@ public class HtmlSourceDecorator {
   private final SnapshotDataDao snapshotDataDao;
 
   public HtmlSourceDecorator(MyBatis myBatis) {
-    snapshotSourceDao = new SnapshotSourceDao(myBatis);
-    snapshotDataDao = new SnapshotDataDao(myBatis);
+    this.snapshotSourceDao = new SnapshotSourceDao(myBatis);
+    this.snapshotDataDao = new SnapshotDataDao(myBatis);
+  }
+
+  @VisibleForTesting
+  protected HtmlSourceDecorator(SnapshotSourceDao snapshotSourceDao, SnapshotDataDao snapshotDataDao) {
+    this.snapshotSourceDao = snapshotSourceDao;
+    this.snapshotDataDao= snapshotDataDao;
   }
 
   public Collection<String> getDecoratedSourceAsHtml(long snapshotId) {
 
-    String snapshotSource = snapshotSourceDao.selectSnapshotSource(snapshotId);
     Collection<SnapshotDataDto> snapshotDataEntries = snapshotDataDao.selectSnapshotData(snapshotId);
 
-    if (snapshotSource != null && snapshotDataEntries != null) {
-      DecorationDataHolder decorationDataHolder = new DecorationDataHolder();
-      for (SnapshotDataDto snapshotDataEntry : snapshotDataEntries) {
-        loadSnapshotData(decorationDataHolder, snapshotDataEntry);
-      }
+    if (!snapshotDataEntries.isEmpty()) {
+      String snapshotSource = snapshotSourceDao.selectSnapshotSource(snapshotId);
+      if(snapshotSource != null) {
+        DecorationDataHolder decorationDataHolder = new DecorationDataHolder();
+        for (SnapshotDataDto snapshotDataEntry : snapshotDataEntries) {
+          loadSnapshotData(decorationDataHolder, snapshotDataEntry);
+        }
 
-      HtmlTextDecorator textDecorator = new HtmlTextDecorator();
-      return textDecorator.decorateTextWithHtml(snapshotSource, decorationDataHolder);
+        HtmlTextDecorator textDecorator = new HtmlTextDecorator();
+        return textDecorator.decorateTextWithHtml(snapshotSource, decorationDataHolder);
+      }
     }
     return null;
   }
diff --git a/sonar-core/src/test/java/org/sonar/core/source/HtmlSourceDecoratorTest.java b/sonar-core/src/test/java/org/sonar/core/source/HtmlSourceDecoratorTest.java
index 091809b01e9..2c122618c78 100644
--- a/sonar-core/src/test/java/org/sonar/core/source/HtmlSourceDecoratorTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/source/HtmlSourceDecoratorTest.java
@@ -23,10 +23,13 @@ package org.sonar.core.source;
 import org.junit.Before;
 import org.junit.Test;
 import org.sonar.core.persistence.AbstractDaoTestCase;
+import org.sonar.core.source.jdbc.SnapshotDataDao;
+import org.sonar.core.source.jdbc.SnapshotSourceDao;
 
 import java.util.List;
 
 import static org.fest.assertions.Assertions.assertThat;
+import static org.mockito.Mockito.*;
 
 public class HtmlSourceDecoratorTest extends AbstractDaoTestCase {
 
@@ -38,11 +41,11 @@ public class HtmlSourceDecoratorTest extends AbstractDaoTestCase {
   @Test
   public void should_highlight_syntax_with_html() throws Exception {
 
-    HtmlSourceDecorator highlighter = new HtmlSourceDecorator(getMyBatis());
+    HtmlSourceDecorator sourceDecorator = new HtmlSourceDecorator(getMyBatis());
 
-    List<String> highlightedSource = (List<String>) highlighter.getDecoratedSourceAsHtml(11L);
+    List<String> decoratedSource = (List<String>) sourceDecorator.getDecoratedSourceAsHtml(11L);
 
-    assertThat(highlightedSource).containsExactly(
+    assertThat(decoratedSource).containsExactly(
       "<span class=\"cppd\">/*</span>",
       "<span class=\"cppd\"> * Header</span>",
       "<span class=\"cppd\"> */</span>",
@@ -55,11 +58,11 @@ public class HtmlSourceDecoratorTest extends AbstractDaoTestCase {
   @Test
   public void should_mark_symbols_with_html() throws Exception {
 
-    HtmlSourceDecorator highlighter = new HtmlSourceDecorator(getMyBatis());
+    HtmlSourceDecorator sourceDecorator = new HtmlSourceDecorator(getMyBatis());
 
-    List<String> highlightedSource = (List<String>) highlighter.getDecoratedSourceAsHtml(12L);
+    List<String> decoratedSource = (List<String>) sourceDecorator.getDecoratedSourceAsHtml(12L);
 
-    assertThat(highlightedSource).containsExactly(
+    assertThat(decoratedSource).containsExactly(
       "/*",
       " * Header",
       " */",
@@ -72,11 +75,11 @@ public class HtmlSourceDecoratorTest extends AbstractDaoTestCase {
   @Test
   public void should_decorate_source_with_multiple_decoration_strategies() throws Exception {
 
-    HtmlSourceDecorator highlighter = new HtmlSourceDecorator(getMyBatis());
+    HtmlSourceDecorator sourceDecorator = new HtmlSourceDecorator(getMyBatis());
 
-    List<String> highlightedSource = (List<String>) highlighter.getDecoratedSourceAsHtml(13L);
+    List<String> decoratedSource = (List<String>) sourceDecorator.getDecoratedSourceAsHtml(13L);
 
-    assertThat(highlightedSource).containsExactly(
+    assertThat(decoratedSource).containsExactly(
       "<span class=\"cppd\">/*</span>",
       "<span class=\"cppd\"> * Header</span>",
       "<span class=\"cppd\"> */</span>",
@@ -90,4 +93,18 @@ public class HtmlSourceDecoratorTest extends AbstractDaoTestCase {
       "}"
     );
   }
+
+  @Test
+  public void should_not_query_sources_if_no_snapshot_data() throws Exception {
+
+    SnapshotSourceDao snapshotSourceDao = mock(SnapshotSourceDao.class);
+    SnapshotDataDao snapshotDataDao = mock(SnapshotDataDao.class);
+
+    HtmlSourceDecorator sourceDecorator = new HtmlSourceDecorator(snapshotSourceDao, snapshotDataDao);
+
+    sourceDecorator.getDecoratedSourceAsHtml(14L);
+
+    verify(snapshotDataDao, times(1)).selectSnapshotData(14L);
+    verify(snapshotSourceDao, times(0)).selectSnapshotSource(14L);
+  }
 }
diff --git a/sonar-core/src/test/resources/org/sonar/core/source/HtmlSourceDecoratorTest/shared.xml b/sonar-core/src/test/resources/org/sonar/core/source/HtmlSourceDecoratorTest/shared.xml
index 6d2de2bcd94..965effbcffe 100644
--- a/sonar-core/src/test/resources/org/sonar/core/source/HtmlSourceDecoratorTest/shared.xml
+++ b/sonar-core/src/test/resources/org/sonar/core/source/HtmlSourceDecoratorTest/shared.xml
@@ -5,6 +5,7 @@
     <snapshots id="11" project_id="1" islast="[true]" />
     <snapshots id="12" project_id="2" islast="[true]" />
     <snapshots id="13" project_id="3" islast="[true]" />
+    <snapshots id="14" project_id="3" islast="[true]" />
 
     <snapshot_data id="101" resource_id="1" snapshot_id="11" data="0,16,cppd;18,25,k;25,31,k;" data_type="highlight_syntax" />
     <snapshot_data id="102" resource_id="2" snapshot_id="12" data="31,41,31;" data_type="symbol" />
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/resource_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/resource_controller.rb
index 1445b6fec67..603fc2e7de0 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/resource_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/resource_controller.rb
@@ -160,7 +160,8 @@ class ResourceController < ApplicationController
     @period = params[:period].to_i unless params[:period].blank?
     @expanded=(params[:expand]=='true')
     @display_manual_violation_form=(current_user && has_role?(:user, @snapshot))
-    if @snapshot.source
+
+    if @snapshot.has_source
       source_lines = @snapshot.highlighting_data || @snapshot.source.syntax_highlighted_lines()
       init_scm()
 
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/snapshot.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/snapshot.rb
index 01c70cb0f3a..6d09897f69b 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/models/snapshot.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/models/snapshot.rb
@@ -256,6 +256,9 @@ class Snapshot < ActiveRecord::Base
     Java::OrgSonarServerUi::JRubyFacade.getInstance().getHighlightedSourceLines(id)
   end
 
+  def has_source
+    SnapshotSource.count('id', :conditions => "snapshot_id = #{id}") > 0
+  end
 
   private
 
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_tabs.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_tabs.html.erb
index dac05c94dca..dec63d2d542 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_tabs.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/resource/_tabs.html.erb
@@ -23,7 +23,7 @@
     <ul class="tablinks">
       <%
          first=true
-         if @snapshot.source && has_role?(:codeviewer, @snapshot)
+         if @snapshot.has_source && has_role?(:codeviewer, @snapshot)
       %>
         <li class="<%= 'first' if first -%>">
           <a href="<%= ApplicationController.root_context -%>/api/sources?resource=<%= @resource.key -%>&amp;format=txt"><%= message('raw') -%></a>