opendev/gerrit
Code Issues Proposed changes

GroupList: Do not store null UUIDs

Browse Source 
A GroupList is a parsed representation of the groups file, which
cannot contain any null entries. Caching null entries in this class is
not appropriate.

In addition to this theoretical objection, it is definitely incorrect
to return a cached GroupReference with null UUID from #resolve.
ProjectConfig will create a distinct GroupReference with a different
name for each group that is missing in the groups file, and it's not
correct to return a GroupReference from resolve with a different name
from the one that was passed in, even if they both have null UUID.

This commit additionally reverts 26fe49b, since the new implementation
of resolve would just return the input anyway, so the call would be a
no-op.

Change-Id: I27e4ed5443122557ab3a719b8bdb052a5e373f58
This commit is contained in:
Dave Borowitz
2016-10-19 17:10:07 -04:00
parent ec2ef4f0c4
commit 79b30d7ade
2 changed files with 8 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
gerrit-server/src/main/java/com/google/gerrit/server/git/GroupList.java
View File

@@ -34,12 +34,9 @@ public class GroupList extends TabFile {
public static final String FILE_NAME = "groups";
private final Project.NameKey project;
private final Map<AccountGroup.UUID, GroupReference> byUUID;
private GroupList(Project.NameKey project,
Map<AccountGroup.UUID, GroupReference> byUUID) {
this.project = project;
private GroupList(Map<AccountGroup.UUID, GroupReference> byUUID) {
this.byUUID = byUUID;
}
@@ -60,7 +57,7 @@ public class GroupList extends TabFile {
groupsByUUID.put(uuid, ref);
}
return new GroupList(project, groupsByUUID);
return new GroupList(groupsByUUID);
}
public GroupReference byUUID(AccountGroup.UUID uuid) {
@@ -70,7 +67,10 @@ public class GroupList extends TabFile {
public GroupReference resolve(GroupReference group) {
if (group != null) {
if (group.getUUID() == null || group.getUUID().get() == null) {
log.warn("attempting to resolve null group in {}: {}", project, group);
// A GroupReference from ProjectConfig that refers to a group not found
// in this file will have a null UUID. Since there may be multiple
// different missing references, it's not appropriate to cache the
// results, nor return null the set from #uuids.
return group;
}
GroupReference ref = byUUID.get(group.getUUID());
@@ -92,8 +92,7 @@ public class GroupList extends TabFile {
public void put(AccountGroup.UUID uuid, GroupReference reference) {
if (uuid == null || uuid.get() == null) {
log.warn("attempting to put null group in {}: {}", project, reference);
return;
return; // See note in #resolve above.
}
byUUID.put(uuid, reference);
}

2
gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
View File

@@ -713,7 +713,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
// no valid UUID for it. Pool the reference anyway so at least
// all rules in the same file share the same GroupReference.
//
ref = resolve(rule.getGroup());
ref = rule.getGroup();
groupsByName.put(ref.getName(), ref);
error(new ValidationError(PROJECT_CONFIG,
"group \"" + ref.getName() + "\" not in " + GroupList.FILE_NAME));