Skip to content

Commit

Permalink
SONAR-7297 Use IssueUpdater and IssueFinder in IssueService
Browse files Browse the repository at this point in the history
  • Loading branch information
@julienlancelot @teryk
julienlancelot authored and teryk committed Dec 30, 2016
1 parent 62894f0 commit f67846d
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 104 deletions.
91 changes: 19 additions & 72 deletions server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
View file
Expand Up @@ -19,17 +19,13 @@
*/ */
package org.sonar.server.issue; package org.sonar.server.issue;


import com.google.common.base.Optional;
import com.google.common.base.Strings; import com.google.common.base.Strings;
import java.util.Collection; import java.util.Collection;
import java.util.Date; import java.util.Date;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import javax.annotation.Nullable; import javax.annotation.Nullable;
import org.sonar.api.ce.ComputeEngineSide; 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.rules.RuleType;
import org.sonar.api.server.ServerSide; import org.sonar.api.server.ServerSide;
import org.sonar.api.user.User; import org.sonar.api.user.User;
Expand All @@ -39,13 +35,8 @@
import org.sonar.core.issue.IssueChangeContext; import org.sonar.core.issue.IssueChangeContext;
import org.sonar.db.DbClient; import org.sonar.db.DbClient;
import org.sonar.db.DbSession; 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.exceptions.BadRequestException;
import org.sonar.server.issue.index.IssueIndex; 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; import org.sonar.server.user.UserSession;


@ServerSide @ServerSide
Expand All @@ -55,23 +46,19 @@ public class IssueService {
private final DbClient dbClient; private final DbClient dbClient;
private final IssueIndex issueIndex; private final IssueIndex issueIndex;


private final IssueFieldsSetter issueUpdater; private final IssueFinder issueFinder;
private final IssueStorage issueStorage; private final IssueFieldsSetter issueFieldsSetter;
private final NotificationManager notificationService; private final IssueUpdater issueUpdater;
private final UserFinder userFinder; private final UserFinder userFinder;
private final UserSession userSession; private final UserSession userSession;


public IssueService(DbClient dbClient, IssueIndex issueIndex, public IssueService(DbClient dbClient, IssueIndex issueIndex, IssueFinder issueFinder, IssueFieldsSetter issueFieldsSetter, IssueUpdater issueUpdater,
IssueStorage issueStorage, UserFinder userFinder, UserSession userSession) {
IssueFieldsSetter issueUpdater,
NotificationManager notificationService,
UserFinder userFinder,
UserSession userSession) {
this.dbClient = dbClient; this.dbClient = dbClient;
this.issueIndex = issueIndex; this.issueIndex = issueIndex;
this.issueStorage = issueStorage; this.issueFinder = issueFinder;
this.issueFieldsSetter = issueFieldsSetter;
this.issueUpdater = issueUpdater; this.issueUpdater = issueUpdater;
this.notificationService = notificationService;
this.userFinder = userFinder; this.userFinder = userFinder;
this.userSession = userSession; this.userSession = userSession;
} }
Expand All @@ -81,7 +68,7 @@ public void assign(String issueKey, @Nullable String assignee) {


DbSession session = dbClient.openSession(false); DbSession session = dbClient.openSession(false);
try { try {
DefaultIssue issue = getByKeyForUpdate(session, issueKey).toDefaultIssue(); DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue();
User user = null; User user = null;
if (!Strings.isNullOrEmpty(assignee)) { if (!Strings.isNullOrEmpty(assignee)) {
user = userFinder.findByLogin(assignee); user = userFinder.findByLogin(assignee);
Expand All @@ -90,8 +77,8 @@ public void assign(String issueKey, @Nullable String assignee) {
} }
} }
IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin()); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin());
if (issueUpdater.assign(issue, user, context)) { if (issueFieldsSetter.assign(issue, user, context)) {
saveIssue(session, issue, context, null); issueUpdater.saveIssue(session, issue, context, null);
} }


} finally { } finally {
Expand All @@ -104,12 +91,12 @@ public void setSeverity(String issueKey, String severity) {


DbSession session = dbClient.openSession(false); DbSession session = dbClient.openSession(false);
try { try {
DefaultIssue issue = getByKeyForUpdate(session, issueKey).toDefaultIssue(); DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue();
userSession.checkComponentUuidPermission(UserRole.ISSUE_ADMIN, issue.projectUuid()); userSession.checkComponentUuidPermission(UserRole.ISSUE_ADMIN, issue.projectUuid());


IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin()); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin());
if (issueUpdater.setManualSeverity(issue, severity, context)) { if (issueFieldsSetter.setManualSeverity(issue, severity, context)) {
saveIssue(session, issue, context, null); issueUpdater.saveIssue(session, issue, context, null);
} }
} finally { } finally {
session.close(); session.close();
Expand All @@ -121,58 +108,18 @@ public void setType(String issueKey, RuleType type) {


DbSession session = dbClient.openSession(false); DbSession session = dbClient.openSession(false);
try { try {
DefaultIssue issue = getByKeyForUpdate(session, issueKey).toDefaultIssue(); DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue();
userSession.checkComponentUuidPermission(UserRole.ISSUE_ADMIN, issue.projectUuid()); userSession.checkComponentUuidPermission(UserRole.ISSUE_ADMIN, issue.projectUuid());


IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin()); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin());
if (issueUpdater.setType(issue, type, context)) { if (issueFieldsSetter.setType(issue, type, context)) {
saveIssue(session, issue, context, null); issueUpdater.saveIssue(session, issue, context, null);
} }
} finally { } finally {
session.close(); 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<RuleDto> 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<RuleDto> getRuleByKey(DbSession session, RuleKey ruleKey) {
Optional<RuleDto> 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 * Search for all tags, whatever issue resolution or user access rights
*/ */
Expand All @@ -195,10 +142,10 @@ public Collection<String> setTags(String issueKey, Collection<String> tags) {


DbSession session = dbClient.openSession(false); DbSession session = dbClient.openSession(false);
try { try {
DefaultIssue issue = getByKeyForUpdate(session, issueKey).toDefaultIssue(); DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue();
IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin()); IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin());
if (issueUpdater.setTags(issue, tags, context)) { if (issueFieldsSetter.setTags(issue, tags, context)) {
saveIssue(session, issue, context, null); issueUpdater.saveIssue(session, issue, context, null);
} }
return issue.tags(); return issue.tags();


Expand Down
58 changes: 26 additions & 32 deletions server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java
View file
Expand Up @@ -69,7 +69,7 @@ public class IssueServiceMediumTest {
public UserSessionRule userSessionRule = UserSessionRule.forServerTester(tester); public UserSessionRule userSessionRule = UserSessionRule.forServerTester(tester);


DbClient db; DbClient db;
IssueIndex IssueIndex; IssueIndex issueIndex;
DbSession session; DbSession session;
IssueService service; IssueService service;
RuleIndexer ruleIndexer; RuleIndexer ruleIndexer;
Expand All @@ -78,7 +78,7 @@ public class IssueServiceMediumTest {
public void setUp() { public void setUp() {
tester.clearDbAndIndexes(); tester.clearDbAndIndexes();
db = tester.get(DbClient.class); db = tester.get(DbClient.class);
IssueIndex = tester.get(IssueIndex.class); issueIndex = tester.get(IssueIndex.class);
session = db.openSession(false); session = db.openSession(false);
service = tester.get(IssueService.class); service = tester.get(IssueService.class);
ruleIndexer = tester.get(RuleIndexer.class); ruleIndexer = tester.get(RuleIndexer.class);
Expand All @@ -93,22 +93,12 @@ private void index() {
tester.get(IssueIndexer.class).indexAll(); 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 @Test
public void assign() { public void assign() {
RuleDto rule = newRule(); RuleDto rule = newRule();
ComponentDto project = newProject(); ComponentDto project = newProject();
ComponentDto file = newFile(project); ComponentDto file = newFile(project);
userSessionRule.login("john"); userSessionRule.login("john").addProjectUuidPermissions(UserRole.USER, project.uuid());


IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project)); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project));


Expand All @@ -117,19 +107,19 @@ public void assign() {
session.commit(); session.commit();
index(); index();


assertThat(IssueIndex.getByKey(issue.getKey()).assignee()).isNull(); assertThat(issueIndex.getByKey(issue.getKey()).assignee()).isNull();


service.assign(issue.getKey(), user.getLogin()); service.assign(issue.getKey(), user.getLogin());


assertThat(IssueIndex.getByKey(issue.getKey()).assignee()).isEqualTo("perceval"); assertThat(issueIndex.getByKey(issue.getKey()).assignee()).isEqualTo("perceval");
} }


@Test @Test
public void unassign() { public void unassign() {
RuleDto rule = newRule(); RuleDto rule = newRule();
ComponentDto project = newProject(); ComponentDto project = newProject();
ComponentDto file = newFile(project); 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")); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project).setAssignee("perceval"));


Expand All @@ -138,19 +128,19 @@ public void unassign() {
session.commit(); session.commit();
index(); index();


assertThat(IssueIndex.getByKey(issue.getKey()).assignee()).isEqualTo("perceval"); assertThat(issueIndex.getByKey(issue.getKey()).assignee()).isEqualTo("perceval");


service.assign(issue.getKey(), ""); service.assign(issue.getKey(), "");


assertThat(IssueIndex.getByKey(issue.getKey()).assignee()).isNull(); assertThat(issueIndex.getByKey(issue.getKey()).assignee()).isNull();
} }


@Test @Test
public void fail_to_assign_on_unknown_user() { public void fail_to_assign_on_unknown_user() {
RuleDto rule = newRule(); RuleDto rule = newRule();
ComponentDto project = newProject(); ComponentDto project = newProject();
ComponentDto file = newFile(project); ComponentDto file = newFile(project);
userSessionRule.login("john"); userSessionRule.login("john").addProjectUuidPermissions(UserRole.USER, project.uuid());


IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project)); IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project));


Expand All @@ -167,31 +157,35 @@ public void set_severity() {
RuleDto rule = newRule(); RuleDto rule = newRule();
ComponentDto project = newProject(); ComponentDto project = newProject();
ComponentDto file = newFile(project); 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)); 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); 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 @Test
public void set_type() { public void set_type() {
RuleDto rule = newRule(); RuleDto rule = newRule();
ComponentDto project = newProject(); ComponentDto project = newProject();
ComponentDto file = newFile(project); 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)); 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); 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 @Test
Expand All @@ -218,34 +212,34 @@ public void set_tags() {
RuleDto rule = newRule(); RuleDto rule = newRule();
ComponentDto project = newProject(); ComponentDto project = newProject();
ComponentDto file = newFile(project); ComponentDto file = newFile(project);
userSessionRule.login("john"); userSessionRule.login("john").addProjectUuidPermissions(UserRole.USER, project.uuid());


IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project)); 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 // Tags are lowercased
service.setTags(issue.getKey(), ImmutableSet.of("bug", "Convention")); 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 // nulls and empty tags are ignored
service.setTags(issue.getKey(), Sets.newHashSet("security", null, "", "convention")); 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 // tag validation
try { try {
service.setTags(issue.getKey(), ImmutableSet.of("pol op")); service.setTags(issue.getKey(), ImmutableSet.of("pol op"));
} catch (Exception exception) { } catch (Exception exception) {
assertThat(exception).isInstanceOf(IllegalArgumentException.class); 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 // unchanged tags
service.setTags(issue.getKey(), ImmutableSet.of("convention", "security")); 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.<String>of()); service.setTags(issue.getKey(), ImmutableSet.<String>of());
assertThat(IssueIndex.getByKey(issue.getKey()).tags()).isEmpty(); assertThat(issueIndex.getByKey(issue.getKey()).tags()).isEmpty();
} }


@Test @Test
Expand Down

0 comments on commit f67846d

Please sign in to comment.