From ba21a617bde854734f20c6975a61548cb70c19cc Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Mon, 5 Jun 2017 17:04:47 +0200 Subject: [PATCH] SONAR-8061 speed up issue assign by removing duplicate SQL --- .../it/authorisation/IssuePermissionTest.java | 2 + .../org/sonar/server/issue/IssueStorage.java | 47 ++++--- .../org/sonar/server/issue/IssueUpdater.java | 38 +++++- .../server/issue/ServerIssueStorage.java | 8 +- .../sonar/server/issue/ws/AssignAction.java | 10 +- .../issue/ws/OperationResponseWriter.java | 14 +- .../server/issue/ws/SearchResponseData.java | 6 + .../server/issue/ws/SearchResponseLoader.java | 126 ++++++++++++++++-- .../sonar/server/issue/IssueStorageTest.java | 16 +-- .../sonar/server/issue/IssueUpdaterTest.java | 52 +++++++- .../server/issue/ws/AssignActionTest.java | 32 ++++- 11 files changed, 290 insertions(+), 61 deletions(-) diff --git a/it/it-tests/src/test/java/it/authorisation/IssuePermissionTest.java b/it/it-tests/src/test/java/it/authorisation/IssuePermissionTest.java index 5d50b39d2d4..61cc647c47a 100644 --- a/it/it-tests/src/test/java/it/authorisation/IssuePermissionTest.java +++ b/it/it-tests/src/test/java/it/authorisation/IssuePermissionTest.java @@ -69,6 +69,8 @@ public class IssuePermissionTest { SonarScanner publicProject = SonarScanner.create(projectDir("shared/xoo-sample")) .setProperty("sonar.projectKey", "publicProject") .setProperty("sonar.projectName", "PublicProject"); + + orchestrator.executeBuild(publicProject); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueStorage.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueStorage.java index f6e9a5c37e5..2d039842c3c 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueStorage.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueStorage.java @@ -24,6 +24,7 @@ import java.util.Collection; import java.util.List; import java.util.Map; import java.util.stream.Collectors; +import java.util.stream.Stream; import java.util.stream.StreamSupport; import org.sonar.api.issue.Issue; import org.sonar.api.issue.IssueComment; @@ -38,10 +39,12 @@ import org.sonar.db.DbClient; import org.sonar.db.DbSession; import org.sonar.db.issue.IssueChangeDto; import org.sonar.db.issue.IssueChangeMapper; +import org.sonar.db.issue.IssueDto; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.collect.Lists.newArrayList; import static java.util.Collections.emptyList; +import static org.sonar.core.util.stream.MoreCollectors.toSet; /** * Save issues into database. It is executed : @@ -72,17 +75,17 @@ public abstract class IssueStorage { save(newArrayList(issue)); } - public void save(DbSession session, DefaultIssue issue) { - doSave(session, newArrayList(issue)); + public IssueDto save(DbSession session, DefaultIssue issue) { + return doSave(session, newArrayList(issue)).iterator().next(); } - public void save(Iterable issues) { + public Collection save(Iterable issues) { try (DbSession session = dbClient.openSession(true)) { - doSave(session, issues); + return doSave(session, issues); } } - private void doSave(DbSession session, Iterable issues) { + private Collection doSave(DbSession session, Iterable issues) { // Batch session can not be used for updates. It does not return the number of updated rows, // required for detecting conflicts. long now = system2.now(); @@ -91,13 +94,15 @@ public abstract class IssueStorage { List issuesToInsert = firstNonNull(issuesNewOrUpdated.get(true), emptyList()); List issuesToUpdate = firstNonNull(issuesNewOrUpdated.get(false), emptyList()); - Collection inserted = insert(session, issuesToInsert, now); - Collection updated = update(issuesToUpdate, now); + Collection inserted = insert(session, issuesToInsert, now); + Collection updated = update(issuesToUpdate, now); - Collection issuesInsertedOrUpdated = new ArrayList<>(issuesToInsert.size() + issuesToUpdate.size()); - issuesInsertedOrUpdated.addAll(inserted); - issuesInsertedOrUpdated.addAll(updated); - doAfterSave(issuesInsertedOrUpdated); + doAfterSave(Stream.concat(inserted.stream(), updated.stream()) + .map(IssueDto::getKey) + .collect(toSet(issuesToInsert.size() + issuesToUpdate.size()))); + + return Stream.concat(inserted.stream(), updated.stream()) + .collect(toSet(issuesToInsert.size() + issuesToUpdate.size())); } protected void doAfterSave(Collection issues) { @@ -107,13 +112,13 @@ public abstract class IssueStorage { /** * @return the keys of the inserted issues */ - private Collection insert(DbSession session, Iterable issuesToInsert, long now) { - List inserted = newArrayList(); + private Collection insert(DbSession session, Iterable issuesToInsert, long now) { + List inserted = newArrayList(); int count = 0; IssueChangeMapper issueChangeMapper = session.getMapper(IssueChangeMapper.class); for (DefaultIssue issue : issuesToInsert) { - String key = doInsert(session, now, issue); - inserted.add(key); + IssueDto issueDto = doInsert(session, now, issue); + inserted.add(issueDto); insertChanges(issueChangeMapper, issue); if (count > BatchSession.MAX_BATCH_SIZE) { session.commit(); @@ -124,19 +129,19 @@ public abstract class IssueStorage { return inserted; } - protected abstract String doInsert(DbSession batchSession, long now, DefaultIssue issue); + protected abstract IssueDto doInsert(DbSession batchSession, long now, DefaultIssue issue); /** * @return the keys of the updated issues */ - private Collection update(List issuesToUpdate, long now) { - Collection updated = new ArrayList<>(); + private Collection update(List issuesToUpdate, long now) { + Collection updated = new ArrayList<>(); if (!issuesToUpdate.isEmpty()) { try (DbSession dbSession = dbClient.openSession(false)) { IssueChangeMapper issueChangeMapper = dbSession.getMapper(IssueChangeMapper.class); for (DefaultIssue issue : issuesToUpdate) { - String key = doUpdate(dbSession, now, issue); - updated.add(key); + IssueDto issueDto = doUpdate(dbSession, now, issue); + updated.add(issueDto); insertChanges(issueChangeMapper, issue); } dbSession.commit(); @@ -145,7 +150,7 @@ public abstract class IssueStorage { return updated; } - protected abstract String doUpdate(DbSession batchSession, long now, DefaultIssue issue); + protected abstract IssueDto doUpdate(DbSession batchSession, long now, DefaultIssue issue); private void insertChanges(IssueChangeMapper mapper, DefaultIssue issue) { for (IssueComment comment : issue.comments()) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueUpdater.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueUpdater.java index 4976b5b5a7a..1c495c803f8 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueUpdater.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueUpdater.java @@ -28,10 +28,15 @@ 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.RuleDefinitionDto; import org.sonar.server.issue.notification.IssueChangeNotification; +import org.sonar.server.issue.ws.SearchResponseData; import org.sonar.server.notification.NotificationManager; +import static java.util.Collections.singleton; +import static java.util.Collections.singletonList; + public class IssueUpdater { private final DbClient dbClient; @@ -44,21 +49,46 @@ public class IssueUpdater { this.notificationService = notificationService; } - public void saveIssue(DbSession session, DefaultIssue issue, IssueChangeContext context, @Nullable String comment) { - issueStorage.save(session, issue); + /** + * Same as {@link #saveIssue(DbSession, DefaultIssue, IssueChangeContext, String)} but populates the specified + * {@link SearchResponseData} with the DTOs (rule and components) retrieved from DB to save the issue. + */ + public SearchResponseData saveIssueAndPreloadSearchResponseData(DbSession session, DefaultIssue issue, IssueChangeContext context, @Nullable String comment) { Optional rule = getRuleByKey(session, issue.getRuleKey()); ComponentDto project = dbClient.componentDao().selectOrFailByUuid(session, issue.projectUuid()); + ComponentDto component = dbClient.componentDao().selectOrFailByUuid(session, issue.componentUuid()); + IssueDto issueDto = saveIssue(session, issue, context, comment, rule, project, component); + + SearchResponseData preloadedSearchResponseData = new SearchResponseData(issueDto); + rule.ifPresent(r -> preloadedSearchResponseData.setRules(singletonList(r))); + preloadedSearchResponseData.addComponents(singleton(project)); + preloadedSearchResponseData.addComponents(singleton(component)); + return preloadedSearchResponseData; + } + + public IssueDto saveIssue(DbSession session, DefaultIssue issue, IssueChangeContext context, @Nullable String comment) { + Optional rule = getRuleByKey(session, issue.getRuleKey()); + ComponentDto project = dbClient.componentDao().selectOrFailByUuid(session, issue.projectUuid()); + ComponentDto component = dbClient.componentDao().selectOrFailByUuid(session, issue.componentUuid()); + return saveIssue(session, issue, context, comment, rule, project, component); + } + + private IssueDto saveIssue(DbSession session, DefaultIssue issue, IssueChangeContext context, @Nullable String comment, + Optional rule, ComponentDto project, ComponentDto component) { + IssueDto issueDto = issueStorage.save(session, issue); notificationService.scheduleForSending(new IssueChangeNotification() .setIssue(issue) .setChangeAuthorLogin(context.login()) - .setRuleName(rule.isPresent() ? rule.get().getName() : null) + .setRuleName(rule.map(RuleDefinitionDto::getName).orElse(null)) .setProject(project.getKey(), project.name()) - .setComponent(dbClient.componentDao().selectOrFailByUuid(session, issue.componentUuid())) + .setComponent(component) .setComment(comment)); + return issueDto; } private Optional getRuleByKey(DbSession session, RuleKey ruleKey) { Optional rule = Optional.ofNullable(dbClient.ruleDao().selectDefinitionByKey(session, ruleKey).orElse(null)); return (rule.isPresent() && rule.get().getStatus() != RuleStatus.REMOVED) ? rule : Optional.empty(); } + } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ServerIssueStorage.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ServerIssueStorage.java index ef46fe55acb..6e0fe687338 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/ServerIssueStorage.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ServerIssueStorage.java @@ -44,21 +44,21 @@ public class ServerIssueStorage extends IssueStorage { } @Override - protected String doInsert(DbSession session, long now, DefaultIssue issue) { + protected IssueDto doInsert(DbSession session, long now, DefaultIssue issue) { ComponentDto component = component(session, issue); ComponentDto project = project(session, issue); int ruleId = rule(issue).getId(); IssueDto dto = IssueDto.toDtoForServerInsert(issue, component, project, ruleId, now); getDbClient().issueDao().insert(session, dto); - return dto.getKey(); + return dto; } @Override - protected String doUpdate(DbSession session, long now, DefaultIssue issue) { + protected IssueDto doUpdate(DbSession session, long now, DefaultIssue issue) { IssueDto dto = IssueDto.toDtoForUpdate(issue, now); getDbClient().issueDao().update(session, dto); - return dto.getKey(); + return dto; } @Override diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java index f4c4792d378..85b1fdf581f 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java @@ -54,7 +54,6 @@ import static org.sonarqube.ws.client.issue.IssuesWsParameters.PARAM_ASSIGNEE; import static org.sonarqube.ws.client.issue.IssuesWsParameters.PARAM_ISSUE; public class AssignAction implements IssuesWsAction { - private static final String DEPRECATED_PARAM_ME = "me"; private static final String ASSIGN_TO_ME_VALUE = "_me"; @@ -107,11 +106,11 @@ public class AssignAction implements IssuesWsAction { userSession.checkLoggedIn(); String assignee = getAssignee(request); String key = request.mandatoryParam(PARAM_ISSUE); - assign(key, assignee); - responseWriter.write(key, request, response); + SearchResponseData preloadedResponseData = assign(key, assignee); + responseWriter.write(key, preloadedResponseData, request, response); } - private void assign(String issueKey, @Nullable String assignee) { + private SearchResponseData assign(String issueKey, @Nullable String assignee) { try (DbSession dbSession = dbClient.openSession(false)) { IssueDto issueDto = issueFinder.getByKey(dbSession, issueKey); DefaultIssue issue = issueDto.toDefaultIssue(); @@ -121,8 +120,9 @@ public class AssignAction implements IssuesWsAction { } IssueChangeContext context = IssueChangeContext.createUser(new Date(system2.now()), userSession.getLogin()); if (issueFieldsSetter.assign(issue, user, context)) { - issueUpdater.saveIssue(dbSession, issue, context, null); + return issueUpdater.saveIssueAndPreloadSearchResponseData(dbSession, issue, context, null); } + return new SearchResponseData(issueDto); } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/OperationResponseWriter.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/OperationResponseWriter.java index 961998f64ff..0365afabd47 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/OperationResponseWriter.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/OperationResponseWriter.java @@ -24,7 +24,7 @@ import org.sonar.api.server.ws.Response; import org.sonar.server.ws.WsUtils; import org.sonarqube.ws.Issues; -import static java.util.Collections.singletonList; +import static java.util.Collections.singleton; import static org.sonar.server.issue.ws.SearchAdditionalField.ALL_ADDITIONAL_FIELDS; public class OperationResponseWriter { @@ -38,12 +38,20 @@ public class OperationResponseWriter { } public void write(String issueKey, Request request, Response response) { - SearchResponseLoader.Collector collector = new SearchResponseLoader.Collector( - ALL_ADDITIONAL_FIELDS, singletonList(issueKey)); + SearchResponseLoader.Collector collector = new SearchResponseLoader.Collector(ALL_ADDITIONAL_FIELDS, singleton(issueKey)); SearchResponseData data = loader.load(collector, null); Issues.Operation responseBody = format.formatOperation(data); WsUtils.writeProtobuf(responseBody, request, response); } + + public void write(String issueKey, SearchResponseData preloadedResponseData, Request request, Response response) { + SearchResponseLoader.Collector collector = new SearchResponseLoader.Collector(ALL_ADDITIONAL_FIELDS, singleton(issueKey)); + SearchResponseData data = loader.load(preloadedResponseData, collector, null); + + Issues.Operation responseBody = format.formatOperation(data); + + WsUtils.writeProtobuf(responseBody, request, response); + } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseData.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseData.java index 5cad4533905..d38dc4962c9 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseData.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseData.java @@ -20,6 +20,7 @@ package org.sonar.server.issue.ws; import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ListMultimap; import java.util.Collection; import java.util.HashMap; @@ -56,6 +57,11 @@ public class SearchResponseData { private final ListMultimap transitionsByIssueKey = ArrayListMultimap.create(); private final Set updatableComments = new HashSet<>(); + public SearchResponseData(IssueDto issue) { + checkNotNull(issue); + this.issues = ImmutableList.of(issue); + } + public SearchResponseData(List issues) { checkNotNull(issues); this.issues = issues; diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java index e0f16181d98..38926c176b0 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/SearchResponseLoader.java @@ -19,6 +19,7 @@ */ package org.sonar.server.issue.ws; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.MultimapBuilder; import com.google.common.collect.SetMultimap; import java.util.Collection; @@ -28,6 +29,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import javax.annotation.Nullable; +import org.sonar.api.rule.RuleKey; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.util.stream.MoreCollectors; import org.sonar.db.DbClient; @@ -36,13 +38,21 @@ import org.sonar.db.component.ComponentDto; import org.sonar.db.issue.IssueChangeDto; import org.sonar.db.issue.IssueDto; import org.sonar.db.protobuf.DbIssues; +import org.sonar.db.rule.RuleDefinitionDto; +import org.sonar.db.user.UserDto; import org.sonar.server.es.Facets; import org.sonar.server.issue.ActionFinder; import org.sonar.server.issue.TransitionService; import org.sonar.server.user.UserSession; import org.sonarqube.ws.client.issue.IssuesWsParameters; +import static com.google.common.base.MoreObjects.firstNonNull; +import static com.google.common.collect.ImmutableSet.copyOf; import static com.google.common.collect.Lists.newArrayList; +import static com.google.common.collect.Sets.difference; +import static java.util.Collections.emptyList; +import static java.util.stream.Stream.concat; +import static org.sonar.core.util.stream.MoreCollectors.toList; import static org.sonar.server.issue.ws.SearchAdditionalField.ACTIONS; import static org.sonar.server.issue.ws.SearchAdditionalField.COMMENTS; import static org.sonar.server.issue.ws.SearchAdditionalField.RULES; @@ -87,9 +97,107 @@ public class SearchResponseLoader { } } + /** + * The issue keys are given by the multi-criteria search in Elasticsearch index. + *

+ * Same as {@link #load(Collector, Facets)} but will only retrieve from DB data which is not already provided by the + * specified preloaded {@link SearchResponseData}.
+ * The returned {@link SearchResponseData} is not the one specified as argument. + *

+ */ + public SearchResponseData load(SearchResponseData preloadedResponseData, Collector collector, @Nullable Facets facets) { + try (DbSession dbSession = dbClient.openSession(false)) { + SearchResponseData result = new SearchResponseData(loadIssues(preloadedResponseData, collector, dbSession)); + collector.collect(result.getIssues()); + + loadRules(preloadedResponseData, collector, dbSession, result); + // order is important - loading of comments complete the list of users: loadComments() is + // before loadUsers() + loadComments(collector, dbSession, result); + loadUsers(preloadedResponseData, collector, dbSession, result); + loadComponents(preloadedResponseData, collector, dbSession, result); + loadOrganizations(dbSession, result); + loadActionsAndTransitions(collector, result); + completeTotalEffortFromFacet(facets, result); + return result; + } + } + + private List loadIssues(SearchResponseData preloadedResponseData, Collector collector, DbSession dbSession) { + List preloadedIssues = preloadedResponseData.getIssues(); + Set preloadedIssueKeys = preloadedIssues.stream().map(IssueDto::getKey).collect(MoreCollectors.toSet(preloadedIssues.size())); + Set issueKeysToLoad = copyOf(difference(ImmutableSet.copyOf(collector.getIssueKeys()), preloadedIssueKeys)); + if (issueKeysToLoad.isEmpty()) { + return preloadedIssues; + } + + List loadedIssues = dbClient.issueDao().selectByKeys(dbSession, issueKeysToLoad); + return concat(preloadedIssues.stream(), loadedIssues.stream()) + .collect(toList(preloadedIssues.size() + loadedIssues.size())); + } + + private void loadUsers(SearchResponseData preloadedResponseData, Collector collector, DbSession dbSession, SearchResponseData result) { + if (collector.contains(USERS)) { + List preloadedUsers = firstNonNull(preloadedResponseData.getUsers(), emptyList()); + Set preloadedLogins = preloadedUsers.stream().map(UserDto::getLogin).collect(MoreCollectors.toSet(preloadedUsers.size())); + Set requestedLogins = collector.get(USERS); + Set loginsToLoad = copyOf(difference(requestedLogins, preloadedLogins)); + + if (loginsToLoad.isEmpty()) { + result.setUsers(preloadedUsers); + } else { + List loadedUsers = dbClient.userDao().selectByLogins(dbSession, loginsToLoad); + result.setUsers(concat(preloadedUsers.stream(), loadedUsers.stream()).collect(toList(preloadedUsers.size() + loadedUsers.size()))); + } + } + } + + private void loadComponents(SearchResponseData preloadedResponseData, Collector collector, DbSession dbSession, SearchResponseData result) { + Collection preloadedComponents = preloadedResponseData.getComponents(); + Set preloadedComponentUuids = preloadedComponents.stream().map(ComponentDto::uuid).collect(MoreCollectors.toSet(preloadedComponents.size())); + Set componentUuidsToLoad = copyOf(difference(collector.getComponentUuids(), preloadedComponentUuids)); + + result.addComponents(preloadedComponents); + if (!componentUuidsToLoad.isEmpty()) { + result.addComponents(dbClient.componentDao().selectByUuids(dbSession, componentUuidsToLoad)); + } + + // always load components and projects, because some issue fields still relate to component ids/keys. + // They should be dropped but are kept for backward-compatibility (see SearchResponseFormat) + result.addComponents(dbClient.componentDao().selectSubProjectsByComponentUuids(dbSession, collector.getComponentUuids())); + addProjectUuids(collector, dbSession, result); + } + + private void addProjectUuids(Collector collector, DbSession dbSession, SearchResponseData result) { + Collection loadedComponents = result.getComponents(); + for (ComponentDto component : loadedComponents) { + collector.addProjectUuid(component.projectUuid()); + } + Set loadedProjectUuids = loadedComponents.stream().filter(cpt -> cpt.uuid().equals(cpt.projectUuid())).map(ComponentDto::uuid).collect(MoreCollectors.toSet()); + Set projectUuidsToLoad = copyOf(difference(collector.getProjectUuids(), loadedProjectUuids)); + if (!projectUuidsToLoad.isEmpty()) { + List projects = dbClient.componentDao().selectByUuids(dbSession, collector.getProjectUuids()); + result.addComponents(projects); + } + } + + private void loadRules(SearchResponseData preloadedResponseData, Collector collector, DbSession dbSession, SearchResponseData result) { + if (collector.contains(RULES)) { + List preloadedRules = firstNonNull(preloadedResponseData.getRules(), emptyList()); + Set preloaedRuleKeys = preloadedRules.stream().map(RuleDefinitionDto::getKey).collect(MoreCollectors.toSet()); + Set ruleKeysToLoad = copyOf(difference(collector.get(RULES), preloaedRuleKeys)); + if (ruleKeysToLoad.isEmpty()) { + result.setRules(preloadedResponseData.getRules()); + } else { + List loadedRules = dbClient.ruleDao().selectDefinitionByKeys(dbSession, ruleKeysToLoad); + result.setRules(concat(preloadedRules.stream(), loadedRules.stream()).collect(toList(preloadedRules.size() + loadedRules.size()))); + } + } + } + private void loadUsers(Collector collector, DbSession dbSession, SearchResponseData result) { if (collector.contains(USERS)) { - result.setUsers(dbClient.userDao().selectByLogins(dbSession, collector.get(USERS))); + result.setUsers(dbClient.userDao().selectByLogins(dbSession, collector.getList(USERS))); } } @@ -112,7 +220,7 @@ public class SearchResponseLoader { private void loadRules(Collector collector, DbSession dbSession, SearchResponseData result) { if (collector.contains(RULES)) { - result.setRules(dbClient.ruleDao().selectDefinitionByKeys(dbSession, collector.get(RULES))); + result.setRules(dbClient.ruleDao().selectDefinitionByKeys(dbSession, collector.getList(RULES))); } } @@ -121,11 +229,7 @@ public class SearchResponseLoader { // They should be dropped but are kept for backward-compatibility (see SearchResponseFormat) result.addComponents(dbClient.componentDao().selectByUuids(dbSession, collector.getComponentUuids())); result.addComponents(dbClient.componentDao().selectSubProjectsByComponentUuids(dbSession, collector.getComponentUuids())); - for (ComponentDto component : result.getComponents()) { - collector.addProjectUuid(component.projectUuid()); - } - List projects = dbClient.componentDao().selectByUuids(dbSession, collector.getProjectUuids()); - result.addComponents(projects); + addProjectUuids(collector, dbSession, result); } private void loadOrganizations(DbSession dbSession, SearchResponseData result) { @@ -232,8 +336,12 @@ public class SearchResponseLoader { } } - List get(SearchAdditionalField key) { - return newArrayList((Set) fieldValues.get(key)); + Set get(SearchAdditionalField key) { + return (Set) fieldValues.get(key); + } + + List getList(SearchAdditionalField key) { + return newArrayList(get(key)); } boolean contains(SearchAdditionalField field) { diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueStorageTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueStorageTest.java index 51201c20090..3ae20473c60 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueStorageTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueStorageTest.java @@ -259,19 +259,19 @@ public class IssueStorageTest { } @Override - protected String doInsert(DbSession session, long now, DefaultIssue issue) { + protected IssueDto doInsert(DbSession session, long now, DefaultIssue issue) { int ruleId = rule(issue).getId(); IssueDto dto = IssueDto.toDtoForComputationInsert(issue, ruleId, now); session.getMapper(IssueMapper.class).insert(dto); - return dto.getKey(); + return dto; } @Override - protected String doUpdate(DbSession session, long now, DefaultIssue issue) { + protected IssueDto doUpdate(DbSession session, long now, DefaultIssue issue) { IssueDto dto = IssueDto.toDtoForUpdate(issue, now); session.getMapper(IssueMapper.class).update(dto); - return dto.getKey(); + return dto; } } @@ -287,19 +287,19 @@ public class IssueStorageTest { } @Override - protected String doInsert(DbSession session, long now, DefaultIssue issue) { + protected IssueDto doInsert(DbSession session, long now, DefaultIssue issue) { int ruleId = rule(issue).getId(); IssueDto dto = IssueDto.toDtoForServerInsert(issue, component, project, ruleId, now); session.getMapper(IssueMapper.class).insert(dto); - return dto.getKey(); + return dto; } @Override - protected String doUpdate(DbSession session, long now, DefaultIssue issue) { + protected IssueDto doUpdate(DbSession session, long now, DefaultIssue issue) { IssueDto dto = IssueDto.toDtoForUpdate(issue, now); session.getMapper(IssueMapper.class).update(dto); - return dto.getKey(); + return dto; } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueUpdaterTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueUpdaterTest.java index ec05df31eca..ccd47a3d5c8 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueUpdaterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueUpdaterTest.java @@ -35,13 +35,16 @@ import org.sonar.db.component.ComponentDbTester; import org.sonar.db.component.ComponentDto; import org.sonar.db.issue.IssueDbTester; import org.sonar.db.issue.IssueDto; +import org.sonar.db.issue.IssueTesting; import org.sonar.db.rule.RuleDbTester; +import org.sonar.db.rule.RuleDefinitionDto; import org.sonar.db.rule.RuleDto; import org.sonar.server.es.EsTester; import org.sonar.server.issue.index.IssueIndexDefinition; import org.sonar.server.issue.index.IssueIndexer; import org.sonar.server.issue.index.IssueIteratorFactory; import org.sonar.server.issue.notification.IssueChangeNotification; +import org.sonar.server.issue.ws.SearchResponseData; import org.sonar.server.notification.NotificationManager; import org.sonar.server.organization.DefaultOrganizationProvider; import org.sonar.server.organization.TestDefaultOrganizationProvider; @@ -100,7 +103,7 @@ public class IssueUpdaterTest { RuleDto rule = ruleDbTester.insertRule(newRuleDto()); ComponentDto project = componentDbTester.insertPrivateProject(); ComponentDto file = componentDbTester.insertComponent(newFileDto(project)); - DefaultIssue issue = issueDbTester.insertIssue(newDto(rule, file, project)).setSeverity(MAJOR).toDefaultIssue(); + DefaultIssue issue = issueDbTester.insertIssue(IssueTesting.newIssue(rule.getDefinition(), project, file)).setSeverity(MAJOR).toDefaultIssue(); IssueChangeContext context = IssueChangeContext.createUser(new Date(), "john"); issueFieldsSetter.setSeverity(issue, BLOCKER, context); @@ -125,7 +128,7 @@ public class IssueUpdaterTest { RuleDto rule = ruleDbTester.insertRule(newRuleDto().setStatus(RuleStatus.REMOVED)); ComponentDto project = componentDbTester.insertPrivateProject(); ComponentDto file = componentDbTester.insertComponent(newFileDto(project)); - DefaultIssue issue = issueDbTester.insertIssue(newDto(rule, file, project)).setSeverity(MAJOR).toDefaultIssue(); + DefaultIssue issue = issueDbTester.insertIssue(IssueTesting.newIssue(rule.getDefinition(), project, file)).setSeverity(MAJOR).toDefaultIssue(); IssueChangeContext context = IssueChangeContext.createUser(new Date(), "john"); issueFieldsSetter.setSeverity(issue, BLOCKER, context); @@ -142,4 +145,49 @@ public class IssueUpdaterTest { return newDto(rule, file, project); } + @Test + public void saveIssue_populates_specified_SearchResponseData_with_rule_project_and_component_retrieved_from_DB() { + RuleDto rule = ruleDbTester.insertRule(newRuleDto()); + ComponentDto project = componentDbTester.insertPrivateProject(); + ComponentDto file = componentDbTester.insertComponent(newFileDto(project)); + IssueDto issueDto = IssueTesting.newIssue(rule.getDefinition(), project, file); + DefaultIssue issue = issueDbTester.insertIssue(issueDto).setSeverity(MAJOR).toDefaultIssue(); + IssueChangeContext context = IssueChangeContext.createUser(new Date(), "john"); + issueFieldsSetter.setSeverity(issue, BLOCKER, context); + + SearchResponseData preloadedSearchResponseData = underTest.saveIssueAndPreloadSearchResponseData(dbTester.getSession(), issue, context, null); + + assertThat(preloadedSearchResponseData.getIssues()) + .hasSize(1); + assertThat(preloadedSearchResponseData.getIssues().iterator().next()) + .isNotSameAs(issueDto); + assertThat(preloadedSearchResponseData.getRules()) + .extracting(RuleDefinitionDto::getKey) + .containsOnly(rule.getKey()); + assertThat(preloadedSearchResponseData.getComponents()) + .extracting(ComponentDto::uuid) + .containsOnly(project.uuid(), file.uuid()); + } + + @Test + public void saveIssue_populates_specified_SearchResponseData_with_no_rule_but_with_project_and_component_if_rule_is_removed() { + RuleDto rule = ruleDbTester.insertRule(newRuleDto().setStatus(RuleStatus.REMOVED)); + ComponentDto project = componentDbTester.insertPrivateProject(); + ComponentDto file = componentDbTester.insertComponent(newFileDto(project)); + IssueDto issueDto = IssueTesting.newIssue(rule.getDefinition(), project, file); + DefaultIssue issue = issueDbTester.insertIssue(issueDto).setSeverity(MAJOR).toDefaultIssue(); + IssueChangeContext context = IssueChangeContext.createUser(new Date(), "john"); + issueFieldsSetter.setSeverity(issue, BLOCKER, context); + + SearchResponseData preloadedSearchResponseData = underTest.saveIssueAndPreloadSearchResponseData(dbTester.getSession(), issue, context, null); + + assertThat(preloadedSearchResponseData.getIssues()) + .hasSize(1); + assertThat(preloadedSearchResponseData.getIssues().iterator().next()) + .isNotSameAs(issueDto); + assertThat(preloadedSearchResponseData.getRules()).isNullOrEmpty(); + assertThat(preloadedSearchResponseData.getComponents()) + .extracting(ComponentDto::uuid) + .containsOnly(project.uuid(), file.uuid()); + } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java index 88fac14b25f..c3194e61e06 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java @@ -23,13 +23,16 @@ import javax.annotation.Nullable; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.mockito.ArgumentCaptor; import org.sonar.api.config.MapSettings; import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.Response; import org.sonar.api.utils.internal.TestSystem2; import org.sonar.db.DbTester; +import org.sonar.db.component.ComponentDto; import org.sonar.db.issue.IssueDto; import org.sonar.db.organization.OrganizationDto; +import org.sonar.db.rule.RuleDefinitionDto; import org.sonar.db.user.UserDto; import org.sonar.server.es.EsTester; import org.sonar.server.exceptions.ForbiddenException; @@ -84,6 +87,7 @@ public class AssignActionTest { new ServerIssueStorage(system2, new DefaultRuleFinder(db.getDbClient(), defaultOrganizationProvider), db.getDbClient(), issueIndexer), mock(NotificationManager.class)), responseWriter); + private ArgumentCaptor preloadedSearchResponseDataCaptor = ArgumentCaptor.forClass(SearchResponseData.class); private WsActionTester ws = new WsActionTester(underTest); @Test @@ -97,7 +101,8 @@ public class AssignActionTest { .execute(); checkIssueAssignee(issue.getKey(), "arthur"); - verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class)); + verify(responseWriter).write(eq(issue.getKey()), preloadedSearchResponseDataCaptor.capture(), any(Request.class), any(Response.class)); + verifyContentOfPreloadedSearchResponseData(issue); } @Test @@ -110,7 +115,8 @@ public class AssignActionTest { .execute(); checkIssueAssignee(issue.getKey(), CURRENT_USER_LOGIN); - verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class)); + verify(responseWriter).write(eq(issue.getKey()), preloadedSearchResponseDataCaptor.capture(), any(Request.class), any(Response.class)); + verifyContentOfPreloadedSearchResponseData(issue); } @Test @@ -123,7 +129,8 @@ public class AssignActionTest { .execute(); checkIssueAssignee(issue.getKey(), CURRENT_USER_LOGIN); - verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class)); + verify(responseWriter).write(eq(issue.getKey()), preloadedSearchResponseDataCaptor.capture(), any(Request.class), any(Response.class)); + verifyContentOfPreloadedSearchResponseData(issue); } @Test @@ -135,7 +142,8 @@ public class AssignActionTest { .execute(); checkIssueAssignee(issue.getKey(), null); - verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class)); + verify(responseWriter).write(eq(issue.getKey()), preloadedSearchResponseDataCaptor.capture(), any(Request.class), any(Response.class)); + verifyContentOfPreloadedSearchResponseData(issue); } @Test @@ -148,7 +156,8 @@ public class AssignActionTest { .execute(); checkIssueAssignee(issue.getKey(), null); - verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class)); + verify(responseWriter).write(eq(issue.getKey()), preloadedSearchResponseDataCaptor.capture(), any(Request.class), any(Response.class)); + verifyContentOfPreloadedSearchResponseData(issue); } @Test @@ -236,6 +245,19 @@ public class AssignActionTest { .execute(); } + private void verifyContentOfPreloadedSearchResponseData(IssueDto issue) { + SearchResponseData preloadedSearchResponseData = preloadedSearchResponseDataCaptor.getValue(); + assertThat(preloadedSearchResponseData.getIssues()) + .extracting(IssueDto::getKey) + .containsOnly(issue.getKey()); + assertThat(preloadedSearchResponseData.getRules()) + .extracting(RuleDefinitionDto::getKey) + .containsOnly(issue.getRuleKey()); + assertThat(preloadedSearchResponseData.getComponents()) + .extracting(ComponentDto::uuid) + .containsOnly(issue.getComponentUuid(), issue.getProjectUuid()); + } + private UserDto insertUser(String login) { UserDto user = db.users().insertUser(login); db.organizations().addMember(db.getDefaultOrganization(), user); -- 2.39.5