GroupList: Do not store null UUIDs
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.
This reverts commit 26fe49b72f6075fdc36dbdcfbbc5c878cbcd8c14.
Change-Id: I27e4ed5443122557ab3a719b8bdb052a5e373f58
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/GroupList.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/GroupList.java
index f4a03cf..bd76ad4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/GroupList.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/GroupList.java
@@ -16,7 +16,6 @@
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import java.io.IOException;
import java.util.ArrayList;
@@ -28,10 +27,11 @@
public class GroupList extends TabFile {
public static final String FILE_NAME = "groups";
+
private final Map<AccountGroup.UUID, GroupReference> byUUID;
private GroupList(Map<AccountGroup.UUID, GroupReference> byUUID) {
- this.byUUID = byUUID;
+ this.byUUID = byUUID;
}
public static GroupList parse(String text, ValidationError.Sink errors)
@@ -56,6 +56,13 @@
public GroupReference resolve(GroupReference group) {
if (group != null) {
+ if (group.getUUID() == null || group.getUUID().get() == null) {
+ // 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());
if (ref != null) {
return ref;
@@ -73,7 +80,10 @@
return byUUID.keySet();
}
- public void put(UUID uuid, GroupReference reference) {
+ public void put(AccountGroup.UUID uuid, GroupReference reference) {
+ if (uuid == null || uuid.get() == null) {
+ return; // See note in #resolve above.
+ }
byUUID.put(uuid, reference);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
index 9b1c3f7..3b1fa09 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
@@ -707,7 +707,7 @@
// 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));
