Merge branch 'stable-3.7' into stable-3.8
* stable-3.7:
Parse author identities when exposing comments
Always lookup imported users when parsing identities
Use custom gerrit class loader to avoid usage of reflection
ChangeTriplet.parse(ChangeTriplet.format(change)) should be true
ChangeTriplet should not decode ChangeId component
Bump Bazel version to 6.2.0
Release-Notes: skip
Forward-Compatible: checked
Change-Id: Ifbea7fcb3f1e6dd3dbc9b47e97e36ce064c517b6
diff --git a/.bazelversion b/.bazelversion
index 5e32542..6abaeb2 100644
--- a/.bazelversion
+++ b/.bazelversion
@@ -1 +1 @@
-6.1.2
+6.2.0
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index bf51252..4bb4aad 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -6920,7 +6920,7 @@
|Field Name ||Description
|`id` ||
The ID of the change in the format "'<project>\~<branch>~<Change-Id>'",
-where 'project', 'branch' and 'Change-Id' are URL encoded. For 'branch' the
+where 'project' and 'branch' are URL encoded. For 'branch' the
`refs/heads/` prefix is omitted.
|`project` ||The name of the project.
|`branch` ||
diff --git a/WORKSPACE b/WORKSPACE
index 29266cd..047da6a 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -40,53 +40,6 @@
],
)
-# TODO: Remove this when java_tools v12.1 included in regular Bazel release
-# See https://github.com/bazelbuild/bazel/issues/17695
-http_archive(
- name = "remote_java_tools",
- sha256 = "0db35ec44745fd15b77d9df954e70a4fcf74554dd5bfe3f6e6cb6bbdc1f1c649",
- urls = [
- "https://mirror.bazel.build/bazel_java_tools/releases/java/v12.1/java_tools-v12.1.zip",
- "https://github.com/bazelbuild/java_tools/releases/download/java_v12.1/java_tools-v12.1.zip",
- ],
-)
-
-http_archive(
- name = "remote_java_tools_linux",
- sha256 = "093ecac3b42fcbc3621d08edc3ae3c8b0bc2bf56a0d9a85ddcdb1e0bcf10cbc7",
- urls = [
- "https://mirror.bazel.build/bazel_java_tools/releases/java/v12.1/java_tools_linux-v12.1.zip",
- "https://github.com/bazelbuild/java_tools/releases/download/java_v12.1/java_tools_linux-v12.1.zip",
- ],
-)
-
-http_archive(
- name = "remote_java_tools_windows",
- sha256 = "1df7cc7fac54f437f43c24c019462e13058f394fdba5a64f566b92e8af18d0cf",
- urls = [
- "https://mirror.bazel.build/bazel_java_tools/releases/java/v12.1/java_tools_windows-v12.1.zip",
- "https://github.com/bazelbuild/java_tools/releases/download/java_v12.1/java_tools_windows-v12.1.zip",
- ],
-)
-
-http_archive(
- name = "remote_java_tools_darwin_x86_64",
- sha256 = "16ca145203a62a1fcd6ae50513c0935d938591cb309b9b1172e257c57873f60d",
- urls = [
- "https://mirror.bazel.build/bazel_java_tools/releases/java/v12.1/java_tools_darwin_x86_64-v12.1.zip",
- "https://github.com/bazelbuild/java_tools/releases/download/java_v12.1/java_tools_darwin_x86_64-v12.1.zip",
- ],
-)
-
-http_archive(
- name = "remote_java_tools_darwin_arm64",
- sha256 = "1d8e575e558782c2ceec0940e424f0e2df56b0df3d7fae68333eaceef2c4e41c",
- urls = [
- "https://mirror.bazel.build/bazel_java_tools/releases/java/v12.1/java_tools_darwin_arm64-v12.1.zip",
- "https://github.com/bazelbuild/java_tools/releases/download/java_v12.1/java_tools_darwin_arm64-v12.1.zip",
- ],
-)
-
http_archive(
name = "rbe_jdk11",
sha256 = "dbcfd6f26589ef506b91fe03a12dc559ca9c84699e4cf6381150522287f0e6f6",
diff --git a/java/com/google/gerrit/common/BUILD b/java/com/google/gerrit/common/BUILD
index 1099919..8f930bb 100644
--- a/java/com/google/gerrit/common/BUILD
+++ b/java/com/google/gerrit/common/BUILD
@@ -23,6 +23,7 @@
":annotations",
"//java/com/google/gerrit/entities",
"//java/com/google/gerrit/extensions:api",
+ "//java/com/google/gerrit/launcher",
"//java/com/google/gerrit/prettify:server",
"//lib:guava",
"//lib:jgit",
diff --git a/java/com/google/gerrit/common/IoUtil.java b/java/com/google/gerrit/common/IoUtil.java
index 09a8993..e691025 100644
--- a/java/com/google/gerrit/common/IoUtil.java
+++ b/java/com/google/gerrit/common/IoUtil.java
@@ -14,15 +14,14 @@
package com.google.gerrit.common;
+import static com.google.gerrit.launcher.GerritLauncher.GerritClassLoader;
+
import com.google.common.collect.Sets;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
import java.net.MalformedURLException;
import java.net.URL;
-import java.net.URLClassLoader;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Collection;
@@ -70,32 +69,22 @@
}
ClassLoader cl = IoUtil.class.getClassLoader();
- if (!(cl instanceof URLClassLoader)) {
- throw noAddURL("Not loaded by URLClassLoader", null);
+ if (!(cl instanceof GerritClassLoader)) {
+ throw noAddURL("Not loaded by GerritClassLoader", null);
}
@SuppressWarnings("resource") // Leave open so classes can be loaded.
- URLClassLoader urlClassLoader = (URLClassLoader) cl;
+ GerritClassLoader gerritClassLoader = (GerritClassLoader) cl;
- Method addURL;
- try {
- addURL = URLClassLoader.class.getDeclaredMethod("addURL", URL.class);
- addURL.setAccessible(true);
- } catch (SecurityException | NoSuchMethodException e) {
- throw noAddURL("Method addURL not available", e);
- }
-
- Set<URL> have = Sets.newHashSet(Arrays.asList(urlClassLoader.getURLs()));
+ Set<URL> have = Sets.newHashSet(Arrays.asList(gerritClassLoader.getURLs()));
for (Path path : jars) {
try {
URL url = path.toUri().toURL();
if (have.add(url)) {
- addURL.invoke(cl, url);
+ gerritClassLoader.addURL(url);
}
- } catch (MalformedURLException | IllegalArgumentException | IllegalAccessException e) {
+ } catch (MalformedURLException | IllegalArgumentException e) {
throw noAddURL("addURL " + path + " failed", e);
- } catch (InvocationTargetException e) {
- throw noAddURL("addURL " + path + " failed", e.getCause());
}
}
}
diff --git a/java/com/google/gerrit/launcher/GerritLauncher.java b/java/com/google/gerrit/launcher/GerritLauncher.java
index 8783593..07a071a 100644
--- a/java/com/google/gerrit/launcher/GerritLauncher.java
+++ b/java/com/google/gerrit/launcher/GerritLauncher.java
@@ -60,6 +60,33 @@
private static final String PKG = "com.google.gerrit.pgm";
public static final String NOT_ARCHIVED = "NOT_ARCHIVED";
+ // Classloader that allows to add additional jars to the classpath.
+ public static class GerritClassLoader extends URLClassLoader {
+ static {
+ ClassLoader.registerAsParallelCapable();
+ }
+
+ /**
+ * Constructs a new URLClassLoader for the given URLs.
+ *
+ * @param urls the URLs from which to load classes and resources
+ * @param parent the parent class loader for delegation
+ */
+ GerritClassLoader(URL[] urls, ClassLoader parent) {
+ super(urls, parent);
+ }
+
+ /**
+ * Appends the additional URL to the list of URLs to search for classes and resources.
+ *
+ * @param url the URL to be added to the search path of URLs
+ */
+ @Override
+ public void addURL(URL url) {
+ super.addURL(url);
+ }
+ }
+
private static ClassLoader daemonClassLoader;
public static void main(String[] argv) throws Exception {
@@ -308,7 +335,7 @@
if (!extapi.isEmpty()) {
parent = URLClassLoader.newInstance(extapi.toArray(new URL[extapi.size()]), parent);
}
- return URLClassLoader.newInstance(jars.values().toArray(new URL[jars.size()]), parent);
+ return new GerritClassLoader(jars.values().toArray(new URL[jars.size()]), parent);
}
private static void extractJar(ZipFile zf, ZipEntry ze, NavigableMap<String, URL> jars)
diff --git a/java/com/google/gerrit/server/change/ChangeTriplet.java b/java/com/google/gerrit/server/change/ChangeTriplet.java
index 1074302..d826735 100644
--- a/java/com/google/gerrit/server/change/ChangeTriplet.java
+++ b/java/com/google/gerrit/server/change/ChangeTriplet.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.change;
import com.google.auto.value.AutoValue;
+import com.google.common.base.MoreObjects;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
@@ -28,7 +29,11 @@
}
private static String format(BranchNameKey branch, Change.Key change) {
- return branch.project().get() + "~" + branch.shortName() + "~" + change.get();
+ return Url.encode(branch.project().get())
+ + "~"
+ + Url.encode(branch.shortName())
+ + "~"
+ + change.get();
}
/**
@@ -50,7 +55,7 @@
String project = Url.decode(triplet.substring(0, y));
String branch = Url.decode(triplet.substring(y + 1, z));
- String changeId = Url.decode(triplet.substring(z + 1));
+ String changeId = triplet.substring(z + 1);
return Optional.of(
new AutoValue_ChangeTriplet(
BranchNameKey.create(Project.nameKey(project), branch), Change.key(changeId)));
@@ -66,6 +71,10 @@
@Override
public final String toString() {
- return format(branch(), id());
+ return MoreObjects.toStringHelper(this)
+ .add("project", branch().project())
+ .add("branch", branch().shortName())
+ .add("id", id())
+ .toString();
}
}
diff --git a/java/com/google/gerrit/server/group/db/AuditLogReader.java b/java/com/google/gerrit/server/group/db/AuditLogReader.java
index 4c1f69b..8da2f50 100644
--- a/java/com/google/gerrit/server/group/db/AuditLogReader.java
+++ b/java/com/google/gerrit/server/group/db/AuditLogReader.java
@@ -51,10 +51,12 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final AllUsersName allUsersName;
+ private final NoteDbUtil noteDbUtil;
@Inject
- public AuditLogReader(AllUsersName allUsersName) {
+ public AuditLogReader(AllUsersName allUsersName, NoteDbUtil noteDbUtil) {
this.allUsersName = allUsersName;
+ this.noteDbUtil = noteDbUtil;
}
// Having separate methods for reading the two types of audit records mirrors the split in
@@ -140,7 +142,7 @@
}
private Optional<ParsedCommit> parse(AccountGroup.UUID uuid, RevCommit c) {
- Optional<Account.Id> authorId = NoteDbUtil.parseIdent(c.getAuthorIdent());
+ Optional<Account.Id> authorId = noteDbUtil.parseIdent(c.getAuthorIdent());
if (!authorId.isPresent()) {
// Only report audit events from identified users, since this was a non-nullable field in
// ReviewDb. May be revisited.
@@ -176,7 +178,7 @@
private Optional<Account.Id> parseAccount(AccountGroup.UUID uuid, RevCommit c, FooterLine line) {
Optional<Account.Id> result =
Optional.ofNullable(RawParseUtils.parsePersonIdent(line.getValue()))
- .flatMap(ident -> NoteDbUtil.parseIdent(ident));
+ .flatMap(noteDbUtil::parseIdent);
if (!result.isPresent()) {
logInvalid(uuid, c, line);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java b/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
index 8f6ad67..881cd96 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNoteUtil.java
@@ -200,14 +200,14 @@
/** The returned {@link Optional} holds the parsed entity or is empty if parsing failed. */
static Optional<AttentionSetUpdate> attentionStatusFromJson(
- Instant timestamp, String attentionString) {
+ Instant timestamp, String attentionString, NoteDbUtil noteDbUtil) {
AttentionStatusInNoteDb inNoteDb =
gson.fromJson(attentionString, AttentionStatusInNoteDb.class);
PersonIdent personIdent = RawParseUtils.parsePersonIdent(inNoteDb.personIdent);
if (personIdent == null) {
return Optional.empty();
}
- Optional<Account.Id> account = NoteDbUtil.parseIdent(personIdent);
+ Optional<Account.Id> account = noteDbUtil.parseIdent(personIdent);
return account.map(
id ->
AttentionSetUpdate.createFromRead(timestamp, id, inNoteDb.operation, inNoteDb.reason));
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index 2d3902c..b98ecdd 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -370,8 +370,7 @@
walkSupplier.get(),
args.changeNoteJson,
args.metrics,
- args.serverId,
- externalIdCache);
+ new NoteDbUtil(args.serverId, externalIdCache));
ChangeNotesState result = parser.parseAll();
// This assignment only happens if call() was actually called, which only
// happens when Cache#get(K, Callable<V>) incurs a cache miss.
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 951a478..b6cdfac 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -64,6 +64,7 @@
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeMessage;
+import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LabelType;
@@ -77,7 +78,6 @@
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
-import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.notedb.ChangeNotesParseApprovalUtil.ParsedPatchSetApproval;
import com.google.gerrit.server.util.LabelVote;
@@ -198,8 +198,7 @@
// the latest record unsets the field).
private Optional<PatchSet.Id> cherryPickOf;
private Instant mergedOn;
- private final ExternalIdCache externalIdCache;
- private final String gerritServerId;
+ private final NoteDbUtil noteDbUtil;
ChangeNotesParser(
Change.Id changeId,
@@ -207,15 +206,13 @@
ChangeNotesRevWalk walk,
ChangeNoteJson changeNoteJson,
NoteDbMetrics metrics,
- String gerritServerId,
- ExternalIdCache externalIdCache) {
+ NoteDbUtil noteDbUtil) {
this.id = changeId;
this.tip = tip;
this.walk = walk;
this.changeNoteJson = changeNoteJson;
this.metrics = metrics;
- this.externalIdCache = externalIdCache;
- this.gerritServerId = gerritServerId;
+ this.noteDbUtil = noteDbUtil;
approvals = new LinkedHashMap<>();
bufferedApprovals = new ArrayList<>();
reviewers = HashBasedTable.create();
@@ -728,7 +725,7 @@
Optional<AttentionSetUpdate> attentionStatus =
ChangeNoteUtil.attentionStatusFromJson(
- Instant.ofEpochSecond(commit.getCommitTime()), attentionString);
+ Instant.ofEpochSecond(commit.getCommitTime()), attentionString, noteDbUtil);
if (!attentionStatus.isPresent()) {
throw invalidFooter(FOOTER_ATTENTION, attentionString);
}
@@ -880,6 +877,11 @@
for (Map.Entry<ObjectId, ChangeRevisionNote> e : rns.entrySet()) {
for (HumanComment c : e.getValue().getEntities()) {
+
+ noteDbUtil
+ .parseIdent(String.format("%s@%s", c.author.getId(), c.serverId))
+ .ifPresent(id -> c.author = new Comment.Identity(id));
+
humanComments.put(e.getKey(), c);
}
}
@@ -1404,7 +1406,8 @@
}
private Account.Id parseIdent(PersonIdent ident) throws ConfigInvalidException {
- return NoteDbUtil.parseIdent(ident, gerritServerId, externalIdCache)
+ return noteDbUtil
+ .parseIdent(ident)
.orElseThrow(
() -> parseException("cannot retrieve account id: %s", ident.getEmailAddress()));
}
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index f4262e7..270fc32 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -240,13 +240,16 @@
private final ChangeNotes.Factory changeNotesFactory;
private final AccountCache accountCache;
+ private final NoteDbUtil noteDbUtil;
private final DiffAlgorithm diffAlgorithm = new HistogramDiff();
private static final Gson gson = OutputFormat.JSON_COMPACT.newGson();
@Inject
- CommitRewriter(ChangeNotes.Factory changeNotesFactory, AccountCache accountCache) {
+ CommitRewriter(
+ ChangeNotes.Factory changeNotesFactory, AccountCache accountCache, NoteDbUtil noteDbUtil) {
this.changeNotesFactory = changeNotesFactory;
this.accountCache = accountCache;
+ this.noteDbUtil = noteDbUtil;
}
/**
@@ -1082,7 +1085,7 @@
}
private Optional<Account.Id> parseIdent(ChangeFixProgress changeFixProgress, PersonIdent ident) {
- Optional<Account.Id> account = NoteDbUtil.parseIdent(ident);
+ Optional<Account.Id> account = noteDbUtil.parseIdent(ident);
if (account.isPresent()) {
changeFixProgress.parsedAccounts.putIfAbsent(account.get(), Optional.empty());
} else {
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUtil.java b/java/com/google/gerrit/server/notedb/NoteDbUtil.java
index 2ad89b2..5fc9244 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUtil.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUtil.java
@@ -23,16 +23,28 @@
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdCache;
+import com.google.gerrit.server.config.GerritServerId;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.Optional;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.util.GitDateFormatter;
import org.eclipse.jgit.util.GitDateFormatter.Format;
+@Singleton
public class NoteDbUtil {
+ private final String serverId;
+ private final ExternalIdCache externalIdCache;
+
+ @Inject
+ public NoteDbUtil(@GerritServerId String serverId, ExternalIdCache externalIdCache) {
+ this.serverId = serverId;
+ this.externalIdCache = externalIdCache;
+ }
+
private static final CharMatcher INVALID_FOOTER_CHARS = CharMatcher.anyOf("\r\n\0");
private static final ImmutableList<String> PACKAGE_PREFIXES =
@@ -40,35 +52,29 @@
private static final ImmutableSet<String> SERVLET_NAMES =
ImmutableSet.of("com.google.gerrit.httpd.restapi.RestApiServlet");
- /** Returns an AccountId for the given email address. */
- public static Optional<Account.Id> parseIdent(PersonIdent ident) {
- String email = ident.getEmailAddress();
- int at = email.indexOf('@');
- if (at >= 0) {
- Integer id = Ints.tryParse(email.substring(0, at));
- if (id != null) {
- return Optional.of(Account.id(id));
- }
- }
- return Optional.empty();
+ /**
+ * Returns an AccountId for the given person's identity and the current serverId. Reverse lookup
+ * the AccountId using the ExternalIdCache if the account has a foreign serverId.
+ *
+ * @param ident the accountId@serverId identity
+ * @return a defined accountId if the account was found, {@link Account#UNKNOWN_ACCOUNT_ID} if the
+ * lookup via external-id did not return any account, or an empty value if the identity was
+ * malformed.
+ */
+ public Optional<Account.Id> parseIdent(PersonIdent ident) {
+ return parseIdent(ident.getEmailAddress());
}
/**
* Returns an AccountId for the given email address and the current serverId. Reverse lookup the
* AccountId using the ExternalIdCache if the account has a foreign serverId.
*
- * @param ident the accountId@serverId identity
- * @param serverId the Gerrit's serverId
- * @param externalIdCache reference to the cache for looking up the external ids
+ * @param email the accountId@serverId email identity
* @return a defined accountId if the account was found, {@link Account#UNKNOWN_ACCOUNT_ID} if the
* lookup via external-id did not return any account, or an empty value if the identity was
* malformed.
- * @throws ConfigInvalidException when the lookup of the external-id failed
*/
- public static Optional<Account.Id> parseIdent(
- PersonIdent ident, String serverId, ExternalIdCache externalIdCache)
- throws ConfigInvalidException {
- String email = ident.getEmailAddress();
+ public Optional<Account.Id> parseIdent(String email) {
int at = email.indexOf('@');
if (at >= 0) {
Integer id = Ints.tryParse(email.substring(0, at));
@@ -85,7 +91,7 @@
.map(ExternalId::accountId)
.or(() -> Optional.of(Account.UNKNOWN_ACCOUNT_ID));
} catch (IOException e) {
- throw new ConfigInvalidException("Unable to lookup external id from cache", e);
+ throw new IllegalArgumentException("Unable to lookup external id from cache", e);
}
}
}
@@ -175,6 +181,4 @@
}
return impl;
}
-
- private NoteDbUtil() {}
}
diff --git a/javatests/com/google/gerrit/server/change/ChangeTripletTest.java b/javatests/com/google/gerrit/server/change/ChangeTripletTest.java
new file mode 100644
index 0000000..e012bd3
--- /dev/null
+++ b/javatests/com/google/gerrit/server/change/ChangeTripletTest.java
@@ -0,0 +1,53 @@
+// Copyright (C) 2018 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.testing.TestChanges;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ChangeTripletTest {
+ private Account.Id userId;
+
+ @Before
+ public void setUp() {
+ userId = Account.id(1);
+ }
+
+ @Test
+ public void testReversibleFormatAndParseWithSpecialChars() throws Exception {
+ Project.NameKey project = Project.nameKey("Repository with special ~!@#$%^&*()-=/.\"'`\\ שלום");
+ Change c = TestChanges.newChange(project, userId);
+ String formatted = ChangeTriplet.format(c);
+ assertThat(formatted)
+ .isEqualTo(
+ String.format(
+ "%s~%s~%s",
+ Url.encode(c.getDest().project().get()),
+ Url.encode(c.getDest().shortName()),
+ c.getKey()));
+
+ ChangeTriplet t = ChangeTriplet.parse(formatted).get();
+ assertThat(t.project()).isEqualTo(project);
+ assertThat(t.branch()).isEqualTo(c.getDest());
+ assertThat(t.id()).isEqualTo(c.getKey());
+ }
+}
diff --git a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java
index 2d90ab4..65eb5b0 100644
--- a/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java
+++ b/javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java
@@ -46,7 +46,6 @@
public class AbstractGroupTest {
protected static final ZoneId ZONE_ID = ZoneId.of("America/Los_Angeles");
protected static final String SERVER_ID = "server-id";
- protected static final String SERVER_NAME = "Gerrit Server";
protected static final String SERVER_EMAIL = "noreply@gerritcodereview.com";
protected static final int SERVER_ACCOUNT_NUMBER = 100000;
protected static final int USER_ACCOUNT_NUMBER = 100001;
@@ -65,7 +64,7 @@
repoManager = new InMemoryRepositoryManager();
allUsersRepo = repoManager.createRepository(allUsersName);
serverAccountId = Account.id(SERVER_ACCOUNT_NUMBER);
- serverIdent = new PersonIdent(SERVER_NAME, SERVER_EMAIL, TimeUtil.now(), ZONE_ID);
+ serverIdent = new PersonIdent(SERVER_ID, SERVER_EMAIL, TimeUtil.now(), ZONE_ID);
userId = Account.id(USER_ACCOUNT_NUMBER);
userIdent = newPersonIdent(userId, serverIdent);
}
@@ -86,7 +85,7 @@
}
protected static void assertServerCommit(CommitInfo commitInfo, String expectedMessage) {
- assertCommit(commitInfo, expectedMessage, SERVER_NAME, SERVER_EMAIL);
+ assertCommit(commitInfo, expectedMessage, SERVER_ID, SERVER_EMAIL);
}
protected static void assertCommit(
@@ -96,7 +95,7 @@
assertThat(commitInfo).author().email().isEqualTo(expectedEmail);
// Committer should always be the server, regardless of author.
- assertThat(commitInfo).committer().name().isEqualTo(SERVER_NAME);
+ assertThat(commitInfo).committer().name().isEqualTo(SERVER_ID);
assertThat(commitInfo).committer().email().isEqualTo(SERVER_EMAIL);
assertThat(commitInfo).committer().date().isEqualTo(commitInfo.author.date);
assertThat(commitInfo).committer().tz().isEqualTo(commitInfo.author.tz);
@@ -111,7 +110,7 @@
}
protected static PersonIdent newPersonIdent() {
- return new PersonIdent(SERVER_NAME, SERVER_EMAIL, TimeUtil.now(), ZONE_ID);
+ return new PersonIdent(SERVER_ID, SERVER_EMAIL, TimeUtil.now(), ZONE_ID);
}
protected static PersonIdent newPersonIdent(Account.Id id, PersonIdent ident) {
diff --git a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
index b2a6790..6d90309 100644
--- a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
+++ b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
@@ -25,6 +25,8 @@
import com.google.gerrit.entities.AccountGroupMemberAudit;
import com.google.gerrit.entities.InternalGroup;
import com.google.gerrit.server.account.GroupUuid;
+import com.google.gerrit.server.account.externalids.DisabledExternalIdCache;
+import com.google.gerrit.server.notedb.NoteDbUtil;
import java.time.Instant;
import java.util.Set;
import org.eclipse.jgit.lib.PersonIdent;
@@ -38,7 +40,8 @@
@Before
public void setUp() throws Exception {
- auditLogReader = new AuditLogReader(allUsersName);
+ auditLogReader =
+ new AuditLogReader(allUsersName, new NoteDbUtil(SERVER_ID, new DisabledExternalIdCache()));
}
@Test
diff --git a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
index a7ec4c6..20e441b 100644
--- a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
@@ -95,6 +95,8 @@
@RunWith(ConfigSuite.class)
public abstract class AbstractChangeNotesTest {
protected static final String LOCAL_SERVER_ID = "gerrit";
+ protected static final String FQ_USER_IDENT =
+ "Gerrit User 1000000 \\u003c1000000@" + LOCAL_SERVER_ID;
private static final ZoneId ZONE_ID = ZoneId.of("America/Los_Angeles");
@@ -328,7 +330,8 @@
String message,
short side,
ObjectId commitId,
- boolean unresolved) {
+ boolean unresolved,
+ String serverId) {
HumanComment c =
new HumanComment(
new Comment.Key(UUID, filename, psId.get()),
@@ -345,6 +348,35 @@
return c;
}
+ protected HumanComment newComment(
+ PatchSet.Id psId,
+ String filename,
+ String UUID,
+ CommentRange range,
+ int line,
+ IdentifiedUser commenter,
+ String parentUUID,
+ Instant t,
+ String message,
+ short side,
+ ObjectId commitId,
+ boolean unresolved) {
+ return newComment(
+ psId,
+ filename,
+ UUID,
+ range,
+ line,
+ commenter,
+ parentUUID,
+ t,
+ message,
+ side,
+ commitId,
+ unresolved,
+ serverId);
+ }
+
protected static Instant truncate(Instant ts) {
return Instant.ofEpochMilli((ts.toEpochMilli() / 1000) * 1000);
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
index 9445f4a..84d2d5d 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
@@ -52,7 +52,9 @@
"Update patch set 1\n"
+ "\n"
+ "Patch-set: 1\n"
- + "Attention: {\"person_ident\":\"Gerrit User 1000000 \\u003c1000000@adce0b11-8f2e-4ab6-ac69-e675f183d871\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added by Administrator using the hovercard menu\"}");
+ + "Attention: {\"person_ident\":\""
+ + FQ_USER_IDENT
+ + "\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added by Administrator using the hovercard menu\"}");
newParser(commit).parseAll();
assertThat(((ChangeNotesCommit) commit).isAttentionSetCommitOnly(false)).isEqualTo(true);
@@ -67,7 +69,9 @@
+ "\n"
+ "Patch-set: 1\n"
+ "Subject: Change subject\n"
- + "Attention: {\"person_ident\":\"Gerrit User 1000000 \\u003c1000000@adce0b11-8f2e-4ab6-ac69-e675f183d871\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added by Administrator using the hovercard menu\"}");
+ + "Attention: {\"person_ident\":\""
+ + FQ_USER_IDENT
+ + "\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added by Administrator using the hovercard menu\"}");
newParser(commit).parseAll();
assertThat(((ChangeNotesCommit) commit).isAttentionSetCommitOnly(false)).isEqualTo(false);
@@ -89,7 +93,9 @@
"Update patch set 1\n"
+ "\n"
+ "Patch-set: 1\n"
- + "Attention: {\"person_ident\":\"Gerrit User 1000000 \\u003c1000000@adce0b11-8f2e-4ab6-ac69-e675f183d871\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added by Administrator using the hovercard menu\"}");
+ + "Attention: {\"person_ident\":\""
+ + FQ_USER_IDENT
+ + "\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added by Administrator using the hovercard menu\"}");
newParser(commit).parseAll();
assertThat(((ChangeNotesCommit) commit).isAttentionSetCommitOnly(true)).isEqualTo(false);
@@ -99,7 +105,12 @@
walk.reset();
ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class);
return new ChangeNotesParser(
- newChange().getId(), tip, walk, changeNoteJson, args.metrics, serverId, externalIdCache);
+ newChange().getId(),
+ tip,
+ walk,
+ changeNoteJson,
+ args.metrics,
+ new NoteDbUtil(serverId, externalIdCache));
}
private RevCommit writeCommit(String body) throws Exception {
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
index 8d7066f..5aea94e 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
@@ -648,8 +648,9 @@
"Update patch set 1\n"
+ "\n"
+ "Patch-set: 1\n"
- + "Attention: {\"person_ident\":\"Gerrit User 1000000"
- + " \\u003c1000000@adce0b11-8f2e-4ab6-ac69-e675f183d871\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added"
+ + "Attention: {\"person_ident\":\""
+ + FQ_USER_IDENT
+ + "\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added"
+ " by Administrator using the hovercard menu\"}",
false);
ChangeNotesParser changeNotesParser = newParser(commit);
@@ -669,8 +670,9 @@
+ "\n"
+ "Patch-set: 1\n"
+ "Subject: Change subject\n"
- + "Attention: {\"person_ident\":\"Gerrit User 1000000"
- + " \\u003c1000000@adce0b11-8f2e-4ab6-ac69-e675f183d871\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added"
+ + "Attention: {\"person_ident\":\""
+ + FQ_USER_IDENT
+ + "\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added"
+ " by Administrator using the hovercard menu\"}",
false);
ChangeNotesParser changeNotesParser = newParser(commit);
@@ -701,8 +703,9 @@
"Update patch set 1\n"
+ "\n"
+ "Patch-set: 1\n"
- + "Attention: {\"person_ident\":\"Gerrit User 1000000"
- + " \\u003c1000000@adce0b11-8f2e-4ab6-ac69-e675f183d871\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added"
+ + "Attention: {\"person_ident\":\""
+ + FQ_USER_IDENT
+ + "\\u003e\",\"operation\":\"ADD\",\"reason\":\"Added"
+ " by Administrator using the hovercard menu\"}",
false);
ChangeNotesParser changeNotesParser = newParser(commit);
@@ -792,6 +795,11 @@
walk.reset();
ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class);
return new ChangeNotesParser(
- newChange().getId(), tip, walk, changeNoteJson, args.metrics, serverId, externalIdCache);
+ newChange().getId(),
+ tip,
+ walk,
+ changeNoteJson,
+ args.metrics,
+ new NoteDbUtil(serverId, externalIdCache));
}
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 15eefcd..50ff860 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -2006,8 +2006,7 @@
rw,
changeNoteJson,
args.metrics,
- serverId,
- externalIdCache);
+ new NoteDbUtil(serverId, externalIdCache));
ChangeNotesState state = notesWithComments.parseAll();
assertThat(state.approvals()).isEmpty();
assertThat(state.publishedComments()).hasSize(1);
@@ -2021,8 +2020,7 @@
rw,
changeNoteJson,
args.metrics,
- serverId,
- externalIdCache);
+ new NoteDbUtil(serverId, externalIdCache));
ChangeNotesState state = notesWithApprovals.parseAll();
assertThat(state.approvals()).hasSize(1);
diff --git a/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java
index bb49a6d..57be12c 100644
--- a/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java
@@ -21,14 +21,22 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.CommentRange;
+import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.git.RepositoryCaseMismatchException;
+import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.AbstractModule;
+import java.io.IOException;
+import java.util.Objects;
import java.util.Optional;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.ObjectId;
import org.junit.Before;
import org.junit.Test;
@@ -66,16 +74,7 @@
public void allowChangeFromImportedServerId() throws Exception {
initServerIds(LOCAL_SERVER_ID, IMPORTED_SERVER_ID);
- ExternalId.Key importedAccountIdKey =
- ExternalId.Key.create(
- ExternalId.SCHEME_IMPORTED,
- changeOwner.getAccountId() + "@" + IMPORTED_SERVER_ID,
- false);
- ExternalId importedAccountId =
- ExternalId.create(importedAccountIdKey, changeOwner.getAccountId(), null, null, null);
-
- when(externalIdCacheMock.byKey(eq(importedAccountIdKey)))
- .thenReturn(Optional.of(importedAccountId));
+ linkImportedAccountTo(changeOwner.getAccountId(), Optional.of(changeOwner.getAccountId()));
Change importedChange = newChange(createTestInjector(IMPORTED_SERVER_ID), false);
Change localChange = newChange();
@@ -87,7 +86,8 @@
@Test
public void rejectChangeWithForeignServerId() throws Exception {
initServerIds(LOCAL_SERVER_ID);
- when(externalIdCacheMock.byKey(any())).thenReturn(Optional.empty());
+
+ linkImportedAccountTo(changeOwner.getAccountId(), Optional.empty());
Change foreignChange = newChange(createTestInjector(FOREIGN_SERVER_ID), false);
@@ -111,4 +111,85 @@
assertThat(newNotes(importedChange).getChange().getOwner())
.isEqualTo(Account.UNKNOWN_ACCOUNT_ID);
}
+
+ @Test
+ public void commentChangeFromImportedServerIdWithAnExistingAccountId() throws Exception {
+ initServerIds(LOCAL_SERVER_ID, IMPORTED_SERVER_ID);
+
+ Account.Id importedAccountId = Account.id(12345);
+ linkImportedAccountTo(importedAccountId, Optional.of(otherUser.getAccountId()));
+ IdentifiedUser importedUserIdentity = userFactory.create(importedAccountId);
+
+ Change importedChange = newChange(createTestInjector(IMPORTED_SERVER_ID), false);
+ ChangeUpdate update = newUpdate(importedChange, importedUserIdentity);
+ update.putComment(
+ HumanComment.Status.PUBLISHED,
+ comment(importedChange, importedUserIdentity, IMPORTED_SERVER_ID));
+ update.commit();
+
+ ChangeNotes notes = newNotes(importedChange);
+
+ ImmutableListMultimap<ObjectId, HumanComment> comments = notes.getHumanComments();
+ assertThat(comments).hasSize(1);
+ HumanComment gotComment = comments.entries().asList().get(0).getValue();
+ assertThat(gotComment.author.getId()).isEqualTo(otherUser.getAccountId());
+ }
+
+ @Test
+ public void commentChangeFromImportedServerIdWithUnknownAccountId() throws Exception {
+ initServerIds(LOCAL_SERVER_ID, IMPORTED_SERVER_ID);
+
+ Account.Id importedAccountId = Account.id(12345);
+ linkImportedAccountTo(importedAccountId, Optional.empty());
+ IdentifiedUser importedUserIdentity = userFactory.create(importedAccountId);
+
+ Change importedChange = newChange(createTestInjector(IMPORTED_SERVER_ID), false);
+ ChangeUpdate update = newUpdate(importedChange, importedUserIdentity);
+ update.putComment(
+ HumanComment.Status.PUBLISHED,
+ comment(importedChange, importedUserIdentity, IMPORTED_SERVER_ID));
+ update.commit();
+
+ ChangeNotes notes = newNotes(importedChange);
+
+ ImmutableListMultimap<ObjectId, HumanComment> comments = notes.getHumanComments();
+ assertThat(comments).hasSize(1);
+ HumanComment gotComment = comments.entries().asList().get(0).getValue();
+ assertThat(gotComment.author.getId()).isEqualTo(Account.UNKNOWN_ACCOUNT_ID);
+ }
+
+ private HumanComment comment(Change change, IdentifiedUser commenter, String serverId) {
+ return newComment(
+ Objects.requireNonNull(change.currentPatchSetId()),
+ "a.txt",
+ "uuid1",
+ new CommentRange(1, 2, 3, 4),
+ 1,
+ commenter,
+ null,
+ TimeUtil.now(),
+ "Comment",
+ (short) 1,
+ ObjectId.zeroId(),
+ false,
+ serverId);
+ }
+
+ private void linkImportedAccountTo(Account.Id importedAccount, Optional<Account.Id> localAccount)
+ throws IOException {
+
+ if (localAccount.isEmpty()) {
+ when(externalIdCacheMock.byKey(any())).thenReturn(Optional.empty());
+ } else {
+ Account.Id localAccountId = localAccount.get();
+ ExternalId.Key importedAccountIdKey =
+ ExternalId.Key.create(
+ ExternalId.SCHEME_IMPORTED, importedAccount + "@" + IMPORTED_SERVER_ID, false);
+ ExternalId linkedExternalId =
+ ExternalId.create(importedAccountIdKey, localAccountId, null, null, null);
+
+ when(externalIdCacheMock.byKey(eq(importedAccountIdKey)))
+ .thenReturn(Optional.of(linkedExternalId));
+ }
+ }
}
