From f67846d313da30b423e32cfbea90f8f71950aa0a Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Fri, 16 Dec 2016 17:48:42 +0100 Subject: [PATCH] SONAR-7297 Use IssueUpdater and IssueFinder in IssueService --- .../org/sonar/server/issue/IssueService.java | 91 ++++--------------- .../server/issue/IssueServiceMediumTest.java | 58 ++++++------ 2 files changed, 45 insertions(+), 104 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java index 5eae3be0c7c..48605ed27d0 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java @@ -19,7 +19,6 @@ */ package org.sonar.server.issue; -import com.google.common.base.Optional; import com.google.common.base.Strings; import java.util.Collection; import java.util.Date; @@ -27,9 +26,6 @@ import java.util.List; import java.util.Map; import javax.annotation.Nullable; import org.sonar.api.ce.ComputeEngineSide; -import org.sonar.api.issue.Issue; -import org.sonar.api.rule.RuleKey; -import org.sonar.api.rule.RuleStatus; import org.sonar.api.rules.RuleType; import org.sonar.api.server.ServerSide; import org.sonar.api.user.User; @@ -39,13 +35,8 @@ import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.IssueChangeContext; import org.sonar.db.DbClient; import org.sonar.db.DbSession; -import org.sonar.db.component.ComponentDto; -import org.sonar.db.issue.IssueDto; -import org.sonar.db.rule.RuleDto; import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.issue.index.IssueIndex; -import org.sonar.server.issue.notification.IssueChangeNotification; -import org.sonar.server.notification.NotificationManager; import org.sonar.server.user.UserSession; @ServerSide @@ -55,23 +46,19 @@ public class IssueService { private final DbClient dbClient; private final IssueIndex issueIndex; - private final IssueFieldsSetter issueUpdater; - private final IssueStorage issueStorage; - private final NotificationManager notificationService; + private final IssueFinder issueFinder; + private final IssueFieldsSetter issueFieldsSetter; + private final IssueUpdater issueUpdater; private final UserFinder userFinder; private final UserSession userSession; - public IssueService(DbClient dbClient, IssueIndex issueIndex, - IssueStorage issueStorage, - IssueFieldsSetter issueUpdater, - NotificationManager notificationService, - UserFinder userFinder, - UserSession userSession) { + public IssueService(DbClient dbClient, IssueIndex issueIndex, IssueFinder issueFinder, IssueFieldsSetter issueFieldsSetter, IssueUpdater issueUpdater, + UserFinder userFinder, UserSession userSession) { this.dbClient = dbClient; this.issueIndex = issueIndex; - this.issueStorage = issueStorage; + this.issueFinder = issueFinder; + this.issueFieldsSetter = issueFieldsSetter; this.issueUpdater = issueUpdater; - this.notificationService = notificationService; this.userFinder = userFinder; this.userSession = userSession; } @@ -81,7 +68,7 @@ public class IssueService { DbSession session = dbClient.openSession(false); try { - DefaultIssue issue = getByKeyForUpdate(session, issueKey).toDefaultIssue(); + DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue(); User user = null; if (!Strings.isNullOrEmpty(assignee)) { user = userFinder.findByLogin(assignee); @@ -90,8 +77,8 @@ public class IssueService { } } IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin()); - if (issueUpdater.assign(issue, user, context)) { - saveIssue(session, issue, context, null); + if (issueFieldsSetter.assign(issue, user, context)) { + issueUpdater.saveIssue(session, issue, context, null); } } finally { @@ -104,12 +91,12 @@ public class IssueService { DbSession session = dbClient.openSession(false); try { - DefaultIssue issue = getByKeyForUpdate(session, issueKey).toDefaultIssue(); + DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue(); userSession.checkComponentUuidPermission(UserRole.ISSUE_ADMIN, issue.projectUuid()); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin()); - if (issueUpdater.setManualSeverity(issue, severity, context)) { - saveIssue(session, issue, context, null); + if (issueFieldsSetter.setManualSeverity(issue, severity, context)) { + issueUpdater.saveIssue(session, issue, context, null); } } finally { session.close(); @@ -121,58 +108,18 @@ public class IssueService { DbSession session = dbClient.openSession(false); try { - DefaultIssue issue = getByKeyForUpdate(session, issueKey).toDefaultIssue(); + DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue(); userSession.checkComponentUuidPermission(UserRole.ISSUE_ADMIN, issue.projectUuid()); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin()); - if (issueUpdater.setType(issue, type, context)) { - saveIssue(session, issue, context, null); + if (issueFieldsSetter.setType(issue, type, context)) { + issueUpdater.saveIssue(session, issue, context, null); } } finally { session.close(); } } - public Issue getByKey(String key) { - return issueIndex.getByKey(key); - } - - // TODO Either use IssueFinder or remove it - @Deprecated - IssueDto getByKeyForUpdate(DbSession session, String key) { - // Load from index to check permission : if the user has no permission to see the issue an exception will be generated - Issue authorizedIssueIndex = getByKey(key); - return dbClient.issueDao().selectOrFailByKey(session, authorizedIssueIndex.key()); - } - - // TODO Either use IssueUpdater or remove it - @Deprecated - void saveIssue(DbSession session, DefaultIssue issue, IssueChangeContext context, @Nullable String comment) { - String projectKey = issue.projectKey(); - if (projectKey == null) { - throw new IllegalStateException(String.format("Issue '%s' has no project key", issue.key())); - } - issueStorage.save(session, issue); - Optional rule = getRuleByKey(session, issue.getRuleKey()); - ComponentDto project = dbClient.componentDao().selectOrFailByKey(session, projectKey); - notificationService.scheduleForSending(new IssueChangeNotification() - .setIssue(issue) - .setChangeAuthorLogin(context.login()) - .setRuleName(rule.isPresent() ? rule.get().getName() : null) - .setProject(project.getKey(), project.name()) - .setComponent(dbClient.componentDao().selectOrFailByKey(session, issue.componentKey())) - .setComment(comment)); - } - - private Optional getRuleByKey(DbSession session, RuleKey ruleKey) { - Optional rule = dbClient.ruleDao().selectByKey(session, ruleKey); - if (rule.isPresent() && rule.get().getStatus() != RuleStatus.REMOVED) { - return rule; - } else { - return Optional.absent(); - } - } - /** * Search for all tags, whatever issue resolution or user access rights */ @@ -195,10 +142,10 @@ public class IssueService { DbSession session = dbClient.openSession(false); try { - DefaultIssue issue = getByKeyForUpdate(session, issueKey).toDefaultIssue(); + DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue(); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin()); - if (issueUpdater.setTags(issue, tags, context)) { - saveIssue(session, issue, context, null); + if (issueFieldsSetter.setTags(issue, tags, context)) { + issueUpdater.saveIssue(session, issue, context, null); } return issue.tags(); diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java index f8d8e0dfcb6..d9fefa24038 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java @@ -69,7 +69,7 @@ public class IssueServiceMediumTest { public UserSessionRule userSessionRule = UserSessionRule.forServerTester(tester); DbClient db; - IssueIndex IssueIndex; + IssueIndex issueIndex; DbSession session; IssueService service; RuleIndexer ruleIndexer; @@ -78,7 +78,7 @@ public class IssueServiceMediumTest { public void setUp() { tester.clearDbAndIndexes(); db = tester.get(DbClient.class); - IssueIndex = tester.get(IssueIndex.class); + issueIndex = tester.get(IssueIndex.class); session = db.openSession(false); service = tester.get(IssueService.class); ruleIndexer = tester.get(RuleIndexer.class); @@ -93,22 +93,12 @@ public class IssueServiceMediumTest { tester.get(IssueIndexer.class).indexAll(); } - @Test - public void get_by_key() { - RuleDto rule = newRule(); - ComponentDto project = newProject(); - ComponentDto file = newFile(project); - IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project)); - - assertThat(service.getByKey(issue.getKey())).isNotNull(); - } - @Test public void assign() { RuleDto rule = newRule(); ComponentDto project = newProject(); ComponentDto file = newFile(project); - userSessionRule.login("john"); + userSessionRule.login("john").addProjectUuidPermissions(UserRole.USER, project.uuid()); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project)); @@ -117,11 +107,11 @@ public class IssueServiceMediumTest { session.commit(); index(); - assertThat(IssueIndex.getByKey(issue.getKey()).assignee()).isNull(); + assertThat(issueIndex.getByKey(issue.getKey()).assignee()).isNull(); service.assign(issue.getKey(), user.getLogin()); - assertThat(IssueIndex.getByKey(issue.getKey()).assignee()).isEqualTo("perceval"); + assertThat(issueIndex.getByKey(issue.getKey()).assignee()).isEqualTo("perceval"); } @Test @@ -129,7 +119,7 @@ public class IssueServiceMediumTest { RuleDto rule = newRule(); ComponentDto project = newProject(); ComponentDto file = newFile(project); - userSessionRule.login("john"); + userSessionRule.login("john").addProjectUuidPermissions(UserRole.USER, project.uuid()); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project).setAssignee("perceval")); @@ -138,11 +128,11 @@ public class IssueServiceMediumTest { session.commit(); index(); - assertThat(IssueIndex.getByKey(issue.getKey()).assignee()).isEqualTo("perceval"); + assertThat(issueIndex.getByKey(issue.getKey()).assignee()).isEqualTo("perceval"); service.assign(issue.getKey(), ""); - assertThat(IssueIndex.getByKey(issue.getKey()).assignee()).isNull(); + assertThat(issueIndex.getByKey(issue.getKey()).assignee()).isNull(); } @Test @@ -150,7 +140,7 @@ public class IssueServiceMediumTest { RuleDto rule = newRule(); ComponentDto project = newProject(); ComponentDto file = newFile(project); - userSessionRule.login("john"); + userSessionRule.login("john").addProjectUuidPermissions(UserRole.USER, project.uuid()); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project)); @@ -167,15 +157,17 @@ public class IssueServiceMediumTest { RuleDto rule = newRule(); ComponentDto project = newProject(); ComponentDto file = newFile(project); - userSessionRule.login("john").addProjectUuidPermissions(UserRole.ISSUE_ADMIN, project.uuid()); + userSessionRule.login("john") + .addProjectUuidPermissions(UserRole.USER, project.uuid()) + .addProjectUuidPermissions(UserRole.ISSUE_ADMIN, project.uuid()); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project).setSeverity(Severity.BLOCKER)); - assertThat(IssueIndex.getByKey(issue.getKey()).severity()).isEqualTo(Severity.BLOCKER); + assertThat(issueIndex.getByKey(issue.getKey()).severity()).isEqualTo(Severity.BLOCKER); service.setSeverity(issue.getKey(), Severity.MINOR); - assertThat(IssueIndex.getByKey(issue.getKey()).severity()).isEqualTo(Severity.MINOR); + assertThat(issueIndex.getByKey(issue.getKey()).severity()).isEqualTo(Severity.MINOR); } @Test @@ -183,15 +175,17 @@ public class IssueServiceMediumTest { RuleDto rule = newRule(); ComponentDto project = newProject(); ComponentDto file = newFile(project); - userSessionRule.login("john").addProjectUuidPermissions(UserRole.ISSUE_ADMIN, project.uuid()); + userSessionRule.login("john") + .addProjectUuidPermissions(UserRole.USER, project.uuid()) + .addProjectUuidPermissions(UserRole.ISSUE_ADMIN, project.uuid()); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project).setType(RuleType.CODE_SMELL)); - assertThat(IssueIndex.getByKey(issue.getKey()).type()).isEqualTo(RuleType.CODE_SMELL); + assertThat(issueIndex.getByKey(issue.getKey()).type()).isEqualTo(RuleType.CODE_SMELL); service.setType(issue.getKey(), RuleType.BUG); - assertThat(IssueIndex.getByKey(issue.getKey()).type()).isEqualTo(RuleType.BUG); + assertThat(issueIndex.getByKey(issue.getKey()).type()).isEqualTo(RuleType.BUG); } @Test @@ -218,19 +212,19 @@ public class IssueServiceMediumTest { RuleDto rule = newRule(); ComponentDto project = newProject(); ComponentDto file = newFile(project); - userSessionRule.login("john"); + userSessionRule.login("john").addProjectUuidPermissions(UserRole.USER, project.uuid()); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project)); - assertThat(IssueIndex.getByKey(issue.getKey()).tags()).isEmpty(); + assertThat(issueIndex.getByKey(issue.getKey()).tags()).isEmpty(); // Tags are lowercased service.setTags(issue.getKey(), ImmutableSet.of("bug", "Convention")); - assertThat(IssueIndex.getByKey(issue.getKey()).tags()).containsOnly("bug", "convention"); + assertThat(issueIndex.getByKey(issue.getKey()).tags()).containsOnly("bug", "convention"); // nulls and empty tags are ignored service.setTags(issue.getKey(), Sets.newHashSet("security", null, "", "convention")); - assertThat(IssueIndex.getByKey(issue.getKey()).tags()).containsOnly("security", "convention"); + assertThat(issueIndex.getByKey(issue.getKey()).tags()).containsOnly("security", "convention"); // tag validation try { @@ -238,14 +232,14 @@ public class IssueServiceMediumTest { } catch (Exception exception) { assertThat(exception).isInstanceOf(IllegalArgumentException.class); } - assertThat(IssueIndex.getByKey(issue.getKey()).tags()).containsOnly("security", "convention"); + assertThat(issueIndex.getByKey(issue.getKey()).tags()).containsOnly("security", "convention"); // unchanged tags service.setTags(issue.getKey(), ImmutableSet.of("convention", "security")); - assertThat(IssueIndex.getByKey(issue.getKey()).tags()).containsOnly("security", "convention"); + assertThat(issueIndex.getByKey(issue.getKey()).tags()).containsOnly("security", "convention"); service.setTags(issue.getKey(), ImmutableSet.of()); - assertThat(IssueIndex.getByKey(issue.getKey()).tags()).isEmpty(); + assertThat(issueIndex.getByKey(issue.getKey()).tags()).isEmpty(); } @Test -- 2.39.5