opendev/gerrit
Code Issues Proposed changes

GetPastAssignees: Return past assignees as list

Browse Source 
When formatting the past assignees as AccountInfo the ordered
LinkedHashSet that was retrieved from NoteDb was converted into an
unordered Set. Hence the results were returned in an undefined order.
This is the reason why the AssigneeIT#testGetPastAssignees() that
verified the correct order was flaky.

Returning the past assignees as set doesn't make sense since JSON
doesn't have sets and Gson converts it to a list anyway.

Change-Id: Ia78d8872d3e9e32928e7954b694c8f76ccdc8e0b
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-09-28 17:43:07 +02:00
parent 6c0b685cd7
commit cf99c9b773
4 changed files with 11 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AssigneeIT.java
View File

@@ -30,7 +30,7 @@ import org.junit.BeforeClass;
import org.junit.Test;
import java.util.Iterator;
import java.util.Set;
import java.util.List;
@NoHttpd
public class AssigneeIT extends AbstractDaemonTest {
@@ -73,7 +73,7 @@ public class AssigneeIT extends AbstractDaemonTest {
PushOneCommit.Result r = createChange();
setAssignee(r, user.email);
setAssignee(r, admin.email);
Set<AccountInfo> assignees = getPastAssignees(r);
List<AccountInfo> assignees = getPastAssignees(r);
assertThat(assignees).hasSize(2);
Iterator<AccountInfo> itr = assignees.iterator();
assertThat(itr.next()._accountId).isEqualTo(user.getId().get());
@@ -107,7 +107,7 @@ public class AssigneeIT extends AbstractDaemonTest {
return gApi.changes().id(r.getChange().getId().get()).getAssignee();
}
private Set<AccountInfo> getPastAssignees(PushOneCommit.Result r)
private List<AccountInfo> getPastAssignees(PushOneCommit.Result r)
throws Exception {
return gApi.changes().id(r.getChange().getId().get()).getPastAssignees();
}

4
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
View File

@@ -155,7 +155,7 @@ public interface ChangeApi {
/**
* Get all past assignees.
*/
Set<AccountInfo> getPastAssignees() throws RestApiException;
List<AccountInfo> getPastAssignees() throws RestApiException;
/**
* Delete the assignee of a change.
@@ -361,7 +361,7 @@ public interface ChangeApi {
}
@Override
public Set<AccountInfo> getPastAssignees() throws RestApiException {
public List<AccountInfo> getPastAssignees() throws RestApiException {
throw new NotImplementedException();
}

2
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@@ -441,7 +441,7 @@ class ChangeApiImpl implements ChangeApi {
}
@Override
public Set<AccountInfo> getPastAssignees() throws RestApiException {
public List<AccountInfo> getPastAssignees() throws RestApiException {
try {
return getPastAssignees.apply(change).value();
} catch (Exception e) {

9
gerrit-server/src/main/java/com/google/gerrit/server/change/GetPastAssignees.java
View File

@@ -14,7 +14,7 @@
package com.google.gerrit.server.change;
import static java.util.stream.Collectors.toSet;
import static java.util.stream.Collectors.toList;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.restapi.Response;
@@ -26,6 +26,7 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.util.Collections;
import java.util.List;
import java.util.Set;
public class GetPastAssignees implements RestReadView<ChangeResource> {
@@ -37,19 +38,19 @@ public class GetPastAssignees implements RestReadView<ChangeResource> {
}
@Override
public Response<Set<AccountInfo>> apply(ChangeResource rsrc)
public Response<List<AccountInfo>> apply(ChangeResource rsrc)
throws OrmException {
Set<Account.Id> pastAssignees =
rsrc.getControl().getNotes().load().getPastAssignees();
if (pastAssignees == null) {
return Response.ok(Collections.emptySet());
return Response.ok(Collections.emptyList());
}
AccountInfoCacheFactory accountInfoFactory = accountInfos.create();
return Response.ok(pastAssignees.stream()
.map(accountInfoFactory::get)
.map(AccountJson::toAccountInfo)
.collect(toSet()));
.collect(toList()));
}
}