From 62d74d700ee3805ab1d2909e23c7e4efaf96c519 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Sat, 10 Jan 2015 11:49:03 +0100 Subject: [PATCH] Fix quality flaws --- .../java/org/sonar/server/db/DbClient.java | 2 +- .../org/sonar/server/es/IndexCreator.java | 2 +- .../org/sonar/server/es/IndexRegistry.java | 3 ++ .../notifications/NotificationService.java | 17 +++++----- .../server/notifications/package-info.java | 24 ++++++++++++++ .../sonar/server/source/ws/HashAction.java | 7 ++--- .../NotificationServiceTest.java | 31 +++++++------------ .../server/source/ws/HashActionTest.java | 5 +-- .../sonar/server/source/ws/SourcesWsTest.java | 3 +- .../sonar/core/source/db/FileSourceDao.java | 3 +- 10 files changed, 56 insertions(+), 41 deletions(-) create mode 100644 server/sonar-server/src/main/java/org/sonar/server/notifications/package-info.java diff --git a/server/sonar-server/src/main/java/org/sonar/server/db/DbClient.java b/server/sonar-server/src/main/java/org/sonar/server/db/DbClient.java index 4689180ce02..5b580190d3f 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/db/DbClient.java +++ b/server/sonar-server/src/main/java/org/sonar/server/db/DbClient.java @@ -88,7 +88,7 @@ public class DbClient implements ServerComponent { this.db = db; this.myBatis = myBatis; - Map map = new IdentityHashMap(); + Map map = new IdentityHashMap<>(); for (DaoComponent daoComponent : daoComponents) { map.put(daoComponent.getClass(), daoComponent); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/es/IndexCreator.java b/server/sonar-server/src/main/java/org/sonar/server/es/IndexCreator.java index 581d0c1fa07..65888f72610 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/es/IndexCreator.java +++ b/server/sonar-server/src/main/java/org/sonar/server/es/IndexCreator.java @@ -32,7 +32,7 @@ import org.sonar.api.ServerComponent; import java.util.Map; /** - * Create registered indices in Elasticsearch. + * Creates/deletes all indices in Elasticsearch during server startup. */ public class IndexCreator implements ServerComponent, Startable { diff --git a/server/sonar-server/src/main/java/org/sonar/server/es/IndexRegistry.java b/server/sonar-server/src/main/java/org/sonar/server/es/IndexRegistry.java index 867cfc301f7..972830067be 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/es/IndexRegistry.java +++ b/server/sonar-server/src/main/java/org/sonar/server/es/IndexRegistry.java @@ -27,6 +27,9 @@ import org.sonar.api.ServerComponent; import java.util.Map; +/** + * This class collects definitions of all Elasticsearch indices during server startup + */ public class IndexRegistry implements ServerComponent, Startable { /** diff --git a/server/sonar-server/src/main/java/org/sonar/server/notifications/NotificationService.java b/server/sonar-server/src/main/java/org/sonar/server/notifications/NotificationService.java index 9843013dbed..c1ff7cb146b 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/notifications/NotificationService.java +++ b/server/sonar-server/src/main/java/org/sonar/server/notifications/NotificationService.java @@ -22,6 +22,7 @@ package org.sonar.server.notifications; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.HashMultimap; import com.google.common.collect.SetMultimap; +import org.picocontainer.Startable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sonar.api.Properties; @@ -33,7 +34,6 @@ import org.sonar.api.notifications.NotificationChannel; import org.sonar.api.notifications.NotificationDispatcher; import org.sonar.api.utils.TimeProfiler; import org.sonar.core.notification.DefaultNotificationManager; -import org.sonar.jpa.session.DatabaseSessionFactory; import java.util.Arrays; import java.util.Collection; @@ -60,7 +60,7 @@ import java.util.concurrent.TimeUnit; project = false, global = false) }) -public class NotificationService implements ServerComponent { +public class NotificationService implements ServerComponent, Startable { private static final Logger LOG = LoggerFactory.getLogger(NotificationService.class); @@ -73,7 +73,6 @@ public class NotificationService implements ServerComponent { private final long delayBeforeReportingStatusInSeconds; private final DefaultNotificationManager manager; private final NotificationDispatcher[] dispatchers; - private final DatabaseSessionFactory databaseSessionFactory; private ScheduledExecutorService executorService; private boolean stopping = false; @@ -81,8 +80,7 @@ public class NotificationService implements ServerComponent { /** * Constructor for {@link NotificationService} */ - public NotificationService(Settings settings, DefaultNotificationManager manager, DatabaseSessionFactory databaseSessionFactory, NotificationDispatcher[] dispatchers) { - this.databaseSessionFactory = databaseSessionFactory; + public NotificationService(Settings settings, DefaultNotificationManager manager, NotificationDispatcher[] dispatchers) { delayInSeconds = settings.getLong(PROPERTY_DELAY); delayBeforeReportingStatusInSeconds = settings.getLong(PROPERTY_DELAY_BEFORE_REPORTING_STATUS); this.manager = manager; @@ -92,11 +90,12 @@ public class NotificationService implements ServerComponent { /** * Default constructor when no channels. */ - public NotificationService(Settings settings, DefaultNotificationManager manager, DatabaseSessionFactory databaseSessionFactory) { - this(settings, manager, databaseSessionFactory, new NotificationDispatcher[0]); + public NotificationService(Settings settings, DefaultNotificationManager manager) { + this(settings, manager, new NotificationDispatcher[0]); LOG.warn("There is no dispatcher - all notifications will be ignored!"); } + @Override public void start() { executorService = Executors.newSingleThreadScheduledExecutor(); executorService.scheduleWithFixedDelay(new Runnable() { @@ -106,15 +105,13 @@ public class NotificationService implements ServerComponent { processQueue(); } catch (Exception e) { LOG.error("Error in NotificationService", e); - } finally { - // Free Hibernate session - databaseSessionFactory.clear(); } } }, 0, delayInSeconds, TimeUnit.SECONDS); LOG.info("Notification service started (delay {} sec.)", delayInSeconds); } + @Override public void stop() { try { stopping = true; diff --git a/server/sonar-server/src/main/java/org/sonar/server/notifications/package-info.java b/server/sonar-server/src/main/java/org/sonar/server/notifications/package-info.java new file mode 100644 index 00000000000..34262049a99 --- /dev/null +++ b/server/sonar-server/src/main/java/org/sonar/server/notifications/package-info.java @@ -0,0 +1,24 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2014 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. + */ + +@ParametersAreNonnullByDefault +package org.sonar.server.notifications; + +import javax.annotation.ParametersAreNonnullByDefault; diff --git a/server/sonar-server/src/main/java/org/sonar/server/source/ws/HashAction.java b/server/sonar-server/src/main/java/org/sonar/server/source/ws/HashAction.java index 5b22eddc1b4..b2914709a71 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/source/ws/HashAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/source/ws/HashAction.java @@ -28,7 +28,6 @@ import org.sonar.api.server.ws.WebService; import org.sonar.api.web.UserRole; import org.sonar.core.component.ComponentDto; import org.sonar.core.persistence.DbSession; -import org.sonar.core.source.db.FileSourceDao; import org.sonar.server.db.DbClient; import org.sonar.server.user.UserSession; @@ -37,11 +36,9 @@ import java.io.OutputStream; public class HashAction implements RequestHandler { private final DbClient dbClient; - private final FileSourceDao fileSourceDao; - public HashAction(DbClient dbClient, FileSourceDao fileSourceDao) { + public HashAction(DbClient dbClient) { this.dbClient = dbClient; - this.fileSourceDao = fileSourceDao; } void define(WebService.NewController controller) { @@ -65,7 +62,7 @@ public class HashAction implements RequestHandler { String componentKey = request.mandatoryParam("key"); UserSession.get().checkComponentPermission(UserRole.CODEVIEWER, componentKey); ComponentDto component = dbClient.componentDao().getByKey(session, componentKey); - String lineHashes = fileSourceDao.selectLineHashes(component.uuid(), session); + String lineHashes = dbClient.fileSourceDao().selectLineHashes(component.uuid(), session); if (lineHashes == null) { response.noContent(); } else { diff --git a/server/sonar-server/src/test/java/org/sonar/server/notifications/NotificationServiceTest.java b/server/sonar-server/src/test/java/org/sonar/server/notifications/NotificationServiceTest.java index e4647eff872..5572f77cffc 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/notifications/NotificationServiceTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/notifications/NotificationServiceTest.java @@ -27,19 +27,12 @@ import org.sonar.api.notifications.Notification; import org.sonar.api.notifications.NotificationChannel; import org.sonar.api.notifications.NotificationDispatcher; import org.sonar.core.notification.DefaultNotificationManager; -import org.sonar.jpa.session.DatabaseSessionFactory; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.same; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.timeout; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; public class NotificationServiceTest { private static String CREATOR_SIMON = "simon"; @@ -55,7 +48,7 @@ public class NotificationServiceTest { private NotificationService service; - private void setUpMocks(String creator, String assignee) { + private void setUpMocks() { when(emailChannel.getKey()).thenReturn("email"); when(gtalkChannel.getKey()).thenReturn("gtalk"); when(commentOnReviewAssignedToMe.getKey()).thenReturn("comment on review assigned to me"); @@ -64,7 +57,7 @@ public class NotificationServiceTest { Settings settings = new Settings().setProperty("sonar.notifications.delay", 1L); - service = new NotificationService(settings, manager, mock(DatabaseSessionFactory.class), + service = new NotificationService(settings, manager, new NotificationDispatcher[] {commentOnReviewAssignedToMe, commentOnReviewCreatedByMe}); } @@ -80,7 +73,7 @@ public class NotificationServiceTest { */ @Test public void scenario1() { - setUpMocks(CREATOR_SIMON, ASSIGNEE_SIMON); + setUpMocks(); doAnswer(addUser(ASSIGNEE_SIMON, emailChannel)).when(commentOnReviewAssignedToMe).dispatch(same(notification), any(NotificationDispatcher.Context.class)); doAnswer(addUser(CREATOR_SIMON, emailChannel)).when(commentOnReviewCreatedByMe).dispatch(same(notification), any(NotificationDispatcher.Context.class)); @@ -104,7 +97,7 @@ public class NotificationServiceTest { */ @Test public void scenario2() { - setUpMocks(CREATOR_EVGENY, ASSIGNEE_SIMON); + setUpMocks(); doAnswer(addUser(ASSIGNEE_SIMON, emailChannel)).when(commentOnReviewAssignedToMe).dispatch(same(notification), any(NotificationDispatcher.Context.class)); doAnswer(addUser(CREATOR_EVGENY, gtalkChannel)).when(commentOnReviewCreatedByMe).dispatch(same(notification), any(NotificationDispatcher.Context.class)); @@ -129,7 +122,7 @@ public class NotificationServiceTest { */ @Test public void scenario3() { - setUpMocks(CREATOR_EVGENY, ASSIGNEE_SIMON); + setUpMocks(); doAnswer(addUser(ASSIGNEE_SIMON, new NotificationChannel[] {emailChannel, gtalkChannel})) .when(commentOnReviewAssignedToMe).dispatch(same(notification), any(NotificationDispatcher.Context.class)); @@ -154,7 +147,7 @@ public class NotificationServiceTest { */ @Test public void scenario4() { - setUpMocks(CREATOR_EVGENY, ASSIGNEE_SIMON); + setUpMocks(); service.start(); service.stop(); @@ -166,7 +159,7 @@ public class NotificationServiceTest { // SONAR-4548 @Test public void shouldNotStopWhenException() { - setUpMocks(CREATOR_SIMON, ASSIGNEE_SIMON); + setUpMocks(); when(manager.getFromQueue()).thenThrow(new RuntimeException("Unexpected exception")).thenReturn(notification).thenReturn(null); doAnswer(addUser(ASSIGNEE_SIMON, emailChannel)).when(commentOnReviewAssignedToMe).dispatch(same(notification), any(NotificationDispatcher.Context.class)); doAnswer(addUser(CREATOR_SIMON, emailChannel)).when(commentOnReviewCreatedByMe).dispatch(same(notification), any(NotificationDispatcher.Context.class)); @@ -180,7 +173,7 @@ public class NotificationServiceTest { @Test public void shouldNotAddNullAsUser() { - setUpMocks(CREATOR_EVGENY, ASSIGNEE_SIMON); + setUpMocks(); doAnswer(addUser(null, gtalkChannel)).when(commentOnReviewCreatedByMe).dispatch(same(notification), any(NotificationDispatcher.Context.class)); service.start(); @@ -192,7 +185,7 @@ public class NotificationServiceTest { @Test public void shouldReturnDispatcherList() { - setUpMocks(CREATOR_SIMON, ASSIGNEE_SIMON); + setUpMocks(); assertThat(service.getDispatchers()).containsOnly(commentOnReviewAssignedToMe, commentOnReviewCreatedByMe); } @@ -201,13 +194,13 @@ public class NotificationServiceTest { public void shouldReturnNoDispatcher() { Settings settings = new Settings().setProperty("sonar.notifications.delay", 1L); - service = new NotificationService(settings, manager, mock(DatabaseSessionFactory.class)); + service = new NotificationService(settings, manager); assertThat(service.getDispatchers()).hasSize(0); } @Test public void shouldLogEvery10Minutes() throws InterruptedException { - setUpMocks(CREATOR_EVGENY, ASSIGNEE_SIMON); + setUpMocks(); // Emulate 2 notifications in DB when(manager.getFromQueue()).thenReturn(notification).thenReturn(notification).thenReturn(null); when(manager.count()).thenReturn(1L).thenReturn(0L); diff --git a/server/sonar-server/src/test/java/org/sonar/server/source/ws/HashActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/source/ws/HashActionTest.java index 650dcafc901..a624fcfd7af 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/source/ws/HashActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/source/ws/HashActionTest.java @@ -59,6 +59,7 @@ public class HashActionTest { @Before public void setUp() throws Exception { when(dbClient.componentDao()).thenReturn(componentDao); + when(dbClient.fileSourceDao()).thenReturn(fileSourceDao); when(dbClient.openSession(false)).thenReturn(mock(DbSession.class)); tester = new WsTester( new SourcesWs( @@ -66,10 +67,10 @@ public class HashActionTest { mock(RawAction.class), mock(ScmAction.class), mock(LinesAction.class), - new HashAction(dbClient, fileSourceDao), + new HashAction(dbClient), mock(IndexAction.class) ) - ); + ); } @Test diff --git a/server/sonar-server/src/test/java/org/sonar/server/source/ws/SourcesWsTest.java b/server/sonar-server/src/test/java/org/sonar/server/source/ws/SourcesWsTest.java index ea2143a9d76..f56d738f99e 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/source/ws/SourcesWsTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/source/ws/SourcesWsTest.java @@ -22,7 +22,6 @@ package org.sonar.server.source.ws; import org.junit.Test; import org.sonar.api.server.ws.WebService; -import org.sonar.core.source.db.FileSourceDao; import org.sonar.server.component.ComponentService; import org.sonar.server.db.DbClient; import org.sonar.server.source.HtmlSourceDecorator; @@ -39,7 +38,7 @@ public class SourcesWsTest { RawAction rawAction = new RawAction(mock(DbClient.class), mock(SourceService.class)); ScmAction scmAction = new ScmAction(mock(SourceService.class), new ScmWriter()); LinesAction linesAction = new LinesAction(mock(SourceLineIndex.class), mock(HtmlSourceDecorator.class), mock(ComponentService.class)); - HashAction hashAction = new HashAction(mock(DbClient.class), mock(FileSourceDao.class)); + HashAction hashAction = new HashAction(mock(DbClient.class)); IndexAction indexAction = new IndexAction(mock(DbClient.class), mock(SourceService.class)); WsTester tester = new WsTester(new SourcesWs(showAction, rawAction, scmAction, linesAction, hashAction, indexAction)); diff --git a/sonar-core/src/main/java/org/sonar/core/source/db/FileSourceDao.java b/sonar-core/src/main/java/org/sonar/core/source/db/FileSourceDao.java index c6c6ea16d6a..3e67c040b32 100644 --- a/sonar-core/src/main/java/org/sonar/core/source/db/FileSourceDao.java +++ b/sonar-core/src/main/java/org/sonar/core/source/db/FileSourceDao.java @@ -22,12 +22,13 @@ package org.sonar.core.source.db; import org.sonar.api.BatchComponent; import org.sonar.api.ServerComponent; +import org.sonar.core.persistence.DaoComponent; import org.sonar.core.persistence.DbSession; import org.sonar.core.persistence.MyBatis; import javax.annotation.CheckForNull; -public class FileSourceDao implements BatchComponent, ServerComponent { +public class FileSourceDao implements BatchComponent, ServerComponent, DaoComponent { private final MyBatis mybatis; -- 2.39.5