opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.15'

Browse Source 
* stable-2.15:
  Fix "Group members can't be added as reviewers" after merging
  Set version to 2.15-SNAPSHOT
  ProjectLevelConfig: Import Collectors.toList static
  Merge parent config values with child values
  Set version to 2.14.8-SNAPSHOT
  Fix the missing DB entry in Gerrit DB
  Fix unit test "addGroupAsReviewersWhenANotPerfectMatchedUserExist"

Change-Id: I18e635f55f69892b1c5c9f203030f4181973fa3c
This commit is contained in:
David Pursehouse
2018-03-15 10:14:46 +09:00
parent d7fcb76493 7ba0df952e
commit 572e51c051
6 changed files with 219 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
java/com/google/gerrit/server/account/AccountManager.java
View File

@@ -143,7 +143,21 @@ public class AccountManager {
try (ReviewDb db = schema.open()) {
ExternalId id = externalIds.get(who.getExternalIdKey());
if (id == null) {
if (who.getUserName().isPresent()) {
ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, who.getUserName().get());
ExternalId existingId = externalIds.get(key);
if (existingId != null) {
// An inconsistency is detected in the database, having a record for scheme "username:"
// but no record for scheme "gerrit:". Try to recover by linking
// "gerrit:" identity to the existing account.
log.warn(
"User {} already has an account; link new identity to the existing account.",
who.getUserName());
return link(existingId.accountId(), who);
}
}
// New account, automatically create and return.
log.info("External ID not found. Attempting to create new account.");
return create(db, who);
}
@@ -383,6 +397,7 @@ public class AccountManager {
public AuthResult link(Account.Id to, AuthRequest who)
throws AccountException, OrmException, IOException, ConfigInvalidException {
ExternalId extId = externalIds.get(who.getExternalIdKey());
log.info("Link another authentication identity to an existing account");
if (extId != null) {
if (!extId.accountId().equals(to)) {
throw new AccountException(
@@ -390,6 +405,7 @@ public class AccountManager {
}
update(who, extId);
} else {
log.info("Linking new external ID to the existing account");
accountsUpdateProvider
.get()
.update(

52
java/com/google/gerrit/server/config/PluginConfigFactory.java
View File

@@ -288,7 +288,32 @@ public class PluginConfigFactory implements ReloadPluginListener {
*/
public Config getProjectPluginConfigWithInheritance(
Project.NameKey projectName, String pluginName) throws NoSuchProjectException {
return getPluginConfig(projectName, pluginName).getWithInheritance();
return getPluginConfig(projectName, pluginName).getWithInheritance(false);
}
/**
* Returns the configuration for the specified plugin that is stored in the '{@code
* <plugin-name>.config}' file in the 'refs/meta/config' branch of the specified project.
* Parameters from the '{@code <plugin-name>.config}' of the parent project are appended to this
* project's '{@code <plugin-name>.config}' files.
*
* <p>E.g.: child project: [mySection "mySubsection"] myKey = childValue
*
* <p>parent project: [mySection "mySubsection"] myKey = parentValue anotherKey = someValue
*
* <p>return: [mySection "mySubsection"] myKey = childValue myKey = parentValue anotherKey =
* someValue
*
* @param projectName the name of the project for which the plugin configuration should be
* returned
* @param pluginName the name of the plugin for which the configuration should be returned
* @return the plugin configuration from the '{@code <plugin-name>.config}' file of the specified
* project with parameters from the parent projects appended to the project values
* @throws NoSuchProjectException thrown if the specified project does not exist
*/
public Config getProjectPluginConfigWithMergedInheritance(
Project.NameKey projectName, String pluginName) throws NoSuchProjectException {
return getPluginConfig(projectName, pluginName).getWithInheritance(true);
}
/**
@@ -310,7 +335,30 @@ public class PluginConfigFactory implements ReloadPluginListener {
*/
public Config getProjectPluginConfigWithInheritance(
ProjectState projectState, String pluginName) {
return projectState.getConfig(pluginName + EXTENSION).getWithInheritance();
return projectState.getConfig(pluginName + EXTENSION).getWithInheritance(false);
}
/**
* Returns the configuration for the specified plugin that is stored in the '{@code
* <plugin-name>.config}' file in the 'refs/meta/config' branch of the specified project.
* Parameters from the '{@code <plugin-name>.config}' of the parent project are appended to this
* project's '{@code <plugin-name>.config}' files.
*
* <p>E.g.: child project: [mySection "mySubsection"] myKey = childValue
*
* <p>parent project: [mySection "mySubsection"] myKey = parentValue anotherKey = someValue
*
* <p>return: [mySection "mySubsection"] myKey = childValue myKey = parentValue anotherKey =
* someValue
*
* @param projectState the project for which the plugin configuration should be returned
* @param pluginName the name of the plugin for which the configuration should be returned
* @return the plugin configuration from the '{@code <plugin-name>.config}' file of the specified
* project with inheriting non-set parameters from the parent projects
*/
public Config getProjectPluginConfigWithMergedInheritance(
ProjectState projectState, String pluginName) {
return projectState.getConfig(pluginName + EXTENSION).getWithInheritance(true);
}
private ProjectLevelConfig getPluginConfig(Project.NameKey projectName, String pluginName)

44
java/com/google/gerrit/server/git/ProjectLevelConfig.java
View File

@@ -14,12 +14,16 @@
package com.google.gerrit.server.git;
import static java.util.stream.Collectors.toList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.project.ProjectState;
import java.io.IOException;
import java.util.Arrays;
import java.util.Set;
import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Config;
@@ -53,6 +57,22 @@ public class ProjectLevelConfig extends VersionedMetaData {
}
public Config getWithInheritance() {
return getWithInheritance(false);
}
/**
* Get a Config that includes the values from all parent projects.
*
* <p>Merging means that matching sections/subsection will be merged to include the values from
* both parent and child config.
*
* <p>No merging means that matching sections/subsections in the child project will replace the
* corresponding value from the parent.
*
* @param merge whether to merge parent values with child values or not.
* @return a combined config.
*/
public Config getWithInheritance(boolean merge) {
Config cfgWithInheritance = new Config();
try {
cfgWithInheritance.fromText(get().toText());
@@ -65,21 +85,41 @@ public class ProjectLevelConfig extends VersionedMetaData {
for (String section : parentCfg.getSections()) {
Set<String> allNames = get().getNames(section);
for (String name : parentCfg.getNames(section)) {
String[] parentValues = parentCfg.getStringList(section, null, name);
if (!allNames.contains(name)) {
cfgWithInheritance.setStringList(section, null, name, Arrays.asList(parentValues));
} else if (merge) {
cfgWithInheritance.setStringList(
section, null, name, Arrays.asList(parentCfg.getStringList(section, null, name)));
section,
null,
name,
Stream.concat(
Arrays.stream(cfg.getStringList(section, null, name)),
Arrays.stream(parentValues))
.sorted()
.distinct()
.collect(toList()));
}
}
for (String subsection : parentCfg.getSubsections(section)) {
allNames = get().getNames(section, subsection);
for (String name : parentCfg.getNames(section, subsection)) {
String[] parentValues = parentCfg.getStringList(section, subsection, name);
if (!allNames.contains(name)) {
cfgWithInheritance.setStringList(
section, subsection, name, Arrays.asList(parentValues));
} else if (merge) {
cfgWithInheritance.setStringList(
section,
subsection,
name,
Arrays.asList(parentCfg.getStringList(section, subsection, name)));
Streams.concat(
Arrays.stream(cfg.getStringList(section, subsection, name)),
Arrays.stream(parentValues))
.sorted()
.distinct()
.collect(toList()));
}
}
}

48
java/com/google/gerrit/server/restapi/change/PostReviewers.java
View File

@@ -194,13 +194,20 @@ public class PostReviewers
Addition byAccountId =
addByAccountId(reviewer, rsrc, state, notify, accountsToNotify, allowGroup, allowByEmail);
Addition wholeGroup = null;
if (byAccountId == null || !byAccountId.exactMatchFound) {
wholeGroup =
addWholeGroup(
reviewer, rsrc, state, notify, accountsToNotify, confirmed, allowGroup, allowByEmail);
if (wholeGroup != null && wholeGroup.exactMatchFound) {
return wholeGroup;
}
}
if (byAccountId != null) {
return byAccountId;
}
Addition wholeGroup =
addWholeGroup(
reviewer, rsrc, state, notify, accountsToNotify, confirmed, allowGroup, allowByEmail);
if (wholeGroup != null) {
return wholeGroup;
}
@@ -216,7 +223,8 @@ public class PostReviewers
null,
CC,
NotifyHandling.NONE,
ImmutableListMultimap.of());
ImmutableListMultimap.of(),
true);
}
@Nullable
@@ -229,9 +237,14 @@ public class PostReviewers
boolean allowGroup,
boolean allowByEmail)
throws OrmException, PermissionBackendException, IOException, ConfigInvalidException {
Account.Id accountId = null;
IdentifiedUser user = null;
boolean exactMatchFound = false;
try {
accountId = accounts.parse(reviewer).getAccountId();
user = accounts.parse(reviewer);
if (reviewer.equalsIgnoreCase(user.getName())
|| reviewer.equals(String.valueOf(user.getAccountId()))) {
exactMatchFound = true;
}
} catch (UnprocessableEntityException | AuthException e) {
// AuthException won't occur since the user is authenticated at this point.
if (!allowGroup && !allowByEmail) {
@@ -242,13 +255,20 @@ public class PostReviewers
return null;
}
ReviewerResource rrsrc = reviewerFactory.create(rsrc, accountId);
ReviewerResource rrsrc = reviewerFactory.create(rsrc, user.getAccountId());
Account member = rrsrc.getReviewerUser().getAccount();
PermissionBackend.ForRef perm =
permissionBackend.user(rrsrc.getReviewerUser()).ref(rrsrc.getChange().getDest());
if (isValidReviewer(member, perm)) {
return new Addition(
reviewer, rsrc, ImmutableSet.of(member.getId()), null, state, notify, accountsToNotify);
reviewer,
rsrc,
ImmutableSet.of(member.getId()),
null,
state,
notify,
accountsToNotify,
exactMatchFound);
}
if (!member.isActive()) {
if (allowByEmail && state == CC) {
@@ -328,7 +348,7 @@ public class PostReviewers
}
}
return new Addition(reviewer, rsrc, reviewers, null, state, notify, accountsToNotify);
return new Addition(reviewer, rsrc, reviewers, null, state, notify, accountsToNotify, true);
}
@Nullable
@@ -357,7 +377,7 @@ public class PostReviewers
return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInvalid, reviewer));
}
return new Addition(
reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify);
reviewer, rsrc, null, ImmutableList.of(adr), state, notify, accountsToNotify, true);
}
private boolean isValidReviewer(Account member, PermissionBackend.ForRef perm)
@@ -395,6 +415,7 @@ public class PostReviewers
final ReviewerState state;
final ChangeNotes notes;
final IdentifiedUser caller;
final boolean exactMatchFound;
Addition(String reviewer) {
result = new AddReviewerResult(reviewer);
@@ -404,6 +425,7 @@ public class PostReviewers
state = REVIEWER;
notes = null;
caller = null;
exactMatchFound = false;
}
protected Addition(
@@ -413,7 +435,8 @@ public class PostReviewers
@Nullable Collection<Address> reviewersByEmail,
ReviewerState state,
@Nullable NotifyHandling notify,
ListMultimap<RecipientType, Account.Id> accountsToNotify) {
ListMultimap<RecipientType, Account.Id> accountsToNotify,
boolean exactMatchFound) {
checkArgument(
reviewers != null || reviewersByEmail != null,
"must have either reviewers or reviewersByEmail");
@@ -427,6 +450,7 @@ public class PostReviewers
op =
postReviewersOpFactory.create(
rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
this.exactMatchFound = exactMatchFound;
}
void gatherResults() throws OrmException, PermissionBackendException {

24
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -168,7 +168,6 @@ import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.PushResult;
import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
@NoHttpd
@@ -1506,7 +1505,6 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(sender.getMessages()).hasSize(1);
}
@Ignore
@Test
public void addReviewerThatIsNotPerfectMatch() throws Exception {
TestTimeUtil.resetWithClockStep(1, SECONDS);
@@ -1551,7 +1549,6 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
}
@Ignore
@Test
public void addGroupAsReviewersWhenANotPerfectMatchedUserExists() throws Exception {
TestTimeUtil.resetWithClockStep(1, SECONDS);
@@ -1560,16 +1557,17 @@ public class ChangeIT extends AbstractDaemonTest {
String oldETag = rsrc.getETag();
Timestamp oldTs = rsrc.getChange().getLastUpdatedOn();
//create a group named "us" with one user: testUser
TestAccount testUser = accountCreator.create("testUser", "testUser@test.com", "testUser");
String testGroup =
createGroupWithRealName(user.fullName.substring(0, user.fullName.length() / 2));
//create a group named "kobe" with one user: lee
TestAccount testUser = accountCreator.create("kobebryant", "kobebryant@test.com", "kobebryant");
TestAccount myGroupUser = accountCreator.create("lee", "lee@test.com", "lee");
String testGroup = createGroupWithRealName("kobe");
GroupApi groupApi = gApi.groups().id(testGroup);
groupApi.description("test group");
groupApi.addMembers(testUser.fullName);
groupApi.addMembers(myGroupUser.fullName);
//ensure that user "user" is not in the group
groupApi.removeMembers(user.fullName);
groupApi.removeMembers(testUser.fullName);
AddReviewerInput in = new AddReviewerInput();
in.reviewer = testGroup;
@@ -1578,11 +1576,11 @@ public class ChangeIT extends AbstractDaemonTest {
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(1);
Message m = messages.get(0);
assertThat(m.rcpt()).containsExactly(testUser.emailAddress);
assertThat(m.body()).contains("Hello " + testUser.fullName + ",\n");
assertThat(m.rcpt()).containsExactly(myGroupUser.emailAddress);
assertThat(m.body()).contains("Hello " + myGroupUser.fullName + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertMailReplyTo(m, testUser.email);
assertMailReplyTo(m, myGroupUser.email);
ChangeInfo c = gApi.changes().id(r.getChangeId()).get();
// When NoteDb is enabled adding a reviewer records that user as reviewer
@@ -1592,7 +1590,7 @@ public class ChangeIT extends AbstractDaemonTest {
Collection<AccountInfo> reviewers = c.reviewers.get(REVIEWER);
assertThat(reviewers).isNotNull();
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId).isEqualTo(testUser.getId().get());
assertThat(reviewers.iterator().next()._accountId).isEqualTo(myGroupUser.getId().get());
// Ensure ETag and lastUpdatedOn are updated.
rsrc = parseResource(r);

64
javatests/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java
View File

@@ -22,6 +22,7 @@ import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.project.ProjectState;
import java.util.Arrays;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.junit.Before;
@@ -114,4 +115,67 @@ public class ProjectLevelConfigIT extends AbstractDaemonTest {
assertThat(state.getConfig(configName).get().toText()).isEqualTo(cfg.toText());
}
@Test
public void withMergedInheritance() throws Exception {
String configName = "test.config";
Config parentCfg = new Config();
parentCfg.setString("s1", null, "k1", "parentValue1");
parentCfg.setString("s1", null, "k2", "parentValue2");
parentCfg.setString("s2", "ss", "k3", "parentValue3");
parentCfg.setString("s2", "ss", "k4", "parentValue4");
pushFactory
.create(
db,
admin.getIdent(),
testRepo,
"Create Project Level Config",
configName,
parentCfg.toText())
.to(RefNames.REFS_CONFIG)
.assertOkStatus();
Project.NameKey childProject = createProject("child", project);
TestRepository<?> childTestRepo = cloneProject(childProject);
fetch(childTestRepo, RefNames.REFS_CONFIG + ":refs/heads/config");
childTestRepo.reset("refs/heads/config");
Config cfg = new Config();
cfg.setString("s1", null, "k1", "parentValue1");
cfg.setString("s1", null, "k2", "parentValue2");
cfg.setString("s2", "ss", "k3", "parentValue3");
cfg.setString("s2", "ss", "k4", "parentValue4");
cfg.setString("s1", null, "k1", "childValue1");
cfg.setString("s2", "ss", "k3", "childValue2");
cfg.setString("s3", null, "k5", "childValue3");
cfg.setString("s3", "ss", "k6", "childValue4");
pushFactory
.create(
db,
admin.getIdent(),
childTestRepo,
"Create Project Level Config",
configName,
cfg.toText())
.to(RefNames.REFS_CONFIG)
.assertOkStatus();
ProjectState state = projectCache.get(childProject);
Config expectedCfg = new Config();
expectedCfg.setStringList("s1", null, "k1", Arrays.asList("childValue1", "parentValue1"));
expectedCfg.setString("s1", null, "k2", "parentValue2");
expectedCfg.setStringList("s2", "ss", "k3", Arrays.asList("childValue2", "parentValue3"));
expectedCfg.setString("s2", "ss", "k4", "parentValue4");
expectedCfg.setString("s3", null, "k5", "childValue3");
expectedCfg.setString("s3", "ss", "k6", "childValue4");
assertThat(state.getConfig(configName).getWithInheritance(true).toText())
.isEqualTo(expectedCfg.toText());
assertThat(state.getConfig(configName).get().toText()).isEqualTo(cfg.toText());
}
}