From d0a51bd2596e1766539c72cab4d909747eea0eaa Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Tue, 11 Dec 2012 16:58:57 +0100 Subject: [PATCH] SONAR-3825 improve error handling + compatibility of sorting with oracle --- .../resources/org/sonar/l10n/core.properties | 4 +- .../core/measure/MeasureFilterEngine.java | 30 ++++--- .../core/measure/MeasureFilterResult.java | 82 +++++++++++++++++++ .../sonar/core/measure/MeasureFilterSql.java | 13 ++- .../org/sonar/core/measure/package-info.java | 24 ++++++ .../core/measure/MeasureFilterEngineTest.java | 29 +++---- .../measure/MeasureFilterExecutorTest.java | 7 +- .../MeasureFilterExecutorTest/shared.xml | 14 ++++ .../java/org/sonar/server/ui/JRubyFacade.java | 5 +- .../controllers/all_projects_controller.rb | 9 +- .../WEB-INF/app/helpers/measures_helper.rb | 22 ++--- .../WEB-INF/app/models/measure_filter.rb | 52 ++++++------ .../models/measure_filter_display_treemap.rb | 14 ++-- .../app/views/all_projects/index.html.erb | 45 +++------- .../views/measures/_display_cloud.html.erb | 14 ++-- .../app/views/measures/_display_list.html.erb | 14 ++-- .../views/measures/_display_treemap.html.erb | 4 +- .../app/views/measures/_sidebar.html.erb | 2 +- .../app/views/measures/search.html.erb | 18 +++- 19 files changed, 259 insertions(+), 143 deletions(-) create mode 100644 sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterResult.java create mode 100644 sonar-core/src/main/java/org/sonar/core/measure/package-info.java diff --git a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties index 05b417f6346..11d96ce8ab6 100644 --- a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties +++ b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties @@ -444,10 +444,10 @@ measure_filter.display.list=List measure_filter.display.treemap=Treemap measure_filter.list.change=Change Columns measure_filter.treemap.change=Change Treemap -measure_filter.too_many_results=Too many results. Please refine your search. measure_filter.add_column_button=Add Column measure_filter.widget.unknown_filter_warning=This widget is configured to display a measure filter that doesn't exist anymore. - +measure_filter.error.UNKNOWN=Unexpected error. Please contact the administrator. +measure_filter.error.TOO_MANY_RESULTS=Too many results. Please refine your search. #------------------------------------------------------------------------------ # diff --git a/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterEngine.java b/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterEngine.java index 69bc849667c..84d3211ecf2 100644 --- a/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterEngine.java +++ b/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterEngine.java @@ -22,7 +22,6 @@ package org.sonar.core.measure; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import org.apache.commons.lang.SystemUtils; -import org.json.simple.parser.ParseException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sonar.api.ServerComponent; @@ -37,38 +36,49 @@ public class MeasureFilterEngine implements ServerComponent { private final MeasureFilterFactory factory; private final MeasureFilterExecutor executor; + private static final int MAX_ROWS = 5000; + public MeasureFilterEngine(MeasureFilterFactory factory, MeasureFilterExecutor executor) { this.executor = executor; this.factory = factory; } - public List execute(Map filterMap, @Nullable Long userId) throws ParseException { + public MeasureFilterResult execute(Map filterMap, @Nullable Long userId) { return execute(filterMap, userId, LoggerFactory.getLogger("org.sonar.MEASURE_FILTER")); } @VisibleForTesting - List execute(Map filterMap, @Nullable Long userId, Logger logger) throws ParseException { + MeasureFilterResult execute(Map filterMap, @Nullable Long userId, Logger logger) { + long start = System.currentTimeMillis(); + MeasureFilterResult result = new MeasureFilterResult(); MeasureFilterContext context = new MeasureFilterContext(); context.setUserId(userId); context.setData(String.format("{%s}", Joiner.on('|').withKeyValueSeparator("=").join(filterMap))); try { - long start = System.currentTimeMillis(); MeasureFilter filter = factory.create(filterMap); List rows = executor.execute(filter, context); - log(context, rows, (System.currentTimeMillis() - start), logger); - return rows; + if (rows.size() <= MAX_ROWS) { + result.setRows(rows); + } else { + result.setError(MeasureFilterResult.Error.TOO_MANY_RESULTS); + } + result.setDurationInMs(System.currentTimeMillis() - start); + log(context, result, logger); + } catch (Exception e) { - throw new IllegalStateException("Fail to execute filter: " + context, e); + result.setError(MeasureFilterResult.Error.UNKNOWN); + logger.error("Fail to execute measure filter: " + context, e); } + return result; } - private void log(MeasureFilterContext context, List rows, long durationMs, Logger logger) { + private void log(MeasureFilterContext context, MeasureFilterResult result, Logger logger) { if (logger.isDebugEnabled()) { StringBuilder log = new StringBuilder(); log.append(SystemUtils.LINE_SEPARATOR); - log.append(" filter: ").append(context.getData()).append(SystemUtils.LINE_SEPARATOR); + log.append("request: ").append(context.getData()).append(SystemUtils.LINE_SEPARATOR); + log.append(" result: ").append(result.toString()).append(SystemUtils.LINE_SEPARATOR); log.append(" sql: ").append(context.getSql()).append(SystemUtils.LINE_SEPARATOR); - log.append("results: ").append(rows.size()).append(" rows in ").append(durationMs).append("ms").append(SystemUtils.LINE_SEPARATOR); logger.debug(log.toString()); } } diff --git a/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterResult.java b/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterResult.java new file mode 100644 index 00000000000..118733e9e59 --- /dev/null +++ b/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterResult.java @@ -0,0 +1,82 @@ +/* + * Sonar, open source software quality management tool. + * Copyright (C) 2008-2012 SonarSource + * mailto:contact AT sonarsource DOT com + * + * Sonar 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. + * + * Sonar 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 Sonar; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02 + */ +package org.sonar.core.measure; + +import javax.annotation.Nullable; + +import java.util.List; + +public class MeasureFilterResult { + + public static enum Error { + TOO_MANY_RESULTS, UNKNOWN + } + + private List rows = null; + private Error error = null; + private long durationInMs; + + MeasureFilterResult() { + } + + public List getRows() { + return rows; + } + + public Error getError() { + return error; + } + + public long getDurationInMs() { + return durationInMs; + } + + MeasureFilterResult setDurationInMs(long l) { + this.durationInMs = l; + return this; + } + + MeasureFilterResult setRows(@Nullable List rows) { + this.rows = rows; + return this; + } + + MeasureFilterResult setError(@Nullable Error err) { + this.error = err; + return this; + } + + public boolean isSuccess() { + return error == null; + } + + @Override + public String toString() { + StringBuilder sb = new StringBuilder(); + if (rows != null) { + sb.append(rows.size()).append(" rows, "); + } + if (error != null) { + sb.append("error=").append(error).append(", "); + } + sb.append(durationInMs).append("ms"); + return sb.toString(); + } +} diff --git a/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterSql.java b/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterSql.java index 21a99ef5197..0da49a1f0f2 100644 --- a/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterSql.java +++ b/sonar-core/src/main/java/org/sonar/core/measure/MeasureFilterSql.java @@ -75,7 +75,7 @@ class MeasureFilterSql { } private void init() { - sql.append("SELECT block.id, max(block.rid) as rid, max(block.rootid) as rootid, max(sortval) as sortval"); + sql.append("SELECT block.id, max(block.rid) AS rid, max(block.rootid) AS rootid, max(sortval) AS sortval1, CASE WHEN sortval IS NULL THEN 1 ELSE 0 END AS sortval2 "); for (int index = 0; index < filter.getMeasureConditions().size(); index++) { sql.append(", max(crit_").append(index).append(")"); } @@ -88,7 +88,7 @@ class MeasureFilterSql { appendConditionBlock(index, condition); } - sql.append(") block GROUP BY block.id"); + sql.append(") block GROUP BY block.id, sortval2"); if (!filter.getMeasureConditions().isEmpty()) { sql.append(" HAVING "); for (int index = 0; index < filter.getMeasureConditions().size(); index++) { @@ -99,14 +99,13 @@ class MeasureFilterSql { } } if (filter.sort().isSortedByDatabase()) { - sql.append(" ORDER BY sortval "); - sql.append(filter.sort().isAsc() ? "ASC" : "DESC"); - sql.append(" NULLS LAST"); + sql.append(" ORDER BY sortval2 ASC, sortval1 "); + sql.append(filter.sort().isAsc() ? "ASC " : "DESC "); } } private void appendSortBlock() { - sql.append(" SELECT s.id, s.project_id AS rid, s.root_project_id AS rootid, ").append(filter.sort().column()).append(" AS sortval"); + sql.append(" SELECT s.id, s.project_id AS rid, s.root_project_id AS rootid, ").append(filter.sort().column()).append(" AS sortval "); for (int index = 0; index < filter.getMeasureConditions().size(); index++) { MeasureFilterCondition condition = filter.getMeasureConditions().get(index); sql.append(", ").append(nullSelect(condition.metric())).append(" AS crit_").append(index); @@ -125,7 +124,7 @@ class MeasureFilterSql { } private void appendConditionBlock(int conditionIndex, MeasureFilterCondition condition) { - sql.append(" SELECT s.id, s.project_id AS rid, s.root_project_id AS rootid, null AS sortval"); + sql.append(" SELECT s.id, s.project_id AS rid, s.root_project_id AS rootid, null AS sortval "); for (int j = 0; j < filter.getMeasureConditions().size(); j++) { sql.append(", "); if (j == conditionIndex) { diff --git a/sonar-core/src/main/java/org/sonar/core/measure/package-info.java b/sonar-core/src/main/java/org/sonar/core/measure/package-info.java new file mode 100644 index 00000000000..6b2f8b948d7 --- /dev/null +++ b/sonar-core/src/main/java/org/sonar/core/measure/package-info.java @@ -0,0 +1,24 @@ +/* + * Sonar, open source software quality management tool. + * Copyright (C) 2008-2012 SonarSource + * mailto:contact AT sonarsource DOT com + * + * Sonar 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. + * + * Sonar 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 Sonar; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02 + */ +@ParametersAreNonnullByDefault +package org.sonar.core.measure; + +import javax.annotation.ParametersAreNonnullByDefault; + diff --git a/sonar-core/src/test/java/org/sonar/core/measure/MeasureFilterEngineTest.java b/sonar-core/src/test/java/org/sonar/core/measure/MeasureFilterEngineTest.java index 06051a65d5d..95a4e8fc992 100644 --- a/sonar-core/src/test/java/org/sonar/core/measure/MeasureFilterEngineTest.java +++ b/sonar-core/src/test/java/org/sonar/core/measure/MeasureFilterEngineTest.java @@ -20,29 +20,26 @@ package org.sonar.core.measure; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; import org.hamcrest.BaseMatcher; import org.hamcrest.Description; -import org.json.simple.parser.ParseException; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.ExpectedException; import org.slf4j.Logger; -import java.util.HashMap; import java.util.Map; +import static org.fest.assertions.Assertions.assertThat; import static org.mockito.Matchers.argThat; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.refEq; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class MeasureFilterEngineTest { - @Rule - public ExpectedException thrown = ExpectedException.none(); - @Test public void should_create_and_execute_filter() throws Exception { - Map filterMap = ImmutableMap.of("qualifiers", (Object) "TRK"); + Map filterMap = ImmutableMap.of("qualifiers", (Object) "TRK"); MeasureFilterFactory factory = mock(MeasureFilterFactory.class); MeasureFilter filter = new MeasureFilter(); when(factory.create(filterMap)).thenReturn(filter); @@ -67,16 +64,16 @@ public class MeasureFilterEngineTest { } @Test - public void throw_definition_of_filter_on_error() throws Exception { - thrown.expect(IllegalStateException.class); - thrown.expectMessage("filter={qualifiers=TRK}"); - - Map filterMap = ImmutableMap.of("qualifiers", (Object)"TRK"); + public void keep_error_but_do_not_fail() throws Exception { + Map filterMap = ImmutableMap.of("qualifiers", (Object) "TRK"); MeasureFilterFactory factory = mock(MeasureFilterFactory.class); when(factory.create(filterMap)).thenThrow(new IllegalArgumentException()); MeasureFilterExecutor executor = mock(MeasureFilterExecutor.class); MeasureFilterEngine engine = new MeasureFilterEngine(factory, executor); - engine.execute(filterMap, 50L); + MeasureFilterResult result = engine.execute(filterMap, 50L); + + assertThat(result.isSuccess()).isFalse(); + assertThat(result.getError()).isEqualTo(MeasureFilterResult.Error.UNKNOWN); } } diff --git a/sonar-core/src/test/java/org/sonar/core/measure/MeasureFilterExecutorTest.java b/sonar-core/src/test/java/org/sonar/core/measure/MeasureFilterExecutorTest.java index 3416b62fa8e..4eeab0003ec 100644 --- a/sonar-core/src/test/java/org/sonar/core/measure/MeasureFilterExecutorTest.java +++ b/sonar-core/src/test/java/org/sonar/core/measure/MeasureFilterExecutorTest.java @@ -46,6 +46,7 @@ public class MeasureFilterExecutorTest extends AbstractDaoTestCase { private static final Metric METRIC_LINES = new Metric.Builder("lines", "Lines", Metric.ValueType.INT).create().setId(1); private static final Metric METRIC_PROFILE = new Metric.Builder("profile", "Profile", Metric.ValueType.STRING).create().setId(2); private static final Metric METRIC_COVERAGE = new Metric.Builder("coverage", "Coverage", Metric.ValueType.FLOAT).create().setId(3); + private static final Metric METRIC_UNKNOWN = new Metric.Builder("unknown", "Unknown", Metric.ValueType.FLOAT).create().setId(4); private MeasureFilterExecutor executor; @@ -224,7 +225,7 @@ public class MeasureFilterExecutorTest extends AbstractDaoTestCase { .setSortOnMetric(METRIC_COVERAGE).setSortAsc(false); List rows = executor.execute(filter, new MeasureFilterContext()); - // Java has coverage but not PHP + // Java project has coverage but not PHP assertThat(rows).hasSize(2); verifyJavaProject(rows.get(0)); verifyPhpProject(rows.get(1)); @@ -236,7 +237,7 @@ public class MeasureFilterExecutorTest extends AbstractDaoTestCase { .setSortOnMetric(METRIC_COVERAGE).setSortAsc(true); List rows = executor.execute(filter, new MeasureFilterContext()); - // Java has coverage but not PHP + // Java project has coverage but not PHP assertThat(rows).hasSize(2); verifyJavaProject(rows.get(0)); verifyPhpProject(rows.get(1)); @@ -245,7 +246,7 @@ public class MeasureFilterExecutorTest extends AbstractDaoTestCase { @Test public void sort_by_missing_numeric_measure() throws SQLException { // coverage measures are not computed - MeasureFilter filter = new MeasureFilter().setResourceQualifiers(Arrays.asList("CLA")).setSortOnMetric(METRIC_COVERAGE); + MeasureFilter filter = new MeasureFilter().setResourceQualifiers(Arrays.asList("CLA")).setSortOnMetric(METRIC_UNKNOWN); List rows = executor.execute(filter, new MeasureFilterContext()); // 2 files, random order diff --git a/sonar-core/src/test/resources/org/sonar/core/measure/MeasureFilterExecutorTest/shared.xml b/sonar-core/src/test/resources/org/sonar/core/measure/MeasureFilterExecutorTest/shared.xml index bd5a18fea6b..8b4a6bbc26e 100644 --- a/sonar-core/src/test/resources/org/sonar/core/measure/MeasureFilterExecutorTest/shared.xml +++ b/sonar-core/src/test/resources/org/sonar/core/measure/MeasureFilterExecutorTest/shared.xml @@ -16,6 +16,12 @@ optimized_best_value="[true]" best_value="100" direction="1" hidden="[false]" delete_historical_data="[null]"/> +